Move `ldk-server-mcp` into the workspace by tnull · Pull Request #188 · lightningdevkit/ldk-server

tnull · 2026-04-15T09:37:11Z

Moving from https://github.com/tnull/ldk-server-mcp

Draft for now.

ldk-reviews-bot · 2026-04-15T09:37:14Z

🎉 This PR is now ready for review!
Please choose at least one reviewer by assigning them on the right bar.
If no reviewers are assigned within 10 minutes, I'll automatically assign one.
Once the first reviewer has submitted a review, a second will be assigned if required.

Move the MCP bridge into the ldk-server workspace and switch it to an in-tree client dependency so workspace builds and tests cover it directly. Co-Authored-By: HAL 9000

benthecarman

Did a quick skim and ran through codex

Codex Review:

  - [P2] Honor storage.disk.dir_path when resolving credentials — /home/ben/projects/ldk-server/ldk-server-mcp/src/
    config.rs:57-60
    If the shared ldk-server config uses a custom storage.disk.dir_path, this parser drops that section entirely, so
    resolve_config() can only look in ~/.ldk-server/... for api_key and tls.crt. In deployments that store node state
    outside the default directory (a setup ldk-server-cli already supports), the MCP bridge will fail to authenticate
    unless users also set both LDK_API_KEY and LDK_TLS_CERT_PATH by hand.
  - [P2] Fall back to the default gRPC address when no base URL is configured — /home/ben/projects/ldk-server/ldk-
    server-mcp/src/config.rs:112-116
    In setups that rely on the standard 127.0.0.1:3536 gRPC address and only use the default credential files on disk,
    this path exits with “Base URL not provided” instead of using the same default as ldk-server-cli. As a result, ldk-
    server-mcp cannot start in the zero-config case unless users create a config file or export LDK_BASE_URL, even
    though the server is running at the normal address.

I think these are two things we fixed in the cli since you created the MCP server.

Support storage.disk.dir_path config and fall back to the default gRPC address when neither LDK_BASE_URL nor a config file is present, matching the behavior of ldk-server-cli. Co-Authored-By: HAL 9000

Fix clippy needless_borrows_for_generic_args in test. Co-Authored-By: HAL 9000

Move routing and invoice default constants into ldk-server-client so they are shared between ldk-server-cli and ldk-server-mcp. Co-Authored-By: HAL 9000

Use the workspace-built MCP binary directly in tests and add focused helper coverage for argument parsing so regressions are caught without requiring a live server. Co-Authored-By: HAL 9000

Build the MCP binary in the e2e harness and exercise its stdio protocol against a live ldk-server so basic MCP functionality is verified without involving an agent. Co-Authored-By: HAL 9000

Drop the tests/ directory from rerun-if-changed since test changes don't affect the mcp binary. Co-Authored-By: HAL 9000

Run MCP-specific formatting, clippy, and crate tests in a dedicated workflow and add a separate job that exercises the MCP e2e sanity suite on Ubuntu. Co-Authored-By: HAL 9000

Remove redundant formatting check (already covered by the main CI workflow) and drop the MCP e2e job (already covered by the e2e-tests workflow). Co-Authored-By: HAL 9000

Describe the MCP bridge as a first-class workspace member and document how to build, test, and sanity-check it alongside the rest of ldk-server. Co-Authored-By: HAL 9000

Replace exhaustive tool listing with a pointer to tools/list to avoid the maintenance burden of keeping the README in sync with every new RPC. Co-Authored-By: HAL 9000

benthecarman

needs rebase too

benthecarman · 2026-04-16T20:37:06Z

@@ -0,0 +1,300 @@
+// This file is Copyright its original authors, visible in version control


most of this file seems shared with logic we have in the cli. maybe also worth putting in the client.

benthecarman · 2026-04-16T20:38:49Z

+};
+use serde_json::{json, Value};
+
+fn hex_encode(bytes: &[u8]) -> String {


we should have hex-conservative available here

benthecarman · 2026-04-16T20:54:29Z

+	}
+
+	pub async fn call_tool(
+		&self, client: &LdkServerClient, name: &str, args: Value,


Using serde_json::Value everywhere makes things feel really flaky. I think it'd make sense to bring back the json deserializers for the request types and then use those. That would make things a lot more type safe and less error prone.

benthecarman · 2026-04-16T20:59:20Z

+	format!("{}:{}", pt.token, pt.index)
+}
+
+fn build_route_parameters(args: &Value) -> RouteParametersConfig {


this file would be hugely cleaned up if we used the proto struct's json deserialzers

benthecarman · 2026-04-16T21:01:49Z

+		},
+	};
+
+	let client = match LdkServerClient::new(cfg.base_url, cfg.api_key, &cfg.tls_cert_pem) {


should we call get_info to start just to make sure we're connected?

benthecarman · 2026-04-16T21:06:04Z

+	pub async fn call_tool(
+		&self, client: &LdkServerClient, name: &str, args: Value,
+	) -> ToolCallResult {
+		for (def, handler) in &self.tools {


Would be better to have this as a HashMap keyed by the name so we don't have to iterate through every time.

benthecarman · 2026-04-16T21:06:30Z

+			if def.name == name {
+				return match handler(client, args).await {
+					Ok(value) => {
+						let text = serde_json::to_string_pretty(&value)


Why pretty print it here? This is just going to the agent, so wouldnt it waste tokens

benthecarman · 2026-04-16T21:28:27Z

+use serde::Serialize;
+use serde_json::Value;
+
+pub const PROTOCOL_VERSION: &str = "2024-11-05";


The latest version is 2025-11-25, might be worth updating too. With my brief chat with claude about the differences that'd effect us is that the error handling is slightly improved and more metadata. It also has the remote mcp stuff using SSE but we can ignore that for now.

benthecarman · 2026-04-16T21:30:14Z

+				let params = request.params.unwrap_or(Value::Null);
+				let tool_name = params.get("name").and_then(|v| v.as_str()).unwrap_or("");
+				let tool_args = params.get("arguments").cloned().unwrap_or(serde_json::json!({}));


instead of the upwrap_ors here we should be returning a INVALID_PARAMS error

benthecarman · 2026-04-16T21:31:27Z

+			amount_msat,
+		})
+		.await
+		.map_err(|e| e.message.clone())?;


Here and a lot of others, you don't need to clone the error message

Instead it actually would make sense to make a converter for LdkServerError to a dedicated error type here. Then we could handle things better like if they server marked the params as invalid vs an actual server error. Then we could actually use INTERNAL_ERROR too

tnull requested a review from benthecarman April 15, 2026 09:37

tnull marked this pull request as draft April 15, 2026 09:37

tnull force-pushed the 2026-04-move-mcp-into-workspace branch from e090553 to 70f8e5f Compare April 15, 2026 09:47

Add ldk-server-mcp to the workspace

83b53f2

Move the MCP bridge into the ldk-server workspace and switch it to an in-tree client dependency so workspace builds and tests cover it directly. Co-Authored-By: HAL 9000

tnull force-pushed the 2026-04-move-mcp-into-workspace branch from 70f8e5f to 3bc8a6e Compare April 15, 2026 09:50

benthecarman reviewed Apr 15, 2026

View reviewed changes

Comment thread .github/workflows/mcp.yml Outdated

Comment thread .github/workflows/mcp.yml Outdated

Comment thread e2e-tests/build.rs Outdated

Comment thread ldk-server-mcp/src/tools/handlers.rs Outdated

Comment thread ldk-server-mcp/README.md

tnull added 10 commits April 16, 2026 11:40

f - Add ldk-server-mcp to the workspace

d2e7c53

Support storage.disk.dir_path config and fall back to the default gRPC address when neither LDK_BASE_URL nor a config file is present, matching the behavior of ldk-server-cli. Co-Authored-By: HAL 9000

f - Add ldk-server-mcp to the workspace

b2c4560

Fix clippy needless_borrows_for_generic_args in test. Co-Authored-By: HAL 9000

f - Add ldk-server-mcp to the workspace

f559ff5

Move routing and invoice default constants into ldk-server-client so they are shared between ldk-server-cli and ldk-server-mcp. Co-Authored-By: HAL 9000

Improve mcp test coverage in workspace

c5902ee

Use the workspace-built MCP binary directly in tests and add focused helper coverage for argument parsing so regressions are caught without requiring a live server. Co-Authored-By: HAL 9000

Add end-to-end sanity checks for ldk-server-mcp

34aafc9

Build the MCP binary in the e2e harness and exercise its stdio protocol against a live ldk-server so basic MCP functionality is verified without involving an agent. Co-Authored-By: HAL 9000

f - Add end-to-end sanity checks for ldk-server-mcp

5e00662

Drop the tests/ directory from rerun-if-changed since test changes don't affect the mcp binary. Co-Authored-By: HAL 9000

Add CI coverage for ldk-server-mcp

de9ea9d

Run MCP-specific formatting, clippy, and crate tests in a dedicated workflow and add a separate job that exercises the MCP e2e sanity suite on Ubuntu. Co-Authored-By: HAL 9000

f - Add CI coverage for ldk-server-mcp

970402a

Remove redundant formatting check (already covered by the main CI workflow) and drop the MCP e2e job (already covered by the e2e-tests workflow). Co-Authored-By: HAL 9000

Document ldk-server-mcp in the workspace

fd2cd37

Describe the MCP bridge as a first-class workspace member and document how to build, test, and sanity-check it alongside the rest of ldk-server. Co-Authored-By: HAL 9000

f - Document ldk-server-mcp in the workspace

8d6d7b0

Replace exhaustive tool listing with a pointer to tools/list to avoid the maintenance burden of keeping the README in sync with every new RPC. Co-Authored-By: HAL 9000

tnull force-pushed the 2026-04-move-mcp-into-workspace branch from 3bc8a6e to 8d6d7b0 Compare April 16, 2026 09:47

tnull requested a review from benthecarman April 16, 2026 09:47

tnull mentioned this pull request Apr 16, 2026

Add initial guides & documentation #186

Merged

tnull marked this pull request as ready for review April 16, 2026 11:03

benthecarman reviewed Apr 16, 2026

View reviewed changes

		@@ -0,0 +1,300 @@
		// This file is Copyright its original authors, visible in version control

Conversation

tnull commented Apr 15, 2026

Uh oh!

ldk-reviews-bot commented Apr 15, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

benthecarman left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

benthecarman left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

ldk-reviews-bot commented Apr 15, 2026 •

edited

Loading