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

consensus: add unit tests for receipts `root_slow and refac #1567

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

Conversation

tcoratger
Copy link
Contributor

Follow up on #1563:

  • Full coverage with unit tests
  • Small refactor

@DaniPopes
Copy link
Member

DaniPopes commented Oct 26, 2024

I don't see the point of this function, after changing &[&T] to &[T] because that collect is unnecessary, it becomes essentially receipts.map(f), which doesn't need to be a method

@tcoratger
Copy link
Contributor Author

I don't see the point of this function, after changing &[&T] to &[T] because that collect is unnecessary, it becomes essentially receipts.map(f), which doesn't need to be a method

@DaniPopes Yes fair point. I think it requires changing some function definitions in reth but I think it can only be beneficial. Here is what I propose:

  • I have a PR open in draft on reth to work on it (and more generally the replacement of Receipts) primitives: use alloy Receipts paradigmxyz/reth#12059
  • Let's do a review on it without using the root_slow function as you propose here.
  • If it is valid then we transform this PR on alloy into a PR for the definitive removal of this function.

What do you think?

@emhane
Copy link
Member

emhane commented Oct 26, 2024

agree with @DaniPopes , this only makes sense if this type is the l1 type, and it implements a new trait ReceiptsRoot, which should then also be implemented on a similar op-alloy type OpReceipts

@tcoratger
Copy link
Contributor Author

@emhane paradigmxyz/reth#12059 is ready for review, if everything is fine and merged then we can delete the function here in alloy :)

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.

3 participants