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

Fix the decoding of unsigned transactions #243

Merged
merged 3 commits into from
Aug 3, 2023

Conversation

pienkowb
Copy link
Contributor

@pienkowb pienkowb commented Jul 6, 2023

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
Copy link
Owner

q9f commented Jul 13, 2023

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

Do you mind adding a test for that?

@pienkowb
Copy link
Contributor Author

Do you mind adding a test for that?

@q9f Done 👍

@codecov-commenter
Copy link

codecov-commenter commented Jul 19, 2023

Codecov Report

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

❗ 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%) ⬆️
lib/eth/tx/eip2930.rb 99.32% <100.00%> (+0.68%) ⬆️
spec/eth/tx_spec.rb 100.00% <100.00%> (ø)

... and 2 files with indirect coverage changes

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

@pienkowb
Copy link
Contributor Author

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

@pienkowb
Copy link
Contributor Author

pienkowb commented Aug 1, 2023

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

@q9f
Copy link
Owner

q9f commented Aug 1, 2023

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

@q9f q9f merged commit 031652b into q9f:main Aug 3, 2023
3 of 10 checks passed
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.

None yet

3 participants