-
Notifications
You must be signed in to change notification settings - Fork 236
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
feat(consensus): Generic Block Type #1319
Conversation
crates/consensus/src/block.rs
Outdated
/// A block body. | ||
#[derive(Debug, Clone, PartialEq, Eq, Default, RlpEncodable, RlpDecodable)] | ||
#[rlp(trailing)] | ||
pub struct BlockBody<T: Encodable + Decodable + Encodable2718 + Decodable2718> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
let's not require anything here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we need Encodable + Decodable for the RlpEncodable, RlpDecodable
auto-derive above right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hmm, I don't think it's needed? those derives will add bounds for respective impl blocks
crates/consensus/src/block.rs
Outdated
/// Taken from [reth-primitives](https://github.com/paradigmxyz/reth) | ||
#[derive(Debug, Clone, PartialEq, Eq, Default, RlpEncodable, RlpDecodable)] | ||
#[rlp(trailing)] | ||
pub struct Block<T: Encodable + Decodable + Encodable2718 + Decodable2718> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
wondering if the en-/decoding of requests is a bit out of scope?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggestions
crates/consensus/src/block.rs
Outdated
/// Taken from [reth-primitives](https://github.com/paradigmxyz/reth) | ||
#[derive(Debug, Clone, PartialEq, Eq, Default, RlpEncodable, RlpDecodable)] | ||
#[rlp(trailing)] | ||
pub struct Block<T: Encodable + Decodable + Encodable2718 + Decodable2718> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this should only use Encodable + Decodable
and it is expected that this is equivalent to
alloy/crates/eips/src/eip2718.rs
Line 219 in 21bc07e
fn network_encode(&self, out: &mut dyn BufMut) { |
see:
alloy/crates/consensus/src/transaction/envelope.rs
Lines 329 to 332 in 21bc07e
impl Encodable for TxEnvelope { | |
fn encode(&self, out: &mut dyn alloy_rlp::BufMut) { | |
self.network_encode(out) | |
} |
because this type is the p2p type
/// Ethereum full block. | ||
/// | ||
/// Withdrawals can be optionally included at the end of the RLP encoded message. | ||
/// | ||
/// Taken from [reth-primitives](https://github.com/paradigmxyz/reth) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Think I've added what you're asking for but LMK if I'm missing something :) |
crates/consensus/src/block.rs
Outdated
/// Block header. | ||
pub header: Header, | ||
/// Block body. | ||
#[rlp(flatten)] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think #[rlp(flatten)]
works? this should probably be manual impl instead
* feat: consensus block type * fix: block type * fix: block type * alloc vec * bind to 2718 as well * fix: add requests * fix: remove 2718 encodables, docs * manual rlp --------- Co-authored-by: Arsenii Kulikov <klkvrr@gmail.com>
Description
Introduces a
Block
type for thealloy-consensus
crate. See #1290