-
Notifications
You must be signed in to change notification settings - Fork 2k
bump(revm
: step 3): reintroduce precompile injection
#10508
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
base: yash/revm-alloy-bumps
Are you sure you want to change the base?
bump(revm
: step 3): reintroduce precompile injection
#10508
Conversation
revm
: step 3): (draft) reintroduce precompile injection
|
||
/// Object-safe trait that enables injecting extra precompiles when using | ||
/// `anvil` as a library. | ||
pub trait PrecompileFactory: Send + Sync + Unpin + Debug { | ||
/// Returns a set of precompiles to extend the EVM with. | ||
fn precompiles(&self) -> Vec<(Address, Precompiles)>; | ||
fn precompiles(&self) -> Vec<PrecompileWithAddress>; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is breaking but its fine. This feature is mostly geared towards anvil as a library users. There aren't many I would assume.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm!
revm
: step 3): (draft) reintroduce precompile injectionrevm
: step 3): reintroduce precompile injection
@@ -472,7 +472,7 @@ where | |||
evm_context, | |||
inspector, | |||
EthInstructions::default(), | |||
FoundryPrecompiles::new(), | |||
PrecompilesMap::from_static(EthPrecompiles::default().precompiles), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this doesn't look right, we should respect the specid here like e.g here https://github.com/nadtech-hub/evm/blob/180b91f3d22a2ae79ba2afbefb520f711f450a85/crates/evm/src/eth/mod.rs#L239-L241
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since tests are passing, can you suggest a test to add to avoid such regression?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
one way to test it is to make sure that BLS precompile is not available before Prague as the bug is that all precompiles are always activated
it's not that important for foundry though I guess, would've been a more severe bug in reth :D
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
…ely based on the spec defined
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
Motivation
Closes: #10411
Builds on top of: #10454 which in turns builds on top of #10183
Solution
Uses alloy-evm's new dynamic precompile injection
Adds tests for OpEvm and EthEvm
Breaking changes
Vec<(Address, Precompiles)>
signature to beVec<PrecompileWithAddress>
as Revm uses this internally and it simplifies the carrying. For end-users the change is expected to be straightforward, they just wrap usingPrecompileWithAddress::from(..)
PR Checklist