Repository navigation
fix(amount): add amount bounds and NGN/stablecoin ratio guard - #268
Nwapu-TrustJah wants to merge 9 commits into
Conversation
0a37808 to
9984c0f
Compare
Miracle656
left a comment
There was a problem hiding this comment.
Thank you for splitting this out of #254 — keeping src/api/ngn.ts untouched was exactly right, and the reasoning comment at the top of src/amount.ts is the best thing in either PR. The bound on a naira amount is justified from kobo denomination, retail limits and the 100× incident, which is how a magic number should be argued. That part I want.
I cannot merge this, though, because of what happened to src/api/linqWebhook.ts. Three things, the first serious.
1. The onramp and bill rails would stop reconciling
The import of reconcileNgnOrder is removed and the ngnOrder branch is reimplemented inline. The inline version writes two fields that do not exist on that model:
...(event.amountNGN != null ? { settledNgn: Number(event.amountNGN) } : {}),
...(event.txHash ? { depositTxHash: event.txHash } : {}),prisma/schema.prisma gives NgnOrder the fields settledNGN String? and txHash String?. There is no settledNgn, and depositTxHash belongs to OfframpOrder, not this table. reconcileNgnOrder in src/linq/ngnOrders.ts writes settledNGN: String(event.amountNGN) and txHash: event.txHash, which is why it works today.
So every onramp or bill webhook carrying an amount or a tx hash throws inside the try, and the catch only calls console.error. The response has already been sent 200, so Linq never retries. The order stays unreconciled permanently.
The reason your build was clean is worth knowing, because it will bite again. I checked it directly. Written as a plain object literal, TypeScript catches it:
error TS2561: Object literal may only specify known properties, but 'settledNgn'
does not exist in type 'NgnOrderUpdateInput'. Did you mean to write 'settledNGN'?
But in the shape this PR uses — one known key (status) present, and the bad keys arriving only through ...(cond ? { … } : {}) spreads — tsc --noEmit reports nothing. Excess-property checking does not apply to properties introduced by a spread. npm run build passing is not evidence here.
It is also the untested branch: running the suite on your head, the coverage report lists src/api/linqWebhook.ts lines 93-109 as uncovered, and that is this exact block. 32 tests pass because src/__tests__/routes/idempotency.test.ts only drives the offramp path.
Please restore reconcileNgnOrder and put the amount check inside it, or in front of the call. There is no need to inline it — and the bill case is the reason that function exists at all: an onramp arrives on-chain where the indexer already sees it, but a biller refusing to vend after the user has paid is invisible unless this runs.
2. There are now three implementations of the same check, and they contradict each other
src/amount.ts, top:sanitizeNairaAmount/crossCheckNairaAmount. Documented, handles strings and decimals, treats amounts as naira in major units, 1% tolerance. Imported nowhere. Coverage confirms lines 43-86 and 99-121 never execute.src/amount.ts, bottom:nairaScale,isNairaAmountPlausible,checkNairaAmount,crossCheckWebhookAmount. Minified onto single lines, no comments, and treats amounts as kobo. This is the one actually wired into the webhook.src/linq/webhook.ts:isPlausibleNgn,crossCheckAmountNgn,MIN_PLAUSIBLE_NGN,MAX_PLAUSIBLE_NGN,NGN_SCALE_FACTOR. Also documented, also imported nowhere.
The two cross-checks are mutually exclusive. crossCheckNairaAmount fails unless the webhook amount ≈ the order amount. crossCheckWebhookAmount fails unless w === o * 100. Only one can describe reality.
3. The wired check has the wrong unit, and would reject real orders
isPlausibleNgn's sibling isNairaAmountPlausible requires Number.isInteger(value) and value >= 100*100, i.e. it reads the value as kobo. amountNGN is naira in major units — the column is Decimal(10,2), and the existing tests pass 2000, 82750, "1000", "82750".
Against the live rail that means:
- a legitimate ₦2,000 onramp →
2000 < 10000→ rejected amountNGN: "1000"as a string →typeof value !== "number"→ rejected- any amount with kobo, e.g.
1500.50→Number.isIntegerfalse → rejected
Your own documented sanitizeNairaAmount handles all three correctly. It is the right function; it just is not the one plugged in.
Separately, the ratio guard in linqWebhook.ts uses ratio > 2 || ratio < 1/2. A 2× band lets a 1.9× discrepancy through, which does not match the 1% tolerance your crossCheckNairaAmount documents.
Smaller things
- The rewrite deletes about 40 lines of comment from
linqWebhook.ts, including theMUST be mounted before express.json()warning (Linq signs the raw body, so re-serialising it breaks verification), the "acknowledge first, reconcile after — Linq times out at 10 seconds" rationale, and the "last delivery wins" caveat. Please keep these; they are each a bug someone already paid for. src/linq/webhook.tshas mojibake in the new comment:₅100,000,000and…100where₦and-were meant.src/api/linqWebhook.tsloses its trailing newline.const network = DEFAULT_NETWORKis now assigned and never read; main used it asexisting.network ?? network.toDisplayAmountcallsBigInt(amount), which throws rather than returning on a non-numeric string. Worth a guard since the rest of the module is careful to return a result instead of throwing.- Moving
buildtoprisma generate && tscis a genuine improvement and I would like it — but please send it on its own, since it changes the build for everyone and has nothing to do with amount bounds.
What I would merge
One implementation, in src/amount.ts, keeping your documented sanitizeNairaAmount and crossCheckNairaAmount and deleting the other two sets. Called from reconcileNgnOrder and from the offramp branch, with src/api/linqWebhook.ts otherwise byte-identical to main including its comments. Tests driving the ngnOrder path, not only the offramp one, so lines 93-109 are covered. That is a much smaller diff than this and it closes the issue properly.
Also: you have #254 open on the same ground. Pick whichever you want to carry forward and close the other, so there is one place to review — your call which.
Summary
Introduces amount boundary checks and NGN-to-stablecoin ratio validation to guard against provider unit discrepancies (such as 100× unit errors).
Related issue
Type of change
Details
src/amount.tsand unit tests insrc/__tests__/amount.test.ts.src/linq/webhook.tsandsrc/api/linqWebhook.ts.src/api/ngn.tsstructure intact without bundling refactor changes.Operator Handling for Webhooks
If an incoming webhook fails amount validation after funds have already moved at the provider end, the order is flagged/logged for manual operator review rather than being left in an unresolved pending state.
Verification
npm test -- src/__tests__/amount.test.ts)npm run buildsucceeds with 0 errors