test: add comprehensive test coverage and E2E integration tests#55
Open
test: add comprehensive test coverage and E2E integration tests#55
Conversation
- Add unit tests across all morph-reth crates (primitives, chainspec, revm, evm, txpool, consensus, payload-builder, payload-types, engine-api, node, rpc) - Add E2E integration test (can_sync) that starts an ephemeral Morph node, produces 10 blocks via Engine API, and verifies chain advancement - Add test_utils module with setup(), advance_chain(), and morph_payload_attributes() helpers following scroll-reth's pattern - Add From<EthPayloadBuilderAttributes> for MorphPayloadBuilderAttributes to enable reth E2E test framework compatibility - Add test-utils feature gate for E2E test dependencies
📝 WalkthroughWalkthroughAdds extensive unit and integration tests across many crates (chainspec, consensus, engine-api, evm, payload, rpc, node, revm, txpool, primitives, etc.), plus a test-utils module and a new integration test; no public API or runtime behavior changes. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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 docstrings
🧪 Generate unit tests (beta)
📝 Coding Plan
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: 5
🧹 Nitpick comments (9)
crates/revm/src/error.rs (2)
158-162:test_into_evm_errorcan validate the wrapped payload too.Matching only
EVMError::Transaction(_)is good, but checking the inner error value would make this assertion more robust.Optional tightening
fn test_into_evm_error() { let morph_err = MorphInvalidTransaction::TokenNotRegistered(1); let evm_err: EVMError<std::convert::Infallible, MorphInvalidTransaction> = morph_err.into(); - assert!(matches!(evm_err, EVMError::Transaction(_))); + assert!(matches!( + evm_err, + EVMError::Transaction(MorphInvalidTransaction::TokenNotRegistered(1)) + )); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/revm/src/error.rs` around lines 158 - 162, The test test_into_evm_error currently only asserts the variant EVMError::Transaction(_) — update it to also extract and assert the wrapped payload equals the original MorphInvalidTransaction::TokenNotRegistered(1); specifically, match or if-let on evm_err (EVMError::Transaction(inner)) and assert_eq!(inner, MorphInvalidTransaction::TokenNotRegistered(1)) so the inner value is validated as well.
111-117: Consider asserting the fullInsufficientTokenBalancemessage.Using exact string comparison here would make this test stricter and reduce false positives from partial matches.
Optional tightening
- assert!(err.to_string().contains("100")); - assert!(err.to_string().contains("50")); + assert_eq!( + err.to_string(), + "Insufficient token balance for gas payment: required 100, available 50" + );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/revm/src/error.rs` around lines 111 - 117, Replace the two partial contains assertions with a single exact-string assertion for the full error message: build the expected string that MorphInvalidTransaction::InsufficientTokenBalance produces (including the required and available U256 values) and assert_eq!(err.to_string(), expected) so the test verifies the entire error formatting rather than only substrings; target the test that constructs err from MorphInvalidTransaction::InsufficientTokenBalance and replaces the two assert!(err.to_string().contains(...)) calls with the single exact equality check against the constructed expected string.crates/evm/src/assemble.rs (1)
136-171: Consider extracting shared test helper to avoid duplication.The
create_test_chainspec()helper is duplicated nearly identically inconfig.rs. Consider extracting it to a sharedtest_utilsmodule within the crate or a#[cfg(test)]public helper to reduce maintenance burden and ensure consistency.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/evm/src/assemble.rs` around lines 136 - 171, Tests duplicate the create_test_chainspec() helper between assemble.rs and config.rs; extract it into a single shared test helper (e.g., a new #[cfg(test)] mod test_utils with a pub fn create_test_chainspec() -> Arc<MorphChainSpec>) and import it from both test modules to avoid duplication. Move the genesis_json creation and Genesis->MorphChainSpec conversion into that helper, keep it behind #[cfg(test)] so it’s not compiled in production, and update references in assemble.rs and config.rs to call test_utils::create_test_chainspec() (or use a use statement) so both test modules reuse the single implementation.crates/txpool/src/transaction.rs (1)
326-332:test_encoded_2718_is_cachedchecks content equality, not cache reuse.On Line 330, identical
Bytescontent can still pass even if encoding is recomputed. Consider asserting pointer identity between twoencoded_2718()calls to verify actual cache reuse.Suggested tightening for cache verification
#[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_eq!(bytes1, bytes2, "cached encoding should be identical"); + assert!( + core::ptr::eq(bytes1.as_ptr(), bytes2.as_ptr()), + "expected the same cached buffer 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 currently compares content equality which can succeed even if encoding is recomputed; update it to assert that the cached Bytes are the same allocation by comparing pointer identity from two calls to encoded_2718() (e.g., compare bytes1.as_ptr() == bytes2.as_ptr() and lengths) so the test verifies reuse of the cached buffer rather than just equal content; locate the test and replace or augment the assert_eq!(bytes1, bytes2) with a pointer identity check on encoded_2718().crates/engine-api/src/builder.rs (2)
1151-1183: Consider renaming test to match actual behavior.The test name
ignores_non_canonical_eventsimplies it sends non-canonical events and verifies they're ignored, but it actually only tests thatCanonicalChainCommittedupdates the head (as the comment at lines 1157-1161 acknowledges). A clearer name liketest_engine_state_tracker_updates_only_on_canonical_commitwould better describe what's being verified.🤖 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 1151 - 1183, Rename the test function to accurately reflect its behavior: change fn test_engine_state_tracker_ignores_non_canonical_events() to a clearer name such as fn test_engine_state_tracker_updates_only_on_canonical_commit() (or similar) and update any inline comment if needed; this makes the intent explicit for EngineStateTracker and its methods current_head() and on_consensus_engine_event() which are being validated by sending a ConsensusEngineEvent::CanonicalChainCommitted.
1185-1195: Test doesn't verify actual concurrency.The test claims to verify concurrent reads but performs sequential reads on a single thread. To truly test concurrent read behavior, you'd need multiple threads accessing
current_head()simultaneously. As-is, this only verifies read consistency across sequential calls.This is fine for basic sanity checking, but consider renaming to
test_engine_state_tracker_multiple_reads_consistentor adding actual thread-based concurrency if the original intent matters.🤖 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 1185 - 1195, The test test_engine_state_tracker_concurrent_reads does not actually exercise concurrency because it calls EngineStateTracker::current_head sequentially on one thread; either rename the test to something like test_engine_state_tracker_multiple_reads_consistent to reflect its current behavior, or change the test to spawn multiple threads that concurrently call EngineStateTracker::current_head after initializing state with record_local_head to verify no panics and consistent results across threads; update assertions to collect and compare results from the spawned threads and join them to ensure true concurrent access was exercised.crates/primitives/src/receipt/envelope.rs (1)
417-451: Add missing 2718 roundtrip coverage forEip2930andEip7702(and consider consolidating).Line 427 and Line 445 currently roundtrip only a subset of variants. Adding explicit 2718 roundtrip checks for
MorphTxType::Eip2930andMorphTxType::Eip7702would complete the matrix; this is also a good place to reduce test duplication.♻️ Suggested consolidation with full variant coverage
- #[test] - fn test_eip2718_roundtrip_legacy() { - let receipt = create_test_receipt(MorphTxType::Legacy); - let mut buf = Vec::new(); - receipt.encode_2718(&mut buf); - let decoded = MorphReceiptEnvelope::decode_2718(&mut buf.as_slice()).unwrap(); - assert_eq!(receipt, decoded); - } - - #[test] - fn test_eip2718_roundtrip_eip1559() { - let receipt = create_test_receipt(MorphTxType::Eip1559); - let mut buf = Vec::new(); - receipt.encode_2718(&mut buf); - let decoded = MorphReceiptEnvelope::decode_2718(&mut buf.as_slice()).unwrap(); - assert_eq!(receipt, decoded); - } - - #[test] - fn test_eip2718_roundtrip_l1msg() { - let receipt = create_test_receipt(MorphTxType::L1Msg); - let mut buf = Vec::new(); - receipt.encode_2718(&mut buf); - let decoded = MorphReceiptEnvelope::decode_2718(&mut buf.as_slice()).unwrap(); - assert_eq!(receipt, decoded); - } - - #[test] - fn test_eip2718_roundtrip_morph() { - let receipt = create_test_receipt(MorphTxType::Morph); - let mut buf = Vec::new(); - receipt.encode_2718(&mut buf); - let decoded = MorphReceiptEnvelope::decode_2718(&mut buf.as_slice()).unwrap(); - assert_eq!(receipt, decoded); - } + #[test] + fn test_eip2718_roundtrip_all_variants() { + for tx_type in [ + MorphTxType::Legacy, + MorphTxType::Eip2930, + MorphTxType::Eip1559, + MorphTxType::Eip7702, + MorphTxType::L1Msg, + MorphTxType::Morph, + ] { + let receipt = create_test_receipt(tx_type); + let mut buf = Vec::new(); + receipt.encode_2718(&mut buf); + let decoded = MorphReceiptEnvelope::decode_2718(&mut buf.as_slice()).unwrap(); + assert_eq!(receipt, decoded); + } + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/primitives/src/receipt/envelope.rs` around lines 417 - 451, The test suite is missing Eip2930 and Eip7702 coverage for the 2718 roundtrip; update the tests so that MorphTxType::Eip2930 and MorphTxType::Eip7702 are also encoded with encode_2718 and decoded with MorphReceiptEnvelope::decode_2718 and asserted equal to the original receipt; to avoid duplication replace the four separate functions (test_eip2718_roundtrip_legacy, _eip1559, _l1msg, _morph) with a single parameterized loop (or a helper function) that iterates over all MorphTxType variants from create_test_receipt and performs the encode_2718 -> decode_2718 -> assert_eq roundtrip for each variant.crates/node/Cargo.toml (1)
59-74: Moveserde_jsonto thetest-utilsfeature as an optional dependency.Currently
serde_jsonis unconditionally included in regular dependencies (line 59) but only used intest_utils.rsbehind#[cfg(feature = "test-utils")]. Add"dep:serde_json"to thetest-utilsfeature block and move the line 59 declaration to:serde_json = { workspace = true, optional = true }Remove it from line 74 (dev-dependencies) since the feature will handle it. This prevents unnecessary dependency bloat in default builds.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/node/Cargo.toml` around lines 59 - 74, Update Cargo.toml so serde_json is an optional dependency tied to the test-utils feature: change the top-level declaration `serde_json.workspace = true` to `serde_json = { workspace = true, optional = true }`, add `"dep:serde_json"` to the `test-utils` feature entry, and remove the `serde_json.workspace = true` line from the [dev-dependencies] block; reference the `test-utils` feature and the `serde_json` dependency to locate edits.crates/rpc/src/eth/transaction.rs (1)
651-715: Add a boundary test forfee_token_idoverflow.The new
try_build_morph_tx_from_requesttests are solid, but the explicit conversion failure path (u16::try_from(...)) is still untested. A case withfee_token_id > u16::MAXwould lock in this validation behavior.➕ Suggested test case
+ #[test] + fn try_build_morph_tx_rejects_fee_token_id_overflow() { + let req = create_basic_transaction_request(); + let overflow_id = U64::from((u16::MAX as u64) + 1); + let result = try_build_morph_tx_from_request(&req, overflow_id, U256::ZERO, None, None); + assert!(result.is_err()); + assert!(result.unwrap_err().contains("invalid token")); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/rpc/src/eth/transaction.rs` around lines 651 - 715, Add a boundary test calling try_build_morph_tx_from_request with a request whose fee_token_id value exceeds u16::MAX (e.g., U64::from(u16::MAX as u64 + 1)) and assert the call returns Err and the error message contains the conversion failure; place this new test alongside the other tests in the same module (use create_basic_transaction_request() to build the request and call try_build_morph_tx_from_request with U64::from(u16::MAX + 1) to exercise the u16::try_from(...) failure path).
🤖 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/consensus/src/validation.rs`:
- Around line 1487-1516: The test currently only checks for a rejection when the
default create_test_chainspec() happens to have FeeVault enabled, making it a
no-op otherwise; modify the
test_validate_header_coinbase_non_zero_with_fee_vault to construct an explicit
Chainspec with FeeVault enabled (either by calling a helper like
create_test_chainspec_with_fee_vault(true) or by mutating the returned chainspec
to set fee_vault enabled) and instantiate MorphConsensus from that explicit
chainspec, then call consensus.validate_header(&sealed) and assert the result is
an Err unconditionally (and check the error string contains "coinbase");
reference functions/values: create_test_chainspec, MorphConsensus::new,
consensus.chain_spec().is_fee_vault_enabled(), and
test_validate_header_coinbase_non_zero_with_fee_vault when making the change.
In `@crates/evm/src/block/receipt.rs`:
- Around line 406-457: The tests test_build_morph_tx_receipt_with_fields and
test_build_morph_tx_receipt_without_fields_fallback currently only check the
variant and l1_fee; update them to pattern-match/destructure the
MorphReceipt::Morph payload returned by
DefaultMorphReceiptBuilder::build_receipt and assert the MorphTx-specific fields
(fee_token_id, fee_rate, token_scale, fee_limit, reference, memo) are present
and equal to the values in MorphReceiptTxFields when morph_tx_fields is Some,
and that those fields are absent or defaulted when morph_tx_fields is None
(verifying the fallback path). Locate the receipt variable in each test, match
it to MorphReceipt::Morph(inner) (or equivalent accessor), and add assertions
against inner.* fields (or the builder helper with_morph_tx_v1() output) instead
of only checking l1_fee and variant.
- Around line 385-403: The test test_build_l1_msg_receipt_no_l1_fee currently
sets MorphReceiptBuilderCtx.l1_fee to U256::ZERO which doesn't catch accidental
routing through with_l1_fee(); change the seeded l1_fee to a non-zero value
(e.g. U256::from(1)) when constructing MorphReceiptBuilderCtx in that test (the
builder used is DefaultMorphReceiptBuilder with tx from create_l1_msg_tx) and
keep the final assertion assert_eq!(receipt.l1_fee(), U256::ZERO) to ensure
L1Msg receipts always report zero even if the context contains a non-zero
l1_fee.
In `@crates/node/src/test_utils.rs`:
- Around line 34-37: The setup helper (pub async fn setup) should validate its
input and immediately return an error when num_nodes == 0; add a guard at the
top of setup that checks if num_nodes == 0 and returns an eyre::Result::Err
(e.g., return Err(eyre::eyre!("num_nodes must be > 0"))) so callers get a clear,
early failure instead of flowing into downstream setup logic (affecting
MorphTestNode/TaskManager/Wallet creation).
In `@crates/txpool/src/morph_tx_validation.rs`:
- Around line 418-460: The test
test_validate_morph_tx_token_fee_path_token_not_found should assert the
deterministic EmptyDB path returns MorphTxError::TokenNotFound only; change the
assertion so validate_morph_tx(&mut db, &input).unwrap_err() is matched strictly
against MorphTxError::TokenNotFound (since TokenFeeInfo::load_for_caller() on
EmptyDB yields Ok(None)), and if you still want to cover the fetch-failure
branch create a separate test using a failing-DB fixture that causes
TokenFeeInfo::load_for_caller() to return an Err which you then assert maps to
MorphTxError::TokenInfoFetchFailed.
---
Nitpick comments:
In `@crates/engine-api/src/builder.rs`:
- Around line 1151-1183: Rename the test function to accurately reflect its
behavior: change fn test_engine_state_tracker_ignores_non_canonical_events() to
a clearer name such as fn
test_engine_state_tracker_updates_only_on_canonical_commit() (or similar) and
update any inline comment if needed; this makes the intent explicit for
EngineStateTracker and its methods current_head() and
on_consensus_engine_event() which are being validated by sending a
ConsensusEngineEvent::CanonicalChainCommitted.
- Around line 1185-1195: The test test_engine_state_tracker_concurrent_reads
does not actually exercise concurrency because it calls
EngineStateTracker::current_head sequentially on one thread; either rename the
test to something like test_engine_state_tracker_multiple_reads_consistent to
reflect its current behavior, or change the test to spawn multiple threads that
concurrently call EngineStateTracker::current_head after initializing state with
record_local_head to verify no panics and consistent results across threads;
update assertions to collect and compare results from the spawned threads and
join them to ensure true concurrent access was exercised.
In `@crates/evm/src/assemble.rs`:
- Around line 136-171: Tests duplicate the create_test_chainspec() helper
between assemble.rs and config.rs; extract it into a single shared test helper
(e.g., a new #[cfg(test)] mod test_utils with a pub fn create_test_chainspec()
-> Arc<MorphChainSpec>) and import it from both test modules to avoid
duplication. Move the genesis_json creation and Genesis->MorphChainSpec
conversion into that helper, keep it behind #[cfg(test)] so it’s not compiled in
production, and update references in assemble.rs and config.rs to call
test_utils::create_test_chainspec() (or use a use statement) so both test
modules reuse the single implementation.
In `@crates/node/Cargo.toml`:
- Around line 59-74: Update Cargo.toml so serde_json is an optional dependency
tied to the test-utils feature: change the top-level declaration
`serde_json.workspace = true` to `serde_json = { workspace = true, optional =
true }`, add `"dep:serde_json"` to the `test-utils` feature entry, and remove
the `serde_json.workspace = true` line from the [dev-dependencies] block;
reference the `test-utils` feature and the `serde_json` dependency to locate
edits.
In `@crates/primitives/src/receipt/envelope.rs`:
- Around line 417-451: The test suite is missing Eip2930 and Eip7702 coverage
for the 2718 roundtrip; update the tests so that MorphTxType::Eip2930 and
MorphTxType::Eip7702 are also encoded with encode_2718 and decoded with
MorphReceiptEnvelope::decode_2718 and asserted equal to the original receipt; to
avoid duplication replace the four separate functions
(test_eip2718_roundtrip_legacy, _eip1559, _l1msg, _morph) with a single
parameterized loop (or a helper function) that iterates over all MorphTxType
variants from create_test_receipt and performs the encode_2718 -> decode_2718 ->
assert_eq roundtrip for each variant.
In `@crates/revm/src/error.rs`:
- Around line 158-162: The test test_into_evm_error currently only asserts the
variant EVMError::Transaction(_) — update it to also extract and assert the
wrapped payload equals the original
MorphInvalidTransaction::TokenNotRegistered(1); specifically, match or if-let on
evm_err (EVMError::Transaction(inner)) and assert_eq!(inner,
MorphInvalidTransaction::TokenNotRegistered(1)) so the inner value is validated
as well.
- Around line 111-117: Replace the two partial contains assertions with a single
exact-string assertion for the full error message: build the expected string
that MorphInvalidTransaction::InsufficientTokenBalance produces (including the
required and available U256 values) and assert_eq!(err.to_string(), expected) so
the test verifies the entire error formatting rather than only substrings;
target the test that constructs err from
MorphInvalidTransaction::InsufficientTokenBalance and replaces the two
assert!(err.to_string().contains(...)) calls with the single exact equality
check against the constructed expected string.
In `@crates/rpc/src/eth/transaction.rs`:
- Around line 651-715: Add a boundary test calling
try_build_morph_tx_from_request with a request whose fee_token_id value exceeds
u16::MAX (e.g., U64::from(u16::MAX as u64 + 1)) and assert the call returns Err
and the error message contains the conversion failure; place this new test
alongside the other tests in the same module (use
create_basic_transaction_request() to build the request and call
try_build_morph_tx_from_request with U64::from(u16::MAX + 1) to exercise the
u16::try_from(...) failure path).
In `@crates/txpool/src/transaction.rs`:
- Around line 326-332: The test test_encoded_2718_is_cached currently compares
content equality which can succeed even if encoding is recomputed; update it to
assert that the cached Bytes are the same allocation by comparing pointer
identity from two calls to encoded_2718() (e.g., compare bytes1.as_ptr() ==
bytes2.as_ptr() and lengths) so the test verifies reuse of the cached buffer
rather than just equal content; locate the test and replace or augment the
assert_eq!(bytes1, bytes2) with a pointer identity check on encoded_2718().
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 8c1abc49-ddd4-4477-abe1-17387e8b139d
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (39)
crates/chainspec/src/constants.rscrates/chainspec/src/hardfork.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/receipt.rscrates/evm/src/config.rscrates/node/Cargo.tomlcrates/node/src/args.rscrates/node/src/lib.rscrates/node/src/node.rscrates/node/src/test_utils.rscrates/node/src/validator.rscrates/node/tests/it/main.rscrates/node/tests/it/sync.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/revm/src/error.rscrates/revm/src/evm.rscrates/revm/src/l1block.rscrates/revm/src/precompiles.rscrates/revm/src/token_fee.rscrates/rpc/Cargo.tomlcrates/rpc/src/error.rscrates/rpc/src/eth/receipt.rscrates/rpc/src/eth/transaction.rscrates/rpc/src/types/receipt.rscrates/rpc/src/types/request.rscrates/txpool/src/error.rscrates/txpool/src/morph_tx_validation.rscrates/txpool/src/transaction.rs
- Fix no-op FeeVault test by creating chainspec with feeVaultAddress and making assertion unconditional - Use non-zero l1_fee in L1Msg receipt test to verify builder ignores it - Destructure MorphReceipt::Morph to assert fee_token_id, fee_rate, token_scale, fee_limit, reference, memo fields - Narrow token-not-found assertion to deterministic TokenNotFound path
There was a problem hiding this comment.
♻️ Duplicate comments (1)
crates/txpool/src/morph_tx_validation.rs (1)
454-456:⚠️ Potential issue | 🟡 MinorTighten TokenNotFound assertion to exact token id.
Line 454 still matches any
token_idvia{ .. }. This can hide regressions in error payload mapping; asserttoken_id: 42explicitly for deterministic coverage.💡 Suggested change
- assert!( - matches!(err, MorphTxError::TokenNotFound { .. }), - "expected TokenNotFound, got {err:?}" - ); + assert_eq!(err, MorphTxError::TokenNotFound { token_id: 42 });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/txpool/src/morph_tx_validation.rs` around lines 454 - 456, The current assertion on the error allows any token_id because it uses the wildcard pattern in matches!(err, MorphTxError::TokenNotFound { .. }); update the assertion to match the exact expected token id (token_id: 42) by changing the pattern to matches!(err, MorphTxError::TokenNotFound { token_id: 42 }) and update the failure message if needed so the test fails when the token_id payload is different; target the MorphTxError::TokenNotFound pattern to ensure deterministic coverage.
🧹 Nitpick comments (4)
crates/evm/src/block/receipt.rs (2)
362-382: Add tests forEip2930andEip7702routing paths.
build_receipthas explicit match arms forMorphTxType::Eip2930andMorphTxType::Eip7702, but this module currently only exercises Legacy/EIP-1559/L1Msg/Morph. Adding two focused tests here would complete branch coverage for tx-type-to-receipt mapping.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/evm/src/block/receipt.rs` around lines 362 - 382, Add two unit tests similar to test_build_eip1559_receipt that exercise the Eip2930 and Eip7702 match arms in build_receipt: create a tx for each type (e.g. create_eip2930_tx and create_eip7702_tx or reuse existing helpers), build a MorphReceiptBuilderCtx::<TestEvm> with that tx, a success result (make_success_result), appropriate l1_fee and cumulative_gas_used, call DefaultMorphReceiptBuilder.build_receipt(ctx), and assert the returned MorphReceipt is the expected variant (matches!(receipt, MorphReceipt::Eip2930(_)) and MorphReceipt::Eip7702(_)) and that receipt.l1_fee() and receipt.cumulative_gas_used() match the ctx values.
510-533: Current log test doesn’t verify pre/main/post ordering contract.
test_build_receipt_with_logsonly checks a single main log count. It misses the core behavior documented in the builder:[pre_fee_logs] + [main tx logs] + [post_fee_logs], including revert behavior where main logs are absent but fee logs remain.♻️ Proposed test additions
@@ #[test] fn test_build_receipt_with_logs() { @@ let receipt = builder.build_receipt(ctx); assert_eq!(TxReceipt::logs(&receipt).len(), 1); } + + #[test] + fn test_build_receipt_log_order_pre_main_post() { + let builder = DefaultMorphReceiptBuilder; + let tx = create_legacy_tx(); + + let pre = Log::new( + Address::repeat_byte(0x11), + vec![B256::repeat_byte(0xA1)], + alloy_primitives::Bytes::from_static(b"pre"), + ) + .unwrap(); + let main = Log::new( + Address::repeat_byte(0x22), + vec![B256::repeat_byte(0xB2)], + alloy_primitives::Bytes::from_static(b"main"), + ) + .unwrap(); + let post = Log::new( + Address::repeat_byte(0x33), + vec![B256::repeat_byte(0xC3)], + alloy_primitives::Bytes::from_static(b"post"), + ) + .unwrap(); + + let ctx = MorphReceiptBuilderCtx::<TestEvm> { + tx: &tx, + result: make_success_with_logs(21_000, vec![main.clone()]), + cumulative_gas_used: 21_000, + l1_fee: U256::ZERO, + morph_tx_fields: None, + pre_fee_logs: vec![pre.clone()], + post_fee_logs: vec![post.clone()], + }; + + let receipt = builder.build_receipt(ctx); + assert_eq!(TxReceipt::logs(&receipt), &[pre, main, post]); + } + + #[test] + fn test_build_reverted_receipt_keeps_fee_logs() { + let builder = DefaultMorphReceiptBuilder; + let tx = create_legacy_tx(); + + let pre = Log::new( + Address::repeat_byte(0x44), + vec![B256::repeat_byte(0xD4)], + alloy_primitives::Bytes::from_static(b"pre"), + ) + .unwrap(); + let post = Log::new( + Address::repeat_byte(0x55), + vec![B256::repeat_byte(0xE5)], + alloy_primitives::Bytes::from_static(b"post"), + ) + .unwrap(); + + let ctx = MorphReceiptBuilderCtx::<TestEvm> { + tx: &tx, + result: make_revert_result(15_000), + cumulative_gas_used: 15_000, + l1_fee: U256::ZERO, + morph_tx_fields: None, + pre_fee_logs: vec![pre.clone()], + post_fee_logs: vec![post.clone()], + }; + + let receipt = builder.build_receipt(ctx); + assert_eq!(TxReceipt::logs(&receipt), &[pre, post]); + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/evm/src/block/receipt.rs` around lines 510 - 533, Update the test suite to assert the full pre/main/post log ordering and revert behavior: in test_build_receipt_with_logs (and add a new test for revert) construct distinct pre_fee_logs, main logs (via make_success_with_logs), and post_fee_logs in the MorphReceiptBuilderCtx used with DefaultMorphReceiptBuilder::build_receipt, then assert TxReceipt::logs(&receipt) yields the concatenation [pre_fee_logs, main logs, post_fee_logs] in order (check counts and that entries match the corresponding Log instances); also add a separate test that uses a revert result (e.g., make_revert_with_logs or equivalent) to verify main tx logs are omitted while pre_fee_logs and post_fee_logs remain present and ordered.crates/consensus/src/validation.rs (2)
1865-1889: Make this fixture valid apart fromgas_used.Right now the header also has default
receipts_root/logs_bloom, andresult.gas_usedhappens to equal the receipt total. That makes the test depend on validation order and doesn't prove the comparison is receipt-derived. Populatereceipts_root/logs_bloomfromresult.receipts, and setresult.gas_usedto match the header so the failure can only come from the cumulative receipt gas mismatch.♻️ Tighten the fixture
let result = alloy_evm::block::BlockExecutionResult { receipts: vec![receipt], requests: Default::default(), - gas_used: 21000, + gas_used: 50000, blob_gas_used: 0, }; + + let receipts_with_bloom: Vec<_> = + result.receipts.iter().map(TxReceipt::with_bloom_ref).collect(); + let receipts_root = + alloy_consensus::proofs::calculate_receipt_root(&receipts_with_bloom); + let logs_bloom = receipts_with_bloom + .iter() + .fold(Bloom::ZERO, |bloom, r| bloom | r.bloom_ref()); // Create a block header with gas_used = 50000 (mismatch!) let header = create_morph_header(Header { nonce: B64::ZERO, ommers_hash: EMPTY_OMMER_ROOT_HASH, gas_limit: 30_000_000, gas_used: 50000, // Does not match receipt + receipts_root, + logs_bloom, timestamp: 1000, base_fee_per_gas: Some(1_000_000), ..Default::default() });🤖 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 1865 - 1889, The test fixture must be valid except for the cumulative gas mismatch: populate the header's receipts_root and logs_bloom from result.receipts so the header isn't left with defaults, and set result.gas_used to match the header's gas_used (the 50000 value in the Header created by create_morph_header) so the only failing check is the cumulative receipt gas vs header.gas_used; update the BlockExecutionResult construction (result.receipts / gas_used) and the header fields (receipts_root, logs_bloom) accordingly so the receipt-derived roots/bloom are consistent while the cumulative gas remains the intended mismatch.
1549-1601: Return a baseTxMorphfrom these helpers.
create_morph_tx_v0()/create_morph_tx_v1()already encode the common defaults, but because they returnMorphTxEnvelope, each negative case still has to restate the fullTxMorphliteral. Abase_morph_tx_v0()/base_morph_tx_v1()helper that returnsTxMorphwould keep each test focused on the one field under test and reduce drift when the schema changes.Also applies to: 1626-1798
🤖 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 1549 - 1601, The helpers create_morph_tx_v0 and create_morph_tx_v1 duplicate a full TxMorph literal because they return MorphTxEnvelope; add base_morph_tx_v0() and base_morph_tx_v1() that return a TxMorph with the shared defaults (chain_id, nonce, gas_limit, fees, to, access_list, version, etc.), then change create_morph_tx_v0/create_morph_tx_v1 to call the new base_* helpers and wrap the returned TxMorph in Signed::new_unchecked(...) to produce the MorphTxEnvelope; update tests that currently construct full TxMorphs (the cases mentioned around the create helpers) to call base_* and override only the field under test.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@crates/txpool/src/morph_tx_validation.rs`:
- Around line 454-456: The current assertion on the error allows any token_id
because it uses the wildcard pattern in matches!(err,
MorphTxError::TokenNotFound { .. }); update the assertion to match the exact
expected token id (token_id: 42) by changing the pattern to matches!(err,
MorphTxError::TokenNotFound { token_id: 42 }) and update the failure message if
needed so the test fails when the token_id payload is different; target the
MorphTxError::TokenNotFound pattern to ensure deterministic coverage.
---
Nitpick comments:
In `@crates/consensus/src/validation.rs`:
- Around line 1865-1889: The test fixture must be valid except for the
cumulative gas mismatch: populate the header's receipts_root and logs_bloom from
result.receipts so the header isn't left with defaults, and set result.gas_used
to match the header's gas_used (the 50000 value in the Header created by
create_morph_header) so the only failing check is the cumulative receipt gas vs
header.gas_used; update the BlockExecutionResult construction (result.receipts /
gas_used) and the header fields (receipts_root, logs_bloom) accordingly so the
receipt-derived roots/bloom are consistent while the cumulative gas remains the
intended mismatch.
- Around line 1549-1601: The helpers create_morph_tx_v0 and create_morph_tx_v1
duplicate a full TxMorph literal because they return MorphTxEnvelope; add
base_morph_tx_v0() and base_morph_tx_v1() that return a TxMorph with the shared
defaults (chain_id, nonce, gas_limit, fees, to, access_list, version, etc.),
then change create_morph_tx_v0/create_morph_tx_v1 to call the new base_* helpers
and wrap the returned TxMorph in Signed::new_unchecked(...) to produce the
MorphTxEnvelope; update tests that currently construct full TxMorphs (the cases
mentioned around the create helpers) to call base_* and override only the field
under test.
In `@crates/evm/src/block/receipt.rs`:
- Around line 362-382: Add two unit tests similar to test_build_eip1559_receipt
that exercise the Eip2930 and Eip7702 match arms in build_receipt: create a tx
for each type (e.g. create_eip2930_tx and create_eip7702_tx or reuse existing
helpers), build a MorphReceiptBuilderCtx::<TestEvm> with that tx, a success
result (make_success_result), appropriate l1_fee and cumulative_gas_used, call
DefaultMorphReceiptBuilder.build_receipt(ctx), and assert the returned
MorphReceipt is the expected variant (matches!(receipt,
MorphReceipt::Eip2930(_)) and MorphReceipt::Eip7702(_)) and that
receipt.l1_fee() and receipt.cumulative_gas_used() match the ctx values.
- Around line 510-533: Update the test suite to assert the full pre/main/post
log ordering and revert behavior: in test_build_receipt_with_logs (and add a new
test for revert) construct distinct pre_fee_logs, main logs (via
make_success_with_logs), and post_fee_logs in the MorphReceiptBuilderCtx used
with DefaultMorphReceiptBuilder::build_receipt, then assert
TxReceipt::logs(&receipt) yields the concatenation [pre_fee_logs, main logs,
post_fee_logs] in order (check counts and that entries match the corresponding
Log instances); also add a separate test that uses a revert result (e.g.,
make_revert_with_logs or equivalent) to verify main tx logs are omitted while
pre_fee_logs and post_fee_logs remain present and ordered.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a7aecbdc-1b01-42a5-967d-f8ecf676e2e7
📒 Files selected for processing (3)
crates/consensus/src/validation.rscrates/evm/src/block/receipt.rscrates/txpool/src/morph_tx_validation.rs
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
1 participant
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
can_sync) that starts an ephemeral Morph node, produces 10 blocks via Engine API, and verifies chain advancementtest_utilsmodule withsetup(),advance_chain()helpers andtest-utilsfeature gate for E2E test dependenciesFrom<EthPayloadBuilderAttributes>forMorphPayloadBuilderAttributesto enable reth E2E test framework compatibilityTest Plan
cargo test --all— all unit tests pass (500+ tests, 0 failures)cargo test --package morph-node --test it --features test-utils -- sync::can_sync— E2E integration test passescargo check --all --tests— compilation clean (excluding pre-existing upstream reth-ethereum-primitives serde issue)Summary by CodeRabbit