wip: better custom evm abstraction #843
Conversation
|
I'm a fan of the direction of this. If the reth dependency is removed in favor of these traits being upstreamed in alloy, down to merge! |
| // ZTODO: Use custom chainspec here, not defaulting to OP Mainnet | ||
| Arc::new(OpChainSpecBuilder::optimism_mainnet().build()), | ||
| ), |
There was a problem hiding this comment.
@clabby Is this client only for OP Mainnet (ie is this safe) or should it work for any chain that uses FPVM? If the latter, I'll construct ChainSpec out of data in the RollupConfig.
There was a problem hiding this comment.
It should work for any chain with a RollupConfig, yep!
| timestamp: u64, | ||
| block_number: u64, | ||
| parent_beacon_block_root: Option<B256>, | ||
| evm: &mut Evm<'_, (), &mut State<&mut TrieDB<F, H>>>, | ||
| // ZTODO: understand default external context better, is it ok to allow any here? | ||
| evm: &mut Evm<'_, C::DefaultExternalContext<'_>, &mut State<&mut TrieDB<F, H>>>, |
There was a problem hiding this comment.
@clabby I think this change is fine to allow the Evm to be parameterized by any DefaultExternalContext rather than forcing it to be (). Do you see any problem with that?
There was a problem hiding this comment.
This is all good. Right now the enforcement on () limits a degree of freedom that consumers might want.
| .modify() | ||
| .with_tx_env(Self::prepare_tx_env(&transaction, raw_transaction)?) | ||
| .build(); | ||
| let reth_tx = TransactionSigned::decode_2718(&mut raw_transaction).map_err(ExecutorError::RLPError)?; |
There was a problem hiding this comment.
@clabby This decoding is failing in the tests. Having trouble figuring out why. If you have any instincts, I'd love your input as I keep digging.
There was a problem hiding this comment.
Likely because some of these txs are deposits. I assume TransactionSigned may not support the deposit (0x7e) variant, given that no features are enabled for reth-primitives.
addresses #839
to do:
ConfigureEvmEnvabstractionDefaultEvmConfigimpl ofConfigureEvmEnv