feat: MCP discovery server search and paid-call agent tools (#132) - #145
Conversation
|
@Anambraboi-1 Great news! 🎉 Based on an automated assessment of this PR, the linked Wave issue(s) no longer count against your application limits. You can now already apply to more issues while waiting for a review of this PR. Keep up the great work! 🚀 |
Miracle656
left a comment
There was a problem hiding this comment.
The structure is good. Injecting fetchWithPayment: typeof fetch rather than letting the server hold signing keys is the right seam — it keeps src/mcp/server.ts free of secrets and makes tests/mcp.test.ts possible without a funded account. MCP_ERROR_CODES is a clean, small taxonomy, and distinguishing ERR_INSUFFICIENT_BALANCE from ERR_PAYMENT_REJECTED from ERR_MISSING_TRUSTLINE is exactly what an agent needs to decide whether to retry, top up, or give up.
Two things in examples/mcp-server/run.ts I'd want changed before this lands, both about an autonomous agent that spends money.
1. No spend ceiling
const fetchWithPayment = wrapFetchWithPaymentFromConfig(globalThis.fetch, { … })No maximum per-payment value is passed. So the wrapper auto-pays whatever amount a 402 demands, and the caller here is an LLM-driven agent in a loop — the one caller that cannot sanity-check a price.
The failure doesn't need a malicious server: a resource server with a units bug quotes 1000× its intended price, the agent pays it, and nothing in the path objects. With a hostile server it's simply a drain. @x402/fetch supports a max-value ceiling for exactly this reason — please set one, and make it an env var with a conservative default so the example teaches the safe pattern. People copy examples verbatim; this one is the template for every agent that follows.
Worth pairing with the /settle rate-limit discussion on #142 — same underlying question of who is allowed to spend how much, from the other side.
2. One key signs on both testnet and mainnet
const secretKey = process.env.MCP_AGENT_SECRET_KEY || Keypair.random().secret();
const testnetSigner = createEd25519Signer(secretKey, 'stellar:testnet');
const pubnetSigner = createEd25519Signer(secretKey, 'stellar:pubnet');The same secret backs both networks. Testnet keys are handled casually by design — pasted into issues, committed to example .env files, printed in CI logs, shared to reproduce a bug. Here that same key holds real mainnet funds.
Separate MCP_AGENT_SECRET_KEY_TESTNET / _MAINNET, following the per-network env convention #138 establishes elsewhere in this repo. The example is where the habit gets set.
While you're there: falling back to Keypair.random() means an unconfigured run silently signs with an unfunded random account and every payment fails for a reason that looks like a network problem. Failing fast with "set MCP_AGENT_SECRET_KEY_TESTNET" would be kinder — and it avoids generating a random key that then signs pubnet payloads.
Minor
@x402/fetch ^2.24.0 alongside @x402/core ^2.8.0 and @x402/stellar ^2.8.0. Caret ranges mean npm resolves all three to current 2.x so it'll work, but the declared floors disagree by 16 minors — and #142 imports @x402/core/facilitator, a subpath that may not exist at the stated 2.8.0 floor. Worth aligning the floors across the x402 family in one go so npm ci on a cold lockfile can't resolve a combination nobody has run.
The src/mcp/ + examples/mcp-server/ split is right, and shipping a runnable agent alongside the server is what makes this credible as an RFP deliverable rather than a library nobody has exercised.
Miracle656
left a comment
There was a problem hiding this comment.
All three addressed, and addressed properly rather than minimally.
Spend ceiling:
maxPrice: process.env.MCP_MAX_PAYMENT_PRICE || '$1.00',Configurable with a conservative default, which is the right shape for an example — people copy these verbatim, so the default is what actually ships in most downstream agents.
Separate keys per network: MCP_AGENT_SECRET_KEY_TESTNET / _MAINNET, so a key handled casually on testnet is no longer the key holding mainnet funds.
Fails fast with a message naming both variables, instead of Keypair.random() silently signing with an unfunded account and producing payment failures that look like network errors.
Version floors aligned — @x402/core and @x402/stellar both to ^2.24.0, matching @x402/fetch, so a cold npm ci can't resolve a combination nobody has run.
Mergeable and green.
One nit, not worth another round on its own: Keypair is still imported in examples/mcp-server/run.ts but no longer used now the random fallback is gone. Worth dropping whenever you next touch the file.
Nice work — this was the PR with the most actual risk in it (an LLM agent in a loop with an open wallet), and it's now the one I'd be least nervous about.
Miracle656
left a comment
There was a problem hiding this comment.
Correcting my own review — I approved this an hour ago on the strength of the three fixes, without building it. That was premature, and I want to be straight about why it happened rather than quietly reverse the approval.
Two reasons the approval was wrong:
- I checked that the fixes addressed my comments, not that the branch still compiles.
- I read "GitGuardian pass" as CI being green. It wasn't CI. The
Typecheck & buildworkflow has never run on this PR — its runs are sitting inaction_required, awaiting maintainer approval for a fork. I've now approved them, so real results should appear shortly.
The three fixes themselves stand and are good: the maxPrice ceiling with a $1.00 default, separate MCP_AGENT_SECRET_KEY_TESTNET / _MAINNET, the fast failure instead of Keypair.random(), and the aligned ^2.24.0 x402 floors. None of that is in question.
Two real type errors
Building locally against this branch's own lockfile:
src/mcp/server.ts(90,62): error TS18046: 'data' is of type 'unknown'.
src/mcp/server.ts(90,80): error TS18046: 'data' is of type 'unknown'.
const data = await res.json();
if ((Array.isArray(data) && data.length === 0) || (data.resources && data.resources.length === 0)) {res.json() is typed unknown, so data.resources doesn't typecheck. Worth fixing with a narrow type or a validated parse rather than a cast — this is a discovery response from a remote server, so it's exactly the input you don't want to assume the shape of. A small type guard here also gives ERR_NO_RESULTS a defensible definition instead of relying on a property that may not exist.
Also needs main merged
src/__tests__/bestRoute.test.ts and tests/aggregator.property.test.ts fail on this branch with:
Error: [vitest] No "Networks" export is defined on the "@stellar/stellar-sdk" mock.
That's #146, and it is not your bug — but it no longer reproduces on main, because #136/#139 landed and changed the code path that hit the incomplete mock. This branch predates them.
main has moved a lot today (#136, #137, #138, #139, #141, #144). Merging it should clear both suites and leave only the two type errors above.
For reference, main at the moment: tsc clean, 245 tests passing.
Sorry for the churn — you fixed what I asked for, and the approval should have come after a build rather than before one.
Miracle656
left a comment
There was a problem hiding this comment.
Fixed — and the fix is better than what I asked for. Approving.
I asked for "a narrow type guard or a validated parse rather than a cast". You wrote the guard:
const hasResources = (d: unknown): d is { resources: unknown[] } =>
typeof d === 'object' && d !== null && 'resources' in d && Array.isArray((d as Record<string, unknown>).resources);That is the right shape for this specific input. A discovery response comes from a remote server we do not control, so res.json() being unknown is the type system telling the truth rather than getting in the way. Casting it would have silenced the compiler and left ERR_NO_RESULTS depending on a property that might not exist. Narrowing it means the error code now has a defensible definition. The unused Keypair import is gone too.
Verified on the actual merge result, not just your branch — "mergeable" only means no textual conflict, and twice today PRs that merged clean turned out to be wrong together:
merge into main: 0 conflicts
tsc --noEmit 0 errors
Tests 257 passed | 1 skipped (258)
That's main's 253 plus your 4. Typecheck & build passes on the PR now too — those runs had been stuck in action_required awaiting fork approval, which is why this looked like it only had GitGuardian earlier.
One thing worth flagging so you don't chase it: my first run of the merge showed 2 failures in auth.test.ts and pairs.test.ts. Three re-runs were clean. There is an intermittent ~1-in-5 flake in this suite — it also cost me a false alarm on #141 this morning. Not yours; I'm filing it separately.
Everything from the earlier rounds survived the rebase: the maxPrice ceiling defaulting to $1.00, separate MCP_AGENT_SECRET_KEY_TESTNET/_MAINNET, the fast failure instead of Keypair.random(), and the aligned ^2.24.0 x402 floors.
Thanks for the patience across three rounds, including one where I withdrew an approval I should not have given. This one is properly checked.
Fixes #132
Changes: