-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Conversation
Encodable2718
, Decodable2718
for TransactionSignedEncodable2718
, Decodable2718
for TransactionSigned
@@ -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()) |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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?
There was a problem hiding this 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()) |
There was a problem hiding this comment.
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?
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? |
sorry about the conflicts -.- let's ignore the test for now and track, doing a new alloy release asap anyway |
f44794f
to
f3822d4
Compare
changed pooled tx decoding a bit, tests should pass now 230f53c |
ci is still failing due to lack of this check in 2718 impl reth/crates/primitives/src/transaction/mod.rs Lines 1436 to 1438 in ba4e411
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 |
addressed in f8f88ae by removing those tests and adding this checks to engine code handling decoding of transactions |
dc02862
to
b712d2b
Compare
There was a problem hiding this 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?
updated in b5e2871 |
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