-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
refactor: use EvmEnv
when setting up Evm
#13800
Conversation
Evm
EvmEnv
when setting up Evm
382c38a
to
199435a
Compare
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.
nice, this is a lot cleaner because this further separates evm(env) from the tx, because we really need this to be two different things
cfg_env_with_handler_cfg, | ||
block_env, |
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.
yeah this is a lot better
crates/evm/src/lib.rs
Outdated
/// including the spec id. | ||
/// | ||
/// This will preserve any handler modifications |
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.
can we update the docs here that this configures the evm for the block and tx
Self { | ||
cfg_env_with_handler_cfg: CfgEnvWithHandlerCfg { | ||
cfg_env: Default::default(), | ||
handler_cfg: Default::default(), |
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.
can we add a sanity doc here that handler_cfg::default will configure optimism if revm's optimism feature is enabled?
let tx_env = RpcNodeCore::evm_config(&this).tx_env(tx, *signer); | ||
let (res, _) = this.transact(&mut db, evm_env.clone(), tx_env)?; |
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.
great, incrementally better
We've introduced
EvmEnv
to encapsulate Evm configuration not related to transaction, however most of the logic still operates on it directly to convert intorevm::Env
. This makes abstracting overEvmEnv
andTxEnv
not feasibleThis PR changes logic to operate directly on
EvmEnv
as much as possible, only converting it torevm::Env
when Evm is actually being createdThis allows us to
TxEnv
because it is now always passed separatelyEvmEnv
easier because it is now also properly separated