Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: better legacy eip155 tx serializing #1438

Merged
merged 2 commits into from
Nov 3, 2023
Merged

Conversation

jxom
Copy link
Member

@jxom jxom commented Nov 2, 2023

Fixes #1433


PR-Codex overview

Focus of the PR:

Fixing the legacy EIP-155 transaction serializing.

Detailed summary:

  • Updated the logic for calculating the value of v in serializeTransaction.ts to fix the legacy EIP-155 transaction serializing.
  • Added tests in serializeTransaction.test.ts to ensure the correct serialization of transactions with different values of v.

✨ Ask PR-Codex anything about this PR by commenting with /codex {your question}

Copy link

changeset-bot bot commented Nov 2, 2023

🦋 Changeset detected

Latest commit: 8efb827

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
viem Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

Copy link

vercel bot commented Nov 2, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated (UTC)
viem ✅ Ready (Inspect) Visit Preview Nov 3, 2023 0:01am

Copy link
Contributor

github-actions bot commented Nov 2, 2023

size-limit report 📦

Path Size Loading time (3g) Running time (snapdragon) Total time
viem (esm) 58.29 KB (+0.05% 🔺) 1.2 s (+0.05% 🔺) 585 ms (+6.42% 🔺) 1.8 s
viem (cjs) 77.33 KB (+0.04% 🔺) 1.6 s (+0.04% 🔺) 779 ms (-0.87% 🔽) 2.4 s
viem (minimal surface - tree-shaking) 3.89 KB (0%) 78 ms (0%) 62 ms (-36.17% 🔽) 140 ms
viem/accounts 89.04 KB (+0.04% 🔺) 1.8 s (+0.04% 🔺) 268 ms (-10.39% 🔽) 2.1 s
viem/accounts (tree-shaking) 19.41 KB (+0.17% 🔺) 389 ms (+0.17% 🔺) 266 ms (+50.56% 🔺) 654 ms
viem/actions 43.33 KB (0%) 867 ms (0%) 2.8 s (-35.93% 🔽) 3.7 s
viem/actions (tree-shaking) 350 B (0%) 10 ms (0%) 76 ms (-7.49% 🔽) 86 ms
viem/chains 18.89 KB (+0.18% 🔺) 378 ms (+0.18% 🔺) 327 ms (+25.4% 🔺) 705 ms
viem/chains (tree-shaking) 470 B (0%) 10 ms (0%) 107 ms (+6.62% 🔺) 117 ms
viem/chains/utils 9.21 KB (+0.25% 🔺) 185 ms (+0.25% 🔺) 144 ms (-0.39% 🔽) 328 ms
viem/chains/utils (tree-shaking) 5.36 KB (0%) 108 ms (0%) 106 ms (-36.21% 🔽) 213 ms
viem/ens 43.33 KB (0%) 867 ms (0%) 3 s (-15.78% 🔽) 3.8 s
viem/ens (tree-shaking) 18.02 KB (0%) 361 ms (0%) 180 ms (-38.68% 🔽) 541 ms

Copy link

codecov bot commented Nov 2, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (339fbf1) 97.10% compared to head (8efb827) 99.78%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1438      +/-   ##
==========================================
+ Coverage   97.10%   99.78%   +2.67%     
==========================================
  Files         400      405       +5     
  Lines       35484    35851     +367     
  Branches     1836     2090     +254     
==========================================
+ Hits        34458    35773    +1315     
+ Misses       1014       72     -942     
+ Partials       12        6       -6     
Files Coverage Δ
src/utils/transaction/serializeTransaction.ts 100.00% <100.00%> (+3.08%) ⬆️

... and 53 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@jxom jxom force-pushed the jxom/fix-legacy-eip155 branch from d059135 to 9e120e5 Compare November 2, 2023 23:59
@jxom jxom changed the title fix: legacy eip155 tx serializing feat: better legacy eip155 tx serializing Nov 3, 2023
@jxom jxom merged commit 8e52fcb into main Nov 3, 2023
@jxom jxom deleted the jxom/fix-legacy-eip155 branch November 3, 2023 22:45
@github-actions github-actions bot mentioned this pull request Nov 3, 2023
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.

bug: serializeTransaction doesn't support other chains then mainnet
1 participant