refactor: migrate to factory-based block executor pattern #21
Conversation
📝 WalkthroughWalkthroughReplaces Eth-based executor/assembler with Morph-specific implementations (executor factory, executor, assembler, receipt builder), adds L1/token fee handling into execution and receipts, updates EVM config/context APIs and Cargo feature/dependency declarations, and moves some primitives feature gating. Changes
Sequence Diagram(s)sequenceDiagram
participant Config as MorphEvmConfig
participant Factory as MorphBlockExecutorFactory
participant EVM as MorphEvm
participant DB as State/DB
participant Receipt as DefaultMorphReceiptBuilder
Config->>Factory: create_executor(evm, ctx)
Factory->>EVM: construct MorphBlockExecutor(evm, spec, receipt_builder)
EVM->>DB: execute transactions (apply, call inspectors)
DB-->>EVM: execution results (gas_used, logs, state diffs)
EVM->>Receipt: build_receipt(MorphReceiptBuilderCtx{tx, result, l1_fee, morph_fields})
Receipt-->>EVM: MorphReceipt
EVM-->>Factory: return receipts, gas_used
Factory-->>Config: forward execution result
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
No actionable comments were generated in the recent review. 🎉 🧹 Recent nitpick comments
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@crates/evm/src/assemble.rs`:
- Around line 66-89: The Header construction in assemble_block currently sets
extra_data to Default::default(), dropping execution_ctx.extra_data; change the
header field initialization to use the extra data from the execution context
(e.g., replace extra_data: Default::default() with extra_data:
execution_ctx.extra_data.clone() or converted as needed) so the Header (inner)
preserves the block/payload extra_data; adjust any cloning or type conversion to
match the Header.extra_data type.
In `@crates/evm/src/block/mod.rs`:
- Around line 119-120: The code uses unwrap() on tx.fee_token_id() and
tx.fee_limit() which can panic; replace both unwrap() calls with fallible
unwrapping (e.g., tx.fee_token_id().ok_or(...)? and tx.fee_limit().ok_or(...)?),
returning a contextual error instead of panicking so the error propagates like
signer_unchecked does; include clear messages such as "missing fee_token_id for
morph tx" / "missing fee_limit for morph tx" when constructing the error so
callers can diagnose failures.
🧹 Nitpick comments (6)
crates/evm/Cargo.toml (1)
16-16:serde-bincode-compatfeature onmorph-primitivesis always enabled, making the feature gate partially redundant.Line 16 unconditionally enables
morph-primitives/serde-bincode-compat. The[features]section at lines 41–43 also listsmorph-primitives/serde-bincode-compat, but it has no additional effect since the feature is already always on. If the intent is to only enable it when downstream crates opt in, move it out of the default dependency features and keep it only in the[features]section. If it should always be on, remove it from the[features]section to avoid confusion.Also applies to: 41-43
crates/evm/src/block/receipt.rs (1)
91-106: Consider logging or warning whentoken_fee_infoisNonefor aMorphTx.A
MorphTx(0x7F) transaction is expected to carry token fee information. Silently falling back towith_l1_feecould mask upstream bugs where token fee info failed to be extracted. Atracing::warn!here would help surface such cases during debugging without breaking execution.🔍 Proposed logging addition
MorphTxType::Morph => { // MorphTx transactions include token fee information if let Some(token_info) = token_fee_info { MorphReceipt::Morph(MorphTransactionReceipt::with_morph_tx( inner, l1_fee, token_info.fee_token_id, token_info.fee_rate, token_info.token_scale, token_info.fee_limit, )) } else { - // Fallback: just include L1 fee if token info is missing + // Fallback: just include L1 fee if token info is missing. + // This is unexpected for MorphTx — log for diagnostics. + tracing::warn!(target: "morph::receipt", "MorphTx missing token_fee_info, falling back to L1 fee only"); MorphReceipt::Morph(MorphTransactionReceipt::with_l1_fee(inner, l1_fee)) } }crates/evm/src/block/factory.rs (1)
6-9: Minor: consolidateMorphEvmFactoryimport with the other crate-level imports.
use crate::evm::MorphEvmFactoryat line 21 could be merged into theuse crate::{...}block at lines 6–9.♻️ Proposed consolidation
use crate::{ block::{DefaultMorphReceiptBuilder, MorphBlockExecutor}, - evm::MorphEvm, + evm::{MorphEvm, MorphEvmFactory}, }; use alloy_evm::{ Database, @@ -19,8 +19,6 @@ use morph_revm::evm::MorphContext; use std::sync::Arc; -use crate::evm::MorphEvmFactory; -Also applies to: 21-21
crates/evm/src/lib.rs (1)
70-76: Consider encapsulatingblock_assemblerbehind an accessor for consistency.
executor_factoryis private with an accessor at line 94, whileblock_assembleris a public field. For consistent encapsulation, consider makingblock_assemblerprivate and adding apub fn block_assembler(&self)accessor, similar to howchain_spec()andevm_factory()are exposed.crates/evm/src/block/mod.rs (2)
43-56:PhantomData<I>is unnecessary sinceIis already constrained by theevmfield.The type parameter
Iappears inevm: MorphEvm<&'a mut State<DB>, I>, soPhantomData<I>adds no value. Removing it simplifies the struct.Proposed fix
pub(crate) struct MorphBlockExecutor<'a, DB: Database, I> { /// The EVM used by executor evm: MorphEvm<&'a mut State<DB>, I>, /// Chain specification spec: &'a MorphChainSpec, /// Receipt builder receipt_builder: &'a DefaultMorphReceiptBuilder, /// Receipts of executed transactions receipts: Vec<MorphReceipt>, /// Total gas used by executed transactions gas_used: u64, - /// Phantom data for inspector type - _phantom: PhantomData<I>, }And update the constructor accordingly:
Self { evm, spec, receipt_builder, receipts: Vec::new(), gas_used: 0, - _phantom: PhantomData, }
91-95: Duplicate hardfork determination logic.The block-number/timestamp extraction and
morph_hardfork_atcall is duplicated betweencalculate_l1_fee(Lines 92–95) andget_token_fee_info(Lines 123–126). Consider extracting a small helper likefn current_hardfork(&self) -> MorphHardforkto reduce duplication.Also applies to: 122-126
panos-xyz
changed the title
panos-xyz
force-pushed
the
refactor/receipt-builder
branch
from
55bad64 to
4e7facd
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@crates/evm/src/assemble.rs`:
- Around line 119-125: Update the comment that wraps the block in MorphHeader to
accurately describe which L2-specific fields are populated later: clarify that
next_l1_msg_index is set later by the payload builder (see the logic around
builder::build/handle in the payload builder), whereas batch_hash remains
Default::default() (B256::ZERO) here and is populated later by the batch
submitter; reference the MorphHeader struct and its fields next_l1_msg_index and
batch_hash so reviewers can easily locate the affected fields.
🧹 Nitpick comments (6)
crates/evm/src/assemble.rs (1)
82-83:U256::to::<u64>()will silently truncate ifnumberexceedsu64::MAX.While block numbers won't realistically exceed
u64::MAX, thetimestampconversion on line 103 has the same pattern. Both are safe in practice, but worth noting for consistency with the rest of the codebase.crates/evm/src/block/receipt.rs (1)
175-193: Silent fallback for missingMorphTxFieldson aMorphTxcould mask upstream bugs.When a transaction has type
Morph(0x7F) butmorph_tx_fieldsisNone, the code silently falls back towith_l1_fee. This is defensive, but aMorphTxwithout its fields is unexpected and likely indicates a bug inget_morph_tx_fields. Consider logging a warning here to aid debugging.Suggested improvement
MorphTxType::Morph => { // MorphTx transactions include MorphTx-specific fields if let Some(fields) = morph_tx_fields { MorphReceipt::Morph(MorphTransactionReceipt::with_morph_tx_v1( inner, l1_fee, fields.version, fields.fee_token_id, fields.fee_rate, fields.token_scale, fields.fee_limit, fields.reference, fields.memo, )) } else { - // Fallback: just include L1 fee if MorphTx fields are missing + // Fallback: just include L1 fee if MorphTx fields are missing. + // This should not happen in normal operation — log for debugging. + tracing::warn!("MorphTx receipt built without MorphTxFields — falling back to L1-fee-only receipt"); MorphReceipt::Morph(MorphTransactionReceipt::with_l1_fee(inner, l1_fee)) } }crates/evm/src/block/mod.rs (2)
63-76:PhantomData<I>is redundant —Ialready appears in theevmfield type.The type parameter
Iis used inevm: MorphEvm<&'a mut State<DB>, I>, so the compiler already knows howIrelates to the struct. The_phantom: PhantomData<I>field is unnecessary.Suggested cleanup
pub(crate) struct MorphBlockExecutor<'a, DB: Database, I> { /// The EVM used by executor evm: MorphEvm<&'a mut State<DB>, I>, /// Chain specification spec: &'a MorphChainSpec, /// Receipt builder receipt_builder: &'a DefaultMorphReceiptBuilder, /// Receipts of executed transactions receipts: Vec<MorphReceipt>, /// Total gas used by executed transactions gas_used: u64, - /// Phantom data for inspector type - _phantom: PhantomData<I>, }And update the constructor accordingly.
396-402:set_state_hookis a no-op — document if this is intentional or a TODO.If state hooks are needed for tracing/debugging in the future, this silent no-op could cause confusion. Consider adding a
TODOorFIXMEcomment if this is planned work.crates/evm/src/lib.rs (2)
111-118: Inconsistent field visibility:executor_factoryis private butblock_assembleris public.Consider making
block_assemblerprivate as well, since it's already accessible viaself.block_assembler()through theConfigureEvmtrait (inconfig.rsline 27). Direct field access bypasses any future encapsulation.Suggested change
pub struct MorphEvmConfig { /// Internal block executor factory that creates `MorphBlockExecutor` instances. executor_factory: MorphBlockExecutorFactory, /// Block assembler for building `MorphHeader` blocks from execution results. - pub block_assembler: MorphBlockAssembler, + block_assembler: MorphBlockAssembler, }
179-237: Test coverage is present but limited to hardfork querying.The test validates config construction and hardfork detection, which is good. Consider adding tests for
create_executorandevm_factorydelegation in a follow-up.
2 participants
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Chores