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

impl Encodable2718, Decodable2718 for TransactionSigned #11218

Merged
merged 10 commits into from
Sep 30, 2024
Merged

Conversation

klkvr
Copy link
Collaborator

@klkvr klkvr commented Sep 25, 2024

First step towards #11153

Implements eip2718 traits for TransactionSigned, removes some of the method and uses methods provided by the trait instead:

  • encode_enveloped -> encode_2718
  • decode_enveloped -> decode_2718
  • length_without_header -> encode_2718_len
  • envelope_encoded -> encoded_2718
  • encode_inner(buf, false) -> encode_2718
  • encode_inner(buf, true) -> network_encode

This now uses decode_signed_fields methods which don't yet include partiy decoding fix from alloy-rs/alloy#1305, so we should probably merge this on next alloy release

@klkvr klkvr changed the title impl Encodable2718, Decodable2718 for TransactionSigned impl Encodable2718, Decodable2718 for TransactionSigned Sep 25, 2024
@@ -52,7 +53,11 @@ impl PayloadBuilderAttributes for OptimismPayloadBuilderAttributes {
.unwrap_or_default()
.into_iter()
.map(|data| {
TransactionSigned::decode_enveloped(&mut data.as_ref())
TransactionSigned::decode_2718(&mut data.as_ref())
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

this is needed to avoid introducing eip2718 error variant on PayloadError. it can be made nicer once we have alloy-rs/alloy#1359

Copy link
Collaborator

Choose a reason for hiding this comment

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

cool, can we track these in a new issue so we don't forget?

Copy link
Collaborator

@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.

this is awesome!!

I suggest we get this in first and do the error cleanup once we have a new alloy bump

@@ -52,7 +53,11 @@ impl PayloadBuilderAttributes for OptimismPayloadBuilderAttributes {
.unwrap_or_default()
.into_iter()
.map(|data| {
TransactionSigned::decode_enveloped(&mut data.as_ref())
TransactionSigned::decode_2718(&mut data.as_ref())
Copy link
Collaborator

Choose a reason for hiding this comment

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

cool, can we track these in a new issue so we don't forget?

@klkvr
Copy link
Collaborator Author

klkvr commented Sep 26, 2024

the ci is currently failing because of the issue addressed in alloy-rs/alloy#1367

@mattsse are we OK with including this after next alloy release?

@mattsse
Copy link
Collaborator

mattsse commented Sep 26, 2024

sorry about the conflicts -.-

let's ignore the test for now and track, doing a new alloy release asap anyway

@klkvr
Copy link
Collaborator Author

klkvr commented Sep 26, 2024

changed pooled tx decoding a bit, tests should pass now 230f53c

@klkvr
Copy link
Collaborator Author

klkvr commented Sep 27, 2024

ci is still failing due to lack of this check in 2718 impl

if !input_data.is_empty() {
return Err(RlpError::UnexpectedLength)
}

looks like it was added in #8296 and is only required for engine? should we instead only perform it there? adding it to decode_2718 would result in it being applied in Decodable impl too which might result in issues with decoding of eg lists of transactions

@klkvr
Copy link
Collaborator Author

klkvr commented Sep 27, 2024

addressed in f8f88ae by removing those tests and adding this checks to engine code handling decoding of transactions

Copy link
Collaborator

@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.

lgtm, we can do the Eip2718Error simplifications separately?

@klkvr
Copy link
Collaborator Author

klkvr commented Sep 30, 2024

lgtm, we can do the Eip2718Error simplifications separately?

updated in b5e2871

@klkvr klkvr enabled auto-merge September 30, 2024 15:25
@klkvr klkvr added this pull request to the merge queue Sep 30, 2024
Merged via the queue into main with commit 42afcbd Sep 30, 2024
36 checks passed
@klkvr klkvr deleted the klkvr/2718-tx branch September 30, 2024 15:58
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.

2 participants