fix: adding additional allow list test coverage#144
Merged
Conversation
📝 WalkthroughWalkthroughAdds extensive tests across ev-revm, node txpool, and integration suites to validate deploy allowlist behavior (including CREATE vs CALL and CREATE2-via-factory scenarios) and makes one crate-local helper function Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
chatton
changed the title
|
Claude finished @chatton's task —— View job Code Review for Deploy Allowlist Test CoverageI've completed a comprehensive review of this PR. This is excellent work that significantly improves test coverage for the deploy allowlist functionality across multiple layers of the system. Todo List:
🎯 Overall Assessment: EXCELLENTThis PR provides comprehensive test coverage for deploy allowlist functionality with well-structured tests across multiple layers. The code is high quality with good documentation and clear test scenarios. ✅ Strengths
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (3)
crates/ev-revm/src/deploy.rs (1)
97-137: Add one activation-height boundary test to complete policy coverage.Current cases always use active settings (
activation_height = 0). A not-yet-active case would lock in expected behavior for future regressions.Proposed test addition
+ #[test] + fn inactive_allowlist_does_not_restrict_deploy() { + let allowed = address!("0x00000000000000000000000000000000000000aa"); + let caller = address!("0x00000000000000000000000000000000000000bb"); + let settings = DeployAllowlistSettings::new(vec![allowed], 100); + let result = check_deploy_allowed(Some(&settings), caller, true, 99); + assert!(result.is_ok()); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/ev-revm/src/deploy.rs` around lines 97 - 137, Add a test that covers the activation-height boundary by creating DeployAllowlistSettings with activation_height > 0 (e.g., 10) and verifying behavior before activation: construct a caller not in the allowlist, build settings with DeployAllowlistSettings::new(vec![allowed_addr], 10), call check_deploy_allowed(Some(&settings), caller, true, /*current_height*/ 0) and assert it returns Ok (or equivalently that settings.is_allowed is not applied pre-activation); this ensures the allowlist is ignored until activation_height is reached and guards against regressions.crates/node/src/txpool.rs (1)
908-972: Optional: consolidate repeated tx construction/state boilerplate in these tests.A small helper for
(validator, pooled_tx, sender_balance, state)setup would make future scenario additions faster and less error-prone.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/node/src/txpool.rs` around lines 908 - 972, Extract the repeated setup in evnode_create_allowed_when_allowlist_is_none, evnode_create_allowed_for_allowlisted_caller, and evnode_call_allowed_for_non_allowlisted_caller into a small test helper that returns the prepared validator, pooled tx, sender_balance and state; e.g. add a function (used by tests) like prepare_evnode_test(settings: Option<DeployAllowlistSettings>, signer: Address, is_create: bool, gas_limit: u64, max_fee_per_gas: u128) -> (ValidatorType, PooledTxType, U256, Option<Box<dyn AccountInfoReader + Send>>) that calls create_test_validator, create_non_sponsored_evnode_create_tx/create_non_sponsored_evnode_tx, create_pooled_tx and computes sender_balance = *pooled.cost() + U256::from(1) and state = None, then update each test to call this helper and assert on validator.validate_evnode(&pooled, sender_balance, &mut state).crates/ev-revm/src/handler.rs (1)
1342-1426: Optional: factor repeated context setup into a small test helper.The new tests are clear, but a shared builder/helper would reduce maintenance noise and keep intent-focused assertions.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/ev-revm/src/handler.rs` around lines 1342 - 1426, Several tests repeat the same Context/EvEvm/handler setup (e.g., in tests allow_deploy_when_allowlist_is_empty, allow_deploy_when_allowlist_is_none, allow_deploy_for_allowlisted_caller, call_tx_allowed_for_non_allowlisted_caller); factor that into a small test helper like build_test_env or TestEnv::new that accepts parameters (caller: Address, tx_kind: TxKind, allowlist: Option<DeployAllowlistSettings>, gas_limit: u64, gas_price: u128) and returns the initialized Context (or EvEvm and TestHandler) so tests call that helper instead of duplicating Context::mainnet().with_db(EmptyDB::default()), setting ctx.block.number/spec/disable_nonce_check/tx fields, creating EvEvm::new and EvHandler::new; update each test to use the helper and assert on handler.validate_against_state_and_deduct_caller() as before.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@crates/ev-revm/src/deploy.rs`:
- Around line 97-137: Add a test that covers the activation-height boundary by
creating DeployAllowlistSettings with activation_height > 0 (e.g., 10) and
verifying behavior before activation: construct a caller not in the allowlist,
build settings with DeployAllowlistSettings::new(vec![allowed_addr], 10), call
check_deploy_allowed(Some(&settings), caller, true, /*current_height*/ 0) and
assert it returns Ok (or equivalently that settings.is_allowed is not applied
pre-activation); this ensures the allowlist is ignored until activation_height
is reached and guards against regressions.
In `@crates/ev-revm/src/handler.rs`:
- Around line 1342-1426: Several tests repeat the same Context/EvEvm/handler
setup (e.g., in tests allow_deploy_when_allowlist_is_empty,
allow_deploy_when_allowlist_is_none, allow_deploy_for_allowlisted_caller,
call_tx_allowed_for_non_allowlisted_caller); factor that into a small test
helper like build_test_env or TestEnv::new that accepts parameters (caller:
Address, tx_kind: TxKind, allowlist: Option<DeployAllowlistSettings>, gas_limit:
u64, gas_price: u128) and returns the initialized Context (or EvEvm and
TestHandler) so tests call that helper instead of duplicating
Context::mainnet().with_db(EmptyDB::default()), setting
ctx.block.number/spec/disable_nonce_check/tx fields, creating EvEvm::new and
EvHandler::new; update each test to use the helper and assert on
handler.validate_against_state_and_deduct_caller() as before.
In `@crates/node/src/txpool.rs`:
- Around line 908-972: Extract the repeated setup in
evnode_create_allowed_when_allowlist_is_none,
evnode_create_allowed_for_allowlisted_caller, and
evnode_call_allowed_for_non_allowlisted_caller into a small test helper that
returns the prepared validator, pooled tx, sender_balance and state; e.g. add a
function (used by tests) like prepare_evnode_test(settings:
Option<DeployAllowlistSettings>, signer: Address, is_create: bool, gas_limit:
u64, max_fee_per_gas: u128) -> (ValidatorType, PooledTxType, U256,
Option<Box<dyn AccountInfoReader + Send>>) that calls create_test_validator,
create_non_sponsored_evnode_create_tx/create_non_sponsored_evnode_tx,
create_pooled_tx and computes sender_balance = *pooled.cost() + U256::from(1)
and state = None, then update each test to call this helper and assert on
validator.validate_evnode(&pooled, sender_balance, &mut state).
There was a problem hiding this comment.
🧹 Nitpick comments (1)
crates/tests/src/test_deploy_allowlist.rs (1)
187-200: Consider asserting exact child runtime bytecode (not only non-empty code).
is_empty()confirms deployment happened, but matching expected runtime makes the CREATE2 verification stricter.♻️ Optional hardening diff
const CREATE2_CHILD_INITCODE: [u8; 22] = alloy_primitives::hex!( "600a600c600039600a6000f3" // initcode: copies 10-byte runtime to memory and returns it "604260005260206000f3" // runtime: returns 0x42 ); +const CREATE2_CHILD_RUNTIME: [u8; 10] = alloy_primitives::hex!("604260005260206000f3"); @@ let child_code = EthApiClient::<TransactionRequest, Transaction, Block, Receipt, Header, Bytes>::get_code( &env.node_clients[0].rpc, expected_child_address, Some(BlockId::latest()), ) .await?; - assert!( - !child_code.is_empty(), - "child contract should be deployed via CREATE2 despite caller not being on the allowlist" - ); + assert_eq!( + child_code.as_ref(), + &CREATE2_CHILD_RUNTIME[..], + "unexpected runtime at expected CREATE2 address" + );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/tests/src/test_deploy_allowlist.rs` around lines 187 - 200, Replace the loose non-empty check on the deployed child contract with an exact runtime-bytecode match: compute the expected runtime bytecode (derived from CREATE2_CHILD_INITCODE or the known compiled runtime for the child), fetch the on-chain code via EthApiClient::get_code (as in the existing call returning child_code) and assert equality between child_code and the expected runtime bytes (or their hash) instead of using child_code.is_empty(); update the assertion message accordingly and keep references to CREATE2_CHILD_INITCODE, init_code_hash, expected_child_address, get_code, and child_code to locate where to change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@crates/tests/src/test_deploy_allowlist.rs`:
- Around line 187-200: Replace the loose non-empty check on the deployed child
contract with an exact runtime-bytecode match: compute the expected runtime
bytecode (derived from CREATE2_CHILD_INITCODE or the known compiled runtime for
the child), fetch the on-chain code via EthApiClient::get_code (as in the
existing call returning child_code) and assert equality between child_code and
the expected runtime bytes (or their hash) instead of using
child_code.is_empty(); update the assertion message accordingly and keep
references to CREATE2_CHILD_INITCODE, init_code_hash, expected_child_address,
get_code, and child_code to locate where to change.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
crates/tests/src/e2e_tests.rscrates/tests/src/lib.rscrates/tests/src/test_deploy_allowlist.rs
There was a problem hiding this comment.
🧹 Nitpick comments (2)
crates/tests/src/test_deploy_allowlist.rs (2)
97-110: Consider extracting a sharedTransactionRequestbuilder for repeated EIP-1559 fields.The same gas/fee/chain/value/input scaffold is repeated three times. A small helper makes intent clearer and reduces drift risk.
Proposed refactor sketch
+let make_tx = |nonce: u64, to: TxKind, data: Bytes| TransactionRequest { + nonce: Some(nonce), + gas: Some(1_000_000), + max_fee_per_gas: Some(20_000_000_000), + max_priority_fee_per_gas: Some(2_000_000_000), + chain_id: Some(chain_id), + value: Some(U256::ZERO), + to: Some(to), + input: TransactionInput { input: None, data: Some(data) }, + ..Default::default() +}; + -let denied_deploy_tx = TransactionRequest { ... }; +let denied_deploy_tx = + make_tx(0, TxKind::Create, Bytes::from_static(&CREATE2_FACTORY_INITCODE));Also applies to: 155-168, 204-217
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/tests/src/test_deploy_allowlist.rs` around lines 97 - 110, Extract the repeated EIP-1559 scaffold into a small helper that returns a base TransactionRequest populated with the common fields (nonce/gas/max_fee_per_gas/max_priority_fee_per_gas/chain_id/value and the TransactionInput with CREATE2_FACTORY_INITCODE), then use that helper to create denied_deploy_tx and the other two requests by overriding only the differing fields (e.g., to/input if needed). Reference the TransactionRequest struct and CREATE2_FACTORY_INITCODE when naming the helper (e.g., build_eip1559_tx or base_transaction_request) so callers replace the duplicated blocks with calls to the helper and apply per-case changes.
112-139: Assert denied deployment by tx hash/receipt instead of requiring an empty block.Line 127–139 currently assumes the whole block has zero transactions. That can become brittle if unrelated/system txs are ever present. It’s more direct to assert the denied tx was not included.
Proposed change
let denied_envelope = TransactionTestContext::sign_tx(non_allowlisted.clone(), denied_deploy_tx).await; let denied_raw: Bytes = denied_envelope.encoded_2718().into(); +let denied_tx_hash = *denied_envelope.tx_hash(); build_block_with_transactions( @@ ) .await?; -let latest_block = env.node_clients[0] - .get_block_by_number(BlockNumberOrTag::Latest) - .await? - .expect("latest block available after denied deploy"); -let denied_tx_count = match latest_block.transactions { - BlockTransactions::Full(ref txs) => txs.len(), - BlockTransactions::Hashes(ref hashes) => hashes.len(), - BlockTransactions::Uncle => 0, -}; -assert_eq!( - denied_tx_count, 0, - "non-allowlisted top-level CREATE should be excluded from the block" -); +let denied_receipt = EthApiClient::< + TransactionRequest, + Transaction, + Block, + Receipt, + Header, + Bytes, +>::transaction_receipt(&env.node_clients[0].rpc, denied_tx_hash) +.await?; +assert!( + denied_receipt.is_none(), + "non-allowlisted top-level CREATE should be excluded from the chain" +);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/tests/src/test_deploy_allowlist.rs` around lines 112 - 139, The test currently asserts the whole block is empty which is brittle; instead compute the denied transaction's hash from denied_envelope/denied_raw and assert that latest_block.transactions does not contain that hash (or call the node client's get_transaction_receipt for that tx hash and assert it returns None/no receipt). Update the assertion area after build_block_with_transactions/get_block_by_number to search latest_block.transactions for the denied tx hash (or query receipt via get_transaction_receipt) and fail if the denied tx is present, leaving other transactions unaffected; use the existing symbols denied_envelope, denied_raw, latest_block, and the node client get_transaction_receipt/get_block_by_number helpers to locate and check the denied transaction.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@crates/tests/src/test_deploy_allowlist.rs`:
- Around line 97-110: Extract the repeated EIP-1559 scaffold into a small helper
that returns a base TransactionRequest populated with the common fields
(nonce/gas/max_fee_per_gas/max_priority_fee_per_gas/chain_id/value and the
TransactionInput with CREATE2_FACTORY_INITCODE), then use that helper to create
denied_deploy_tx and the other two requests by overriding only the differing
fields (e.g., to/input if needed). Reference the TransactionRequest struct and
CREATE2_FACTORY_INITCODE when naming the helper (e.g., build_eip1559_tx or
base_transaction_request) so callers replace the duplicated blocks with calls to
the helper and apply per-case changes.
- Around line 112-139: The test currently asserts the whole block is empty which
is brittle; instead compute the denied transaction's hash from
denied_envelope/denied_raw and assert that latest_block.transactions does not
contain that hash (or call the node client's get_transaction_receipt for that tx
hash and assert it returns None/no receipt). Update the assertion area after
build_block_with_transactions/get_block_by_number to search
latest_block.transactions for the denied tx hash (or query receipt via
get_transaction_receipt) and fail if the denied tx is present, leaving other
transactions unaffected; use the existing symbols denied_envelope, denied_raw,
latest_block, and the node client get_transaction_receipt/get_block_by_number
helpers to locate and check the denied transaction.
randygrok
approved these changes
Mar 3, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Type of Change
Related Issues
Fixes #(issue)
Checklist
Testing
Additional Notes
Summary by CodeRabbit