-
Notifications
You must be signed in to change notification settings - Fork 0
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: support encodable receipt to use alloy calculate_receipt_root #74
base: main
Are you sure you want to change the base?
Conversation
…xample works from this
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.
@pedrohba1 see my suggestions. But also, let's revert the original change in another PR before we do this, as discussed 🙏
receipt: ReceiptWithBloom, | ||
state_root: Vec<u8>, | ||
} | ||
|
||
impl Typed2718 for FullReceipt { | ||
fn ty(&self) -> u8 { |
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.
Please give this a more meaningful name 🙏
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 one I unfortunately cannot change the name. I need this ty
function from Typed2718 from alloy-eip
. Looks weird, but alloy-eip does call a type ty
, which is quite annying that now we have tx_type
, r#type
and ty
all in 3 different external crates which means the same in all.
On our codebase though it seems to be always tx_type
, I'm good with it.
I could do this though:
impl FullReceipt {
pub fn transaction_type(&self) -> u8 {
self.ty() // Calls the `Typed2718` implementation
}
}
impl Typed2718 for FullReceipt {
fn ty(&self) -> u8 {
self.tx_type.ty()
}
}
But I don't think it is necessary since we can already use full_receipt.tx_type
the way the code is in the way it is implemented.
This is the PR attempting to the original change. it does revert the please-release merge, I ask for some attention into reading the PR, because I proposed keeping some of the changes in another PR linked there. |
@pedrohba1 converting this to draft while there are still CI failures 👍 |
Example now works from this. Although, this leaves
state_root
unused. Should I just remove the unused code in this case?