Repository navigation
fix(offramp): require coin on GET /offramp/rate and return naira per unit of that asset - #257
Conversation
…acle656#237) GET /offramp/rate previously called Linq's /b2b/rate which always returns naira per dollar. Dividing a naira amount by that gives the right USDC figure by coincidence (1 USDC = 1 USD) but the wrong figure for any other asset by exactly that asset's price — the same defect that caused the onramp to buy 0.6 XLM instead of 3.6 for a 1000 NGN purchase. Changes: - src/api/ngn.ts: export ngnPerCoin so the offramp can reuse the same price path - src/api/offramp.ts: require coin query param (400 without it), call ngnPerCoin(), cache per-coin, response is { coin, rate, indicative: true } - src/__tests__/routes/offramp.test.ts: add 7 tests pinning XLM vs USDC rates differ, 400 on missing/unknown coin, no currency field, 502 on pricing failure - src/openapi/schemas.ts: add offrampRateQuerySchema, update offrampRateResponseSchema (coin enum, rate description, drop currency) - src/openapi/build.ts: add request.query to /offramp/rate registration, update summary - openapi.json: regenerated to match new schema
|
@Daveeee-tech 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
Miracle656
left a comment
There was a problem hiding this comment.
The reasoning is right and the money argument is right. But merging this alone takes cash-out away from every installed APK, and I want to make sure that is not discovered in production.
What happens on merge
Both clients call the endpoint with no coin:
frontend/wallet/lib/offramp.ts:221 await call<OfframpRate>('/offramp/rate')
frontend/wallet/lib/offramp.ts:242 return call<OfframpRate>('/offramp/rate')
frontend/mobile/lib/offramp.ts:149 await call<OfframpRate>('/offramp/rate');
frontend/mobile/lib/offramp.ts:179 return call<OfframpRate>('/offramp/rate');
And line 149 is inside isOfframpAvailable(), which treats any error as "the offramp is not available":
export async function isOfframpAvailable(): Promise<boolean> {
try {
await call<OfframpRate>('/offramp/rate');
...
} catch {
void AsyncStorage.setItem(AVAILABILITY_KEY, '0');
return false;
}
}So a 400 on a coin-less request does not surface as "pick a coin". It makes the Cash out tile vanish from the app, for everyone, immediately on deploy — and it looks like the backend is down. The web wallet redeploys with main; installed APKs do not update on our schedule, so those users lose cash-out until they happen to upgrade.
We have been here before: #216 was a breaking offramp change and the lesson recorded from it was ship the client first.
What I would like instead
Keep the strictness, stage the rollout:
- Now: reject an unknown coin with a 400 — that is new behaviour nothing depends on. For a missing coin, keep answering as today, and add the coin the answer is priced for to the response body so a client can tell. Log it as deprecated so we can see who is still asking.
- Then: update both clients to send
coin, includingisOfframpAvailable()— which should probably probe with an explicit coin rather than relying on the rate call at all, since "is the service up" and "what is the rate for XLM" are different questions. - Then, once the APK with that change is actually out, flip a missing coin to a 400 and delete the compatibility path.
I know step 1 looks like exactly what you argued against — "defaulting here would reproduce that silently". The difference is that it does not change anything: a coin-less caller gets the same answer it gets today, which is correct for USDC and wrong for XLM, and is already wrong in both clients right now. This PR cannot fix those clients; only shipping them can. Making the server refuse does not make an old APK compute XLM correctly, it just stops it computing at all.
What is good here and should survive
ngnPerCoin shared with the onramp so both rails quote from one source — that is what makes the two rails agree, and it is the actual fix for the class of bug.
The per-coin cache, so an XLM quote does not evict a USDC quote. The single-slot cache would have made the rate flap between assets under concurrent users, which is a second, subtler version of the same wrongness.
And the comment carries the evidence — ₦1,000 buying 0.6 XLM instead of 3.6. Writing the real number into the code is why the next person will not quietly undo this.
Split out the client changes, or restructure as above, and I will merge it quickly.
…lled APKs A coin-less request to /offramp/rate is how every installed build asks for the rate, and the mobile client reads any error there as "cash-out is down" and hides the tile. Refusing it does not make an old APK price XLM correctly - it stops that build pricing at all, for every user on it, until they happen to update. Miracle656#216 was this mistake and the recorded lesson is ship the client first. Defaulting for a missing coin is not the silent dollar-rate bug this endpoint was built to kill: ngnPerCoin("usdc") *is* naira per dollar, so a coin-less caller gets back exactly the number it gets today, and the response now names the coin it was priced for so a client can tell. Making the server strict cannot fix clients that are not in this repository; only shipping them can. Changes: - src/api/offramp.ts: an unknown or blank coin still 400s (new behaviour, nothing depends on it). A missing coin is priced for usdc and logged as deprecated, once per User-Agent, so the log lists the callers still asking rather than flooding. ngnPerCoin, the shared price path, and the per-coin cache are unchanged. - src/openapi/schemas.ts: coin is optional in the query schema, with the deprecation and the switch-over condition in its description; the response's coin field says what it is quoted per. No OpenAPI `deprecated` flag - these routes are live, and src/__tests__/openapiCoverage.test.ts pins that. - src/__tests__/routes/offramp.test.ts: the coin-less request now asserts the USDC answer, the coin it is labelled with, one deprecation log per caller, and 400 for unknown/blank/repeated coin. Explicit-coin cases are untouched. - docs/offramp-orders.md: the three stages, and the gate on stage 3 - a build carrying the client change must be out and the log line quiet. Stage 2 (send coin, and stop using this endpoint as the availability probe) lives in the wallet repos, not here. - openapi.json, clients/react-query/src/schema.d.ts: hand-updated to match, because this checkout has no node runtime or node_modules. Regenerate with `npm run docs:openapi` and `npm run generate -w clients/react-query` before merging; schema.d.ts was already stale from the previous commit.
…ps borrowing the rate endpoint
Stage 2's client change needs somewhere to ask "is cash-out up" that is not
"what is the rate for XLM". Until now the only way a client could find out was
/offramp/rate, and it read any error there as no-cash-out - which is why the coin
requirement had to be staged rather than enforced, and why a coin-less request is
the most reliable availability probe an old build has.
The new route answers from process config only and makes no provider call, so an
app opening on every install does not spend the rate allowance all users share.
It returns a body as well as a status because a redirect, a proxy error page, or a
captive portal is a reply, and a client that treats "got a reply" as "available"
offers cash-out and then fails on the first tap.
- src/api/offramp.ts: GET /availability -> { available: true }, no-store. The
router's existing 503 when LINQ_API_KEY is unset is the negative answer, so
nothing about deployment config is duplicated here.
- src/openapi/schemas.ts, build.ts: the response schema and the registration. No
502 documented, because nothing here reaches the provider; 503 is documented
because it is the answer a CTA is hidden on.
- src/__tests__/routes/offramp.test.ts: answers without touching the price path,
no-store, and 503 without a key.
- docs/offramp-orders.md: the gate's contract, and stage 2 written out as the
client repos have to satisfy it - including that a build holding a cached
unavailable flag from the old rate-based probe stays wrong until that key is
cleared.
- openapi.json, clients/react-query/src/schema.d.ts: hand-written again, same
caveat as the previous commit - regenerate before merge.
…re wrong Writing the client half found two claims in the rollout doc that the code does not support. The cached availability flag needed no migration. A build that cached `0` while the rate call was 400ing stores "the rails did not answer", and every launch overwrites it with a live probe — so the stale value self-heals rather than needing a key bump. And the coin each screen sends is a per-screen constant, because both cash-out screens are USDC-only today. The rule was never "not a constant", it was "the name sits where the asset is known" — a lib that hard-coded `usdc` on the caller's behalf is what made the pricing bug invisible.
…racle656#218 Four failures, one cause. Miracle656#218 stopped running a COUNT for every list query and made `total` opt-in through `includeTotal=true`, which is the right trade for an endpoint on the hot path — but it updated the unit tests and the README and left these three files asking `body.total` of a response that no longer computes one, so the integration job has been red since. The three cases that assert an exact count now ask for it, which is also what tests the flag end to end. The one case in `e2e.test.ts` that only wanted `total >= 1` does not need a COUNT at all: the two lines after it prove a specific row came back, which is a stronger claim about the same response. The incoming case gained the other half — without the flag, no `total`, and `hasMore` says the page is complete. `src/api.ts` said `{ total, limit, offset, transfers }` in three route docstrings, which is the sentence that kept leading readers to the old shape.
"reports both networks with differing cursors on /status" got a `networks` object with `testnet` in it and no `mainnet`. `/status` lists the loops in this process and, finding none it can see, falls back to `enabledNetworks()` — which with `NETWORKS` unset is `currentNetwork()`, and with `STELLAR_NETWORK` unset that is testnet. The single key the response had was the fallback answering, not the registry. The harness registers its loops through `_createLoopForTesting` and the DB assertions around it pass, so why the route did not see them through the module-level registry is still unanswered; what is certain is that the assertion should not depend on which of the two paths answered. Setting `NETWORKS=testnet,mainnet` makes the harness describe the deployment it is simulating, and then both routes to the answer agree. It also matches what a real dual-chain process looks like: `NETWORKS` is what an operator sets, and `?network=mainnet` is rejected by the same list, so a test that wants mainnet reported has to declare mainnet enabled.
…ed on without The SDK workflow fails when src/schema.d.ts differs from what `npm run generate` reads out of openapi.json, and the committed copy had fallen behind three routes: /ngn/customers/status and /fees were never carried over, and /status gained the per-network object plus nullable lag fields when the indexer stopped being a single loop. So the gap reads as belonging to the offramp change it happens to sit next to. It does not — the check has been red on main since the commit that documented /fees, and this branch only inherits it by touching openapi.json, which is the path filter that wakes the workflow up. The one genuinely new line is the @enum on the rate reply's coin. The file is generated, so the diff is the generator's own output rather than a hand-written guess at it: no npm on this machine, so it came from the log of the run that just failed, applied as a patch and checked with git apply before it was staged.
…d of sulking `npm test` used to finish in about 30 seconds, and did as recently as the run before this one. Every job since has sat on that step with nothing to show for it: the build job declared no timeout-minutes, so GitHub's six-hour default applies, and a job that is still running has no downloadable log. A hang ends up indistinguishable from CI being slow, both from outside and from in here. Two deadlines, one at each level. The step gets 15 minutes with a KILL 30 seconds behind it, because a jest left holding an open handle may not answer TERM, and a killed process flushes the output it already produced — which is the point: the log names the suite that stuck. The job gets 20 minutes so a step that ignores both still ends. This does not fix the hang and is not pretending to. It turns a six-hour non-answer into a fifteen-minute failure with a readable log, which is the shortest path to the handle itself.
…ever exits CI's unit step hung for hours on a suite that finishes in 22 seconds. The deadline added in the previous commit turned that into a readable log, and the log said: `Cannot log after tests are done`, once every 10 seconds, with `[indexer/testnet] loop crashed, restarting in 10s` between each. A case calls startAllIndexers to check which networks it would run, gets its answer, and leaves behind a loop that crashes on the absent database, schedules a restart, crashes again, and schedules another. That chain is what jest waits on, so the run never ends and never reports. It is not this branch's case and not this branch's timer — the call came in with the per-network status work on main, and every branch that merges with it inherits the hang, which is why main has been red since the morning too. The restart wait is unref'd, along with the pause between two polls. Neither is a reason to keep a process alive; the API listening is, and `main` always starts one. Held to the event loop they are immortal in either direction: a loop that keeps crashing restarts forever, and a healthy one polls forever. `_resetIndexerLoops` now cancels pending restarts as well as dropping loop state, so the seam that already exists for tidying between cases tidies the timer too. That alone does not fix the run above — the restart is scheduled after the last case ends, so there is no next beforeEach to catch it — which is why the unref is the actual fix and this is the hygiene that goes with it.
The timeout did exactly what it was added to do: it ended a six-hour non-answer in fifteen minutes and produced the log that named the crashed indexer loop in the commit before this one. It has no further work here, and keeping it costs more than it is worth inside a pull request about naira rates. A pull request that changes a workflow file holds every later commit from the same author for a collaborator to approve. That is the right rule — a fork should not get to run its own edited CI unchecked — but it means the actual fix in this branch sits unverified until someone clicks, for a reason that has nothing to do with offramp rates. The deadline goes to its own pull request, where it can be approved for what it is.
|
One thing for whoever picks this up: the unit suite cannot be verified on this branch until a run is approved, and the box in the description is unchecked because of that, not because it looks like failing. What I found about the hang, since it is not this PR's bug:
A case on Three notes on the shape of it:
The other two red checks were pre-existing and are explained in the description: the integration cases asserting the pre-#218 shape ( If you approve the held runs, |
Merging main into this branch kept both halves of `_resetIndexerLoops` but dropped the `restartTimers` declaration it iterates, so nothing compiled: typecheck, the Docker build behind the integration suite and the chaos image all failed on the same four TS2304 errors. Co-authored-by: Qoder <noreply@qoder.com>
All 816 unit tests pass and Jest still exits 1, because the loop started by the Miracle656#238 case is still polling when the environment goes away: its next RPC call lazily requires a module from a torn-down sandbox and throws after the summary has printed. Before the restart timer was unref'd the process never got that far — it hung instead — so this is the same abandoned loop, now visible. `while (true)` becomes `while (loops.get(net) === loop)`. `_resetIndexerLoops` already means "these loops are no longer wanted" to the callers that use it, so let the loop agree; the last case in a file now also stops its own loops in an `afterEach`, rather than leaving the next `beforeEach` to do it after Jest has already torn everything down. Co-authored-by: Qoder <noreply@qoder.com>
All 816 unit tests pass and Jest still exits 1, because the loop the Miracle656#238 case starts outlives the suite: its pending work rejects once the mock database is gone, the crash handler announces "restarting in 10s" after the summary has printed, and Jest counts output after teardown as a failure. Unref'ing the restart timer (40a3b42) made the process exit; this is what it now prints on the way out. Silence is only correct if the loop was dropped on purpose, so the request to keep a network running is tracked separately from the loop's own state: `_resetIndexerLoops` drops it, and a rejection that arrives afterwards ends the loop instead of reporting and restarting it. In production nothing ever drops it, so a crashed loop still gets its restart and its log. Co-authored-by: Qoder <noreply@qoder.com>
Silencing the dropped loop (35d07d4) still left Jest exiting 1 with 816 tests passing. The remaining output is the loop the Miracle656#238 case starts: its startup awaits a database that does not exist in a unit run and a public testnet RPC, so every line it prints — and the retry it fires after the first one fails — lands after the suite has finished. Jest counts that as a failure, and a require from inside that retry is the ReferenceError the earlier runs showed. The case never wanted a loop. It asks which networks a half-configured set yields and why the other is skipped, so that question now has its own function and `startAllIndexers` is just the part that acts on it. A second case covers what Miracle656#238 actually broke — a missing endpoint used to throw out of main() — and it can call the starter safely, because with nothing runnable it starts nothing. Co-authored-by: Qoder <noreply@qoder.com>
Now that the suite reports instead of hanging it can be seen failing: the first balance case exceeds the 5s timeout. It is not the route. The stale-read middleware (Miracle656#164) probes RPC health on every GET, and this was the one app-level suite that left `../../rpc` real, so the first request went out to the public testnet Soroban endpoint and waited for it — the rest of the file was fast because the answer is cached for 3s, which made it look like a one-off. The late `[api] Unhandled error: ... reading 'map'` in the same run is the tail of that request arriving after the case had already been abandoned. Co-authored-by: Qoder <noreply@qoder.com>
Miracle656
left a comment
Miracle656
left a comment
There was a problem hiding this comment.
This is the best response to a review I have had in this sweep. You did what I asked and then did the part I had not thought through.
The staged removal is exactly right, and it is written down where it will be found:
// 1. now - unknown coin 400s; missing coin answers as USDC and logs.
// 2. next - both clients send `coin`, and the availability check moves to
// `GET /offramp/availability` above instead of borrowing this
// endpoint. veil#1124. Stage 3 is what those clients unblock.
// 3. then - once an APK carrying step 2 is actually out and
// `logCoinlessRateRequest` has gone quiet, a missing coin 400s.
A plan in a PR description evaporates. A plan in the file, naming the client issue and the signal that says stage 3 is safe, is one somebody can finish six weeks from now.
And you separated the two questions I had only complained about. GET /offramp/availability means "is cash-out up" stops borrowing "what is the rate for an asset you did not name". That was the actual defect under the breaking change — isOfframpAvailable() treating any error from a pricing endpoint as the feature being down. I suggested the clients should probe something else eventually; you built the thing for them to probe.
The distinction you draw is the one that matters:
Answering is not the silent defaulting the onramp refuses… a coin-less caller gets back exactly the number it gets today — correct for USDC, wrong for XLM, and already wrong in both clients. Refusing does not make an old APK price XLM correctly; it only stops it pricing at all.
That is the argument I was reaching for and did not state that cleanly. Refusing feels stricter and buys nothing an old build can act on.
logCoinlessRateRequest firing once per caller is the right shape too — enough to know who is still on the old contract, not enough to drown the log. And it gives stage 3 an objective trigger rather than a guess.
Everything from the first review survives: ngnPerCoin shared with the onramp so both rails quote from one source, the per-coin cache so an XLM quote cannot evict a USDC one, and the ₦1,000-bought-0.6-XLM-instead-of-3.6 evidence kept in the comment.
753 tests pass, tsc --noEmit clean, verified against current main. Integration tests is pre-existing red on main and not yours.
Merging. veil#1124 is the next link in the chain — I will pick it up.
2 participants
closes #237
Summary
GET /offramp/ratetook nocoinand passed through whatever the provider's/b2b/ratereturned. ForUSDSUIthat is naira per dollar, which a client can mistake for naira per unit of any other asset. The onramp had the same defect and it cost real money (₦1,000 bought 0.6 XLM instead of 3.6).This PR makes the offramp price the asset it was asked for, and names it in the answer. It does not cut off builds that are already installed — the requirement is staged, because a missing
coinis not a caller asking to pick a coin, it is a phone running a build from before the parameter existed.What changed
GET /offramp/rate—coinis validated: present-but-unknown is a 400 naming the accepted values. A missingcoinis still answered, priced asusdc, and logged once perUser-Agentas deprecated.usdcis the asset the endpoint returned beforecoinexisted, so the coin-less answer is numerically what installed builds already read, and the log is what says it must not stay that way.Response shape —
currencyis gone (it was alwaysNGN, and the field was being read as the priced asset, which is the bug).coinnames what the number is per unit of:{ "coin": "xlm", "rate": 214.9, "indicative": true }Still
indicative: true— the binding rate is the one locked into the order, not this quote.GET /offramp/availability— new, and the reason this PR is bigger than the rate handler. Cash-out gated on/offramp/rateand read any error as "no cash-out", which made a pricing endpoint the health check for a feature: a change to the rate contract was an outage for every build that gated on it, and a coin-less rate request could be answered as a failure of the rails./availabilityanswers the question the gate is actually asking, never contacts the provider, and returns503when the deployment has noLINQ_API_KEY.Pricing — via the existing
ngnPerCoin, shared with the onramp. No second price path, no new arithmetic.The rollout, and where each stage stands
coincoinusdc, logged deprecatedcoin; the gate moves to/offramp/availabilityStage 3 needs the released APKs to be out and
logCoinlessRateRequestto have gone quiet in the production log for as long as it takes an install to update. Neither is true yet, so the coin-less branch stays.Caller audit
/offramp/ratehas two callers, not one:frontend/wallet/lib/offramp.ts(web) andfrontend/mobile/lib/offramp.ts(Expo). Both are updated in veil#1124, which also moves both availability probes off the rate endpoint.cli/bin/wraith.jsand the Python/Go SDKs do not touch the offramp routes.Tests
coinrequired-shape cases: unknown coin 400s, missing coin answers asusdc, and the deprecation log fires once perUser-Agent(test seam__resetCoinlessRateLog)./offramp/availabilityanswers without a provider call;503when the key is absent.Integration testspassed on runs39138211341and39140948244.All rate tests mock
global.fetch. No test reaches Linq: the naira rails are mainnet-only with no sandbox.CI work that came with this
None of these were caused by the offramp change; three of them were red on
mainfirst, and the branch inherits them because it touchesopenapi.json.2f82272— the integration list cases still asserted the response shape from before perf(api): drop the default COUNT from list queries, add hasMore and opt-in includeTotal #218 dropped the defaultCOUNT. Fixed by asking for the count (includeTotal=true) where a case asserts one, and asserting the page instead where it does not.c5b5b36— the dual-network harness never told/statusit runs two chains, so it read back testnet only.81293b6—clients/react-query/src/schema.d.tswas stale againstopenapi.jsonby three routes (/ngn/customers/status,/fees, and/status's per-network object). Red onmainsince/feeswas documented. Regenerated from the generator's own diff rather than hand-written.40a3b42— the one that matters:npm testwas not slow, it was stuck. A case added onmaincallsstartAllIndexers(["testnet","mainnet"])to check which networks it would run, gets its answer, and leaves behind a loop that crashes on the absent database, schedules a restart 10s later, crashes again, forever. Jest waits on the process, so the run never ends and never reports; the suite finishes in 22 seconds.e746aa4gave the step a deadline to make that readable,40a3b42fixes the cause by unref'ing the restart wait and the inter-poll pause (a crash-restart is not a reason to keep a process alive — the API listening is), and_resetIndexerLoopsnow cancels pending restarts.e746aa4's workflow edit is reverted in3dd5691: a PR that changes a workflow file holds every later commit for approval, which would leave this fix unverified for a reason unrelated to naira rates. The deadline belongs in its own PR.Notes for reviewers
coinvalidation againstGET /onramp/rate; the error shape is intentionally identical.cointoday, the cost is every installed build that has not updated, and the log line says what that count currently looks like.cli/bin/wraith.js:116printsShowing ${result.transfers.length} of ${result.total} total transfers, which has readof undefined totalsince perf(api): drop the default COUNT from list queries, add hasMore and opt-in includeTotal #218 madetotalopt-in. No CI covers the CLI, so it is invisible to the suite. Happy to take it as a separate issue.node/npm, so every claim above about green is a CI run, and the one run that has not executed is called out as unverified.