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.

Sign up for GitHub

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

Jump to bottom

fix(eips/consensus): correctly decode txs on TxEnvelope #148

Merged
merged 2 commits into from
Jan 23, 2024
Merged

fix(eips/consensus): correctly decode txs on TxEnvelope #148

merged 2 commits into from
Jan 23, 2024

Conversation

Evalir
Copy link
Contributor

@Evalir Evalir commented Jan 23, 2024 •
edited
Loading

Motivation

Closes #141.

TxEnvelope wasn't able to properly decode txs due to a few issues with the eip-2718 network_decode impl. Namely, as the issue says:

  • The original buffer was getting modified when decoding the header instead of being copied, leading to issues decoding legacy txs.
  • The last check was wrong—we should only perform the check if we have a header.

Solution

Correct the checks.

PR Checklist

  • Added Tests
  • Added Documentation
  • Breaking changes

Rjected
Rjected reviewed Jan 23, 2024
View reviewed changes
crates/eips/src/eip2718.rs Outdated Show resolved Hide resolved
@Evalir Evalir marked this pull request as draft January 23, 2024 20:23
@Evalir Evalir marked this pull request as ready for review January 23, 2024 21:10
@Evalir Evalir force-pushed the evalir/fix-envelope branch from 628de72 to 055a211 Compare January 23, 2024 21:11
@Evalir Evalir requested a review from Rjected January 23, 2024 21:11
Rjected
Rjected approved these changes Jan 23, 2024
View reviewed changes
Copy link
Contributor

@Rjected Rjected left a comment

Choose a reason for hiding this comment

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

LGTM!

@Evalir Evalir merged commit 269d902 into main Jan 23, 2024
14 checks passed
@Evalir Evalir deleted the evalir/fix-envelope branch January 23, 2024 21:20
@mattsse
Copy link
Member

mattsse commented Jan 23, 2024

nice!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Rjected Rjected Rjected approved these changes

@DaniPopes DaniPopes Awaiting requested review from DaniPopes DaniPopes is a code owner

@gakonst gakonst Awaiting requested review from gakonst gakonst is a code owner

@prestwich prestwich Awaiting requested review from prestwich

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[Bug] TxEnvelope rlp decoding returns unexpected length
3 participants
@Evalir @mattsse @Rjected