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

chore: convert rcp-types-eth block Header to consensus Header #1014

Merged

Conversation

teddav
Copy link
Contributor

@teddav teddav commented Jul 4, 2024

There are currently multiple Block Header types (@mattsse told me it's because of legacy naming).
This PR adds a way to convert one into the other. I needed that in order to then RLP encode the Header. I added tests at the bottom to make sure the RLP encoding is correct and correctly recomputes the block hash

Copy link
Member

@mattsse mattsse left a comment

Choose a reason for hiding this comment

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

nits

@@ -149,6 +149,60 @@ impl Header {
}
}

impl TryFrom<Header> for alloy_consensus::Header {
type Error = String;
Copy link
Member

Choose a reason for hiding this comment

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

This should be ConversionError

Copy link
Contributor Author

@teddav teddav Jul 5, 2024

Choose a reason for hiding this comment

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

done. i just used a "::Custom" error, since the ConversionError seems to be made for Transactions, i didnt want to add other variants to it

crates/rpc-types-eth/src/block.rs Outdated Show resolved Hide resolved
@mattsse mattsse merged commit 40e6860 into alloy-rs:main Jul 5, 2024
22 checks passed
ben186 pushed a commit to ben186/alloy that referenced this pull request Jul 27, 2024
…rs#1014)

* chore: convert rcp-types-eth block Header to consensus Header

* clippy

* custom conversion error
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