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.

Sign up for GitHub

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

Jump to bottom

feat(api,rpc): improve engine API abstraction #6871

Merged
merged 10 commits into from
Mar 5, 2024
Merged

feat(api,rpc): improve engine API abstraction #6871

merged 10 commits into from
Mar 5, 2024

Conversation

fgimenez
Copy link
Member

Closes #6734

@fgimenez fgimenez force-pushed the fgimenez/improve-engine-api-abstraction branch from 438f144 to e3182bb Compare February 29, 2024 10:56
mattsse
mattsse requested changes Feb 29, 2024
View reviewed changes
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.

I like this.

we have a few more complications with the OP V3 type

crates/node-optimism/src/engine.rs Outdated
@@ -15,6 +18,9 @@ impl EngineTypes for OptimismEngineTypes {
type PayloadAttributes = OptimismPayloadAttributes;
type PayloadBuilderAttributes = OptimismPayloadBuilderAttributes;
type BuiltPayload = EthBuiltPayload;
type ExecutionPayloadV1 = ExecutionPayloadV1;
type ExecutionPayloadV2 = ExecutionPayloadEnvelopeV2;
type ExecutionPayloadV3 = ExecutionPayloadEnvelopeV3;
Copy link
Collaborator

Choose a reason for hiding this comment

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

once merged we can add a dedicated OP V3 payload type in alloy because this is now diverged from ethereum

and I think we also want to change the BuiltPayload type to a new OptimismBuiltPayload that then should include the ChainSpec so we no longer need the additional check parent_beacon_block_root check

crates/rpc/rpc-engine-api/src/engine_api.rs Outdated
.map(|payload| payload.into_v1_payload())?)
.map_err(|_| EngineApiError::UnknownPayload)?
.try_into()
.map_err(|_| EngineApiError::UnknownPayload)
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'd like to log a warn!(err) here

crates/rpc/rpc-engine-api/src/engine_api.rs Show resolved Hide resolved
@mattsse mattsse added the C-enhancement New feature or request label Feb 29, 2024
@fgimenez fgimenez force-pushed the fgimenez/improve-engine-api-abstraction branch from e3182bb to c703308 Compare February 29, 2024 15:15
@fgimenez fgimenez changed the title Improve engine API abstraction feat(api,rpc): omprove engine API abstraction Feb 29, 2024
@fgimenez fgimenez changed the title feat(api,rpc): omprove engine API abstraction feat(api,rpc): improve engine API abstraction Feb 29, 2024
@fgimenez fgimenez force-pushed the fgimenez/improve-engine-api-abstraction branch 6 times, most recently from 717f8de to 87b8880 Compare February 29, 2024 18:26
@mattsse mattsse mentioned this pull request Mar 1, 2024
Merged
@fgimenez fgimenez force-pushed the fgimenez/improve-engine-api-abstraction branch from 87b8880 to 2216fb6 Compare March 1, 2024 09:52
@fgimenez fgimenez mentioned this pull request Mar 1, 2024
Merged
3 tasks
@fgimenez fgimenez force-pushed the fgimenez/improve-engine-api-abstraction branch 2 times, most recently from 1d4ea00 to 99f1fb0 Compare March 4, 2024 09:48
@fgimenez fgimenez mentioned this pull request Mar 4, 2024
Merged
3 tasks
@fgimenez fgimenez force-pushed the fgimenez/improve-engine-api-abstraction branch 2 times, most recently from c668de6 to fbfc2bf Compare March 4, 2024 15:07
@fgimenez fgimenez marked this pull request as ready for review March 4, 2024 15:09
@fgimenez fgimenez requested a review from mattsse March 4, 2024 15:10
mattsse
mattsse requested changes Mar 5, 2024
View reviewed changes
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.

great!

last nits

crates/payload/optimism/src/lib.rs Outdated
Comment on lines 43 to 44
#[derive(Debug, Clone, PartialEq, Eq, Default)]
#[non_exhaustive]
Copy link
Collaborator

Choose a reason for hiding this comment

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

we can remove the default+ non_exhaustive now to prevent default chainspec and instead require at least the Chainspec in a new function because we want to prevent misuse of Chainspec::default

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure, done

crates/payload/builder/src/optimism.rs Outdated Show resolved Hide resolved
crates/payload/builder/src/optimism.rs Outdated
Comment on lines 276 to 264
attributes.parent_beacon_block_root().unwrap_or_else(FixedBytes::<32>::default)
} else {
FixedBytes::<32>::default()
};
Copy link
Collaborator

Choose a reason for hiding this comment

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

this should now return an error because it is required for v3

crates/payload/builder/src/optimism.rs Outdated Show resolved Hide resolved
crates/payload/builder/src/optimism.rs Outdated
if chain_spec.is_cancun_active_at_timestamp(attributes.timestamp()) {
attributes.parent_beacon_block_root().unwrap_or_else(FixedBytes::<32>::default)
} else {
panic!("V3 is only possible after cancun");
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 we also need to set the zero hash here (pre cancun):

Starting at Ecotone, the parentBeaconBlockRoot must be set to the L1 origin parentBeaconBlockRoot, or a zero bytes32 if the Dencun functionality with parentBeaconBlockRoot is not active on L1.

https://github.com/ethereum-optimism/specs/blob/main/specs/protocol/exec-engine.md#engine_getpayloadv3

but perhaps we should treat this as an error and use TryFrom impl

Copy link
Member Author

Choose a reason for hiding this comment

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

Ok, for now I have set the zero hash for the pre cancun case. If we want to impl TryFrom, what should be the error type?

Copy link
Collaborator

Choose a reason for hiding this comment

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

hmm, need to think about this

crates/payload/builder/src/optimism.rs Outdated Show resolved Hide resolved
@fgimenez fgimenez requested a review from onbjerg as a code owner March 5, 2024 15:13
@fgimenez fgimenez force-pushed the fgimenez/improve-engine-api-abstraction branch from 29ea33d to c8bd102 Compare March 5, 2024 15:27
@mattsse mattsse added this pull request to the merge queue Mar 5, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Mar 5, 2024
@mattsse mattsse added this pull request to the merge queue Mar 5, 2024
Merged via the queue into paradigmxyz:main with commit 96fcdfb Mar 5, 2024
30 checks passed
rkrasiuk pushed a commit that referenced this pull request Mar 9, 2024
@fgimenez @mattsse @rkrasiuk
feat(api,rpc): improve engine API abstraction (#6871)
e320968 
Co-authored-by: Matthias Seitz <[email protected]>
@rkrasiuk rkrasiuk mentioned this pull request Mar 9, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mattsse mattsse mattsse approved these changes

@Rjected Rjected Awaiting requested review from Rjected Rjected is a code owner

@gakonst gakonst Awaiting requested review from gakonst gakonst is a code owner

@onbjerg onbjerg Awaiting requested review from onbjerg onbjerg is a code owner

Assignees
No one assigned
Labels
C-enhancement New feature or request
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Improve Engine API abstraction
2 participants
@fgimenez @mattsse