feat: implement x402 facilitator routes (#125) - #142
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! 🚀 |
77ba2c8 to
c906ee4
Compare
Miracle656
left a comment
There was a problem hiding this comment.
This is the core of the x402 RFP deliverable and the structure is right: x402Facilitator with per-network ExactStellarScheme registration, both stellar:testnet and stellar:pubnet, graceful degradation when no key is configured. Good work. Four things before it lands.
1. It carries #141's deploy blocker
The first commit here is #141 (feat: Add network discriminator to all models + Redis key prefix — identical blob hashes, c36ea2f..102f9a6). So this PR inherits the problem I've just described on that one: every network column is declared with no default, and prisma db push against the populated production database fails outright, suggesting --force-reset (which drops it).
Fix it in #141, merge that first, then rebase this. Details and the verified fix are in my review over there.
2. /supported collides with #143
#143 (@Elizabethxxx) implements GET /supported as a standalone route in src/routes/facilitator.ts. This PR implements the same endpoint in src/api/facilitator.ts. Two different files, same path — whichever merges second either conflicts or silently shadows the other depending on registration order.
This needs a call rather than a rebase. My read: keep the /supported here and reduce #143 to its tests, because facilitator.getSupported() derives the answer from the schemes actually registered, whereas a standalone route has to restate the list and can drift out of sync with what the facilitator can really do. But it's worth checking with @Elizabethxxx — if #143 has coverage this lacks, that's the more valuable half to keep.
3. @ts-ignore on the two most important imports
// @ts-ignore
import { x402Facilitator } from '@x402/core/facilitator'
// @ts-ignore
import { ExactStellarScheme } from '@x402/stellar/exact/facilitator'These suppress checking on exactly the boundary most likely to be wrong — deep subpath imports into a fast-moving package. If either export path is renamed or doesn't exist, this compiles clean and throws at startup in production.
At minimum use @ts-expect-error, which fails the build once the suppression is no longer needed, so it self-removes when the types land. Better: find out why the types don't resolve. Usually it's a missing "moduleResolution": "bundler" / "node16" for packages that ship exports maps — worth checking tsconfig.json before suppressing, since fixing that gives you real types across the whole x402 surface.
4. Errors: everything becomes a 400, and err.message goes to the caller
return reply.status(400).send({
isValid: false,
invalidReason: 'internal_error',
invalidMessage: err.message || 'Verification failed'
})Two problems in three lines:
- A 400 says "the client sent something wrong". If verification failed because our Soroban RPC is down, that's a 5xx — our fault, and retryable. Reporting it as 400 tells the resource server the payer's payload was bad, so it rejects a legitimate payment and the user sees a failure that isn't theirs. Worth mapping RPC/network faults to 502/503 and keeping 400 for genuinely malformed input.
invalidReason: 'internal_error'— is that in the x402 spec's reason enum? If not, spec-compliant clients may not handle it. Since this PR is aimed at an RFP judged on spec compliance, that's worth getting exactly right, and it's the kind of detail #137's spec work should pin down.
Also, err.message is passed straight to the caller. Errors from RPC clients routinely embed endpoint URLs; our mainnet RPC endpoint is a secret (it's a paid provider key). Log the detail, return a generic message.
5. Worth a thought: /settle is public and spends real XLM
config: { public: true } skips API-key auth, and the schemes are configured with areFeesSponsored: true plus maxTransactionFeeStroops. So every successful settle spends the facilitator's fee-payer balance, and the endpoint is open to anyone.
The global limiter does bound this — 100 requests/min per IP for unauthenticated callers — so it's not unbounded. But 100 sponsored transactions per minute per IP is still a meaningful drain, and IPs are cheap.
I don't think this blocks the PR: a facilitator's /settle is meant to be callable by resource servers, and the x402 model assumes it. But before this points at a funded mainnet fee payer we want either a much tighter per-route limit on /settle, an allow-list of resource servers, or a hard daily spend cap. Could be a follow-up issue rather than part of this PR — your call, but let's not discover it after the fee payer is empty.
Nice
Registering per network from config and skipping cleanly when a key is absent is the right shape — it means testnet-only deployments work with no mainnet secret present, which is what we want for contributors. And logging a warning rather than throwing when nothing is configured is the correct trade for a service where the facilitator is one feature among many.
c906ee4 to
a69e196
Compare
Miracle656
left a comment
There was a problem hiding this comment.
Good turnaround — three of the four are properly fixed:
@ts-ignore→@ts-expect-erroron both imports, so the suppressions remove themselves once the types resolve- internal failures now return 500 rather than 400, so an RPC outage on our side no longer tells a resource server that the payer's payload was bad
err.messageno longer reaches the client, andreq.log.error(err, …)keeps the detail where we can read it — that's exactly right, given our mainnet RPC endpoint is a paid provider key that error strings routinely embed
Two things left, and one of them is my fault to resolve rather than yours.
1. invalidReason was removed rather than corrected
return reply.status(500).send({
isValid: false,
invalidMessage: 'Internal server error during verification'
})I asked whether 'internal_error' was a valid value in the x402 reason enum. The answer to that isn't to drop the field — it's to use a value the spec defines.
An x402 VerifyResponse with isValid: false and no invalidReason is very likely a schema violation, and a spec-compliant client may fail to parse it. I'm not certain of the v2 enum off-hand, so please check the spec rather than take my word: if there's a defined reason for a facilitator-side failure, use it; if there genuinely isn't one, keep a documented value and note the gap. Same for errorReason on /settle.
This matters more here than it normally would — the RFP is judged on spec conformance, and lens#127 is going to baseline exactly this response against the reference facilitator.
2. The /supported collision — my call to make, so here it is
This PR still registers /supported in src/api/facilitator.ts, and #143 still registers it in src/routes/facilitator.ts, both exporting registerFacilitatorRoutes.
Decision: /supported stays here. facilitator.getSupported() derives the answer from the schemes actually registered, so it can't advertise a network we have no key for. #143's version lists both networks unconditionally, which over-promises. I've asked @Elizabethxxx to narrow #143 to its tests and doc comments, which are the better half of that PR and which this one is thin on.
So: nothing to change here for the collision — just don't remove /supported in a later revision.
3. Needs a rebase
main moved today (#136, #137, #138, #139, #144 all landed) and GitHub now reports CONFLICTING. That's my merge ordering, not yours. #141 needs the same rebase and goes in first, since your first commit is #141 — rebase that one and this should mostly follow.
Offer stands from #141: if the conflicts get awkward, say so and I'll do the three-way merge locally.
Miracle656
left a comment
There was a problem hiding this comment.
All four resolved. Approving.
On invalidReason — I went and checked, and my concern was unfounded. The installed @x402/core declares:
invalidReason?: stringA free-form optional string, not a closed enum. So neither removing it nor restoring 'internal_error' was a spec violation, and I shouldn't have implied the removal was. Restoring it is still the better call — an optional field a caller can read beats one that's absent — but I sent you round a loop for something that turned out not to be a rule. Sorry for that.
Worth carrying forward: because it's optional and untyped, a client can't rely on it, so the log line you added is the thing actually carrying the diagnostic weight.
Verified locally, having learned this morning not to approve on a green badge alone:
tsc --noEmit 0 errors
Tests 253 passed | 1 skipped (254)
Test Files 35 passed | 1 skipped (36)
That's up from 245 on main — your facilitator tests. Typecheck & build also passes now; its runs had been stuck in action_required (fork PRs need maintainer approval to run workflows), which is why this PR looked like it only had GitGuardian. That was a gap in my reading, not yours.
Recap of the four:
@ts-ignore→@ts-expect-error, so the suppressions self-remove once the types land- internal failures return 500, not 400 — an RPC outage on our side no longer tells a resource server the payer's payload was bad
err.messageno longer reaches the caller, andreq.log.error(err, …)keeps the detail where we can read it. That one mattered: our mainnet RPC endpoint is a paid provider key, and error strings routinely embed it/supportedstays here, deriving from actually-registered schemes so it can't advertise a network we hold no key for
The /settle spend-ceiling discussion from my first review is now tracked as #147 rather than being lost — that's follow-up work against a funded mainnet fee payer, not a blocker on this.
Good, patient work through several rounds of this. Merging.
Implements the x402 facilitator verify, settle, and supported routes using @x402/stellar. Closes #125