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

evm: use Header AT in ConfigureEvmEnv #10968

Merged
merged 9 commits into from
Sep 17, 2024
Merged

Conversation

tcoratger
Copy link
Contributor

@tcoratger tcoratger commented Sep 17, 2024

Related #10746

Should close #10971

Comment on lines +173 to +192
fn fill_block_env(&self, block_env: &mut BlockEnv, header: &Self::Header, after_merge: bool) {
block_env.number = U256::from(header.number);
block_env.coinbase = header.beneficiary;
block_env.timestamp = U256::from(header.timestamp);
if after_merge {
block_env.prevrandao = Some(header.mix_hash);
block_env.difficulty = U256::ZERO;
} else {
block_env.difficulty = header.difficulty;
block_env.prevrandao = None;
}
block_env.basefee = U256::from(header.base_fee_per_gas.unwrap_or_default());
block_env.gas_limit = U256::from(header.gas_limit);

// EIP-4844 excess blob gas of this block, introduced in Cancun
if let Some(excess_blob_gas) = header.excess_blob_gas {
block_env.set_blob_excess_gas_and_price(excess_blob_gas);
}
}

Copy link
Member

Choose a reason for hiding this comment

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

this code is repeated 3 times. pls open issue to make this the default impl for the trait method when alloy-rs/alloy#1301 is complete.

Copy link
Collaborator

Choose a reason for hiding this comment

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

can you elaborate, not really following how that issue is related here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done #10971

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 I have the impression that what is pointed out here is the fact that since we are now using an associated type for header, in the current state of the code, we are no longer able to provide a default implementation for fill_block_env in the definition of ConfigureEvmEnv directly (because we no longer have access to header.number for example).

So we have to repeat this same implementation each time we implement the ConfigureEvmEnv trait, which gives us the same piece of code 3 times here.

In alloy it is proposed to add a BlockHeader trait that Header will have to implement and which will allow direct access to the fields via getters to put this default implementation back in ConfigureEvmEnv.

Here is the related PR on alloy as a reference alloy-rs/alloy#1302

@emhane emhane added this pull request to the merge queue Sep 17, 2024
Merged via the queue into paradigmxyz:main with commit 5e9f381 Sep 17, 2024
36 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

evm: have fill_block_env default impl in ConfigureEvmEnv
3 participants