-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
feat: unify ReceiptWithBloom from Alloy #13088
Conversation
crates/primitives/src/receipt.rs
Outdated
self.rlp_header_with_bloom(bloom).encode(out); | ||
self.rlp_encode_fields_with_bloom(bloom, out); | ||
} |
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 wrong, the rlp encoding should be equivalent to ReceiptWithBloomEncoder::encode_inner(... true)
and EIP-2718 should be equivalent to encode_inner(..., false)
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.
i have now removed the ReceiptWithBloomEncoder
struct and transferred some functions to the receipt impl. But I believe I could instead shorten the Receipt
impl block as well and directly use ReceiptWithBloom
encoding functions? But then this Receipt
as extra fields for OP and also tx_type
.. What do you say?
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.
Also, shall I try removing ReceiptWithBloomRef
struct as well? Looking to shorten the code.
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.
i have now removed the
ReceiptWithBloomEncoder
struct and transferred some functions to the receipt impl. But I believe I could instead shorten theReceipt
impl block as well and directly useReceiptWithBloom
encoding functions? But then thisReceipt
as extra fields for OP and alsotx_type
.. What do you say?
no, we need to keep the current Receipt
type as is, and define all encoding on it
ReceiptWithBloomRef
should be removed now
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.
Thanks! Will do.
@klkvr Awaiting your inputs. |
Thanks @klkvr 🫡 |
@@ -654,7 +471,7 @@ mod tests { | |||
#[cfg(feature = "optimism")] | |||
#[test] | |||
fn decode_deposit_receipt_regolith_roundtrip() { | |||
let data = hex!("7ef9010c0182b741b9010000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000c0833d3bbf"); | |||
let data = hex!("b901107ef9010c0182b741b9010000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000c0833d3bbf"); |
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.
had to update those tests because they were attempting to us ReceiptWithBloom::decode
on EIP-2718 encoding while it expects network encoding
this was working before because the decoding impl wasn't strict enough but this kind of encoding shouldn't really happen at all as receipts are only being decoded in p2p
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.
great
Co-authored-by: Arsenii Kulikov <klkvrr@gmail.com>
Closes: #12701