Uh oh!
There was an error while loading. Please reload this page.
Move ldk-server-mcp into the workspace - #188
Conversation
🎉 This PR is now ready for review! |
70f8e5f to
3bc8a6eCompare
benthecarman
left a comment
There was a problem hiding this comment.
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.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
3bc8a6e to
8d6d7b0CompareUh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
b76ffeb to
b0292ffCompare| // string, so we manually extract the string and turn it into bytes instead of delegating to the | ||
| // proto-typed deserializer. | ||
| pub async fn handle_sign_message(client: &LdkServerClient, args: Value) -> Result<Value, McpError> { | ||
| let message = args |
There was a problem hiding this comment.
any reason we aren't using the json decoder here? and in handle_verify_signature
There was a problem hiding this comment.
Hmm, see the comment above. Now introduced some extra {SignMessage,VerifySignature}Args structs deriving Deserialize to make this a bit cleaner. Let me know what you think.
| - **Spec**: https://spec.modelcontextprotocol.io/ | ||
| - **Transport**: stdio (one JSON-RPC 2.0 message per line) | ||
| - **Methods implemented**: `initialize`, `tools/list`, `tools/call` | ||
| - **Notifications handled**: `notifications/initialized` (ignored, no response) |
There was a problem hiding this comment.
I don't see us handling this anywhere, looks like we'd return METHOD_NOT_FOUND
There was a problem hiding this comment.
No, we just skip in main:
// Notifications have no id — do not respondif request.id.is_none(){continue;}b0292ff to
9295c38Compare| runs-on: ubuntu-24.04 | ||
| steps: | ||
| - uses: actions/checkout@v6 | ||
| - uses: dtolnay/rust-toolchain@nightly |
| - name: Get the current date | ||
| run: echo "date=$(date +'%Y-%m-%d')" >> $GITHUB_ENV | ||
| - name: Create Pull Request | ||
| uses: peter-evans/create-pull-request@v8 |
| runs-on: ubuntu-24.04 | ||
| steps: | ||
| - uses: actions/checkout@v6 | ||
| - uses: dtolnay/rust-toolchain@nightly |
| runs-on: ubuntu-24.04 | ||
| steps: | ||
| - uses: actions/checkout@v6 | ||
| - uses: dtolnay/rust-toolchain@nightly |
| - name: Get the current date | ||
| run: echo "date=$(date +'%Y-%m-%d')" >> $GITHUB_ENV | ||
| - name: Create Pull Request | ||
| uses: peter-evans/create-pull-request@v8 |
9295c38 to
9d74698Comparebenthecarman
commented
May 1, 2026
Can you squash? |
9d74698 to
0ff77b7Comparetnull
commented
May 5, 2026
Squashed without further changes. |
benthecarman
left a comment
There was a problem hiding this comment.
final thing and then i think this is good
| if request.expiry_secs == 0 { | ||
| request.expiry_secs = DEFAULT_EXPIRY_SECS; |
There was a problem hiding this comment.
I think we'll need to do similar logic for DEFAULT_MAX_TOTAL_CLTV_EXPIRY_DELTA, DEFAULT_MAX_PATH_COUNT, and DEFAULT_MAX_CHANNEL_SATURATION_POWER_OF_HALF where relevant
acfab96 to
e729511Comparee729511 to
429b1a7Comparetnull
commented
May 7, 2026
Squashed without further changes. |
benthecarman
commented
May 7, 2026
the proto ci is failing now? |
Derive `serde::Deserialize` (alongside the existing `serde::Serialize`) and `#[serde(default)]` on all generated prost messages so that JSON payloads can be deserialized directly into the request types. This reinstates the deserializers that were dropped in 8af3189 so that clients like `ldk-server-mcp` can parse tool arguments straight into the typed proto structs instead of threading every field through `serde_json::Value`. Generated with the assistance of AI (Claude).
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
Use the workspace-built MCP binary directly in tests so regressions are caught without requiring a live server and without recompiling from inside each test. Generated with the assistance of AI (Claude).
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
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
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
429b1a7 to
540bd3cComparetnull
commented
May 8, 2026
Ah, silent merge conflict it seems. Rebased. |
Uh oh!
There was an error while loading. Please reload this page.

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