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(sdk): ChainSpec generic over hardforks #10943

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

emhane
Copy link
Member

@emhane emhane commented Sep 16, 2024

Closes #10468

Makes ChainSpec generic over hard forks, leaving ChainHardforks as default generic.

@emhane emhane added C-debt Refactor of code section that is hard to understand or maintain A-op-reth Related to Optimism and op-reth A-sdk Related to reth's use as a library labels Sep 16, 2024
crates/chainspec/src/api.rs Outdated Show resolved Hide resolved
@klkvr
Copy link
Collaborator

klkvr commented Sep 18, 2024

hmm, wondering what's the plan here? reuse ChainSpec with different generic for optimism? what would that generic look like?

@emhane
Copy link
Member Author

emhane commented Sep 18, 2024

hmm, wondering what's the plan here? reuse ChainSpec with different generic for optimism? what would that generic look like?

yepp. and any rollup with same chain spec type asides hard forks. it's a building block, to make it easier for rollups to enjoy the customisability of the new reth sdk.

@mattsse
Copy link
Collaborator

mattsse commented Sep 18, 2024

hmm, the chainspec already has some additional chain specific internals, like deposit contract and for other chains it's likely that some additional fields should be included there as well.

so it's imo easier to roll your own, like we already prepped with OpChainspec

@klkvr
Copy link
Collaborator

klkvr commented Sep 18, 2024

hmm, wondering what's the plan here? reuse ChainSpec with different generic for optimism? what would that generic look like?

yepp. and any rollup with same chain spec type asides hard forks. it's a building block, to make it easier for rollups to enjoy the customisability of the new reth sdk.

makes sense!

I've changed EthereumHardforks trait a bit in #10978, because it seemed reasonable to move final_paris_total_difficulty fn to it. because of that, we now have to implement/require trait on ChainSpec directly which I think breaks this usecase a bit. should we consider moving paris_block_and_final_difficulty to ChainHardforks to support this?

@emhane
Copy link
Member Author

emhane commented Sep 18, 2024

hmm, wondering what's the plan here? reuse ChainSpec with different generic for optimism? what would that generic look like?

yepp. and any rollup with same chain spec type asides hard forks. it's a building block, to make it easier for rollups to enjoy the customisability of the new reth sdk.

makes sense!

I've changed EthereumHardforks trait a bit in #10978, because it seemed reasonable to move final_paris_total_difficulty fn to it. because of that, we now have to implement/require trait on ChainSpec directly which I think breaks this usecase a bit. should we consider moving paris_block_and_final_difficulty to ChainHardforks to support this?

let's move it to Hardforks trait. it's supertrait of EthereumHardforks.

@emhane
Copy link
Member Author

emhane commented Sep 18, 2024

hmm, the chainspec already has some additional chain specific internals, like deposit contract and for other chains it's likely that some additional fields should be included there as well.

so it's imo easier to roll your own, like we already prepped with OpChainspec

it's written as a bullet point in your notion sdk notes?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-op-reth Related to Optimism and op-reth A-sdk Related to reth's use as a library 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.

Make reth_chainspec::ChainSpec generic over hard fork
4 participants