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

feat: ChainSpec associated type #10292

Merged
merged 12 commits into from
Aug 22, 2024
Merged

feat: ChainSpec associated type #10292

merged 12 commits into from
Aug 22, 2024

Conversation

klkvr
Copy link
Collaborator

@klkvr klkvr commented Aug 14, 2024

First step for #10272

Adds ChainSpec trait and assoc type to ChainSpecProvider and NodeTypes, for now most of the logic is hardcoded to use the default ChainSpec

crates/chainspec/src/lib.rs Outdated Show resolved Hide resolved
crates/chainspec/src/trait.rs Outdated Show resolved Hide resolved
@@ -231,7 +232,7 @@ where
+ BlockIdReader
+ CanonChainTracker
+ StageCheckpointReader
+ ChainSpecProvider
+ ChainSpecProvider<ChainSpec = ChainSpec>
Copy link
Member

Choose a reason for hiding this comment

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

nice, setting the AT to a concrete type makes it possible to impl this in incremental prs.

Copy link
Collaborator

Choose a reason for hiding this comment

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

this is great and allows us to gradually add support for chainspec trait across more components

Comment on lines 30 to 31
/// The type used for configuration of the EVM.
type ChainSpec: ChainSpecTrait;
Copy link
Member

Choose a reason for hiding this comment

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

what's your argument for finding it appropriate to put ChainSpec here? I'd argue it's more appropriate as AT in FullNodeTypes trait, because looking at the other ATs contained in both traits NodeTypes and FullNodeTypes, ChainSpec semantically fits better into FullNodeTypes - the chain spec is more about initialisation of the chain, like db and provider, rather than compilation of the reth program for a specific network, like primitives and engine types.

Copy link
Collaborator Author

@klkvr klkvr Aug 15, 2024

Choose a reason for hiding this comment

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

I don't have strong opinion here, so would be happy to move it. My understanding was that FullNodeTypes keeps stateful components which are expected to change their behavior during node operation, while NodeTypes configures underlying types/configs for those components

Copy link
Member

Choose a reason for hiding this comment

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

is the chain spec stateless? what about hardforks?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

is the chain spec stateless? what about hardforks?

It can't change on its own during node operation (thus allowing us to keep it in an Arc everywhere). Primitive types can also change between hardforks

@emhane emhane added the C-debt Refactor of code section that is hard to understand or maintain label Aug 21, 2024
@emhane
Copy link
Member

emhane commented Aug 21, 2024

pending @mattsse or @joshieDo

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 is great!

this is exactly what we're looking for,

I think we can merge this as is, so more incremental work is unlocked.

cc @joshieDo I want to find a way to integrate this together with your generic type work, I think we should actually try to make NodeType the generic that we pass to everything

smol conflict @klkvr

crates/chainspec/src/api.rs Show resolved Hide resolved
@@ -231,7 +232,7 @@ where
+ BlockIdReader
+ CanonChainTracker
+ StageCheckpointReader
+ ChainSpecProvider
+ ChainSpecProvider<ChainSpec = ChainSpec>
Copy link
Collaborator

Choose a reason for hiding this comment

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

this is great and allows us to gradually add support for chainspec trait across more components

@emhane emhane added this pull request to the merge queue Aug 22, 2024
Merged via the queue into main with commit f2e0bc0 Aug 22, 2024
35 checks passed
@emhane emhane deleted the klkvr/chainspec branch August 22, 2024 12:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-debt Refactor of code section that is hard to understand or maintain
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants