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: add activation_block method for Ethereum hardforks #5723

Merged
merged 6 commits into from
Jan 13, 2024

Conversation

tcoratger
Copy link
Contributor

Should close #5707.

@tcoratger tcoratger requested a review from gakonst as a code owner December 10, 2023 22:38
@@ -51,6 +51,37 @@ pub enum Hardfork {
Cancun,
}

impl Hardfork {
/// Returns the activation block number for each Ethereum hardfork.
pub fn activation_block(&self) -> Option<u64> {
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 rename this to mainnet_activation_block

but what we want here is activation_block(&self, chain: Chain)
see referenced foundry issue.
I'm fine if fn activation_block only handles Chain::mainnet in this PR
we don't have to fill all blocks for other chains, just to have it so they can be added one by one.
this will mostly be useful for foundry because I intend to use it in anvil eventually

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@mattsse In this case, isn't it better to implement this method for the enum Chain itself by doing activation_block(&self, hardfork: Hardfork)? Because otherwise we have a circular dependency, right? Because the reth-primitives crate itself uses the Hardfork enum. Maybe I'm missing something here.

Also, I wanted to fill in the block activations for certain L2s but do you know where I could have the precise information for each of them? There is no equivalent to the precise Ethereum specification https://ethereum.github.io/execution-specs/autoapi/ethereum/index.html for the other L2s.

Copy link
Collaborator

Choose a reason for hiding this comment

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

we'd like to migrate to alloy-chains now #5429 which gets rid of the circular dep issue, looks like this is a blocker for this PR

I want this functionality in Hardfork and not chainid, so chain crate stays lean

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@mattsse Please tell me if this is better now.

@github-actions github-actions bot added the S-stale This issue/PR is stale and will close with no further activity label Jan 9, 2024
@gakonst
Copy link
Member

gakonst commented Jan 12, 2024

cc @mattsse @tcoratger alloy_chains is now merged as of #5952

@github-actions github-actions bot removed the S-stale This issue/PR is stale and will close with no further activity label Jan 13, 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.

great!

I only rearranged the hardfork match arms

@tcoratger
Copy link
Contributor Author

great!

I only rearranged the hardfork match arms

Thanks :)

@mattsse mattsse enabled auto-merge January 13, 2024 10:22
@mattsse mattsse added the C-enhancement New feature or request label Jan 13, 2024
@mattsse mattsse added this pull request to the merge queue Jan 13, 2024
Merged via the queue into paradigmxyz:main with commit b08371b Jan 13, 2024
28 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Implement Activation Block Data for Ethereum Hardforks
3 participants