Skip to content

fix: keep legacy transaction v computation in BigInt to avoid precision loss - #210

Merged
sherifahmed990 merged 2 commits into
devfrom
fix/128-legacy-v-precision
Jun 12, 2026
Merged

fix: keep legacy transaction v computation in BigInt to avoid precision loss#210
sherifahmed990 merged 2 commits into
devfrom
fix/128-legacy-v-precision

Conversation

@Sednaoui

@Sednaoui Sednaoui commented Jun 11, 2026

Copy link
Copy Markdown
Member

Fixes #128

Red/green verified: test/utils7702.test.js signs a legacy tx with chainId 4337433743374337433 (> 2^53) and parses it back with ethers; on pre-fix dev the recovered sender is wrong (1 failed), with the fix both chainId and sender recover exactly (2 passed).

Summary by CodeRabbit

  • Bug Fixes

    • Fixed legacy transaction signing for very large chain IDs so transactions preserve correct chain ID and sender recovery.
  • Tests

    • Added unit tests verifying signing and sender recovery for large and standard chain IDs.
  • Documentation

    • Updated changelog with the bug fix entry.

@coderabbitai

coderabbitai Bot commented Jun 11, 2026

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 14f17f53-afc9-469f-8f31-11b016a995dd

📥 Commits

Reviewing files that changed from the base of the PR and between 4ff3f2a and 436764b.

📒 Files selected for processing (1)
  • CHANGELOG.md
✅ Files skipped from review due to trivial changes (1)
  • CHANGELOG.md

📝 Walkthrough

Walkthrough

Fixes a precision bug in createAndSignLegacyRawTransaction by computing legacy v using pure BigInt arithmetic; adds tests verifying chainId preservation and sender recovery for large (>2^53) and small chain IDs, and documents the change in CHANGELOG.md.

Changes

Legacy Transaction v Computation Fix

Layer / File(s) Summary
BigInt v computation fix and validation
src/utils7702.ts, test/utils7702.test.js
The legacy transaction signature v is computed with pure bigint arithmetic (BigInt(signature.yParity) + chainId * 2n + 35n) instead of converting chainId to Number. Tests sign and parse transactions to validate chainId preservation and sender recovery for very large and small chain IDs.
Changelog documentation
CHANGELOG.md
Adds an UNRELEASED Bug Fix entry describing the switch from Number(chainId) to BigInt in createAndSignLegacyRawTransaction to prevent rounding errors in v computation.

🎯 3 (Moderate) | ⏱️ ~20 minutes

🐰 I hopped on bytes both large and small,
Switched to BigInt to catch them all,
No more rounding in the v we send,
Chain IDs safe from end to end,
A thump, a test, precision for all! 🥕

🚥 Pre-merge checks | ✅ 3 | ❌ 2

❌ Failed checks (1 warning, 1 inconclusive)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 50.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
Description check ❓ Inconclusive The description provides issue reference, test verification details, and test results but lacks structured sections (Summary, Test, Risk/Compatibility) from the template. Expand description to follow the template structure with explicit Summary, Test, and Risk/Compatibility sections for clarity.
✅ Passed checks (3 passed)
Check name Status Explanation
Title check ✅ Passed The title accurately describes the main change: fixing legacy transaction v computation to use BigInt for precision handling with large chain IDs.
Linked Issues check ✅ Passed The code changes fully address issue #128 by replacing Number(chainId) with pure BigInt arithmetic in createAndSignLegacyRawTransaction to prevent precision loss for large chain IDs.
Out of Scope Changes check ✅ Passed All changes (CHANGELOG.md, src/utils7702.ts, test/utils7702.test.js) are directly scoped to fixing issue #128 with no unrelated modifications present.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.


Comment @coderabbitai help to get the list of available commands and usage tips.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 4ff3f2a39c

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread test/utils7702.test.js
// producing an unrecoverable signature. Verified independently by parsing
// the raw transaction with ethers and checking the recovered sender.

const ak = require("../dist/index.cjs");

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P1 Badge Update the built dist artifacts

This new regression test imports ../dist/index.cjs, but the commit only updates src/utils7702.ts; the checked-in dist/index.cjs still computes v with Number(chainId), while package.json also points main/exports at dist. In the current tree, npx jest --runTestsByPath test/utils7702.test.js --runInBand fails because the large-chainId case parses back as 4337433743374337518n, so CI and any consumers using the committed package entrypoint still get the old precision-loss behavior until the dist outputs are rebuilt and committed.

Useful? React with 👍 / 👎.

@sherifahmed990 sherifahmed990 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM

@sherifahmed990
sherifahmed990 merged commit f2a19e5 into dev Jun 12, 2026
3 checks passed
@sherifahmed990
sherifahmed990 deleted the fix/128-legacy-v-precision branch June 12, 2026 23:26
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.

Fix precision loss in legacy transaction v computation (utils7702.ts)

2 participants