ithacaxyz / odyssey

A testnet open-source Layer 2 from the future, co-designed with the developer tools stack.
https://www.ithaca.xyz/updates/odyssey
Apache License 2.0
137 stars 28 forks source link

Possible redundant reset `HandlerCfg` #43

Open 0xurb opened 1 day ago

0xurb commented 1 day ago

Not sure also about that

https://github.com/ithacaxyz/odyssey/blob/811f87443605fc181ed00fb2099ef361d4444447/crates/node/src/evm.rs#L145-L146

Because, Odyssey's evm used reth's trait ConfigureEvm with .optimism(). So, we are set spec id here and is_optimism = true on ConfigureEvm::evm (via optimism register handler) than do it again via ConfigureEvmEnv::fill_cfg_and_block_env?

onbjerg commented 1 day ago

I don't follow, can you rephrase?

0xurb commented 1 day ago

For sure. On Evm configuration we are setup evm builder with .optimism() here: https://github.com/ithacaxyz/odyssey/blob/811f87443605fc181ed00fb2099ef361d4444447/crates/node/src/evm.rs#L222-L234

That means we are modifying handler config with:

    /// Handler for optimism
    #[cfg(feature = "optimism")]
    pub fn optimism<SPEC: Spec>() -> Self {
        let mut handler = Self::mainnet::<SPEC>();
        handler.cfg.is_optimism = true;
        handler.append_handler_register(HandleRegisters::Plain(
            crate::optimism::optimism_handle_register::<DB, EXT>,
        ));
        handler
    }

Usage in reth for example here https://github.com/paradigmxyz/reth/blob/c4d7b591834055bdf3afed96b696e0a2cef0f383/crates/optimism/evm/src/execute.rs#L276-L288

Question: do we need to do below, as we are already set that by calling .optimism(): https://github.com/ithacaxyz/odyssey/blob/811f87443605fc181ed00fb2099ef361d4444447/crates/node/src/evm.rs#L145-L146