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: support encodable receipt to use alloy calculate_receipt_root #74

Draft
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

pedrohba1
Copy link
Contributor

Example now works from this. Although, this leaves state_root unused. Should I just remove the unused code in this case?

Copy link
Contributor

@suchapalaver suchapalaver left a 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 {
Copy link
Contributor

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 🙏

Copy link
Contributor Author

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.

@pedrohba1
Copy link
Contributor Author

@pedrohba1 see my suggestions. But also, let's revert the original change in another PR before we do this, as discussed 🙏

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.

@suchapalaver suchapalaver marked this pull request as draft February 17, 2025 15:29
@suchapalaver
Copy link
Contributor

@pedrohba1 converting this to draft while there are still CI failures 👍

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