test: add unit tests across all morph-reth crates#62
Merged
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
💤 Files with no reviewable changes (1)
📝 WalkthroughWalkthroughAdds extensive unit tests across many crates, removes the Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested labels
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)
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 |
chengwenxi
approved these changes
Mar 31, 2026
Contributor
chengwenxi
left a comment
chengwenxi
left a comment
There was a problem hiding this comment.
LGTM. 500+ unit tests across all crates — excellent coverage improvement. Production code changes are correct refactors. Minor items for follow-up:
- Remove duplicate
use alloy_consensus::Transaction as TransactionTraitintransaction.rs - Add
prevrandao == B256::ZEROassertion totest_next_evm_env_increments_block_number - Restore or replace
test_validate_l1_messages_in_block_skipped_messages_allowed(or document new semantics from PR #59) - Remove unnecessary
use std::boxed::Box/use std::string::Stringinprecompiles.rs
Add #[cfg(test)] modules to every crate with comprehensive coverage: - chainspec: constants, hardfork ordering, chain spec construction - primitives: header, receipt envelope, transaction encoding/decoding - revm: error types, EVM config, L1 block fee calculation, precompiles, token fee logic, transaction environment - evm: block assembler, receipt builder, EVM config hardfork mapping - consensus: header validation, L1 message sequencing, timestamp rules - txpool: error types, MorphTx validation, transaction type handling - payload-builder: execution info, builder configuration - payload-types: attributes, executable/safe L2 data, params - engine-api: builder state tracker, error types, validator - rpc: error types, receipt/transaction serialization, request types - node: CLI args, engine validator Minor accompanying refactors: - Remove unused From<MorphConsensusError> for ConsensusError impl - Remove unused morph-evm dev-dependency from revm crate - Rename query_erc20_balance to query_balance_via_system_call - Clean up EIP-4788 trace override imports
panos-xyz
force-pushed
the
panos/unit-tests
branch
from
424e915 to
880a235
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/revm/src/token_fee.rs (1)
248-269:⚠️ Potential issue | 🟠 MajorPropagate DB errors out of the new helper.
read_token_balance_with_fallback()still distinguishesEVMError::Database(_)from other call failures, but this helper now converts everyErrintoOk(U256::ZERO). That masks storage/backend failures as a zero balance and turnsTokenInfoFetchFailedinto spurious balance validation errors.Suggested fix
fn query_balance_via_system_call<DB, I>( evm: &mut MorphEvm<DB, I>, token: Address, account: Address, ) -> Result<U256, EVMError<DB::Error, MorphInvalidTransaction>> where DB: Database, { let calldata = encode_balance_of_calldata(account); match evm.system_call_one(token, calldata) { Ok(result) if result.is_success() => { if let Some(output) = result.output() && output.len() >= 32 { return Ok(U256::from_be_slice(&output[..32])); } Ok(U256::ZERO) } Ok(_) => Ok(U256::ZERO), - Err(_) => Ok(U256::ZERO), + Err(err) => Err(err), } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/revm/src/token_fee.rs` around lines 248 - 269, The helper query_balance_via_system_call currently swallows all evm.system_call_one errors and returns Ok(U256::ZERO), masking real DB/backend failures; change the Err(_) arm to propagate the error (return Err(e)) so EVMError::Database(...) is forwarded to callers (e.g., read_token_balance_with_fallback) instead of being treated as a zero balance; keep the existing behavior for Ok(result) when the call is non-success or output is missing (return Ok(U256::ZERO)), but ensure the Err path returns the original EVMError from system_call_one.
🧹 Nitpick comments (5)
crates/txpool/src/transaction.rs (1)
326-332: Tighten the cache test to assert identity, not just equal bytes.
test_encoded_2718_is_cachedcurrently proves deterministic encoding, but not that the same cached instance is reused. Consider asserting pointer identity too.♻️ Suggested improvement
#[test] fn test_encoded_2718_is_cached() { let tx = create_legacy_pooled_tx(); - let bytes1 = tx.encoded_2718().clone(); - let bytes2 = tx.encoded_2718().clone(); - assert_eq!(bytes1, bytes2, "cached encoding should be identical"); - assert!(!bytes1.is_empty()); + let bytes1 = tx.encoded_2718(); + let bytes2 = tx.encoded_2718(); + assert!(std::ptr::eq(bytes1, bytes2), "should return the same cached Bytes instance"); + assert!(!bytes1.is_empty()); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/txpool/src/transaction.rs` around lines 326 - 332, The test test_encoded_2718_is_cached should assert that the cached buffer instance is reused, not just that two encodings are equal; change the test to obtain the two values without forcing independent allocations and assert pointer identity (e.g., if encoded_2718() returns an Arc<Vec<u8>> use Arc::ptr_eq(&a, &b), or if it returns a Vec<u8> or slice compare their as_ptr() addresses) instead of only assert_eq!(bytes1, bytes2), and keep the non-empty check; reference the test function name test_encoded_2718_is_cached and the encoded_2718() accessor when making this change.crates/chainspec/src/spec.rs (1)
134-137: Remove the duplicated section banner.Line 134-Line 137 repeats the same “Chain Specification Parser (CLI)” header that already exists a few lines above, so trimming this duplicate will keep the file structure cleaner.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/chainspec/src/spec.rs` around lines 134 - 137, Remove the duplicated banner "Chain Specification Parser (CLI)" found in the second occurrence (the block starting with // ============================================================================= and the header line) so only the first instance remains; locate the duplicate comment block containing the exact string "Chain Specification Parser (CLI)" and delete that repeated banner lines (the surrounding // ============================================================================= lines and the header comment) to leave a single header for the Chain Specification Parser (CLI).crates/engine-api/src/builder.rs (1)
1189-1197: Test intent/name mismatch in non-canonical-event case.At Line 1189,
test_engine_state_tracker_ignores_non_canonical_eventsdoesn’t actually dispatch a non-canonical event, so the test currently validates only initial state. Consider renaming it or adding an explicit non-canonical event assertion.Suggested minimal rename
- fn test_engine_state_tracker_ignores_non_canonical_events() { + fn test_engine_state_tracker_initial_state_without_events() {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/engine-api/src/builder.rs` around lines 1189 - 1197, The test named test_engine_state_tracker_ignores_non_canonical_events currently only constructs EngineStateTracker::default and asserts tracker.current_head().is_none() but never dispatches a non-canonical event; either rename the test to reflect it only checks initial state (e.g., test_engine_state_tracker_initial_state_is_none) or modify it to actually send a non-canonical event and assert the head remains unchanged — locate the test function test_engine_state_tracker_ignores_non_canonical_events and either change its name or call the appropriate non-canonical event handler on the EngineStateTracker (e.g., use the same event enum variant used elsewhere such as LiveSyncProgress or another non-canonical variant) and then assert tracker.current_head().is_none() after the event.crates/node/src/validator.rs (1)
556-582: Make the Jade-off assertion independent ofMORPH_HOODI.This test depends on Hoodi's current hardfork schedule, so a future Jade activation will turn it into a false failure even if
validate_state_root()is unchanged. A local chainspec fixture with Jade explicitly disabled, or at least an explicit precondition assert, would keep it stable.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/node/src/validator.rs` around lines 556 - 582, The test test_validate_state_root_jade_not_active_always_ok currently relies on the global MORPH_HOODI schedule via test_chain_spec(); make it deterministic by either creating a local chainspec fixture with Jade explicitly disabled (e.g., construct a ChainSpec passed into MorphEngineValidator::new(...) where the Jade activation is None or set beyond the test block) or add an explicit precondition assert that Jade is not active before calling validate_state_root(&recovered, ...). Update the test to use that local chainspec or assert the precondition so MorphEngineValidator::new(test_chain_spec()) and the call to validate_state_root are guaranteed to run with Jade off regardless of external hardfork changes.crates/consensus/src/validation.rs (1)
960-976: Add back one success case for skipped L1 queue indices.The docs and implementation intentionally allow
header.next_l1_msg_index > last_queue_index + 1, but this section now only covers exact-match and too-small values. Keeping one positive test for the>path would protect that Morph-specific invariant.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/consensus/src/validation.rs` around lines 960 - 976, The test suite removed a positive case that verifies header.next_l1_msg_index can be greater than last_queue_index+1; add back a success test that calls validate_l1_messages_in_block with a sequence of L1 messages (e.g., create_l1_msg_tx(0), create_l1_msg_tx(1), create_l1_msg_tx(2), create_regular_tx()) and a header next_l1_msg_index strictly larger than last+1 (for example 5) and assert the call returns Ok; create a test named like test_validate_l1_messages_in_block_skipped_next_l1_msg_index_success (or update the existing test set) to ensure validate_l1_messages_in_block accepts the > case.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crates/payload/builder/src/builder.rs`:
- Around line 789-792: The test shows ExecutionInfo::new stores u64::MAX in
next_l1_message_index but that is unsafe because the builder later increments
the index with a plain + 1 and will wrap/panic; fix by preventing creation or
unsafe increment: either (A) change ExecutionInfo::new to validate and reject
u64::MAX (return Result or assert that next_l1_message_index != u64::MAX) so
invalid state cannot be constructed, or (B) change the builder's increment site
(the code that does next_l1_message_index + 1) to use checked_add/saturating_add
and return a clear validation error instead of wrapping; update the test for
ExecutionInfo::new accordingly if you choose validation on construction. Ensure
you reference ExecutionInfo::new, next_l1_message_index, and the builder
increment (+ 1) when making the change.
---
Outside diff comments:
In `@crates/revm/src/token_fee.rs`:
- Around line 248-269: The helper query_balance_via_system_call currently
swallows all evm.system_call_one errors and returns Ok(U256::ZERO), masking real
DB/backend failures; change the Err(_) arm to propagate the error (return
Err(e)) so EVMError::Database(...) is forwarded to callers (e.g.,
read_token_balance_with_fallback) instead of being treated as a zero balance;
keep the existing behavior for Ok(result) when the call is non-success or output
is missing (return Ok(U256::ZERO)), but ensure the Err path returns the original
EVMError from system_call_one.
---
Nitpick comments:
In `@crates/chainspec/src/spec.rs`:
- Around line 134-137: Remove the duplicated banner "Chain Specification Parser
(CLI)" found in the second occurrence (the block starting with //
=============================================================================
and the header line) so only the first instance remains; locate the duplicate
comment block containing the exact string "Chain Specification Parser (CLI)" and
delete that repeated banner lines (the surrounding //
=============================================================================
lines and the header comment) to leave a single header for the Chain
Specification Parser (CLI).
In `@crates/consensus/src/validation.rs`:
- Around line 960-976: The test suite removed a positive case that verifies
header.next_l1_msg_index can be greater than last_queue_index+1; add back a
success test that calls validate_l1_messages_in_block with a sequence of L1
messages (e.g., create_l1_msg_tx(0), create_l1_msg_tx(1), create_l1_msg_tx(2),
create_regular_tx()) and a header next_l1_msg_index strictly larger than last+1
(for example 5) and assert the call returns Ok; create a test named like
test_validate_l1_messages_in_block_skipped_next_l1_msg_index_success (or update
the existing test set) to ensure validate_l1_messages_in_block accepts the >
case.
In `@crates/engine-api/src/builder.rs`:
- Around line 1189-1197: The test named
test_engine_state_tracker_ignores_non_canonical_events currently only constructs
EngineStateTracker::default and asserts tracker.current_head().is_none() but
never dispatches a non-canonical event; either rename the test to reflect it
only checks initial state (e.g.,
test_engine_state_tracker_initial_state_is_none) or modify it to actually send a
non-canonical event and assert the head remains unchanged — locate the test
function test_engine_state_tracker_ignores_non_canonical_events and either
change its name or call the appropriate non-canonical event handler on the
EngineStateTracker (e.g., use the same event enum variant used elsewhere such as
LiveSyncProgress or another non-canonical variant) and then assert
tracker.current_head().is_none() after the event.
In `@crates/node/src/validator.rs`:
- Around line 556-582: The test
test_validate_state_root_jade_not_active_always_ok currently relies on the
global MORPH_HOODI schedule via test_chain_spec(); make it deterministic by
either creating a local chainspec fixture with Jade explicitly disabled (e.g.,
construct a ChainSpec passed into MorphEngineValidator::new(...) where the Jade
activation is None or set beyond the test block) or add an explicit precondition
assert that Jade is not active before calling validate_state_root(&recovered,
...). Update the test to use that local chainspec or assert the precondition so
MorphEngineValidator::new(test_chain_spec()) and the call to validate_state_root
are guaranteed to run with Jade off regardless of external hardfork changes.
In `@crates/txpool/src/transaction.rs`:
- Around line 326-332: The test test_encoded_2718_is_cached should assert that
the cached buffer instance is reused, not just that two encodings are equal;
change the test to obtain the two values without forcing independent allocations
and assert pointer identity (e.g., if encoded_2718() returns an Arc<Vec<u8>> use
Arc::ptr_eq(&a, &b), or if it returns a Vec<u8> or slice compare their as_ptr()
addresses) instead of only assert_eq!(bytes1, bytes2), and keep the non-empty
check; reference the test function name test_encoded_2718_is_cached and the
encoded_2718() accessor when making this change.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a064bbac-4c24-478b-8b3c-ed7b26066d37
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (43)
crates/chainspec/src/constants.rscrates/chainspec/src/hardfork.rscrates/chainspec/src/spec.rscrates/consensus/src/error.rscrates/consensus/src/validation.rscrates/engine-api/src/builder.rscrates/engine-api/src/error.rscrates/engine-api/src/validator.rscrates/evm/src/assemble.rscrates/evm/src/block/mod.rscrates/evm/src/block/receipt.rscrates/evm/src/config.rscrates/node/src/args.rscrates/node/src/validator.rscrates/payload/builder/src/builder.rscrates/payload/types/src/attributes.rscrates/payload/types/src/executable_l2_data.rscrates/payload/types/src/lib.rscrates/payload/types/src/params.rscrates/payload/types/src/safe_l2_data.rscrates/primitives/src/header.rscrates/primitives/src/receipt/envelope.rscrates/primitives/src/receipt/mod.rscrates/primitives/src/transaction/envelope.rscrates/revm/Cargo.tomlcrates/revm/src/error.rscrates/revm/src/evm.rscrates/revm/src/handler.rscrates/revm/src/l1block.rscrates/revm/src/precompiles.rscrates/revm/src/token_fee.rscrates/revm/src/tx.rscrates/rpc/Cargo.tomlcrates/rpc/src/error.rscrates/rpc/src/eth/mod.rscrates/rpc/src/eth/receipt.rscrates/rpc/src/eth/transaction.rscrates/rpc/src/types/receipt.rscrates/rpc/src/types/request.rscrates/rpc/src/types/transaction.rscrates/txpool/src/error.rscrates/txpool/src/morph_tx_validation.rscrates/txpool/src/transaction.rs
💤 Files with no reviewable changes (2)
- crates/consensus/src/error.rs
- crates/revm/Cargo.toml
Comment on lines
+789
to
+792
| fn test_execution_info_new_with_max_index() { | ||
| let info = ExecutionInfo::new(u64::MAX); | ||
| assert_eq!(info.next_l1_message_index, u64::MAX); | ||
| } |
There was a problem hiding this comment.
u64::MAX is still unsafe in the real L1-message path.
This boundary case only proves ExecutionInfo::new() stores u64::MAX. The builder later increments the queue index with a plain + 1, so an L1 message at u64::MAX will still wrap/panic instead of surfacing a validation error.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/payload/builder/src/builder.rs` around lines 789 - 792, The test shows
ExecutionInfo::new stores u64::MAX in next_l1_message_index but that is unsafe
because the builder later increments the index with a plain + 1 and will
wrap/panic; fix by preventing creation or unsafe increment: either (A) change
ExecutionInfo::new to validate and reject u64::MAX (return Result or assert that
next_l1_message_index != u64::MAX) so invalid state cannot be constructed, or
(B) change the builder's increment site (the code that does
next_l1_message_index + 1) to use checked_add/saturating_add and return a clear
validation error instead of wrapping; update the test for ExecutionInfo::new
accordingly if you choose validation on construction. Ensure you reference
ExecutionInfo::new, next_l1_message_index, and the builder increment (+ 1) when
making the change.
The merge of main into the branch produced a duplicate [dev-dependencies] section which breaks cargo.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
#[cfg(test)]modules to every morph-reth crate with comprehensive coverageScope
This PR contains only inline unit tests (no E2E test infrastructure or integration tests — those will follow in separate PRs).
Minor accompanying refactors:
From<MorphConsensusError> for ConsensusErrorimplmorph-evmdev-dependency from revm cratequery_erc20_balance→query_balance_via_system_callDependencies
Some tests cover behavior fixed in:
Those PRs should merge first to avoid trivial conflicts.
Test plan
cargo check --all --tests— compiles cleancargo clippy --all --all-targets -- -D warnings— no warningscargo test --all— all tests passSummary by CodeRabbit
New Features
Bug Fixes
Tests