Skip to content

Fix the decoding of unsigned transactions#243

Merged
q9f merged 3 commits intoq9f:mainfrom
pienkowb:fix-unsigned-tx-decoding
Aug 3, 2023
Merged

Fix the decoding of unsigned transactions#243
q9f merged 3 commits intoq9f:mainfrom
pienkowb:fix-unsigned-tx-decoding

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

pienkowb commented Jul 17, 2023

Do you mind adding a test for that?

@q9f Done 👍

@codecov-commenter
Copy link

codecov-commenter commented Jul 19, 2023 •
edited
Loading

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

pienkowb commented Jul 19, 2023

@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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@q9f q9f q9f approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@pienkowb @q9f @codecov-commenter