Skip to content

feat: add ChainSpec AT to EngineTypes#11054

Merged
klkvr merged 2 commits intomainfrom
klkvr/engine-chainspec
Sep 23, 2024
Merged

feat: add ChainSpec AT to EngineTypes#11054
klkvr merged 2 commits intomainfrom
klkvr/engine-chainspec

Conversation

@klkvr
Copy link
Member

@klkvr klkvr commented Sep 19, 2024

Currently we use concrete chainspec on EngineTypes, we need to change that to make it possible to make engine generic over chainspec.

This could be approached differently with a generic on EngineTypes — I don't have strong opinion here, though adding it as a generic would require additional generic on engine types which I think would be nice to avoid

Copy link
Collaborator

@emhane emhane left a comment

Choose a reason for hiding this comment

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

seems a bit double up, like an indicator we need to take a step back, and revise the overall design, make a dependency graph. what's the complete list of type that need chainspec associated type? does payload builder need it too for example?

@klkvr
Copy link
Member Author

klkvr commented Sep 20, 2024

seems a bit double up, like an indicator we need to take a step back, and revise the overall design, make a dependency graph. what's the complete list of type that need chainspec associated type? does payload builder need it too for example?

I think generally for components we can take one of the 2 approaches wrt chainspec — given that it's immutable it's not a big deal to just keep a copy of it on every component, and removing it from trait method signatures. we took this approach for ConfigureEvm to avoid introducing more generics in #10748

with EngineTypes it's a bit trickier as it only needs chainspec for validate_version_specific_fields which doesn't have a receiver, and refactoring existing code to make EngineTypes stateful would be more expensive than just adding a generic imo

for payload builder we should probably take other approach and avoid generic, especially given that current builder implementations already keep a ConfigureEvm instance and thus a chainspec which we can reuse

in general I think it's fine for any component to rely on chainspec? given that it contains core chain configuration which is often useful

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 think this makes sense (atm)

this function is a bit out of place here and also only works because our executionpayload inputs are the same for op and eth.

I assume we can even remove this function and move it into a new trait/type that is a Validator or similar, that the EngineApi impl then uses which wraps the chainspec and exposes the functions that are used in here:

/// Consensus configuration
chain_spec: Arc<ChainSpec>,

// validate timestamp according to engine rules
validate_payload_timestamp(
&self.inner.chain_spec,
EngineApiMessageVersion::V2,
attributes.timestamp(),
)?;

something like:

trait EngineApiPayloadValidator<PayloadTypes> {
  
  fn validate_payload_timestamp(&self;

  fn validate_version_specific_fields(&self 
}

perhaps

@joshieDo joshieDo added the A-sdk Related to reth's use as a library label Sep 21, 2024
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.

this seems appropriate rn, but I think the best solution would still be making the validator stateful, but we can tackle this separately.

@klkvr klkvr added this pull request to the merge queue Sep 23, 2024
Merged via the queue into main with commit cf294ce Sep 23, 2024
@klkvr klkvr deleted the klkvr/engine-chainspec branch September 23, 2024 15:16
@klkvr klkvr mentioned this pull request Sep 23, 2024
theochap pushed a commit to ethereum-optimism/optimism that referenced this pull request Jan 22, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

A-sdk Related to reth's use as a library

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants