search

q9f / eth.rb

a straightforward library to build, sign, and broadcast ethereum transactions anywhere you can run ruby.
https://q9f.github.io/eth.rb
Apache License 2.0
196 stars 85 forks source link

Fix the decoding of unsigned transactions #243

Closed pienkowb closed 1 year ago

pienkowb commented 1 year ago

Currently, the transaction decoding methods (both for EIP-1559 and EIP-2930) try to recover a sender address for unsigned transactions, which results in the following error:

lib/eth/chain.rb:161:in `+': nil can't be coerced into Integer (TypeError)

        v = 2 * chain_id + 35 + recovery_id
                                ^^^^^^^^^^^

This PR fixes this issue by checking if recovery_id is present.

q9f commented 1 year ago

Thanks. You can ignore the failing actions (lack of permissions for external contributors).

Do you mind adding a test for that?

pienkowb commented 1 year ago

Do you mind adding a test for that?

@q9f Done 👍

codecov-commenter commented 1 year ago

Codecov Report

Merging #243 (6efcd1a) into main (6f19a28) will decrease coverage by 0.18%. The diff coverage is 100.00%.

:exclamation: Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more.

@@            Coverage Diff             @@
##             main     #243      +/-   ##
==========================================
- Coverage   99.66%   99.48%   -0.18%     
==========================================
  Files          77       77              
  Lines        4439     4460      +21     
==========================================
+ Hits         4424     4437      +13     
- Misses         15       23       +8
Files Changed Coverage Δ
lib/eth/tx/legacy.rb 100.00% <ø> (ø)
lib/eth/tx/eip1559.rb 99.35% <100.00%> (+0.65%) :arrow_up:
lib/eth/tx/eip2930.rb 99.32% <100.00%> (+0.68%) :arrow_up:
spec/eth/tx_spec.rb 100.00% <100.00%> (ø)

... and 2 files with indirect coverage changes

:mega: We’re building smart automated test selection to slash your CI/CD build times. Learn more

pienkowb commented 1 year ago

@q9f I've added a test case for an EIP-2930 transaction too, so that the test coverage is not decreased.

pienkowb commented 1 year ago

@q9f Is there anything else we need before this PR can be merged?

q9f commented 1 year ago

Sorry, it's slow season over hear. I'll take care of it now :)