fix(l2ps): handle post-fork OS-magnitude amounts losslessly in L2PSTransactions + batch aggregator#873
Conversation
…ansactions + batch aggregator Companion to #863 (top-level `transactions` widening) and #872 (`fromEntityToWireNumber` post-fork string fallback). Audit of every remaining `Number.MAX_SAFE_INTEGER` and `Number(bigint)` site that touches a money field found two more places that silently corrupt or crash on post-fork OS values: 1) `L2PSTransactions.amount` was `@Column("bigint")`, same `int8` cap as the top-level `transactions.amount` widened in #863. L2PS replays per-tx amounts inside aggregated L1 batches, so a 10 % move out of a 10^18 DEM wallet (10^26 OS) overflows the same way the top-level table did before #863. Widened to `numeric(38, 0)` with the shared `bigintNumericTransformer`. New migration `1779834500000-WidenL2PSTransactionsAmountToNumeric` matches the shape of `1779834000000-WidenTransactionsMoneyColsToNumeric`. 2) `L2PSBatchAggregator.zkTransactions` was doing `BigInt(Math.floor(Number(rawAmount)))` for every tx amount. The wire shape is `string | number | bigint`; routing the post-fork decimal-string path through `Number()` first silently truncates to the nearest double-precision value (10^26 OS becomes 10^26 minus ~70 low bits of junk) before `BigInt()` is even reached. Rewrote the coercion to handle each input type directly: `bigint` and `number` pass through, decimal-integer strings go straight to `BigInt()`, fractional strings are truncated at the decimal point. No lossy `Number()` round-trip on the wide path. Both are pre-existing post-fork overflow bombs that would have fired the first time an L2PS batch carried a real founder-wallet transfer. Audit of remaining `MAX_SAFE_INTEGER` sites confirms the others are either dead code (`subOperations.transferNative` — no live caller), seed values for `reduce()` (`l2ps_hashes.ts`), block-number caps (`omniprotocol/sync.ts`), or legacy-GCR migration internals that no longer ship as a live runtime path.
Qodo reviews are paused for this user.Troubleshooting steps vary by plan Learn more → On a Teams plan? Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center? |
|
Warning Review limit reached
More reviews will be available in 50 minutes and 41 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Greptile SummaryThis PR fixes two post-fork overflow paths in the L2PS subsystem: the
Confidence Score: 4/5Safe to merge; both fixes are narrowly scoped to prevent post-fork OS-magnitude amounts from crashing the consensus loop or silently truncating through a double-precision cast. The column widening and migration are clean and consistent with the companion change in #863. The BigInt dispatch in L2PSBatchAggregator correctly eliminates the lossy Number() round-trip for the expected wire shapes. The one residual concern is that scientific-notation strings (e.g. "1e+26") produced by JS's own Number.toString() for values above 10^21 will fall through to the catch block and silently produce 0n rather than a diagnosable error. src/libs/l2ps/L2PSBatchAggregator.ts — the string branch of the amount dispatch has no explicit handling for scientific-notation strings; anything matching /[eE]/ will throw inside BigInt() and silently become 0n via the catch block. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A["rawAmount from encrypted_tx.content.amount"] --> B{typeof rawAmount}
B -->|"undefined or null"| C["amount = 0n"]
B -->|"bigint"| D["amount = rawAmount"]
B -->|"number"| E{isFinite?}
E -->|No| F["amount = 0n"]
E -->|Yes| G["amount = BigInt(Math.trunc(rawAmount))"]
B -->|"string or other"| H["s = String(rawAmount).trim()"]
H --> I{has decimal point?}
I -->|Yes| J["strip after dot"]
I -->|No| K["use s as-is"]
J --> L["BigInt(s)"]
K --> L
L --> M{valid decimal integer?}
M -->|Yes| N["amount = BigInt result"]
M -->|"No e.g. 1e+26"| O["catch block: amount = 0n"]
C & D & G & F & N & O --> P["ZK tx: senderBefore=amount, senderAfter=0n, receiverAfter=amount"]
Reviews (1): Last reviewed commit: "fix(l2ps): handle post-fork OS-magnitude..." | Re-trigger Greptile |
| } else { | ||
| // string path — accept decimal-integer strings as | ||
| // BigInt() does natively; reject fractional strings | ||
| // by truncating at the decimal point if present. | ||
| const s = String(rawAmount).trim() | ||
| const i = s.indexOf(".") | ||
| amount = BigInt(i >= 0 ? s.slice(0, i) : s) | ||
| } |
There was a problem hiding this comment.
The else (string) branch trims and passes s directly to BigInt(), but BigInt() does not accept JavaScript scientific-notation strings (e.g. "1e+26"). If rawAmount is a string produced by someNumber.toString() for a value greater than 10^21 — which is exactly how a JS runtime serialises large floats to string — BigInt("1e+26") throws a SyntaxError, the catch block fires, and amount becomes 0n. The amount is then silently dropped from the ZK batch proof. The pre-fix path would at least emit a recognisably wrong (lossy double) value; 0n is harder to detect post-hoc. A simple guard (e.g. detecting e/E in the trimmed string) would convert the silent loss into a diagnosable failure.
Companion to #863 (top-level
transactionswidening) + #872 (fromEntityToWireNumberpost-fork string fallback). Audit found two more post-fork overflow bombs:1. L2PSTransactions.amount widened
@Column("bigint")→@Column("numeric(38, 0)")with shared transformer. L2PS replays per-tx amounts inside aggregated L1 batches, so a 10 % move out of a 10^18 DEM wallet (10^26 OS) overflows int8 the same way the top-level table did. Migration1779834500000mirrors1779834000000.2. L2PSBatchAggregator BigInt(Math.floor(Number(rawAmount))) anti-pattern
The wire shape is
string | number | bigint. Routing the post-fork decimal-string throughNumber()first silently truncates to the nearest double-precision value (10^26 OS becomes 10^26 minus ~70 low bits of junk) beforeBigInt()is even reached. Rewritten to handle each input type directly: bigint/number pass through, decimal-integer strings go straight toBigInt().Audit results for remaining MAX_SAFE_INTEGER sites
subOperations.transferNativel2ps_hashes.ts:220reduce()omniprotocol/sync.ts:146forks/migrations/osDenomination.tstransformers.tsBoth fixed bombs would have fired the first time an L2PS batch carried a real founder-wallet transfer. Same fix shape as #863.