consensus: add BlockHeader getter trait
#1302
Conversation
tcoratger
requested review from
DaniPopes,
gakonst,
mattsse,
onbjerg,
prestwich and
emhane
as code owners
| @@ -638,6 +638,158 @@ impl<'a> arbitrary::Arbitrary<'a> for Header { | |||
| } | |||
| } | |||
|
|
|||
| /// Trait for extracting specific Ethereum block data from a header | |||
| pub trait BlockHeader { | |||
There was a problem hiding this comment.
from reth pov there's an argument for limiting these functions to the absolute necessary fields, or introduce sub traits, for example for evm you only need certain fields.
but since this doesn't clash with rpc, I think all of those are fine, and we could still do sub traits
There was a problem hiding this comment.
Let me know if you think we should split in subtraits now or if we should do that in a follow up if needed
There was a problem hiding this comment.
I think we can do in a followup if needed, realistically we only need this trait for one type really, the chain's header type and for op we can even reuse the L1 type
mattsse
dismissed
their stale review
after thinking about this, this might be redundant since we already have HeaderResponse if we unify rpc with consensus, so I'd like to hold off here
|
@emhane @mattsse Now that paradigmxyz/reth#10691 is merged, in order to completely remove |
I think we merge as is, because I'm getting blocked on several prs over and over again because some getters are missing. in the end it seems to result in that we are using all the fields on the primitive types we abstract away with these traits, i.e. reth primitives are minimal types. |
|
also for rpc header we want |
|
@emhane FYI just opened a follow up issue here on reth paradigmxyz/reth#11127 |
consensus: add BlockHeader getter trait
Should close #1301