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(consensus): populate chain id when decoding signed legacy txs #137

Merged
merged 7 commits into from
Jan 19, 2024

Conversation

Evalir
Copy link
Contributor

@Evalir Evalir commented Jan 19, 2024

Motivation

The chain id was not being populated when decoding a signed legacy tx, leading to decoding issues.

Closes #125

Solution

Populate the chain id param when decoding.

PR Checklist

  • Added Tests
  • Added Documentation
  • Breaking changes

@Evalir Evalir requested a review from mattsse January 19, 2024 16:04
@Evalir Evalir changed the title fix(consensus): populate chain id for legacy txs when decoding signed legacy txs fix(consensus): populate chain id when decoding signed legacy txs Jan 19, 2024
@Evalir Evalir requested a review from DaniPopes January 19, 2024 16:18
Copy link
Member

@mattsse mattsse left a comment

Choose a reason for hiding this comment

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

I'm a bit confused why TxLegacy is also Encodeable and Decodeable,

this can be misused easily, because that's most likely never what you want?

re signature and eip155:

https://github.com/paradigmxyz/reth/blob/77f38574e20d2871c7e38cf99fae2fc5cd1cee02/crates/primitives/src/transaction/signature.rs#L98-L98

@Evalir
Copy link
Contributor Author

Evalir commented Jan 19, 2024

@mattsse agreed it feels a bit odd to have TxLegacy as encodable/decodable, but I think we should not include removing that in this PR as this is a bugfix for the signed decoding case Signed<TxLegacy>. Let's add a separate issue for this and fix in a new PR

@Evalir Evalir requested a review from mattsse January 19, 2024 16:22
@Evalir Evalir requested a review from mattsse January 19, 2024 16:28
Copy link
Member

@mattsse mattsse left a comment

Choose a reason for hiding this comment

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

last nit

crates/consensus/src/transaction/legacy.rs Show resolved Hide resolved
@Evalir Evalir merged commit 892a384 into main Jan 19, 2024
14 checks passed
@Evalir Evalir deleted the evalir/recover-legacy-tx-test branch January 19, 2024 16:35
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] alloy_consensus: Incorrect legacy signer being recovered
3 participants