New interface feedback for cheatcode inspector

[agostbiro]

Hey,

following #1865 I did a spike to implement a Foundry-style cheatcode inspector using the new interfaces. I really like the new design, but I ran into difficulties when trying to implement the transact and rollFork(uint256 forkId, bytes32 transaction) cheatcodes. Both of these cheatcodes initiate transactions from a call step in the cheatcode inspector.

I'll detail the issues below. I created a minimal repro with comments explaining the setup, but it's still ~500 LOC, so I will link to the relevant sections.

JournaledState<DB>

Moving the database inside the journal state in the EVM context prevents mutably borrowing both at the same from a &mut Context. This causes problems detailed here. There is also a failing test case related to this here.

I think this could be solved by lifting the DB to the Context and introducing a temporary struct that has mutable journaled state and database fields and implementing the JournaledState trait for this temporary struct.

Precompiles

When using an inspector, the PrecompileProvider needs an InspectorContext as its context. The InspectorContext needs to know the inspector type and the database type. This prevents declaring a method that takes the precompile provider as a generic parameter and constructs an inspector in the body to execute a transaction. There is an example here.

I think this could be solved by just passing the block, transaction and config environments to the PrecompileProvider instead of the entire context.

I also noticed that precompiles are no longer accessible in Inspector::call. Previously this was available via EvmContext::precompiles, but as far as I can tell, removing this doesn't block any cheatcode functionality.

Missing implementations

The following seems to be only implemented for mainnet types:

• Evm::transact() implementation hard-codes mainnet HaltReason in the EVMResult return type.
• InspectorCtx for InspectorContext implementation hard-codes mainnet EthInterpreter. It should be possible to pass a generic interpreter type here.

I also noticed that NoOpInspector::default() doesn't work with mainnet types due to unsatisfied trait bounds.

---

It's possible that I overlooked something and there is a solution for the mentioned issues. I'm also happy to submit PRs to resolve them. Let me know please!

[rakita]

Will go over all points but for this point

Evm::transact() implementation hard-codes mainnet HaltReason in the EVMResult return type.

Just made a PR: #1931 this makes output Generic and work for fn transact (and new fn exec)

[rakita]

When using an inspector, the PrecompileProvider needs an InspectorContext as its context. The InspectorContext needs to know the inspector type and the database type. This prevents declaring a method that takes the precompile provider as a generic parameter and constructs an inspector in the body to execute a transaction. There is an example here.

PrecompileProvider has a nice abstraction but that does not mean you should use it directly. I will need to think about this, it sucks a little that PrecompileProvider is initialized from trait and can't be accessible from evm.handler.exec... to for example add precompiles. I see that this is missing for op-revm.

I think this could be solved by just passing the block, transaction and config environments to the PrecompileProvider instead of the entire context.

There were requests for the context and access to the Database. One of example is #1867
It can be valuable, that is why full context is available.

I also noticed that precompiles are no longer accessible in Inspector::call. Previously this was available via EvmContext::precompiles, but as far as I can tell, removing this doesn't block any cheatcode functionality.

They are not accessible, they are moved from context to the Handler (Frame handler) as it fits better, it seems it is not a big change as you usually want to setup precompiles before running the tx.

[rakita]

JournaledState
Moving the database inside the journal state in the EVM context prevents mutably borrowing both at the same from a &mut Context. This causes problems detailed here. There is also a failing test case related to this here.

       context
            .journaled_state
            .database
            // TODO this clone causes test failure
            .clone()
            .method_that_constructs_inspector(
                &mut context.journaled_state,
                Env {
                    block: context.block.clone(),
                    tx: context.tx.clone(),
                    cfg: context.cfg.clone(),
                },
            );,

It looks to me that this can be done as:

       context
            .journaled_state
            .method_that_constructs_inspector(
                Env {
                    block: context.block.clone(),
                    tx: context.tx.clone(),
                    cfg: context.cfg.clone(),
                },
            );,

You could implement those functions on top of the Journal (Ups i still need to make it as generic item in Context) as you require both of this. We can figure something out.

I think this could be solved by lifting the DB to the Context and introducing a temporary struct that has mutable journaled state and database fields and implementing the JournaledState trait for this temporary struct.

That was previous state, and i got a lot of self.journal.load(address, &mut self.database). This order feels more natural.

[agostbiro]

Thanks for the detailed reply and for the fixes @rakita !

I upgraded the repro to the latest REVM main version and I'm happy to report that the approach with the generic journaled state works well and tests pass now. The downside is that I had to copy over much of the existing journaled state implementation.

[agostbiro]

Actually I realize now there is no need to have a custom journaled state implementation. Fixed in agostbiro/multichain-cheatcodes@f78cb24.