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

primitives: use alloy Receipts with Option #12162

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

Conversation

tcoratger
Copy link
Contributor

Should close #7238

Superseed #12059

@tcoratger
Copy link
Contributor Author

Hey @joshieDo @mattsse anything to add here or can we merge?

I'm thinking about doing a follow up then to remove re-import from alloy and so be able to work on receipt pruning easily.

@tcoratger
Copy link
Contributor Author

@mattsse Nice bump here, is it ready to merge if I fix conflicts?

Comment on lines -359 to +383
receipts: Receipts::from(value.0.receipts),
receipts: value.0.receipts.into_iter().map(Some).collect::<Vec<_>>().into(),
Copy link
Member

Choose a reason for hiding this comment

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

this new syntax is more complicated, I'm missing the point of this change

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Before we had this implemented in reth

impl From<Vec<Receipt>> for Receipts {
    fn from(block_receipts: Vec<Receipt>) -> Self {
        Self { receipt_vec: vec![block_receipts.into_iter().map(Option::Some).collect()] }
    }
}

but now the situation is more generic in alloy with a generic parameter T https://github.com/alloy-rs/alloy/blob/86812dba8507e6f7741f666303a53cafef93c8c1/crates/consensus/src/receipt/receipts.rs#L81 so that this is not available anymore. But this should just be a temporary situation since we would like to remove soon the Option<> in the definition of Receipts in reth after the merge of this PR and some follow up work

Copy link
Member

Choose a reason for hiding this comment

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

we have Receipts<T> in reth too now #12358, #12454

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The goal is was to use directly the alloy type, just importing it:

  1. Use the Receipts alloy type everywhere with pub type Receipts = alloy_consensus::Receipts<Option<Receipt>>;
  2. Do some work to arrive at pub type Receipts = alloy_consensus::Receipts<Receipt>; (especially on pruning which is blocker right now)

But if we want to integrate a new Receipts type into reth maybe this PR is not relevant anymore

Copy link
Member

Choose a reason for hiding this comment

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

I would hold off a bit with this pr until we have gotten further underway with #12454

@mattsse mattsse requested a review from klkvr as a code owner November 23, 2024 04:53
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.

Migrate to alloy Receipt type
3 participants