chore: add e2e testcase for contract size #98
Conversation
|
Claude finished @tac0turtle's task —— View job PR Review: E2E Test for Contract SizeI've completed my review of this PR that adds contract size limit validation functionality and e2e tests. Here's my comprehensive feedback: Todo List:
✅ StrengthsExcellent Test Coverage & Design:
Clean Architecture & Implementation:
Configuration Validation:
🔧 Minor Issues & Suggestions1. Magic Number Documentation (
|
Summary of ChangesHello @tac0turtle, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly improves the testing coverage for the contract size limit feature within the EVM and node configuration. It ensures that the system correctly enforces default contract size restrictions, allows for configurable overrides, and validates these configurations to prevent erroneous settings. The addition of both unit and end-to-end tests provides robust verification of this critical EVM behavior. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request adds e2e tests for contract size limits, along with the necessary configuration validation and test helpers. The changes are logically sound and the tests cover the intended functionality well. I've identified a few areas with code duplication across tests and test helpers. Addressing these will improve the long-term maintainability of the codebase. My comments provide specific suggestions for refactoring.
| #[test] | ||
| fn contract_size_limit_rejects_oversized_code() { | ||
| let caller = address!("0x0000000000000000000000000000000000000abc"); | ||
| let initcode = oversized_initcode(OVERSIZED_RUNTIME_SIZE); | ||
|
|
||
| let mut state = empty_state(); | ||
| state.insert_account( | ||
| caller, | ||
| AccountInfo { | ||
| balance: U256::from(10_000_000_000u64), | ||
| nonce: 0, | ||
| code_hash: KECCAK_EMPTY, | ||
| code: None, | ||
| }, | ||
| ); | ||
|
|
||
| let mut evm_env: alloy_evm::EvmEnv<SpecId> = EvmEnv::default(); | ||
| evm_env.cfg_env.chain_id = 1; | ||
| evm_env.cfg_env.spec = SpecId::CANCUN; | ||
| evm_env.block_env.number = U256::from(1); | ||
| evm_env.block_env.basefee = 1; | ||
| evm_env.block_env.gas_limit = 30_000_000; | ||
|
|
||
| let mut evm = EvEvmFactory::new(alloy_evm::eth::EthEvmFactory::default(), None, None, None) | ||
| .create_evm(state, evm_env); | ||
|
|
||
| let tx = crate::factory::TxEnv { | ||
| caller, | ||
| kind: TxKind::Create, | ||
| gas_limit: 15_000_000, | ||
| gas_price: 1, | ||
| value: U256::ZERO, | ||
| data: initcode, | ||
| ..Default::default() | ||
| }; | ||
|
|
||
| let result_and_state = evm | ||
| .transact_raw(tx) | ||
| .expect("oversized create executes to a halt"); | ||
|
|
||
| let ExecutionResult::Halt { reason, .. } = result_and_state.result else { | ||
| panic!("expected oversized code to halt"); | ||
| }; | ||
| assert!( | ||
| matches!(reason, HaltReason::CreateContractSizeLimit), | ||
| "expected create code size limit halt" | ||
| ); | ||
| } | ||
|
|
||
| #[test] | ||
| fn contract_size_limit_allows_oversized_code_when_configured() { | ||
| let caller = address!("0x0000000000000000000000000000000000000def"); | ||
| let initcode = oversized_initcode(OVERSIZED_RUNTIME_SIZE); | ||
|
|
||
| let mut state = empty_state(); | ||
| state.insert_account( | ||
| caller, | ||
| AccountInfo { | ||
| balance: U256::from(10_000_000_000u64), | ||
| nonce: 0, | ||
| code_hash: KECCAK_EMPTY, | ||
| code: None, | ||
| }, | ||
| ); | ||
|
|
||
| let mut evm_env: alloy_evm::EvmEnv<SpecId> = EvmEnv::default(); | ||
| evm_env.cfg_env.chain_id = 1; | ||
| evm_env.cfg_env.spec = SpecId::CANCUN; | ||
| evm_env.block_env.number = U256::from(1); | ||
| evm_env.block_env.basefee = 1; | ||
| evm_env.block_env.gas_limit = 30_000_000; | ||
|
|
||
| let mut evm = EvEvmFactory::new( | ||
| alloy_evm::eth::EthEvmFactory::default(), | ||
| None, | ||
| None, | ||
| Some(ContractSizeLimitSettings::new(64 * 1024, 0)), | ||
| ) | ||
| .create_evm(state, evm_env); | ||
|
|
||
| let tx = crate::factory::TxEnv { | ||
| caller, | ||
| kind: TxKind::Create, | ||
| gas_limit: 15_000_000, | ||
| gas_price: 1, | ||
| value: U256::ZERO, | ||
| data: initcode, | ||
| ..Default::default() | ||
| }; | ||
|
|
||
| let result_and_state = evm.transact_raw(tx).expect("oversized create executes"); | ||
|
|
||
| let ExecutionResult::Success { .. } = result_and_state.result else { | ||
| panic!("expected oversized code to deploy with custom limit"); | ||
| }; | ||
|
|
||
| let created_address = result_and_state | ||
| .result | ||
| .created_address() | ||
| .expect("created address available"); | ||
| let state: EvmState = result_and_state.state; | ||
| let created_account = state | ||
| .get(&created_address) | ||
| .expect("created contract must be in state"); | ||
| let code = created_account.info.code.as_ref().expect("code stored"); | ||
| assert_eq!( | ||
| code.len(), | ||
| OVERSIZED_RUNTIME_SIZE, | ||
| "contract runtime size should match initcode payload" | ||
| ); | ||
| } |
There was a problem hiding this comment.
There's significant code duplication between contract_size_limit_rejects_oversized_code and contract_size_limit_allows_oversized_code_when_configured. The setup for state, caller account, and evm_env is nearly identical.
To improve readability and maintainability, consider extracting this common setup into a helper function. This function could take the ContractSizeLimitSettings as a parameter and return the configured EVM instance and other necessary test data.
| /// Creates a reusable chain specification with a custom contract size limit. | ||
| pub fn create_test_chain_spec_with_contract_size_limit( | ||
| contract_size_limit: usize, | ||
| activation_height: Option<u64>, | ||
| ) -> Arc<ChainSpec> { | ||
| let mut genesis: Genesis = | ||
| serde_json::from_str(include_str!("../assets/genesis.json")).expect("valid genesis"); | ||
|
|
||
| let mut extras = serde_json::Map::new(); | ||
| extras.insert("contractSizeLimit".to_string(), json!(contract_size_limit)); | ||
| if let Some(height) = activation_height { | ||
| extras.insert( | ||
| "contractSizeLimitActivationHeight".to_string(), | ||
| json!(height), | ||
| ); | ||
| } | ||
|
|
||
| genesis | ||
| .config | ||
| .extra_fields | ||
| .insert("evolve".to_string(), serde_json::Value::Object(extras)); | ||
|
|
||
| Arc::new( | ||
| ChainSpecBuilder::default() | ||
| .chain(reth_chainspec::Chain::from_id(TEST_CHAIN_ID)) | ||
| .genesis(genesis) | ||
| .cancun_activated() | ||
| .build(), | ||
| ) | ||
| } |
There was a problem hiding this comment.
The logic for creating a ChainSpec (loading genesis, building with ChainSpecBuilder) is duplicated between this new function create_test_chain_spec_with_contract_size_limit and the existing create_test_chain_spec_with_extras.
To reduce duplication, you could refactor create_test_chain_spec_with_extras to be more generic, allowing it to handle different "extra" configurations, and have the public helper functions call it. This would centralize the ChainSpec creation logic.
| fn oversized_initcode(runtime_size: usize) -> Bytes { | ||
| const INIT_PREFIX_LEN: usize = 14; | ||
|
|
||
| assert!( | ||
| runtime_size <= u16::MAX as usize, | ||
| "runtime size must fit in PUSH2" | ||
| ); | ||
|
|
||
| let size_hi = ((runtime_size >> 8) & 0xff) as u8; | ||
| let size_lo = (runtime_size & 0xff) as u8; | ||
| let mut initcode = Vec::with_capacity(INIT_PREFIX_LEN + runtime_size); | ||
|
|
||
| initcode.extend_from_slice(&[ | ||
| 0x61, | ||
| size_hi, | ||
| size_lo, // PUSH2 size | ||
| 0x60, | ||
| INIT_PREFIX_LEN as u8, // PUSH1 offset | ||
| 0x60, | ||
| 0x00, // PUSH1 0 | ||
| 0x39, // CODECOPY | ||
| 0x61, | ||
| size_hi, | ||
| size_lo, // PUSH2 size | ||
| 0x60, | ||
| 0x00, // PUSH1 0 | ||
| 0xf3, // RETURN | ||
| ]); | ||
| initcode.resize(INIT_PREFIX_LEN + runtime_size, 0u8); | ||
|
|
||
| Bytes::from(initcode) | ||
| } |
There was a problem hiding this comment.
This oversized_initcode function is a duplicate of the one found in crates/ev-revm/src/factory.rs. To improve maintainability and avoid inconsistencies, this function should be defined in a single, shared location.
Consider creating a common test utility module or crate (e.g., a test-utils crate or exposing it from ev-revm under a test-utils feature flag) that both ev-revm's tests and the e2e tests can depend on.
Description
This pr adds a e2e test for verifying the contract size limit is handled correctly.