Repository navigation
Fix #187: Make the offramp order endpoint idempotent and add zod validation - #210
Conversation
|
@Glorious21 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.
Thanks for this — the zod amount guard is a real improvement and the P2002 race is the right thing to have spotted. But the branch is cut from 07e6f72, which is before #216 landed, and #216 rewrote exactly this path. That makes most of the diff either already-done or actively backwards against what's on main now. Asking for a rebase plus two fixes.
1. The replay path you're adding already exists on main, and main's is stricter
src/api/offramp.ts on main already does the findUnique on network_idempotencyKey, already 409s when a different wallet reuses the key, and already re-issues the order credentials on replay before returning replayed: true. Please rebase onto current main and keep only what's genuinely new (the zod guard and the P2002 fallback).
Same for the tests: src/__tests__/routes/offramp.test.ts now exists on main at 436 lines (it's where "re-issues the token, keeps the id, and does not create a second order" and "gives nothing away to another wallet reusing the key" live). Your 152-line file is an add/add conflict with it — the PR currently shows CONFLICTING. Fold your five cases into the existing file rather than replacing it, and drop the jest.mock("../../db") wholesale stub: the file on main drives the real code path and your mock would hide the credential logic the other 22 tests are asserting.
2. src/api/offramp.ts lines 247–266 — the P2002 fallback leaks the provider's order id and returns no token
res.status(200).json({
id: existingAfterCreate.orderId, // <-- Linq's idorderId is the provider's id. #216 made the rule that it never leaves the server — clients get publicId, and GET /offramp/orders/:id only accepts that shape (isOrderId). Two consequences here: the provider id leaks, and the client gets back an id that the lookup route will reject, with no accessToken at all, so it can never read the order it just paid into. The success path four lines above and the replay branch both go through issueOrderCredentials — the P2002 branch needs to do the same, i.e. id: creds.orderId + accessToken: creds.token after an update, and res.setHeader("Cache-Control", "no-store") like its neighbours.
While you're there: the fallback should also apply main's wallet-ownership check before answering, or the race window becomes a way to read someone else's order by guessing their key.
3. The key still isn't bound to the request body
This is the one I'd most like to see, given the PR title. Right now — on main and after this PR — a replay only has to match walletAddress. Send the same idempotencyKey with a different amountNGN, or a different bankAccount/bankCode, and you get 200 replayed: true carrying the first order's figures. The caller reasonably reads that as "my ₦X payout to account Y was accepted" when neither the amount nor the destination is the one they just sent. On a router that moves real naira that's the failure worth closing.
Suggested shape: store a hash of the idempotency-relevant fields (amountNGN/amountStableCoin, bankAccount, bankCode, accountName, walletAddress) alongside the row, and on replay 409 with something like "This idempotencyKey was used with different order details" when the hash differs — same refusal class as the existing wallet-mismatch 409. Replay stays replay only when the request is genuinely identical.
Smaller things
- Line 156:
Number(process.env.MAX_OFFRAMP_AMOUNT || 10_000_000)— a non-numeric env value yieldsNaN, andz.number().max(NaN)fails every amount, so a typo in config takes cash-out down silently. Fall back to the default when the parse isn't finite. - Line 157: one
amountSchemais reused for bothamountNGNandamountStableCoin. 10,000,000 is a sane naira ceiling and a meaningless USDC one; they want separate bounds. - The
zodbump4.4.3 → 4.6.5inpackage.json/package-lock.jsonisn't needed for anything here — please drop it so the diff stays about the router.
CI is green on the branch, so none of this is a test failure — it's the rebase that changes the picture. Happy to re-review as soon as it's on current main.
Found while reviewing #210, but this is a bug on `main`, not in the PR. `POST /offramp/orders` looks an order up by `(network, idempotencyKey)` and, once the wallet matches, returns 200 with `replayed: true` — carrying the **first** order's amount, rate and deposit address. It never compared the amounts. So a caller that reused a key with a changed amount was answered 200 { amountNGN: <the OLD figure>, replayed: true } and reads that as "my payout was accepted". The money follows the original order. On the only money-moving router in this service, a silent success for a request nobody made is the worst available outcome. An idempotency key promises "this key means this request". Returning a different request's result under a 200 breaks that promise quietly, which is the part that matters — a 409 is recoverable, a wrong confirmed amount is not. Now a mismatch is refused in the same class as the existing wallet-mismatch 409. Compared numerically, because both sides are decimal strings: "50" and "50.00" are the same order and must not 409, and there is a test for that so the guard cannot be tightened into a false refusal later. **Partial, deliberately.** `OfframpOrder` stores the amounts but not `bankAccount`, `bankCode`, `bankName` or `accountName`, so a replay that changes only the *destination* still passes. Closing that needs a stored hash of the idempotency-relevant fields — a new column and a migration, on a database this service shares with Lens that is already near its tier's storage limit, and whose deploy has not run since 2026-09-23. That is a decision to take deliberately rather than a fix to slip into a review pass, so it is called out in the code comment rather than guessed at. Verified: reverting only the source change leaves the new test failing (1 failed / 21 passed); with it, 22 pass. Full suite 47 suites / 564 tests, `tsc --noEmit` clean. (`npx prisma generate` is required first or the client is stale and `publicId` appears to not exist.) Claude-Session: https://claude.ai/code/session_01USgemLt4Rnz4SGB1Srf3GB
…ce as a replay, bind the key to the request Rebased onto main after Miracle656#216. The replay path, wallet check and amount check already live there, so this keeps only what was missing: zod amount validation with separate naira and stablecoin bounds and a safe fallback for a bad env value; a unique-violation fallback that goes through the same replay checks and returns our id and a fresh token; and a keyed fingerprint of the request stored with the order so a replay that changes the destination is refused. The zod version bump is dropped.
5b19f38 to
5ba5a8e
Compare
|
Rebased onto current main (it is now a single commit on top of 417d31d) and reworked per the review. CI should be able to run again; the conflict is gone. 1. Rebase / what is kept. I dropped everything that main already does after #216 (the replay lookup, the cross-wallet 409, credential re-issue, and the 2. P2002 fallback. The replay logic is now one helper used by both the up-front lookup and the lost-race insert, so a concurrent duplicate gets exactly the sequential checks: wallet ownership, amount, then the request fingerprint. It answers 3. Key bound to the body. The row now stores Smaller things.
Verification. |
|
Merged. This is careful work on the one router where carelessness costs real money, and the reasoning in the comments is doing real work — particularly replacing your own earlier The parts I checked hardest and agree with:
Two things I fixed while merging, so you know what changed1. The migration was missing — this was the blocker. Worth internalising for next time: on this repo a 2. An additive test conflict. One known limitation, not blockingThe fingerprint HMACs with Verified on the merge result rather than on your branch: |
… did not hold
Pins each Linq rail's actual replay behaviour against the fake provider, and
documents it in `docs/idempotency.md`. The finding is the valuable part:
- offramp orders carry an idempotencyKey, and Linq is believed to return the
original order for a key it has seen. Believed, not documented to us.
- customers are keyed on customerRef, and Linq returns the existing one.
- onramp orders and bills carry NO key. A retry after a lost answer places a
second, unpaid order, and the caller is given the second.
`retryOnProviderFailure` wraps all of them, so that third case is a live edge
rather than a theoretical one.
**`src/linq/http.ts` said otherwise, and was wrong.** Its comment read "The
retry sends the identical body, idempotencyKey included, so the provider can
recognise it as the same order" — true for the offramp, false for onramp orders
and bills, which carry no key at all. That is exactly the sentence that would
let someone wrap a fourth unkeyed call in the same helper. Replaced with this
PR's three-case account.
Two resolutions while merging:
- `src/linq/client.ts` conflicted because the branch re-adds
`retryOnProviderFailure` there, where it lived before #261 consolidated the
transport into `src/linq/http.ts`. Kept `main`'s layout and moved the
improved documentation to where the function now lives.
- `src/__tests__/routes/idempotency.test.ts` asserted the concurrent-duplicate
loser receives a 500, pinned as a known edge. #210 closed that edge earlier
today — the route now catches the unique violation, reads the winning row
back and answers through the same replay path, so the loser gets 200 and the
order. Assertion and name updated to the new behaviour, with the history in
the comment.
That second one is the suite doing its job: it noticed a deliberate behaviour
change the day it landed.
Verified on the merge result: `tsc --noEmit` silent, 1,025 tests pass across 68
suites.
Closes #190
Claude-Session: https://claude.ai/code/session_01USgemLt4Rnz4SGB1Srf3GB
Fixes #187. Added zod validation for amounts and P2002 error catching for idempotency.