Repository navigation
fix(graphql): add per-request network resolution to getPrice, getBestRoute, getPriceHistory and listPairs - #580
teethaking wants to merge 6 commits into
Conversation
|
@teethaking 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.
I think this is the wrong branch. The description and the diff do not describe the same change, and what is actually here would re-break main.
The diff does not contain the work the body describes
The body says the change is in src/api/graphql.ts — an optional network argument on getPrice, getBestRoute, getPriceHistory and listPairs, with x-network as a fallback. src/api/graphql.ts is not in this diff at all. What is:
.kiro/specs/usage-billing-quota/.config.kiro +1
.kiro/specs/usage-billing-quota/requirements.md +125
src/__tests__/x402-quota.test.ts +9
src/api/usage.ts -1
src/index.ts +81
That is usage/billing-quota work, not GraphQL network resolution.
What the src/index.ts change would do
Those 81 lines re-add an inline Fastify app — const app = Fastify(...), app.register(cors), app.register(compress) and the whole route-registration block — which main moved into buildApp() in src/app.ts. Merged, src/index.ts ends up with two:
80: const app = Fastify({ logger: { level: 'warn' } })
151: const app = await buildApp()
npx tsc --noEmit:
src/index.ts(80,9): error TS2451: Cannot redeclare block-scoped variable 'app'.
src/index.ts(80,15): error TS2304: Cannot find name 'Fastify'.
src/index.ts(81,22): error TS2304: Cannot find name 'cors'.
src/index.ts(82,22): error TS2304: Cannot find name 'compress'.
src/index.ts(101,22): error TS2304: Cannot find name 'rateLimit'.
This is not hypothetical: that exact error was on main this morning at a9e7b0da and was fixed by 344ce4f. Merging this puts it straight back. The cause is a branch cut before the buildApp() refactor whose merge kept the old inline block.
Also
.kiro/specs/ are editor-assistant artifacts rather than source — we stripped a committed .kiro/ directory out of #96 for the same reason. Worth adding to .gitignore so it stops happening.
What to do
Push the branch that actually has the GraphQL change, rebased on current main, and let this one go. #359 is worth doing and the reasoning in your description is right — the priceUpdated subscription already takes an optional network while the four queries do not, which makes two halves of one API disagree. Making an unrecognised network an explicit error rather than a silent fallback is the correct choice too.
Before you re-request, run npx tsc --noEmit and npx vitest run on the rebased branch. The duplicate-app case is caught by the first command in about two seconds, and it is the thing to check for on any branch that predates the buildApp() split — #427 and #429 both have the same injury in different files.
The inline app setup (Fastify(), cors, compress, rateLimit, route registrations) was left over from before the buildApp() refactor and caused TS2451 (duplicate const app) and TS2304 (Fastify/cors/compress not imported) when merged with main. Removed the entire inline block. src/index.ts now does only: const app = await buildApp() which is the correct path post-refactor. Also added .kiro/ to .gitignore so IDE spec/hook artifacts do not land in future PRs (same reason .kiro/ was stripped from Divinity-Labs#96).
|
@Miracle656 review |
Miracle656
left a comment
There was a problem hiding this comment.
Scanned clean. Per-request network resolution in the GraphQL resolvers is a real gap — x-network is honoured on the REST routes but a GraphQL caller gets whatever the process default is, which means a mainnet query can quietly answer with testnet prices. Worth fixing.
Three things block it, and the first is the big one.
1. The branch is 195 commits behind main.
Against current main this diff removes roughly 19,000 lines from src/. That is not a conflict to resolve, it is most of the last two months of work being deleted because the branch predates it. The largest:
| Deleted | Lines |
|---|---|
src/ingest/venues/blend.ts |
661 |
src/__tests__/phoenixIngester.test.ts |
657 |
src/ingest/venues/phoenix.ts |
581 |
Two entire venue ingesters and their tests. Also in there: src/routes/facilitator.ts's x402 spend ceiling, which landed this morning on a money path.
A rebase this large is genuinely unpleasant, and I would not blame you for preferring to start again: the actual change you want — resolving the network per request inside the resolvers — is probably a day's work against current main and would be a much smaller, more reviewable diff than reconciling 195 commits. Your call, but I would take a fresh branch.
2. .kiro/ artifacts are committed
.kiro/specs/usage-billing-quota/.config.kiro
.kiro/specs/usage-billing-quota/requirements.md
Editor/agent scratch files. We have had these land before; they should not be in the repo. Worth adding .kiro/ to your global gitignore so it cannot happen again.
3. src/app.ts is touched
Last time I looked at this branch it re-added an inline Fastify app construction, which collides with the real one and gives Cannot redeclare block-scoped variable 'app'. src/app.ts has changed substantially since — it now carries the structured logger, the error handler, the CORS delegator and the /v1 versioning hook — so whatever this branch does to it needs redoing against the current file rather than merged into it.
What to aim at when you redo it
The resolvers should take the network from the request context the same way the REST routes do, so one request means one network end to end. src/middleware/network.ts declares req.network; the mechanism exists, it just is not reaching the GraphQL context. And once it does, a test that issues the same query against both networks and asserts different prices is the one that proves it — a single-network test will pass whether or not the plumbing works.
Summary
GraphQL queries were scoped to the active network only — a caller on a deployment running both networks had no way to query the other one, while the priceUpdated subscription already accepted an optional network argument, leaving the two halves of the same API inconsistent. This PR adds an optional network argument to all four queries, honours the x-network header as a fallback, makes unrecognised network values an explicit error, and routes findPair lookups through the requested network's pair list.
Closes #359
What changed
src/api/graphql.ts
Network resolution order applied to getPrice, getBestRoute, getPriceHistory, and listPairs
Unrecognised network value
findPair scoping
Schema
No addresses, contract IDs, or keys are elided anywhere in code, comments, or test fixtures — all written in full
Tests (src/tests/graphqlNetworkResolution.test.ts — new, following the house pattern in graphqlSubscription.test.ts)
How to verify
Checklist