Move ldk-server-mcp into the workspace#188
Move ldk-server-mcp into the workspace#188tnull wants to merge 6 commits intolightningdevkit:mainfrom
ldk-server-mcp into the workspace#188Conversation
|
🎉 This PR is now ready for review! |
70f8e5f to
3bc8a6e
Compare
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.
3bc8a6e to
8d6d7b0
Compare
b76ffeb to
b0292ff
Compare
| // 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 respond
if request.id.is_none() {
continue;
}b0292ff to
9295c38
Compare
| 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
9d74698
Compare
|
Can you squash? |
9d74698 to
0ff77b7
Compare
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
e729511
Compare
e729511 to
429b1a7
Compare
Squashed without further changes. |
|
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
540bd3c
Compare
Ah, silent merge conflict it seems. Rebased. |

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