fix(l2ps): canonicalise inner native amount across the osDenomination fork#926
Conversation
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? |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughL2PSTransactionExecutor now enforces fork-aware denomination handling in native "send" transactions. Amount and fee canonicalization to OS units is gated by fork activation status, balance validation occurs at OS magnitudes, and generated GCR edits reflect fork-dependent amount formats. Error handling prevents invalid amounts from proceeding. ChangesFork-aware denomination handling in L2PS transactions
Sequence DiagramsequenceDiagram
participant Client
participant L2PSTransactionExecutor
participant SharedState
participant Denomination
participant GCR
participant L1Account
Client->>L2PSTransactionExecutor: submit native send(rawAmount, receiver)
L2PSTransactionExecutor->>SharedState: getSharedState()
SharedState-->>L2PSTransactionExecutor: referenceHeight
L2PSTransactionExecutor->>Denomination: isForkActive("osDenomination", referenceHeight)
Denomination-->>L2PSTransactionExecutor: forkActive
L2PSTransactionExecutor->>Denomination: canonicalizeAmountToOs(rawAmount / L2PS_TX_FEE)
Denomination-->>L2PSTransactionExecutor: amountCanonical, feeCanonical (or error)
L2PSTransactionExecutor->>L1Account: compare balance >= amountCanonical + feeCanonical
L2PSTransactionExecutor->>GCR: emit fee remove (editFee) and transfer remove/add (editAmount)
GCR-->>Client: success/failure response
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint install failed. For unrecoverable errors, disable the tool in CodeRabbit configuration. 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 a critical denomination mismatch in the L2PS native-send executor where post-
Confidence Score: 5/5Safe to merge — the canonicalization logic directly mirrors the L1 native-send path in executeNativeTransaction.ts, using the same helpers, the same reference height source, and the same denomination functions. The fork-aware amount handling is consistent with the established L1 path: same helpers, same reference height source, correct balance comparison units in both fork regimes. Both previously flagged issues are addressed. No logic errors or missing guards were found. No files require special attention. Important Files Changed
Reviews (3): Last reviewed commit: "review: address greptile P2 — uniform er..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Two P2 fixes from review on PR #926: 1. **Uniform error path for canonicalisation.** `L2PS_TX_FEE` canonicalisation was outside the try-catch that guards the wire- amount call. `L2PS_TX_FEE = 1` is a valid constant and won't throw in practice, but the inconsistency meant any unexpected drift in the helper's input shape would surface as the generic `"Execution failed"` from the outer catch instead of the specific `"Invalid amount"` wording. Move both calls inside the same guard. 2. **Avoid `Number(bigint)` for the GCR edit shape.** Pre-fork, `amountCanonical` is a raw DEM-magnitude `bigint` with no scaling. Converting via `Number()` silently truncates anything above `Number.MAX_SAFE_INTEGER` (~9 quadrillion DEM) while the balance check upstream had already passed the full-precision amount. Use `.toString()` on both edits — `GCREditBalance.amount` accepts `number | string`, the rest of the pipeline handles string already for post-fork OS magnitudes. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
… fork (DEM-755)
The L2PS native-send executor was treating the inner `amount` as a raw
DEM number and computing arithmetic via `BigInt(amount)` directly. The
L1 native path goes through `canonicalizeAmountToOs`; the L2PS executor
did not. Two failure modes follow:
1. After the osDenomination fork, an L2PS native transfer moves about
one part in a billion of what the sender intended, because a post-
fork OS magnitude is treated the same way a pre-fork DEM number
was. The balance check uses the same wrong magnitude, so the
transfer also charges a fee that does not match the wire shape.
2. The pre-edit validation rejected anything that was not a JS
`number`, so any post-fork SDK that sends an OS-string amount was
refused outright with "must be a positive integer".
Mirror the L1 pattern exactly:
- Pull the reference block height from shared state and read
`isForkActive("osDenomination", ...)`.
- Pass the wire amount through `canonicalizeAmountToOs` to get a
bigint magnitude regardless of pre- or post-fork shape; surface
the helper's error message instead of the old number-only check.
- Canonicalise the fee the same way so the balance check arithmetic
agrees on units (1 DEM = 10^9 OS).
- Emit GCR edits in the magnitude downstream consumers expect:
OS string post-fork (matches what `serializerGate` writes for L1),
legacy DEM number pre-fork.
`L2PS_TX_FEE` itself stays at 1 (in DEM); only the arithmetic is
canonicalised.
Surfaced by the PATH-OS L2PS hardening report. No unit test added in
this PR — the executor depends on the full DB stack (Datasource +
GCRMain + HandleGCR); the canonicalisation primitive itself is
already covered in `testing/forks/amountCanonical.test.ts`. End-to-end
verification through the existing dev-node battery once the dev
environment is rebooted (tracked under DEM-728).
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Two P2 fixes from review on PR #926: 1. **Uniform error path for canonicalisation.** `L2PS_TX_FEE` canonicalisation was outside the try-catch that guards the wire- amount call. `L2PS_TX_FEE = 1` is a valid constant and won't throw in practice, but the inconsistency meant any unexpected drift in the helper's input shape would surface as the generic `"Execution failed"` from the outer catch instead of the specific `"Invalid amount"` wording. Move both calls inside the same guard. 2. **Avoid `Number(bigint)` for the GCR edit shape.** Pre-fork, `amountCanonical` is a raw DEM-magnitude `bigint` with no scaling. Converting via `Number()` silently truncates anything above `Number.MAX_SAFE_INTEGER` (~9 quadrillion DEM) while the balance check upstream had already passed the full-precision amount. Use `.toString()` on both edits — `GCREditBalance.amount` accepts `number | string`, the rest of the pipeline handles string already for post-fork OS magnitudes. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Shitikyan
force-pushed
the
fix/l2ps-executor-canonicalize-amount
branch
from
ead3d66 to
45605cb
Compare
2 participants
Summary
The L2PS native-send executor was treating the inner
amountas a raw DEM number. Post-osDenomination-fork this means every L2PS native transfer moves about one part in a billion of what the sender intended, and a post-fork OS string amount is rejected by the executor's number-only validation. Surfaced by the PATH-OS L2PS hardening report.Mirror the L1 native path exactly:
isForkActive(\"osDenomination\", lastBlockNumber)from shared statecanonicalizeAmountToOsto get a bigint magnitude regardless of pre- or post-fork shapeL2PS_TX_FEEthe same way so the balance check arithmetic agrees on units (1 DEM = 10⁹ OS)L2PS_TX_FEEitself stays at 1; only the arithmetic is canonicalised.Test plan
tsc --noEmit --skipLibCheckclean on the touched fileNo unit test added: the executor depends on the full DB stack (Datasource + GCRMain + HandleGCR), and the canonicalisation primitive it now calls is already covered in
testing/forks/amountCanonical.test.ts.Linked
🤖 Generated with Claude Code
Summary by CodeRabbit