Repository navigation
Fix #187: Make the offramp order endpoint idempotent and add zod validation - #210
Glorious21 wants to merge 1 commit into
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. |
Fixes #187. Added zod validation for amounts and P2002 error catching for idempotency.