evm: wider use of RethEvmBuilder
#9944
Conversation
tcoratger
requested review from
mattsse,
Rjected,
fgimenez,
rakita and
gakonst
as code owners
crates/optimism/evm/src/lib.rs
Outdated
| fn evm_with_env<DB: Database>( | ||
| &self, | ||
| db: DB, | ||
| env: EnvWithHandlerCfg, | ||
| ) -> Evm<'_, Self::DefaultExternalContext<'_>, DB> { | ||
| let mut evm = self.evm(db); | ||
| evm.modify_spec_id(env.spec_id()); | ||
| evm.context.evm.env = env.env; | ||
| evm | ||
| } |
There was a problem hiding this comment.
remind me, why do we need to impl this trait function now?
There was a problem hiding this comment.
Because in the old version, evm_with_env took as first line the result of self.evm() with a .optimism() component inside for the Optimism case.
But since now we use our RethEvmBuilder, we can no longer count on the same methodology, the implementation of evm_with_env being:
/// Returns a new EVM with the given database configured with the given environment settings,
/// including the spec id.
/// /// This will preserve any handler modifications fn evm_with_env<DB: Database>( &self, db: DB, env: EnvWithHandlerCfg, ) -> Evm<'_, Self::DefaultExternalContext<'_>, DB> { RethEvmBuilder::new(db, self.default_external_context()).with_env(env.into()).build() } ` and not taking into account `self.evm()` where the dependency on `.optimism` is present.
I wanted to tackle this in a follow up to propose something more unified or in this PR if you prefer.There was a problem hiding this comment.
hmm, tbh I'm very confused by this, not obvious to me what this pr achieves.
In general I find it hard to reason about what to invoke on evm and I don't fully get why we now need to override this function when it previously wasn't the case.
There was a problem hiding this comment.
Yes I understand, let me proposed something better in this pr in a bit :) will be clearer I think
There was a problem hiding this comment.
@mattsse I just proposed a very small change. I think we can merge like that. It's already a first path forward.
But to summarize for the rest, the problem I encounter here is with the .optimism() which only intervenes under the optimism cfg but which pollutes my construction with the Reth EVM builder because it intervenes before the build and in the three functions evm, evm_with_env and evm_with_inspector. This is what prevents for the moment to use the Reth EVM builder here.
emhane
added
the
C-debt
Follow up of #9813
Wider use of
RethEvmBuilderin the code.Another follow up will aim to use
RethEvmBuilderin the examples as well with the possibility to add precompiles directly.