feat: balance pre-checks for L2PS transactions (rebased onto stabilisation)#937
Merged
Merged
Conversation
…ient funds before processing
Correctness — fee-bearing scope
- handleL2PS.ts (checkSenderBalance) and validateTransaction.ts
(checkL2PSBalance) both unconditionally added `L2PS_TX_FEE` to the
required-balance check, but the executor only burns that fee inside
`L2PSTransactionExecutor.handleNativeTransaction()` for
`nativeOperation === "send"`. The pre-checks therefore rejected
every non-`native/send` L2PS payload (web2, crosschain, gcr_edits-
only) with a false-negative balance error. Now the fee is added
only when the inner tx is genuinely fee-bearing, and
`sendAmount` is shape-validated up front instead of being silently
coerced to 0 on a malformed payload.
Correctness — fail-closed validate path
- validateTransaction.ts previously returned `null` on every "cannot
verify" outcome (no l2ps_uid, L2PS not loaded, decryption error).
`confirmTransaction` reads `null` as "no balance error", which means
the node signed and broadcast `ValidityData { valid: true }` for txs
it never actually verified — a fail-open hole that turned every
operational failure into a stamp of approval. Every previously-`null`
failure path now returns a precise `[BALANCE ERROR]` string so
`confirmTransaction` surfaces it instead of vouching for the tx.
Correctness — TOCTOU between balance check and executor debit
- handleL2PS.ts now funnels every `(check + insert + execute)`
sequence through a per-sender in-process lock (`withSenderLock`).
Without it, two concurrent broadcasts from the same wallet both
read the same pre-debit balance and both pass the gate; the
executor then debits twice from a wallet that had funds for one.
In-process serialisation is sufficient because every L2PS tx for a
given sender arrives through one node entry point; cross-node
ordering is handled downstream by the mempool + consensus pipeline.
The lock map drops entries once a sender's queue drains, so
long-lived senders don't leak entries.
Reliability — safe failure-path logging
- manageExecution.ts:84 previously called `JSON.stringify(returnValue.extra)`
in the broadcastTx failure-log branch. If `extra` carried a circular
reference or a `BigInt`, stringify itself would throw, abort the
error-handling path, and leave the client without any response.
Replaced with a `safeStringifyExtra()` helper that serialises BigInts
as `"123n"` and degrades to `<unserialisable: ...>` instead of
throwing.
Net `tsc --noEmit --skipLibCheck` delta on this branch:
baseline `origin/testnet`: 36 pre-existing errors
this branch: 36 — 0 net new
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
After rebasing onto stabilisation, two type drifts surfaced in the native-amount balance branch: - `tx.content.amount` was widened from `number` to `number | string` by the v4 bigint-widening work; the literal `> 0` and `<` comparisons no longer type-check, and a plain `Number()` cast would round any post-fork OS magnitude. Coerce through `BigInt(... ?? 0)` so the comparison stays precise for the full balance range. - `GCR.getGCRNativeBalance` was renamed to `getAccountBalance` on stabilisation and now returns `bigint` directly. Match the new signature. Behaviour preserved: any tx with positive `amount` whose sender lacks the required balance still gets rejected with the same `[BALANCE ERROR]` message, only with bigint-precise numbers. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Contributor
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? |
Contributor
|
Warning Review limit reached
More reviews will be available in 53 minutes and 32 seconds. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ 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 (5)
WalkthroughThis PR implements pre-signing balance validation for Layer 2 Protocol Service (L2PS) encrypted transactions and standard transactions. The L2PS fee constant is exported, transaction validation gains GCR balance checks and L2PS-specific decryption+balance verification, and the L2PS handler introduces concurrent per-sender processing with upfront balance validation to prevent insufficient-fund errors. ChangesL2PS Balance Validation
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 adds sender balance pre-checks at two enforcement points —
Confidence Score: 4/5Safe to merge for the balance-enforcement goal; the TOCTOU lock and OS-unit canonicalization are correctly implemented. The main rough edge is that The src/libs/blockchain/routines/validateTransaction.ts — Important Files Changed
Sequence Diagram%%{init: {'theme': 'neutral'}}%%
sequenceDiagram
participant Client
participant confirmTx as confirmTransaction
participant handleL2PS as handleL2PS
participant balanceCheck as checkInnerTxBalance
participant Lock as withSenderLock
participant Executor as L2PSTransactionExecutor
Client->>confirmTx: l2psEncryptedTx (confirmTx)
confirmTx->>confirmTx: verify outer tx signature
confirmTx->>confirmTx: checkL2PSBalance(tx)
Note over confirmTx: decryptTx (no inner sig verify)
confirmTx->>balanceCheck: checkInnerTxBalance(decryptedTx)
balanceCheck->>Executor: getBalance(sender)
Executor-->>balanceCheck: balance (OS units)
balanceCheck-->>confirmTx: "null | error"
confirmTx-->>Client: signed ValidityData
Client->>handleL2PS: l2psEncryptedTx (broadcastTx)
handleL2PS->>handleL2PS: decryptAndValidate (+ inner sig verify)
handleL2PS->>Lock: withSenderLock(sender)
Lock->>balanceCheck: checkInnerTxBalance(decryptedTx)
balanceCheck->>Executor: getBalance(sender)
Executor-->>balanceCheck: balance (OS units)
balanceCheck-->>Lock: "null | error"
Lock->>Executor: execute (debit applied)
Executor-->>Lock: result
Lock-->>handleL2PS: result
handleL2PS-->>Client: RPCResponse
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
sequenceDiagram
participant Client
participant confirmTx as confirmTransaction
participant handleL2PS as handleL2PS
participant balanceCheck as checkInnerTxBalance
participant Lock as withSenderLock
participant Executor as L2PSTransactionExecutor
Client->>confirmTx: l2psEncryptedTx (confirmTx)
confirmTx->>confirmTx: verify outer tx signature
confirmTx->>confirmTx: checkL2PSBalance(tx)
Note over confirmTx: decryptTx (no inner sig verify)
confirmTx->>balanceCheck: checkInnerTxBalance(decryptedTx)
balanceCheck->>Executor: getBalance(sender)
Executor-->>balanceCheck: balance (OS units)
balanceCheck-->>confirmTx: "null | error"
confirmTx-->>Client: signed ValidityData
Client->>handleL2PS: l2psEncryptedTx (broadcastTx)
handleL2PS->>handleL2PS: decryptAndValidate (+ inner sig verify)
handleL2PS->>Lock: withSenderLock(sender)
Lock->>balanceCheck: checkInnerTxBalance(decryptedTx)
balanceCheck->>Executor: getBalance(sender)
Executor-->>balanceCheck: balance (OS units)
balanceCheck-->>Lock: "null | error"
Lock->>Executor: execute (debit applied)
Executor-->>Lock: result
Lock-->>handleL2PS: result
handleL2PS-->>Client: RPCResponse
Reviews (3): Last reviewed commit: "review: address bot findings on PR #937" | Re-trigger Greptile |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/libs/blockchain/routines/validateTransaction.ts`:
- Around line 276-304: The balance-check logic duplicates validation and wrongly
assumes sendAmount is a number; extract a shared helper (e.g.,
L2PSTransactionExecutor.parseAmountOrThrow or a util parseL2PSAmount) that
accepts number|string, converts to BigInt safely (for numbers ensure
integer/non-negative, for strings allow large integers via BigInt), rejects
non-integer/negative values with a clear error, and returns BigInt; update
validateTransaction's check (currently in the block that reads
nativePayload.args and computes amount/fee) and handleL2PS.checkSenderBalance to
call this helper and compare BigInt(totalRequired) with the BigInt balance from
L2PSTransactionExecutor.getBalance to avoid type-mismatch and duplicated logic.
- Around line 276-304: The native send amount check currently rejects string
amounts; update the block that reads nativePayload.args so sendAmount can be
number|string and is converted using the same canonicalizer used in
L2PSTransactionExecutor.handleNativeTransaction (call
canonicalizeAmountToOs(sendAmount)) before validation and assignment to amount;
keep the checks for numeric/finite/non-negative after canonicalization, compute
fee/totalRequired as before, and then compare balance using
L2PSTransactionExecutor.getBalance(sender) as currently implemented (ensure
canonicalizeAmountToOs is imported/available).
In `@src/libs/network/routines/transactions/handleL2PS.ts`:
- Around line 141-158: handleL2PS.ts incorrectly assumes the inner tx amount is
a number (casts args to [string, number] and checks typeof sendAmount ===
"number"), which rejects valid post-fork string amounts and duplicates logic
from validateTransaction.ts; update the parsing in the fee-bearing branch of
handleL2PS (where feeBearing, sendAmount, amount are used) to accept string or
number (change the args typing to [string, string|number] or treat sendAmount as
string|number), normalize/parse string amounts into a numeric value (or BigInt
if required by L2PSTransactionExecutor.getBalance comparisons) with proper
validation (finite, non-negative), and factor this normalization into a shared
helper used by both handleL2PS and checkL2PSBalance in validateTransaction.ts to
avoid duplication and ensure consistent behavior.
- Around line 105-110: The cleanup check always fails because previous.then(()
=> next) creates a fresh Promise object so strict equality with the stored map
value never matches; fix by creating and storing the exact Promise instance you
later compare against: when you set senderLocks.set(sender, <promise>) (the
promise created from previous.then(() => next)), capture that promise in a local
variable (e.g., const expected = previous.then(() => next)) and use
senderLocks.set(sender, expected) and then compare senderLocks.get(sender) ===
expected before calling senderLocks.delete(sender); update the code around the
senderLocks set/get logic (symbols: senderLocks, sender, previous, next) so the
same Promise object is used for both storage and comparison.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 620b154f-f66b-4f45-8010-eb7fdeb76c99
📒 Files selected for processing (4)
src/libs/blockchain/routines/validateTransaction.tssrc/libs/l2ps/L2PSTransactionExecutor.tssrc/libs/network/manageExecution.tssrc/libs/network/routines/transactions/handleL2PS.ts
P1 — DEM/OS unit mismatch made the L2PS balance gate a silent no-op post-fork
Both `checkSenderBalance` and `checkL2PSBalance` were adding
`L2PS_TX_FEE` (DEM-unit `1`) and `sendAmount` (DEM units) but
comparing against `L2PSTransactionExecutor.getBalance()` which returns
the post-osDenomination OS-magnitude value (~10⁹× larger). A wallet
with `0.5 DEM` passed the pre-check (`5×10⁸ >= 1`) and only failed at
execution time, which is exactly what the gate was supposed to
prevent. Hoisted both call sites onto a new shared helper
`src/libs/l2ps/balanceCheck.ts::checkInnerTxBalance` that canonicalises
amount + fee to OS via `canonicalizeAmountToOs` — identical to what
the executor does for the actual debit — so the pre-check and the
debit can never disagree on units again. Also handles `number | string`
send-amount shapes (the v4 bigint widening surfaced strings here).
P1 — withSenderLock map leaked unboundedly
The cleanup-time identity check compared `senderLocks.get(sender)`
against a freshly-allocated `previous.then(() => next)`, but each
`.then()` call produces a new promise object. The strict-equality
check could never be true, so the `delete` line was dead code and the
map grew one entry per sender forever. Stash the chained promise in
a local const and reuse it on both the write and the cleanup-time
check, so we compare the same object.
P1 — `BigInt(tx.content.amount)` could crash confirmTransaction
An adversarial / misbehaving client shipping a fractional or
non-numeric `amount` would hit `BigInt(1.5)` / `BigInt("1.5")` and
throw an uncaught `RangeError`/`SyntaxError`, breaking the entire RPC
validation pipeline instead of returning a signed-invalid
ValidityData. Wrapped the BigInt cast in a try/catch that
short-circuits with a clean `[AMOUNT ERROR]` reason.
P1 — `result.response?.extra` still went through implicit toString()
The original safe-stringify fix in `manageExecution.ts:84` only
guarded `returnValue.extra`. The same log line still interpolated
`result.response?.extra` via a template literal — implicit `toString()`
on a BigInt or circular-ref object would re-throw and abort the
error-handling branch. Routed it through `safeStringifyExtra` too.
Refactor — collapse duplicated balance-check
Both `checkSenderBalance` (handleL2PS) and `checkL2PSBalance`
(validateTransaction) had near-identical logic that shipped the same
unit-mismatch bug independently. They now share `checkInnerTxBalance`;
the call sites just unwrap their respective decryption paths and
delegate. Drops ~80 lines of duplication and keeps the invariant
enforced in one place.
`tsc --noEmit --skipLibCheck` delta on this branch:
baseline `origin/stabilisation`: 21 pre-existing errors
this branch: 21 — 0 net new
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Contributor
Author
|
Bot review threads on this PR are stale relative to the latest commits — all findings have been addressed in code. Re-triggering the bots so the resolved threads reflect the actual state. @coderabbitai review |
Contributor
|
✅ Action performedFull review finished. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Rebased follow-up to #680 (which was opened in Feb against the old
testnetbase and has been sitting cold for 4 months). The two original feat + review commits are cherry-picked onto currentstabilisation; a third commit re-aligns the typing where stabilisation moved on (tx.content.amountwidened tonumber | string,GCR.getGCRNativeBalancerenamed togetAccountBalance).Closes #680.
What this brings up
confirmTransaction—tx.content.amount > 0paths now reject senders without sufficient funds before signingValidityData. Adjusted to BigInt comparisons for post-fork OS magnitudes.confirmTransaction— decrypts the inner tx, verifies sender balance, fee added only onnative/sendper executor parity. Fail-closed: any "cannot verify" outcome (missing l2ps_uid, L2PS not loaded, decryption error) returns a precise error instead of silently passing.checkSenderBalanceinhandleL2PS— same fee-scoping; serialised per-sender via in-processwithSenderLockto close the TOCTOU window between balance read and executor debit.manageExecution—JSON.stringifyofreturnValue.extracould throw on circular refs / BigInts and abort the client response. Replaced with a guardedsafeStringifyExtrathat survives both.Review history baked in
This PR ships #680's content with the bot-review fixes already applied:
L2PS_TX_FEEonly added when the inner tx isnative/send.[BALANCE ERROR]string instead ofnull.JSON.stringifyreplaced withsafeStringifyExtrathat handles circular refs and BigInt.Type-check delta
origin/stabilisationTest plan
BALANCE ERRORbefore signingL2PS network not loadedand submit l2psEncryptedTx viaconfirmTransaction→ expect explicitBALANCE ERROR(fail-closed)🤖 Generated with Claude Code
Summary by CodeRabbit