Skip to content

Fix #187: Make the offramp order endpoint idempotent and add zod validation - #210

Open
Glorious21 wants to merge 1 commit into
Miracle656:mainfrom
Glorious21:fix/issue-187-offramp-idempotency
Open

Glorious21 wants to merge 1 commit into
Miracle656:mainfrom
Glorious21:fix/issue-187-offramp-idempotency

Conversation

@Glorious21

Copy link
Copy Markdown
Contributor

Fixes #187. Added zod validation for amounts and P2002 error catching for idempotency.

@drips-wave

drips-wave Bot commented Sep 29, 2026

Copy link
Copy Markdown

@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! 🚀

Learn more about application limits

@Miracle656 Miracle656 left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 id

orderId 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 yields NaN, and z.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 amountSchema is reused for both amountNGN and amountStableCoin. 10,000,000 is a sane naira ceiling and a meaningless USDC one; they want separate bounds.
  • The zod bump 4.4.3 → 4.6.5 in package.json/package-lock.json isn'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.

Miracle656 added a commit that referenced this pull request Oct 1, 2026
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.
@Glorious21
Glorious21 force-pushed the fix/issue-187-offramp-idempotency branch from 5b19f38 to 5ba5a8e Compare October 16, 2026 14:24
@Glorious21

Copy link
Copy Markdown
Contributor Author

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 offramp.test.ts replacement with its jest.mock("../../db") stub). The tests are now added to the existing src/__tests__/routes/offramp.test.ts and use its in-memory db mock, so the real credential path is still exercised. The zod bump is gone, so package.json and the lockfile are untouched.

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 200, replayed: true, id is our ofr_ id (never the provider's), a fresh accessToken comes from issueOrderCredentials after the update, and Cache-Control: no-store is set. If the winning row cannot be read back it falls through to the generic 500 with no database text.

3. Key bound to the body. The row now stores requestHash, an HMAC-SHA256 over the amounts, bankAccount, bankCode, accountName, refundAddress and walletAddress. A replay that differs gets 409 "This idempotencyKey was used with different order details" (same class as the wallet-mismatch 409), and the first order's token is left untouched. One deviation from the suggested shape: it is keyed with LINQ_API_KEY rather than a bare hash, because a plain hash of a ten-digit account number can be reversed by trying them all, and this table deliberately keeps no bank details. The cost is that rotating that key makes a retry of an order created before the rotation 409 once; this is noted in docs/offramp-orders.md. The column is nullable (added by prisma db push, which the server runs at boot), and rows without it are held to the amount check only, as before. I did not backfill it on replay, since that would adopt whatever the replay asked for.

Smaller things.

  • MAX_OFFRAMP_AMOUNT is replaced by OFFRAMP_MAX_AMOUNT_NGN (default 10,000,000) and OFFRAMP_MAX_AMOUNT_STABLECOIN (default 10,000). A value that is not a positive finite number falls back to the default instead of becoming NaN.
  • Amounts must be a JSON number or a plain decimal string. "abc", negatives, zero, "Infinity", "1e999", booleans, arrays and objects get a 400 before any lookup or provider call, and one of amountNGN / amountStableCoin is now required.

Verification. src/__tests__/routes/offramp.test.ts: 49 pass (the 22 existing plus 27 new); 23 of the new ones fail against main's router. tsc --noEmit is clean. The full jest run is green apart from openapiCoverage.test.ts, which fails because GET /fees is not in the spec on main (that is #239, unrelated to this change). Docs updated in docs/offramp-orders.md and docs/ngn-rails.md.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Make the offramp order endpoint idempotent, and test it

2 participants