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: simplify OpBuiltPayload #14152

Merged
merged 3 commits into from
Feb 2, 2025
Merged

chore: simplify OpBuiltPayload #14152

merged 3 commits into from
Feb 2, 2025

Conversation

klkvr
Copy link
Member

@klkvr klkvr commented Feb 1, 2025

There's no need to keep ChainSpec, attributes and blobs in OpBuiltPayload.

This should make it easier to reuse the type with custom blocks

Copy link
Collaborator

@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.

lgtm

Comment on lines +852 to +855
pub fn into_sealed_block(self) -> SealedBlock<N::Block> {
let block = Arc::unwrap_or_clone(self.block.recovered_block);
block.into_sealed_block()
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

nice, that makes sense

Comment on lines -144 to -150
/// The blobs, proofs, and commitments in the block. If the block is pre-cancun, this will be
/// empty.
pub(crate) sidecars: Vec<BlobTransactionSidecar>,
/// The rollup's chainspec.
pub(crate) chain_spec: Arc<OpChainSpec>,
/// The payload attributes.
pub(crate) attributes: OpPayloadBuilderAttributes,
Copy link
Collaborator

Choose a reason for hiding this comment

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

these can indeed go

/// Block execution data for the payload, if any.
pub(crate) executed_block: Option<ExecutedBlockWithTrieUpdates<OpPrimitives>>,
pub(crate) block: ExecutedBlockWithTrieUpdates<OpPrimitives>,
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this is a reasonable, if you need a remote builder you can still write your own builder

@@ -306,7 +280,8 @@ impl From<OpBuiltPayload> for OpExecutionPayloadEnvelopeV4 {
// Spec:
// <https://github.com/ethereum/execution-apis/blob/fe8e13c288c592ec154ce25c534e26cb7ce0530d/src/engine/cancun.md#specification-2>
should_override_builder: false,
blobs_bundle: sidecars.into_iter().collect::<Vec<_>>().into(),
// No blobs for OP.
blobs_bundle: BlobsBundleV1 { blobs: vec![], commitments: vec![], proofs: vec![] },
Copy link
Collaborator

Choose a reason for hiding this comment

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

@klkvr klkvr added this pull request to the merge queue Feb 2, 2025
Merged via the queue into main with commit 0c3cccc Feb 2, 2025
44 checks passed
@klkvr klkvr deleted the klkvr/simplify-op-payload branch February 2, 2025 16:44
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