Conversation
|
Review posted (see below) |
|
Placeholder - ignore |
|
test with backtick |
ReviewGood overall structure. The validation layering (CLI → builder → inline-config) is thorough and the idempotent system contract deployment mirrors mega-evme correctly. A few issues worth addressing: Must fix — Alloy exact-pin change needs justification (REVIEW.md rule) All alloy deps were bumped from 1.0.22 to =1.0.23 (exact pin) and alloy-primitives from 1.2.1 to 1.3.1. REVIEW.md requires that changes to alloy explain the fork-specific need and test coverage. The PR description is silent on why. If mega-evm 1.5.1 requires these exact versions to avoid a diamond-dep conflict, that should be stated explicitly. Must fix — MegaCtx is defined twice pub type MegaCtx is declared in both crates/evm/core/src/evm.rs and crates/evm/evm/src/inspectors/stack.rs. The definitions are structurally identical today, but if they ever drift the compiler will not catch it — callers silently get the wrong type. The stack.rs definition should be removed and replaced with: use foundry_evm_core::evm::MegaCtx; Should fix — Backend::is_megaeth is pub All other Backend fields are private; this one exposes mutable access to any caller. A stray backend.is_megaeth = true from an unrelated path could silently reroute execution. Use pub(crate) or a dedicated setter. Should fix — SystemTxInvalidCallee mapped to CallNotAllowedInsideStatic An invalid callee in a system transaction has nothing to do with static calls. Users who see CallNotAllowedInsideStatic in a MegaETH test failure will search for a staticcall that does not exist. At minimum add a comment documenting this lossy approximation. |
|
All four issues from the previous review have been addressed:
One new minor issue noted inline on |
|
LGTM. All feedback from both prior review rounds has been addressed:
No further concerns. |
Summary
Adds
--megaethmode toforge testandforge coverage, routing execution throughmega-evm1.5.1 while preserving Foundry's test / coverage / trace UX.Core integration
MegaEvmwith a dedicated inspector fan-out forMegaCtx(tracer / log collector / revert diagnostic wired; cheatcodes and isolation intentionally skipped).code_hashalready matches, preserve existingbalance/nonce— mirrors mega-evme's pattern.mega-evmExecutionResult/HaltReasoninto their revm counterparts for downstream reporting.Flag validation
New shared
EvmOpts::validate_megaeth()rejects unsupported combinations at fail-fast CLI layer, before any network request:--megaeth --isolate--megaeth --fork-url <URL>--megaeth --gas-reportforge-config: isolate = true+--megaethApplied to both
forge testandforge coverage. Coverage also now correctly forwards--isolate/--odysseyto the runner (previously silently dropped).Test plan
cargo nextest run -p forge --test cli -E 'test(~megaeth)'make testcargo fmt --all --checkcargo clippy --workspace --all-targets --all-featurescargo nextest run -p forge --test cli megaeth_live_cross_validate --run-ignored=only— passes locally and installsmega-evme@1.5.1from crates.io intotarget/mega-evme-1.5.1/on first runNote
Alloy version bump
alloy-*pinned to=1.0.23— required bymega-evm1.5.1'sCargo.toml.