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
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds extensive unit and integration tests, test utilities, and CI/test configuration across many crates (node, payload, evm, revm, txpool, rpc, consensus, engine-api, primitives, etc.), plus one new conversion impl ( Changes
Sequence Diagram(s)(silently omitted — changes are primarily tests and do not introduce new multi-component control flow requiring diagrams) 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 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
- Add make_deploy_tx() helper for contract deployment in E2E tests - Add 4 precompile functional unit tests: ecrecover, sha256, identity, KZG absence across all hardfork profiles - Add 7 E2E EVM tests: contract deploy/state, constructor revert, cross-block state persistence, BLOCKHASH custom Morph formula, SELFDESTRUCT disabled behavior, L1 fee with calldata - Add L1 Gas Price Oracle Curie storage to test genesis (curieBlock=0 transitions at genesis so apply_curie_hard_fork never runs — storage must be pre-populated for L1 fee tests to work)
Add block building, consensus, engine, hardfork, L1 messages, MorphTx, RPC, txpool integration tests and test helpers. Include nextest config and updated Cargo dependencies.
- receipt.rs: 3 unit tests verifying pre/post fee logs survive main tx revert and correct log ordering ([pre_fee, main_tx, post_fee]) - morph_tx.rs: E2E test verifying token balance decreases after MorphTx v0 with ERC20 fee - morph_tx.rs: E2E test verifying token fee is still charged when main tx reverts (deploy runtime-REVERT contract, call with MorphTx v0, assert status=false but token balance decreased + receipt fee fields)
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
There was a problem hiding this comment.
CodeQL found more than 20 potential problems in the proposed changes. Check the Files changed tab for more details.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
crates/node/Cargo.toml (1)
93-101: Consider usingdep:prefix for consistency.Line 94 uses
"reth-e2e-test-utils"without thedep:prefix, while other optional dependencies in the feature list usedep:(e.g.,dep:reth-tasks,dep:tokio). Sincereth-e2e-test-utilsis listed as an optional dependency on line 62, this should be"dep:reth-e2e-test-utils"for consistency with the other entries. The current form implicitly enables the feature of the same name on the crate, which may or may not be intended.♻️ Suggested fix for consistency
test-utils = [ - "reth-e2e-test-utils", + "dep:reth-e2e-test-utils", "dep:reth-tasks", "dep:alloy-signer", "dep:tokio",🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/node/Cargo.toml` around lines 93 - 101, The feature list "test-utils" includes "reth-e2e-test-utils" without the dep: prefix which is inconsistent with the other entries; update the "test-utils" feature array to use "dep:reth-e2e-test-utils" so it references the optional dependency (reth-e2e-test-utils) explicitly rather than implicitly enabling a feature of the same name, ensuring consistency with entries like "dep:reth-tasks" and "dep:tokio".crates/node/tests/it/morph_tx.rs (1)
73-79: Use the tracked wallet nonce instead of0..3.This loop already mutates
wallet.inner_nonce, but the txs are signed withiinstead. That bakes in a zero-start assumption and makes the test stop exercising real nonce sequencing if the fixture wallet ever begins with a non-zero nonce.♻️ Suggested refactor
- for i in 0..3 { - let raw_tx = MorphTxBuilder::new(wallet.chain_id, wallet.inner.clone(), i) + for _ in 0..3 { + let nonce = wallet.inner_nonce; + let raw_tx = MorphTxBuilder::new(wallet.chain_id, wallet.inner.clone(), nonce) .with_v1_eth_fee() .build_signed()?; node.rpc.inject_tx(raw_tx).await?; wallet.inner_nonce += 1; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/node/tests/it/morph_tx.rs` around lines 73 - 79, The loop uses the loop index `i` when constructing transactions (MorphTxBuilder::new(..., i)) which assumes a zero start; instead compute and use the wallet's tracked nonce so signing matches the wallet state: set a local starting_nonce = wallet.inner_nonce, iterate e.g. for n in starting_nonce..starting_nonce + 3 and pass `n` to MorphTxBuilder::new, keep the existing wallet.inner_nonce += 1 after injecting each tx so the fixture's nonce advances correctly; update any variable names (`i`) to `n` or `nonce` to make the intent clear.
🤖 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/node/src/test_utils.rs`:
- Around line 376-378: After calling node.update_forkchoice(block_hash,
block_hash) you must wait for the canonical head to advance before returning;
update_forkchoice() does not guarantee provider.latest() has moved, which causes
races for back-to-back advance_empty_block() calls. Modify the helper (the code
around node.submit_payload and node.update_forkchoice) to call the existing
sync/wait utility (e.g., node.sync_to(block_hash) or poll provider.latest()
until it equals block_hash) after update_forkchoice so the function only returns
once the provider's latest head equals block_hash.
- Around line 440-448: The Authorization struct is being created with a
hardcoded nonce (Authorization { nonce: 0 }) which causes the EIP-7702
authorization to mismatch the transaction nonce; update the Authorization
construction in the helper to use the function's nonce parameter (e.g., set
nonce: U256::from(nonce)) so the signed authorization produced by
signer.sign_hash_sync(&authorization.signature_hash()) and converted with
authorization.into_signed(...) matches the transaction nonce used elsewhere.
In `@crates/node/tests/it/helpers.rs`:
- Around line 236-241: The helper currently treats send-time rejections from
payload_builder_handle.send_new_payload(attrs).await as Err, breaking the
helper's Ok(error_message) contract; change the handling of send_new_payload's
Err branch so that a send-time rejection is converted into the expected
Ok(error_message) return instead of propagating an Err. Locate the call to
payload_builder_handle.send_new_payload in the helper (the payload_id
assignment) and replace the map_err(...) that wraps the send failure with logic
that captures the error and returns Ok(formatted_error_message) (or maps to a
Result::Ok with the same error string) so negative tests see the expected
Ok(error_message) rather than a harness error.
---
Nitpick comments:
In `@crates/node/Cargo.toml`:
- Around line 93-101: The feature list "test-utils" includes
"reth-e2e-test-utils" without the dep: prefix which is inconsistent with the
other entries; update the "test-utils" feature array to use
"dep:reth-e2e-test-utils" so it references the optional dependency
(reth-e2e-test-utils) explicitly rather than implicitly enabling a feature of
the same name, ensuring consistency with entries like "dep:reth-tasks" and
"dep:tokio".
In `@crates/node/tests/it/morph_tx.rs`:
- Around line 73-79: The loop uses the loop index `i` when constructing
transactions (MorphTxBuilder::new(..., i)) which assumes a zero start; instead
compute and use the wallet's tracked nonce so signing matches the wallet state:
set a local starting_nonce = wallet.inner_nonce, iterate e.g. for n in
starting_nonce..starting_nonce + 3 and pass `n` to MorphTxBuilder::new, keep the
existing wallet.inner_nonce += 1 after injecting each tx so the fixture's nonce
advances correctly; update any variable names (`i`) to `n` or `nonce` to make
the intent clear.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ba649fa7-bd9f-4c21-b2c5-1884e5a2349f
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (17)
.config/nextest.tomlcrates/evm/src/block/receipt.rscrates/node/Cargo.tomlcrates/node/src/test_utils.rscrates/node/tests/assets/test-genesis.jsoncrates/node/tests/it/block_building.rscrates/node/tests/it/consensus.rscrates/node/tests/it/engine.rscrates/node/tests/it/evm.rscrates/node/tests/it/hardfork.rscrates/node/tests/it/helpers.rscrates/node/tests/it/l1_messages.rscrates/node/tests/it/main.rscrates/node/tests/it/morph_tx.rscrates/node/tests/it/rpc.rscrates/node/tests/it/txpool.rscrates/revm/src/precompiles.rs
✅ Files skipped from review due to trivial changes (4)
- .config/nextest.toml
- crates/node/tests/assets/test-genesis.json
- crates/node/tests/it/evm.rs
- crates/evm/src/block/receipt.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- crates/node/tests/it/main.rs
…itives CodeQL: - Add .github/codeql/codeql-config.yml to suppress rust/hard-coded-cryptographic-value in test files (test wallets use standard Hardhat/Foundry test mnemonics — not production secrets) - Add .github/workflows/codeql-analysis.yml referencing the config CodeRabbit (major): - advance_empty_block: add sync_to() after update_forkchoice() to avoid race condition where provider.latest() hasn't advanced yet - make_eip7702_tx: use nonce parameter for authorization nonce instead of hardcoded 0 (EIP-7702 replay protection) CodeRabbit (minor): - error.rs: tighten test_into_evm_error to assert exact inner error value - Cargo.toml: use dep: prefix for reth-e2e-test-utils in test-utils feature - helpers.rs: return send-time payload builder errors as Ok(msg) in expect_payload_build_failure (matches contract for negative tests) - morph_tx_validation.rs: tighten TokenNotFound assertion to exact token_id
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
crates/node/src/test_utils.rs (2)
727-730: Clarify whetherMorphTxBuilderis checked or intentionally raw.It starts in an invalid state (
version = 0,fee_token_id = 0),with_reference/with_memodo not enforce their documented constraints, andwith_raw_morph_config()is described as the escape hatch even thoughbuild_signed()already signs unchecked inputs. That makes the normal path easy to misuse in positive-path tests.Either validate the “safe” path in
build_signed()and add an explicit unchecked variant for negative tests, or document this builder as intentionally permissive.Also applies to: 782-785, 816-824, 834-863
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/node/src/test_utils.rs` around lines 727 - 730, MorphTxBuilder currently allows invalid states (version=0, fee_token_id=0) and methods like with_reference and with_memo don't enforce their documented constraints while build_signed will happily sign unchecked inputs; update MorphTxBuilder so build_signed performs strict validation of version, fee_token_id, reference/memo constraints and morph config, add an explicit unchecked factory/terminal (e.g., build_signed_unchecked or keep with_raw_morph_config as the documented escape hatch) for negative tests, and enforce/validate constraints inside with_reference and with_memo to fail-fast; update or add clear doc comments on MorphTxBuilder, with_reference, with_memo, with_raw_morph_config, and build_signed to state which path is validated vs permissive.
503-514: Prefer a deterministic recipient in the transfer helper.
Address::random()makes the helper generate different tx hashes and block hashes on every run, which makes E2E failures harder to reproduce. Deriving the recipient fromnoncekeeps each block distinct without injecting entropy into the harness.♻️ Proposed fix
async fn transfer_tx_with_nonce(chain_id: u64, signer: PrivateKeySigner, nonce: u64) -> Bytes { + let mut recipient = [0u8; 20]; + recipient[0] = 0x42; + recipient[12..].copy_from_slice(&nonce.to_be_bytes()); + let tx = TransactionRequest { nonce: Some(nonce), value: Some(U256::from(100)), - to: Some(TxKind::Call(Address::random())), + to: Some(TxKind::Call(Address::new(recipient))), gas: Some(21_000), max_fee_per_gas: Some(20_000_000_000u128), max_priority_fee_per_gas: Some(20_000_000_000u128),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/node/src/test_utils.rs` around lines 503 - 514, The helper currently uses a non-deterministic recipient via TxKind::Call(Address::random()), which makes tx/block hashes vary; replace that with a deterministic address derived from the nonce so runs are reproducible: when building TransactionRequest (the tx), compute a deterministic Address from nonce (e.g., Address::from_low_u64_be(nonce.as_u64()) or by hashing the nonce and using Address::from_slice) and use TxKind::Call(that_address) before calling TransactionTestContext::sign_tx and signed.encoded_2718().into().
🤖 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/node/src/test_utils.rs`:
- Around line 521-522: The docs/comments reference APIs that have changed:
replace the nonexistent morph_payload_attributes_with_l1_msgs reference with the
correct utility name (or remove if it was moved), update examples that call
L1MessageBuilder::new() to reflect its deterministic/new-sender signature (use
the current constructor parameters instead of assuming a random sender), and
change MorphTxBuilder examples to call build_signed() synchronously (remove
async/await and .await) so the examples match the actual API; also remove the
"ignore" doc-test flags or update the code blocks to compile as real rustdoc
tests after these fixes so rustdoc will detect future drift (search for
L1MessageBuilder::new, morph_payload_attributes_with_l1_msgs, and
MorphTxBuilder::build_signed to locate all affected blocks).
---
Nitpick comments:
In `@crates/node/src/test_utils.rs`:
- Around line 727-730: MorphTxBuilder currently allows invalid states
(version=0, fee_token_id=0) and methods like with_reference and with_memo don't
enforce their documented constraints while build_signed will happily sign
unchecked inputs; update MorphTxBuilder so build_signed performs strict
validation of version, fee_token_id, reference/memo constraints and morph
config, add an explicit unchecked factory/terminal (e.g., build_signed_unchecked
or keep with_raw_morph_config as the documented escape hatch) for negative
tests, and enforce/validate constraints inside with_reference and with_memo to
fail-fast; update or add clear doc comments on MorphTxBuilder, with_reference,
with_memo, with_raw_morph_config, and build_signed to state which path is
validated vs permissive.
- Around line 503-514: The helper currently uses a non-deterministic recipient
via TxKind::Call(Address::random()), which makes tx/block hashes vary; replace
that with a deterministic address derived from the nonce so runs are
reproducible: when building TransactionRequest (the tx), compute a deterministic
Address from nonce (e.g., Address::from_low_u64_be(nonce.as_u64()) or by hashing
the nonce and using Address::from_slice) and use TxKind::Call(that_address)
before calling TransactionTestContext::sign_tx and signed.encoded_2718().into().
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 1c949a1d-f9ef-4b35-bec9-7fef0d7461db
📒 Files selected for processing (5)
crates/node/Cargo.tomlcrates/node/src/test_utils.rscrates/node/tests/it/helpers.rscrates/revm/src/error.rscrates/txpool/src/morph_tx_validation.rs
✅ Files skipped from review due to trivial changes (1)
- crates/revm/src/error.rs
🚧 Files skipped from review as they are similar to previous changes (3)
- crates/txpool/src/morph_tx_validation.rs
- crates/node/Cargo.toml
- crates/node/tests/it/helpers.rs
- validation.rs: test_verify_receipts_empty now uses the well-known Ethereum empty-trie root constant instead of calling calculate_receipt_root(&[]) — avoids circular test that would pass even if the root computation were wrong - l1block.rs: test_calculate_tx_l1_cost_curie now uses a pre-computed expected value (6_922_798_658_591) instead of replicating the formula inline — same fix for circular test anti-pattern - builder.rs (payload): test_best_transaction_attributes now asserts the returned basefee and blob_fee fields instead of discarding result - builder.rs (engine-api): add three unit tests covering all branches of resolve_fcu_block_tag_hash (L1 tag present / stale fallback / fresh block zero) - test_utils.rs: fix three doc-comment inaccuracies (non-existent morph_payload_attributes_with_l1_msgs reference, wrong sender default description, async .await? on synchronous build_signed)
Run morph-node integration tests (--features test-utils) as a separate job with --test-threads=1 and 8MB stack size, matching tempo's pattern of isolating E2E tests from unit tests.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
crates/consensus/src/validation.rs (2)
1828-1835: Tighten the overflow test assertion to pin failure mode.
is_err()is broad; this could pass for unrelated regressions. Assert the error content/variant so the test specifically guards overflow handling innext_l1_msg_indexcomputation.✅ Suggested assertion upgrade
- let result = validate_l1_messages_in_block(&txs, 0); - assert!(result.is_err()); + let err = validate_l1_messages_in_block(&txs, 0).unwrap_err().to_string(); + assert!(err.contains("expected 18446744073709551615")); + assert!(err.contains("got 0"));🤖 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 1828 - 1835, The test test_validate_l1_messages_in_block_queue_index_overflow currently only checks result.is_err(); tighten it to assert the specific overflow error from validate_l1_messages_in_block (the error variant returned by the next_l1_msg_index computation) so the test fails only for the intended overflow case. Locate the test function test_validate_l1_messages_in_block_queue_index_overflow and modify its assertion to match the concrete error variant or message produced by next_l1_msg_index (e.g., the specific enum variant or error string returned by validate_l1_messages_in_block) instead of using is_err().
1492-1519: Refactor duplicated chainspec fixture construction.This test inlines a second genesis JSON that can drift from
create_test_chainspec(). Consider a shared helper that toggles FeeVault to keep fixtures consistent.♻️ Suggested refactor
- // Create a chainspec with FeeVault explicitly enabled - let genesis_json = serde_json::json!({ - "config": { - "chainId": 1337, - "homesteadBlock": 0, - "eip150Block": 0, - "eip155Block": 0, - "eip158Block": 0, - "byzantiumBlock": 0, - "constantinopleBlock": 0, - "petersburgBlock": 0, - "istanbulBlock": 0, - "berlinBlock": 0, - "londonBlock": 0, - "bernoulliBlock": 0, - "curieBlock": 0, - "morph203Time": 0, - "viridianTime": 0, - "emeraldTime": 0, - "morph": { - "feeVaultAddress": "0x530000000000000000000000000000000000000a" - } - }, - "alloc": {} - }); - let genesis: Genesis = serde_json::from_value(genesis_json).unwrap(); - let chain_spec = Arc::new(MorphChainSpec::from(genesis)); + let chain_spec = create_test_chainspec_with_fee_vault(true);🤖 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 1492 - 1519, This test duplicates chainspec construction by inlining genesis_json and creating a MorphChainSpec from it; extract a shared helper (e.g., create_test_chainspec_with_fee_vault or add a flag to create_test_chainspec) to produce a Genesis/MorphChainSpec with FeeVault enabled and reuse it here instead of re-creating genesis_json and calling MorphChainSpec::from; update the test to call that helper (referencing create_test_chainspec, MorphChainSpec, and the feeVaultAddress/morph.feeVaultAddress field) so the fixture remains consistent across tests.crates/node/src/test_utils.rs (2)
725-745:MorphTxBuilder::new()starts from an invalid tx shape.The default state is
version = 0+fee_token_id = 0, so forgetting to pick a fee mode produces a structurally invalid MorphTx even though this constructor advertises "sensible defaults". Defaulting to the valid v1 ETH-fee shape would make the common path safer and leavewith_raw_morph_config()as the explicit escape hatch for negative tests.♻️ Possible adjustment
- /// Defaults to v0, fee_token_id=0 (must call `with_v0_token_fee` or - /// `with_v1_eth_fee` before building). + /// Defaults to v1 with ETH fee payment so `build_signed()` yields a valid + /// transaction unless a test opts into another mode. pub fn new(chain_id: u64, signer: PrivateKeySigner, nonce: u64) -> Self { Self { chain_id, signer, nonce, gas_limit: 100_000, max_fee_per_gas: 20_000_000_000u128, max_priority_fee_per_gas: 20_000_000_000u128, to: TxKind::Call(Address::with_last_byte(0x42)), value: U256::ZERO, input: Bytes::new(), - version: 0, + version: 1, fee_token_id: 0, fee_limit: U256::ZERO, reference: None, memo: None, }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/node/src/test_utils.rs` around lines 725 - 745, MorphTxBuilder::new currently initializes a structurally invalid default (version = 0 and fee_token_id = 0); change the constructor to produce a valid v1 ETH-fee morph tx by default—either call the existing with_v1_eth_fee() inside new or set version = 1 and fee_token_id to the ETH fee token id constant used elsewhere; also update the doc comment to state that new defaults to v1 ETH-fee and keep with_raw_morph_config() as the explicit escape hatch for negative tests.
138-156: MakeHoodi/Mainnetschedule evaluation explicit.Using
SystemTime::now()here means the same test can exercise different fork sets as wall-clock time crosses a scheduled activation. That makes these presets hard to reproduce for historical commits and can also vary across skewed CI machines. An explicit reference timestamp would keep the helper deterministic.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/node/src/test_utils.rs` around lines 138 - 156, The current code computes a non-deterministic timestamp in the local variable now (via SystemTime::now()) which makes the fork activation logic for keys ending with "Time" non-reproducible; change this to use an explicit, deterministic reference timestamp provided to the helper (e.g., add a reference_ts: u64 parameter or a test-only constant) and replace the now variable with that value so the match against reference["config"][key] uses reference_ts; update any callers of this helper to pass a fixed timestamp (or provide a default constant) so tests are deterministic when deciding to set 0u64 or u64::MAX for time-based forks.
🤖 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/node/src/test_utils.rs`:
- Around line 521-522: The intra-doc link to advance_block_with_l1_messages is
unresolved because that function lives in the test suite, so replace the
markdown-style intra-doc link [`advance_block_with_l1_messages`] with plain code
formatting (e.g. `advance_block_with_l1_messages`) in the comment that also
references L1MessageBuilder; ensure the doc comment uses backticks for the
function name and leaves L1MessageBuilder as-is or similarly formatted so
rustdoc won’t try to resolve a non-public test symbol.
---
Nitpick comments:
In `@crates/consensus/src/validation.rs`:
- Around line 1828-1835: The test
test_validate_l1_messages_in_block_queue_index_overflow currently only checks
result.is_err(); tighten it to assert the specific overflow error from
validate_l1_messages_in_block (the error variant returned by the
next_l1_msg_index computation) so the test fails only for the intended overflow
case. Locate the test function
test_validate_l1_messages_in_block_queue_index_overflow and modify its assertion
to match the concrete error variant or message produced by next_l1_msg_index
(e.g., the specific enum variant or error string returned by
validate_l1_messages_in_block) instead of using is_err().
- Around line 1492-1519: This test duplicates chainspec construction by inlining
genesis_json and creating a MorphChainSpec from it; extract a shared helper
(e.g., create_test_chainspec_with_fee_vault or add a flag to
create_test_chainspec) to produce a Genesis/MorphChainSpec with FeeVault enabled
and reuse it here instead of re-creating genesis_json and calling
MorphChainSpec::from; update the test to call that helper (referencing
create_test_chainspec, MorphChainSpec, and the
feeVaultAddress/morph.feeVaultAddress field) so the fixture remains consistent
across tests.
In `@crates/node/src/test_utils.rs`:
- Around line 725-745: MorphTxBuilder::new currently initializes a structurally
invalid default (version = 0 and fee_token_id = 0); change the constructor to
produce a valid v1 ETH-fee morph tx by default—either call the existing
with_v1_eth_fee() inside new or set version = 1 and fee_token_id to the ETH fee
token id constant used elsewhere; also update the doc comment to state that new
defaults to v1 ETH-fee and keep with_raw_morph_config() as the explicit escape
hatch for negative tests.
- Around line 138-156: The current code computes a non-deterministic timestamp
in the local variable now (via SystemTime::now()) which makes the fork
activation logic for keys ending with "Time" non-reproducible; change this to
use an explicit, deterministic reference timestamp provided to the helper (e.g.,
add a reference_ts: u64 parameter or a test-only constant) and replace the now
variable with that value so the match against reference["config"][key] uses
reference_ts; update any callers of this helper to pass a fixed timestamp (or
provide a default constant) so tests are deterministic when deciding to set 0u64
or u64::MAX for time-based forks.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 834c87f7-5c7d-4fa7-9659-739a66107fc1
📒 Files selected for processing (6)
.github/workflows/test.ymlcrates/consensus/src/validation.rscrates/engine-api/src/builder.rscrates/node/src/test_utils.rscrates/payload/builder/src/builder.rscrates/revm/src/l1block.rs
✅ Files skipped from review due to trivial changes (2)
- crates/revm/src/l1block.rs
- crates/payload/builder/src/builder.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- crates/engine-api/src/builder.rs
GitHub Actions ubuntu-latest defaults to vm.max_map_count=65530. MDBX allocates multiple memory-mapped regions per database instance; after ~15 sequential E2E tests the limit is exhausted (error code 12 = ENOMEM). Setting 2097152 matches reth/MDBX recommended production value.
cargo test --test-threads=1 runs tests sequentially but in the same
process. Reth nodes leak MDBX file descriptors across tests because
the async runtime/TaskManager doesn't fully shut down between tests
("Global executor already set"). After ~15 tests MDBX hits ENOMEM.
nextest runs each test in its own process, guaranteeing full MDBX
cleanup. This matches how Tempo handles the same issue.
- Switch E2E job from cargo test to cargo nextest
- Configure nextest CI profile: threads-required=2 so only 1 E2E test
runs at a time on the 2-slot CI runner
- Remove ineffective vm.max_map_count sysctl (wasn't the root cause)
-E 'test(it::)' matches test function names (e.g. block_building::...). Since the binary is already selected via --test it, no extra -E filter is needed. Also fix the nextest.toml override to use binary(it).
engine-api/builder.rs: extract header_and_body_from_executable_data() so assemble_l2_block can reuse the same header projection logic. Recompute executable_data.hash using the projected header (mix_hash=0, nonce=0, extra_data=empty, parent_beacon_block_root=None, requests_hash=None) to match what execution_payload_from_executable_data will reconstruct from ExecutableL2Data — those fields are absent from the struct so the import side can only fill them with fixed values. payload/builder: add executable_data_hash() and use it instead of sealed_block.hash() for ExecutableL2Data.hash. One extra keccak256 per block (negligible at 1s/block) in exchange for hash round-trip correctness without protocol changes. ci: add Clippy step for test-utils feature gate test: add make_eip4844_tx helper and use it in blob rejection test test: use real signed EIP-4844 tx instead of fake bytes in txpool test test: rename tx_max_fee_below_base_fee test to reflect actual behavior test: add next_l1_msg_index_can_skip_past_included_messages test: add engine_assembleL2Block / engine_newL2Block / engine_validateL2Block RPC tests test: add eth_getTransactionReceipt and eth_getTransactionByHash Morph fields tests
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