Feat: aurora-evm-tests - ethjson refactoring#95
Conversation
…t/ethjson-refactoring
…t/ethjson-refactoring
WalkthroughMassive refactor: removes legacy ethjson-based test crates and EIP-152/ethcore-builtin code, renames and reorganizes jsontests into aurora-evm-jsontests, adds a new typed test harness (state + vm), precompiles (including KZG), blob/EIP helpers, JSON/hex utilities, CI/workspace updates, and small executor API/digest adjustments. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor CLI
participant Runner as Test Runner
participant Parser as Types & Parsers
participant Backend as MemoryBackend / Executor
participant Precomp as Precompiles Registry
participant Verifier as State Validator / Dumper
CLI->>Runner: run test(file, TestConfig)
Runner->>Parser: parse JSON → StateTestCase / VmTestCase
Runner->>Parser: compute MemoryVicinity (blob prices, fees)
Runner->>Backend: init MemoryBackend & StackExecutor
Runner->>Precomp: build PrecompileSet for Spec
Runner->>Backend: execute tx via Runtime
Backend->>Precomp: invoke precompile (if applicable)
Precomp-->>Backend: precompile result + gas
Backend-->>Runner: execution result (state, gas, output)
Runner->>Verifier: validate post-state hash & return
alt mismatch
Verifier-->>Runner: return failure details
else match
Verifier-->>Runner: return success
end
Runner-->>CLI: aggregated TestExecutionResult
Estimated Code Review Effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
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: 9
🧹 Nitpick comments (6)
evm-tests/jsontests/src/types/account_state.rs (1)
45-45: Remove trailing spaces in documentation.- /// Converts the `AccountsState` into a `BTreeMap` of `H160` addresses to `MemoryAccount`. + /// Converts the `AccountsState` into a `BTreeMap` of `H160` addresses to `MemoryAccount`.evm-tests/jsontests/src/state_dump.rs (1)
1-2: Address or remove the vague TODO comment.The TODO comment lacks specificity about what needs to be fixed. Either provide more details about the required fix or remove the comment if it's no longer relevant.
Would you like me to help identify what needs to be fixed based on the code analysis?
evm-tests/jsontests/src/assertions.rs (1)
1-422: File is entirely commented out.This assertion module appears to be temporarily disabled. Is this intentional pending integration with the refactored test execution framework?
Would you like assistance in adapting these assertions to work with the new test types?
evm-tests/jsontests/src/state.rs (1)
173-440: Significant amount of code is commented out.The core test execution logic (lines 173-440) is commented out. This appears to be a work-in-progress refactoring. Consider:
- Creating a TODO comment explaining why this code is commented
- Opening an issue to track the completion of this refactoring
- Removing the commented code if it's no longer needed
Would you like help completing the refactoring of the test execution logic?
evm-tests/jsontests/src/types/mod.rs (2)
127-142: Remove commented-out code.The commented-out code at the bottom of the method should be removed as it adds clutter and confusion.
71-71: Fix typo in enum variant name.The enum variant
GasPriseEip1559has a typo - it should beGasPriceEip1559.- return Err(InvalidTxReason::GasPriseEip1559); + return Err(InvalidTxReason::GasPriceEip1559);Also update the enum definition on line 293:
- GasPriseEip1559, + GasPriceEip1559,
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (26)
.github/workflows/lint.yml(1 hunks).github/workflows/rust.yml(3 hunks).gitignore(1 hunks)evm-tests/ethjson/src/spec/mod.rs(1 hunks)evm-tests/ethjson/src/state.rs(1 hunks)evm-tests/ethjson/src/test_helpers/state.rs(0 hunks)evm-tests/ethjson/src/vm.rs(1 hunks)evm-tests/jsontests/Cargo.toml(1 hunks)evm-tests/jsontests/src/assertions.rs(1 hunks)evm-tests/jsontests/src/config.rs(1 hunks)evm-tests/jsontests/src/execution_results.rs(1 hunks)evm-tests/jsontests/src/lib.rs(0 hunks)evm-tests/jsontests/src/main.rs(8 hunks)evm-tests/jsontests/src/precompiles.rs(1 hunks)evm-tests/jsontests/src/state.rs(14 hunks)evm-tests/jsontests/src/state_dump.rs(1 hunks)evm-tests/jsontests/src/types/account_state.rs(1 hunks)evm-tests/jsontests/src/types/blob.rs(1 hunks)evm-tests/jsontests/src/types/info.rs(1 hunks)evm-tests/jsontests/src/types/json_utils.rs(1 hunks)evm-tests/jsontests/src/types/mod.rs(1 hunks)evm-tests/jsontests/src/types/spec.rs(1 hunks)evm-tests/jsontests/src/types/transaction.rs(1 hunks)evm-tests/jsontests/src/types/vm.rs(1 hunks)evm-tests/jsontests/src/utils.rs(11 hunks)evm-tests/jsontests/src/vm.rs(3 hunks)
💤 Files with no reviewable changes (2)
- evm-tests/ethjson/src/test_helpers/state.rs
- evm-tests/jsontests/src/lib.rs
🧰 Additional context used
🧬 Code Graph Analysis (3)
evm-tests/jsontests/src/types/account_state.rs (1)
evm-tests/jsontests/src/types/json_utils.rs (7)
btree_h256_h256_from_str(80-96)deserialize_bytes_from_str_opt(202-211)deserialize_u256_from_str(117-121)h160_from_str(53-60)h160_from_str(103-103)h160_from_str(256-256)strip_0x_prefix(8-10)
evm-tests/jsontests/src/types/transaction.rs (1)
evm-tests/jsontests/src/types/json_utils.rs (10)
deserialize_bytes_from_str_opt(202-211)deserialize_h160_from_str(100-104)deserialize_h160_from_str_opt(251-258)deserialize_h256_from_u256_str_opt(134-140)deserialize_u256_from_str(117-121)deserialize_u256_from_str_opt(145-151)deserialize_u8_from_str_opt(165-171)deserialize_vec_h256_from_str(237-247)deserialize_vec_of_hex(185-197)deserialize_vec_u256_from_str(227-234)
evm-tests/jsontests/src/types/blob.rs (1)
evm-tests/jsontests/src/types/mod.rs (1)
from(211-228)
🪛 GitHub Actions: Lint
evm-tests/jsontests/src/state.rs
[warning] 147-147: Unused variable 'states'. If intentional, prefix it with an underscore.
[warning] 164-164: Unused variable 'data_max_fee'. If intentional, prefix it with an underscore.
[warning] 165-165: Unused variable 'data_fee'. If intentional, prefix it with an underscore.
[warning] 171-171: Unused variable 'original_state'. If intentional, prefix it with an underscore.
[warning] 172-172: Unused variable 'vicinity'. If intentional, prefix it with an underscore.
evm-tests/jsontests/src/types/mod.rs
[error] 104-104: Unresolved import: 'crate::utils::u256_to_h256'. Suggestion: a similar path exists: 'aurora_evm::utils'.
[error] 83-83: No field '0' on type '&StateTestCase'. Possibly meant 'self.pre_state.0.transaction.max_priority_fee_per_gas'.
[error] 96-96: No field '0' on type '&StateTestCase'. Possibly meant 'self.pre_state.0.env.random'.
[error] 112-112: Type mismatch: expected 'U256', found '[u64; 4]'. Suggestion: wrap the expression in 'primitive_types::U256'.
[error] 114-114: Method 'unwrap_caller' not found for '&StateTestCase'.
[error] 116-116: No field '0' on type '&StateTestCase'. Possibly meant 'self.pre_state.0.env.number'.
[error] 117-117: No field '0' on type '&StateTestCase'. Possibly meant 'self.pre_state.0.env.author'.
[error] 118-118: No field '0' on type '&StateTestCase'. Possibly meant 'self.pre_state.0.env.timestamp'.
[error] 119-119: No field '0' on type '&StateTestCase'. Possibly meant 'self.pre_state.0.env.difficulty'.
[error] 120-120: No field '0' on type '&StateTestCase'. Possibly meant 'self.pre_state.0.env.gas_limit'.
[error] 122-122: Type mismatch: expected 'U256', found '[u64; 4]'. Suggestion: wrap the expression in 'primitive_types::U256'.
[error] 124-124: Type mismatch: expected 'Option', found 'Option'.
[error] 125-125: Type mismatch: expected 'Vec', found 'Vec'.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: semgrep-cloud-platform/scan
- GitHub Check: ethereum-tests-32-bit
- GitHub Check: ethereum-tests
- GitHub Check: unit-tests-32-bit
🔇 Additional comments (36)
.gitignore (1)
13-13: LGTM! Appropriate test directory exclusion.Adding
ethtests/to .gitignore is correct as test directories typically contain fixtures or generated data that shouldn't be version controlled.evm-tests/ethjson/src/state.rs (1)
1-1: LGTM! Documentation update aligns with refactoring.The module documentation change from "State deserialization types" to "EVM state types" is appropriate and provides a more general description of the module's purpose.
evm-tests/ethjson/src/vm.rs (1)
1-1: LGTM! Consistent documentation update.The documentation change from "Vm json deserialization" to "EVM vm types" is consistent with the broader refactoring effort to generalize module descriptions.
evm-tests/ethjson/src/spec/mod.rs (1)
51-51: Removal ofSpecre-export is safeNo references to
ethjson::spec::Specremain in the codebase; removing thepub use self::spec::Spec;export will not break any consumers..github/workflows/rust.yml (4)
79-79: LGTM! Package name updated consistently.The package name change from
evm-jsonteststoaurora-evm-jsontestsis correct and aligns with the broader refactoring effort.
88-88: LGTM! Consistent package naming in VM tests.The package name update is properly applied to the VM tests execution.
141-141: LGTM! 32-bit state tests updated correctly.The package name change is consistently applied to the 32-bit test execution.
148-148: LGTM! 32-bit VM tests updated correctly.The package name change is consistently applied to the 32-bit VM test execution.
.github/workflows/lint.yml (1)
27-28: LGTM! Package name update is consistent with refactoring.The change correctly updates the Clippy command to target the renamed package
aurora-evm-jsontests, aligning with the broader refactoring objectives.evm-tests/jsontests/Cargo.toml (5)
2-2: LGTM! Package rename aligns with refactoring objectives.The package name change from
evm-jsonteststoaurora-evm-jsontestsis consistent with the overall refactoring to modernize the test framework.
4-4: LGTM! License change from GPL-3.0 to MIT.The license change is explicitly mentioned in the PR objectives as part of removing the GPL-3.0 license.
12-15: LGTM! Aggressive clippy lints support code quality goals.The addition of comprehensive clippy lints with deny level is appropriate for a refactoring effort and helps maintain high code quality standards.
19-19: LGTM! Explicit serde feature enables serialization functionality.Adding the explicit "serde" feature to primitive-types is likely needed for the new serialization/deserialization functionality in the refactored codebase.
40-40: LGTM! New dump-state feature supports state debugging.The new
dump-statefeature that depends onaurora-evm/with-serdeis consistent with the refactoring objectives and provides useful debugging capabilities.evm-tests/jsontests/src/config.rs (1)
1-18: LGTM! Well-designed configuration module with appropriate structure.The configuration module provides a clean separation of concerns with:
VerboseOutputstruct grouping verbosity-related flagsTestConfigstruct aggregating configuration elements- Proper use of
Option<Spec>for flexible fork specification- Appropriate clippy annotation for multiple boolean fields
The design supports the modularization objectives of the refactoring.
evm-tests/jsontests/src/types/info.rs (2)
4-33: LGTM! Well-structured metadata handling with proper serde configuration.The
Infostruct demonstrates good practices:
- Comprehensive field mapping using serde attributes
deny_unknown_fieldsfor strict deserialization- Proper use of
Option<T>for optional fields- Field renaming and aliasing for JSON compatibility
BTreeMapfor structured label storageThe structure supports rich test metadata capture as intended by the refactoring.
35-41: LGTM! Clean Git resolution structure with required fields.The
EelsResolutionstruct appropriately models Git resolution metadata with required string fields and proper serde field mapping.evm-tests/jsontests/src/execution_results.rs (4)
6-14: LGTM! Comprehensive failed test detail capture.The
FailedTestDetailsstruct appropriately captures all necessary information for debugging failed tests, including test metadata, expected/actual hashes, and full state information.
16-21: LGTM! Clean result aggregation structure.The
TestExecutionResultstruct provides a clear interface for tracking test execution statistics and detailed failure information.
23-32: LGTM! Proper constructor implementation with safety attributes.The constructor follows Rust best practices:
const fnfor compile-time constructionmust_useattribute to prevent accidental discarding- Clippy allow annotation for reasonable new() without Default
- Zero-initialized fields for clean starting state
34-39: LGTM! Efficient result merging implementation.The
mergemethod efficiently combines results usingextend()for vector merging and simple addition for counters, enabling proper result accumulation across test runs.evm-tests/jsontests/src/types/spec.rs (3)
6-53: Well-structured enum definition with comprehensive documentation.The
Specenum is properly designed with appropriate trait derivations and clear documentation for each hard fork variant.
55-81: Robust string parsing with comprehensive fork name support.The
FromStrimplementation correctly handles all fork names and their aliases, with descriptive error messages for unknown values.
82-107: Clean deserialization implementation following serde best practices.The custom
Deserializeimplementation correctly uses the visitor pattern and delegates to theFromStrimplementation for consistency.evm-tests/jsontests/src/vm.rs (1)
11-99: Clean refactoring to use the new VmTestCase type system.The VM test runner has been successfully refactored to use the new
VmTestCasetype with its accessor methods, maintaining the original test logic while improving code organization.evm-tests/jsontests/src/types/account_state.rs (2)
10-36: Well-designed account state representation with proper conversions.The
StateAccountstruct correctly models Ethereum account state with appropriate deserialization for each field. The conversion toMemoryAccountproperly handles the optional code field.
54-64: Correct address deserialization with proper hex prefix handling.The custom deserialization correctly strips the "0x" prefix and converts string keys to
H160addresses.evm-tests/jsontests/src/state_dump.rs (1)
26-43: Well-designed trait with appropriate default implementations.The
StateTestsDumpertrait provides a clean interface for state dumping with sensible no-op defaults.evm-tests/jsontests/src/utils.rs (6)
53-56: LGTM! Cleaner boolean check.The refactoring from match to if-else improves readability for a simple boolean condition.
143-143: Good performance optimization.Using
bytecount::countis more efficient than manual iteration for counting zero bytes.
167-170: Improved readability with digit grouping.Adding underscores to large numeric constants enhances readability.
Also applies to: 253-253, 257-257
326-328: Better type conversion practice.Using
u128::from()instead ofascasting is more explicit and type-safe.
429-429: More idiomatic iteration and safer type conversion.
- Direct iteration over references is cleaner than using
.iter()- Using
usize::try_from()for explicit conversion is safer than implicit castingAlso applies to: 444-444, 474-474
601-609: Function relocated without changes.The
unwrap_to_statefunction was moved to the bottom of the file without any logic modifications.evm-tests/jsontests/src/types/transaction.rs (1)
1-107: Well-structured transaction types.The transaction structures are properly designed with:
- Appropriate serde attributes for JSON deserialization
- Clear documentation for EIP references
- Proper use of optional fields and custom deserializers
evm-tests/jsontests/src/main.rs (1)
130-135: Good refactoring to use TestConfig struct.The introduction of
TestConfigimproves code organization by encapsulating test configuration parameters in a single struct, making the code more maintainable and reducing parameter passing.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
evm-tests/jsontests/src/types/mod.rs (2)
95-106: Fix unresolved reference and incorrect conversion in PREVRANDAO/randomness handling
self.env.randomis already an Option (see StateEnv.random). The conversion usingcrate::utils::u256_to_h256(r.0)is both unresolved and semantically wrong. Use the value directly.Apply this diff:
- let block_randomness = if *spec > Spec::Berlin { - self.env.random.map(|r| { - // Convert between U256 and H256. U256 is in little-endian but since H256 is just - // a string-like byte array, it's big endian (MSB is the first element of the array). - // - // Byte order here is important because this opcode has the same value as DIFFICULTY - // (0x44), and so for older forks of Ethereum, the threshold value of 2^64 is used to - // distinguish between the two: if it's below, the value corresponds to the DIFFICULTY - // opcode, otherwise to the PREVRANDAO opcode. - crate::utils::u256_to_h256(r.0) - }) - } else { - None - }; + let block_randomness = if *spec > Spec::Berlin { + self.env.random + } else { + None + };
111-126: Replace missing unwrap_caller and convert blob hashes to U256
self.unwrap_caller()doesn’t exist; use the transaction’s sender/origin instead.MemoryVicinity.blob_hashesexpects Vec, buttx.blob_versioned_hashesprovides Vec. Convert each H256 to U256.Apply this diff:
Ok(MemoryVicinity { gas_price, effective_gas_price, - origin: self.unwrap_caller(), + origin: tx + .sender + .ok_or(InvalidTxReason::CreateTransaction)?, block_hashes: Vec::new(), block_number: self.env.block_number, block_coinbase: self.env.block_coinbase, block_timestamp: self.env.block_timestamp, block_difficulty: self.env.block_difficulty, block_gas_limit: self.env.block_gas_limit, chain_id: U256::one(), block_base_fee_per_gas, block_randomness, blob_gas_price: blob_gas_price.map(|bgp| bgp.blob_gas_price), - blob_hashes, + blob_hashes: blob_hashes + .into_iter() + .map(|h| U256::from_big_endian(h.as_bytes())) + .collect(), })Verification needed:
- Confirm the Transaction struct has a
sender: Option<H160>(or adjust toorigin: H160if that’s what exists). If not present, either resolve the sender before calling this method or add the field.Run this to locate and inspect the Transaction fields:
#!/bin/bash # Find the Transaction struct and its fields rg -nU $'struct\\s+Transaction\\s*\\{[\\s\\S]*?\\}' -g '!target' | sed -n '1,200p' # Look for common caller fields rg -n "sender\\s*:|origin\\s*:|from\\s*:" -S
🧹 Nitpick comments (3)
evm-tests/jsontests/src/types/mod.rs (3)
121-121: Chain ID hardcoded to 1 may be inconsistent with VM testsVM tests in vm.rs use zero for chain_id (per snippet), while state tests set
U256::one(). Decide on a unified convention or make chain_id configurable via StateEnv to avoid inconsistent execution contexts.
293-294: Typo: GasPriseEip1559 -> GasPriceEip1559Fix the spelling to avoid confusion and improve readability. Update all usages accordingly.
Apply this diff and adjust the references at Line 71 (and elsewhere):
- GasPriseEip1559, + GasPriceEip1559,And:
- return Err(InvalidTxReason::GasPriseEip1559); + return Err(InvalidTxReason::GasPriceEip1559);
210-229: Minor: From<&StateEnv> could avoid moving envCurrent impl consumes StateEnv. If you only need to borrow, providing
impl From<&StateEnv> for MemoryVicinity(or an additionalimpl) can reduce moves/clones when reused across calls.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these settings in your CodeRabbit configuration.
📒 Files selected for processing (1)
evm-tests/jsontests/src/types/mod.rs(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
evm-tests/jsontests/src/types/mod.rs (5)
evm-tests/jsontests/src/types/json_utils.rs (7)
deserialize_bytes_from_str(175-180)deserialize_bytes_from_str_opt(202-211)deserialize_h160_from_str(100-104)deserialize_h256_from_u256_str(108-113)deserialize_h256_from_u256_str_opt(134-140)deserialize_u256_from_str(117-121)deserialize_u64_from_str_opt(155-161)evm-tests/jsontests/src/types/vm.rs (1)
get_memory_vicinity(70-87)evm-tests/jsontests/src/utils.rs (3)
u256_to_h256(8-10)new(182-188)new(217-233)evm-tests/jsontests/src/types/blob.rs (1)
new(66-72)evm-tests/jsontests/src/types/account_state.rs (1)
from(28-35)
🪛 GitHub Actions: Lint
evm-tests/jsontests/src/types/mod.rs
[error] 104-104: E0433: unresolved import: crate::utils::u256_to_h256. Help: a similar path exists: aurora_evm::utils.
[error] 114-114: E0599: no method named unwrap_caller found for '&StateTestCase'.
[error] 125-125: E0308: mismatched types: expected Vec, found Vec.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: ethereum-tests
- GitHub Check: unit-tests-32-bit
- GitHub Check: ethereum-tests-32-bit
🔇 Additional comments (1)
evm-tests/jsontests/src/types/mod.rs (1)
57-64: Double-check const-fn viability
get_memory_vicinityis a const fn, but it uses constructs that may not be const-stable across toolchains (e.g., Vec::new, map, min, match). If not required in const contexts, consider droppingconstto simplify constraints.
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (2)
evm-tests/jsontests/src/state.rs (1)
129-129: Fix iterator usage per Clippy warningThe Clippy error indicates incorrect usage of the iterator - you're iterating over keys when you need key-value pairs.
- for (spec, _states) in &test.post_states { + for (spec, _states) in test.post_states.iter() {evm-tests/jsontests/src/types/mod.rs (1)
70-77: Avoid panicking in const fn - return error for missing gas priceUsing
expect("expect gas price")will cause a panic ifgas_priceis None for pre-London transactions. This is problematic for a library that processes external test data.Consider returning a proper error instead:
} else { if tx.max_fee_per_gas.is_some() { return Err(InvalidTxReason::GasPriseEip1559); } - tx.gas_price.expect("expect gas price") + tx.gas_price.ok_or(InvalidTxReason::GasPriceLessThenBlockBaseFee)? };Or better yet, add a dedicated error variant:
pub enum InvalidTxReason { IntrinsicGas, OutOfFund, + MissingGasPrice, // ... rest of variantsThen use:
- tx.gas_price.expect("expect gas price") + tx.gas_price.ok_or(InvalidTxReason::MissingGasPrice)?
🧹 Nitpick comments (9)
evm-tests/jsontests/Cargo.toml (1)
39-40: Consider documenting thedump-statefeatureThe new
dump-statefeature enables state dumping functionality, which depends onaurora-evm/with-serde. Consider adding a comment explaining when and why developers should enable this feature.[features] enable-slow-tests = [] print-debug = ["aurora-evm/print-debug"] - +# Enables state dumping functionality for debugging test execution dump-state = ["aurora-evm/with-serde"]evm-tests/jsontests/src/types/transaction.rs (1)
42-47: Field name inconsistency:init_codesvsinitcodesThe
init_codesfield is renamed to"initcodes"during deserialization (line 44), which could cause confusion. Consider using a consistent naming convention.Either rename the field to match the JSON:
- pub init_codes: Option<Vec<u8>>, + pub initcodes: Option<Vec<u8>>,Or document why the different naming is necessary with a comment.
evm-tests/jsontests/src/state.rs (4)
137-141: Typo in comment: "Geet" should be "Get"- // Geet gasometer config for the current spec + // Get gasometer config for the current spec
146-151: Remove or properly handle unused variablesVariables
_data_max_feeand_data_feeare computed but never used, suggesting incomplete implementation. Similarly for_original_stateand_vicinitybelow.If these will be used when the commented code is restored, consider adding a TODO comment explaining the implementation plan. Otherwise, remove the unused computations:
- // EIP-4844 - let _data_max_fee = calc_max_data_fee(&gasometer_config, &test.transaction); - let _data_fee = calc_data_fee( - &gasometer_config, - &test.transaction, - blob_gas_price.as_ref(), - ); - - let _original_state = test.pre_state.as_ref().to_memory_accounts(); - let _vicinity = test.get_memory_vicinity(spec, blob_gas_price); + // TODO: Implement state test execution logic + // The following will be needed when implementation is complete: + // - data_max_fee and data_fee for EIP-4844 blob transactions + // - original_state for state transitions + // - vicinity for execution context
155-422: Large commented-out code block needs resolutionThere's a significant amount of commented-out implementation code (lines 155-422). This appears to be the actual test execution logic that's been temporarily disabled.
The commented code appears to contain the core test execution logic. Would you like me to help:
- Review and update this code to work with the new type system?
- Create a GitHub issue to track the implementation of this functionality?
- Identify what dependencies or changes are blocking this implementation?
Please clarify the plan for this code - will it be restored in a follow-up PR or is it being replaced with a different implementation?
463-474: Consider adding Spec variants validationThe
get_gasometer_configfunction returnsNonefor unsupported Spec variants. Consider documenting which specs are supported or adding a comment about future support.const fn get_gasometer_config(s: &Spec) -> Option<Config> { match s { Spec::Istanbul => Some(Config::istanbul()), Spec::Berlin => Some(Config::berlin()), Spec::London => Some(Config::london()), Spec::Merge => Some(Config::merge()), Spec::Shanghai => Some(Config::shanghai()), Spec::Cancun => Some(Config::cancun()), Spec::Prague => Some(Config::prague()), + // Earlier specs (Frontier, Homestead, etc.) are not yet supported _ => None, } }evm-tests/jsontests/src/types/mod.rs (3)
266-266: Typo in enum variant: "GasPriseEip1559" should be "GasPriceEip1559"- GasPriseEip1559, + GasPriceEip1559,Also update the usage on line 74:
- return Err(InvalidTxReason::GasPriseEip1559); + return Err(InvalidTxReason::GasPriceEip1559);
58-59: Fix grammatical error in documentation- /// Get the memory vicinity for the transaction, which includesState test data. + /// Get the memory vicinity for the transaction, which includes state test data.
14-20: Module visibility inconsistencyMost submodules are private except
blobwhich is public. Consider making the visibility consistent based on the intended API surface.If
blobneeds to be public for external use, document why. Otherwise:mod account_state; -pub mod blob; +mod blob; mod info; mod json_utils; mod spec; mod transaction; mod vm;If blob types need to be accessible, consider re-exporting specific items:
pub use spec::Spec; pub use vm::VmTestCase; +pub use blob::{BlobExcessGasAndPrice, calc_data_fee, calc_max_data_fee};
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these settings in your CodeRabbit configuration.
📒 Files selected for processing (5)
evm-tests/jsontests/Cargo.toml(1 hunks)evm-tests/jsontests/src/state.rs(13 hunks)evm-tests/jsontests/src/types/json_utils.rs(1 hunks)evm-tests/jsontests/src/types/mod.rs(1 hunks)evm-tests/jsontests/src/types/transaction.rs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- evm-tests/jsontests/src/types/json_utils.rs
🧰 Additional context used
🧬 Code Graph Analysis (3)
evm-tests/jsontests/src/types/mod.rs (4)
evm-tests/jsontests/src/types/json_utils.rs (7)
deserialize_bytes_from_str(175-180)deserialize_bytes_from_str_opt(202-211)deserialize_h160_from_str(100-104)deserialize_h256_from_u256_str(108-113)deserialize_h256_from_u256_str_opt(134-140)deserialize_u256_from_str(117-121)deserialize_u64_from_str_opt(155-161)evm-tests/jsontests/src/types/vm.rs (1)
get_memory_vicinity(70-87)evm-tests/jsontests/src/types/blob.rs (1)
new(66-72)evm-tests/jsontests/src/types/account_state.rs (1)
from(28-35)
evm-tests/jsontests/src/state.rs (3)
evm-tests/jsontests/src/types/blob.rs (4)
calc_data_fee(168-183)calc_max_data_fee(153-161)new(66-72)from_env(86-96)evm-tests/jsontests/src/execution_results.rs (2)
new(26-32)merge(34-38)evm/src/runtime/mod.rs (14)
new(56-70)istanbul(335-396)berlin(400-402)berlin(561-583)london(406-408)london(585-607)merge(412-414)merge(609-631)shanghai(418-420)shanghai(633-656)cancun(424-426)cancun(658-666)prague(430-432)prague(668-676)
evm-tests/jsontests/src/types/transaction.rs (2)
evm-tests/jsontests/src/types/json_utils.rs (9)
deserialize_bytes_from_str_opt(202-211)deserialize_h160_from_str(100-104)deserialize_h160_from_str_opt(252-259)deserialize_h256_from_u256_str_opt(134-140)deserialize_u256_from_str(117-121)deserialize_u256_from_str_opt(145-151)deserialize_u8_from_str_opt(165-171)deserialize_vec_of_hex(185-197)deserialize_vec_u256_from_str(227-234)evm/src/runtime/eval/system.rs (1)
sha3(11-33)
🪛 GitHub Actions: Lint
evm-tests/jsontests/src/state.rs
[error] 129-129: Clippy error: for_kv_map - iterating over a map's keys in test.post_states. Use '.iter()' for key-value pairs.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: unit-tests-32-bit
- GitHub Check: ethereum-tests-32-bit
- GitHub Check: ethereum-tests
🔇 Additional comments (1)
evm-tests/jsontests/Cargo.toml (1)
12-15: Strong linting configuration is good!The addition of comprehensive Clippy linting rules (
all,pedantic,nursery) withdenylevel will help maintain high code quality standards. This is especially valuable for a test framework where reliability is crucial.
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (8)
Cargo.toml (1)
27-29: Prefer crates.io release forgeneric-array.We’re still overriding
generic-arrayvia a git commit even though the inline comment says we only need v0.14.6 forsha3 = 0.10. Using crates.io keeps supply-chain checks and cargo caching working, while the git override introduces avoidable churn. Please constrain the crate directly instead of pinning to a forked revision unless there’s a tracked upstream fix that isn’t released yet.[patch.crates-io] -# Constrain to v0.14.6 to ensure compatibility with sha3 0.10 and prevent unexpected upgrades -generic-array = { git = "https://github.com/fizyk20/generic-array", rev = "effb209dd7e61b8226e194dec2979314e27598d1" } +# Constrain to v0.14.6 to ensure compatibility with sha3 0.10 and prevent unexpected upgrades +generic-array = "=0.14.6"evm-tests/src/precompiles/kzg.rs (1)
52-64: Importsize_ofso this module builds
size_of::<G1Points>()andsize_of::<G2Points>()won’t compile becausesize_ofisn’t in scope. Add the missing import.use c_kzg::{Bytes32, Bytes48, KzgProof, KzgSettings, BYTES_PER_G1_POINT, BYTES_PER_G2_POINT}; use core::convert::TryInto; use core::hash::{Hash, Hasher}; + use core::mem::size_of;evm-tests/src/types/ecrecover.rs (1)
6-25: Guard against short signatures before indexing
signature[64]and&signature[0..64]panic when fewer than 65 bytes are provided, so malformed inputs crash the process instead of returningExitError. Add a length check up front.pub fn ecrecover(hash: H256, signature: &[u8]) -> Result<H160, ExitError> { + if signature.len() < 65 { + return Err(ExitError::Other(Cow::from( + "ECRecoverErr invalid signature length", + ))); + } let hash = libsecp256k1::Message::parse_slice(hash.as_bytes()) .map_err(|e| ExitError::Other(Cow::from(e.to_string())))?;evm-tests/src/state.rs (1)
63-71: Avoid panicking when blob gas price is absent
BlobExcessGasAndPrice::from_envlegitimately returnsNonefor fixtures that omit blob gas metadata to exercise validation errors. Callingcalc_data_feebefore we know the price exists trips the internalexpect("expect blob_gas_price")and aborts the run instead of bubbling up anInvalidTxReason. Guard the call so we only computedata_feewhen a blob gas price is actually present.- let data_fee = calc_data_fee( - &gasometer_config, - &test.transaction, - blob_gas_price.as_ref(), - ); + let data_fee = if blob_gas_price.is_some() { + calc_data_fee( + &gasometer_config, + &test.transaction, + blob_gas_price.as_ref(), + ) + } else { + None + };evm-tests/src/types/vm.rs (1)
50-71: Guard optional VM fields instead ofunwrap()Many VM fixtures legitimately omit
out,gas, orpost, so theseunwrap()s will panic the harness before the failure can be reported. Treat the optionals as such and default/skip when they are absent.- pub fn get_output(&self) -> Vec<u8> { - self.output.clone().unwrap() - } + pub fn get_output(&self) -> Vec<u8> { + self.output.clone().unwrap_or_default() + } @@ - pub fn get_gas_left(&self) -> u64 { - self.gas_left.unwrap().as_u64() - } + pub fn get_gas_left(&self) -> u64 { + self.gas_left.unwrap_or_default().as_u64() + } @@ - pub fn validate_state(&self, state: &BTreeMap<H160, MemoryAccount>) -> bool { - &self - .post_state - .clone() - .unwrap() - .to_memory_accounts_state() - .0 - == state - } + pub fn validate_state(&self, state: &BTreeMap<H160, MemoryAccount>) -> bool { + self.post_state + .as_ref() + .map(|post| { + let expected = post.to_memory_accounts_state(); + expected.0.iter().eq(state.iter()) + }) + .unwrap_or(true) + }evm-tests/src/types/blob.rs (1)
178-187: Still panics whenblob_gas_priceis missing
calc_data_feecontinues to unwrap withexpect, so a malformed/missingblob_gas_pricewill crash the test runner instead of returningNone, despite the function already signaling failure viaOption. Please eliminate the panic path and short‑circuit gracefully.@@ -pub fn calc_data_fee( - config: &Config, - tx: &Transaction, - blob_gas_price: Option<&BlobExcessGasAndPrice>, -) -> Option<U256> { - config.has_shard_blob_transactions.then(|| { - U256::from( - blob_gas_price - .expect("expect blob_gas_price") - .blob_gas_price, - ) - .saturating_mul(U256::from(get_total_blob_gas( - tx.blob_versioned_hashes.len(), - ))) - }) -} +pub fn calc_data_fee( + config: &Config, + tx: &Transaction, + blob_gas_price: Option<&BlobExcessGasAndPrice>, +) -> Option<U256> { + if !config.has_shard_blob_transactions { + return None; + } + let blob_gas_price = blob_gas_price?; + Some( + U256::from(blob_gas_price.blob_gas_price).saturating_mul(U256::from( + get_total_blob_gas(tx.blob_versioned_hashes.len()), + )), + ) +}evm-tests/src/types/json_utils.rs (1)
25-35: Clamp u8 hex inputs to two charactersThe guard still allows up to 16 hex digits, so heavily padded values slip through even though a
u8can only be represented by two hex chars; earlier feedback called this out. Tighten the bound to keep fixtures canonical.-pub fn u8_from_str<'de, D: Deserializer<'de>>(value: &str) -> Result<u8, D::Error> { - if value.len() > 16 { +pub fn u8_from_str<'de, D: Deserializer<'de>>(value: &str) -> Result<u8, D::Error> { + if value.len() > 2 { return Err(Error::custom( format!("u8 value too big (length={})", value.len()).as_str(), )); }evm-tests/src/types/mod.rs (1)
74-81: Removeexpectand surface a proper legacy gas price errorLegacy transactions with missing
gas_pricestill trigger a panic, so malformed fixtures will take down the runner instead of returning anInvalidTxReason. Please introduce a dedicatedMissingLegacyGasPricevariant (or reuse an equivalent one) and propagate it here.@@ - } else { - if tx.max_fee_per_gas.is_some() { - return Err(InvalidTxReason::GasPriceEip1559); - } - tx.gas_price.expect("expect gas price") - }; + } else { + if tx.max_fee_per_gas.is_some() { + return Err(InvalidTxReason::GasPriceEip1559); + } + tx.gas_price + .ok_or(InvalidTxReason::MissingLegacyGasPrice)? + }; @@ - GasFloorMoreThanGasLimit, + GasFloorMoreThanGasLimit, + MissingLegacyGasPrice,
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
evm-tests/src/precompiles/assets/g1_points.binis excluded by!**/*.binevm-tests/src/precompiles/assets/g2_points.binis excluded by!**/*.bin
📒 Files selected for processing (71)
Cargo.toml(2 hunks)evm-tests/Cargo.toml(1 hunks)evm-tests/EIP-152/Cargo.toml(0 hunks)evm-tests/EIP-152/LICENSE(0 hunks)evm-tests/EIP-152/benches/bench.rs(0 hunks)evm-tests/EIP-152/src/avx2.rs(0 hunks)evm-tests/EIP-152/src/lib.rs(0 hunks)evm-tests/EIP-152/src/portable.rs(0 hunks)evm-tests/ethcore-builtin/Cargo.toml(0 hunks)evm-tests/ethcore-builtin/src/bls.rs(0 hunks)evm-tests/ethcore-builtin/src/kzg.rs(0 hunks)evm-tests/ethcore-builtin/src/lib.rs(0 hunks)evm-tests/ethjson/Cargo.toml(0 hunks)evm-tests/ethjson/src/bytes.rs(0 hunks)evm-tests/ethjson/src/hash.rs(0 hunks)evm-tests/ethjson/src/lib.rs(0 hunks)evm-tests/ethjson/src/maybe.rs(0 hunks)evm-tests/ethjson/src/spec/account.rs(0 hunks)evm-tests/ethjson/src/spec/authority_round.rs(0 hunks)evm-tests/ethjson/src/spec/basic_authority.rs(0 hunks)evm-tests/ethjson/src/spec/builtin.rs(0 hunks)evm-tests/ethjson/src/spec/clique.rs(0 hunks)evm-tests/ethjson/src/spec/engine.rs(0 hunks)evm-tests/ethjson/src/spec/ethash.rs(0 hunks)evm-tests/ethjson/src/spec/genesis.rs(0 hunks)evm-tests/ethjson/src/spec/hardcoded_sync.rs(0 hunks)evm-tests/ethjson/src/spec/instant_seal.rs(0 hunks)evm-tests/ethjson/src/spec/mod.rs(0 hunks)evm-tests/ethjson/src/spec/null_engine.rs(0 hunks)evm-tests/ethjson/src/spec/params.rs(0 hunks)evm-tests/ethjson/src/spec/seal.rs(0 hunks)evm-tests/ethjson/src/spec/spec.rs(0 hunks)evm-tests/ethjson/src/spec/state.rs(0 hunks)evm-tests/ethjson/src/spec/step_duration.rs(0 hunks)evm-tests/ethjson/src/spec/validator_set.rs(0 hunks)evm-tests/ethjson/src/state.rs(0 hunks)evm-tests/ethjson/src/test_helpers/blockchain/block.rs(0 hunks)evm-tests/ethjson/src/test_helpers/blockchain/header.rs(0 hunks)evm-tests/ethjson/src/test_helpers/blockchain/mod.rs(0 hunks)evm-tests/ethjson/src/test_helpers/difficulty.rs(0 hunks)evm-tests/ethjson/src/test_helpers/mod.rs(0 hunks)evm-tests/ethjson/src/test_helpers/skip.rs(0 hunks)evm-tests/ethjson/src/test_helpers/state.rs(0 hunks)evm-tests/ethjson/src/test_helpers/tester.rs(0 hunks)evm-tests/ethjson/src/test_helpers/transaction.rs(0 hunks)evm-tests/ethjson/src/test_helpers/trie/input.rs(0 hunks)evm-tests/ethjson/src/test_helpers/trie/mod.rs(0 hunks)evm-tests/ethjson/src/transaction.rs(0 hunks)evm-tests/ethjson/src/uint.rs(0 hunks)evm-tests/ethjson/src/vm.rs(0 hunks)evm-tests/src/assertions.rs(1 hunks)evm-tests/src/config.rs(1 hunks)evm-tests/src/execution_results.rs(1 hunks)evm-tests/src/main.rs(10 hunks)evm-tests/src/precompiles.rs(1 hunks)evm-tests/src/precompiles/kzg.rs(1 hunks)evm-tests/src/state.rs(1 hunks)evm-tests/src/state_dump.rs(1 hunks)evm-tests/src/types/account_state.rs(1 hunks)evm-tests/src/types/blob.rs(1 hunks)evm-tests/src/types/ecrecover.rs(1 hunks)evm-tests/src/types/eip_4844.rs(1 hunks)evm-tests/src/types/eip_7623.rs(1 hunks)evm-tests/src/types/eip_7702.rs(1 hunks)evm-tests/src/types/info.rs(1 hunks)evm-tests/src/types/json_utils.rs(1 hunks)evm-tests/src/types/mod.rs(1 hunks)evm-tests/src/types/spec.rs(1 hunks)evm-tests/src/types/transaction.rs(1 hunks)evm-tests/src/types/vm.rs(1 hunks)evm-tests/src/vm.rs(1 hunks)
💤 Files with no reviewable changes (48)
- evm-tests/ethjson/src/spec/engine.rs
- evm-tests/ethjson/src/test_helpers/blockchain/block.rs
- evm-tests/ethjson/src/bytes.rs
- evm-tests/ethjson/src/spec/params.rs
- evm-tests/ethjson/src/test_helpers/mod.rs
- evm-tests/ethjson/src/test_helpers/difficulty.rs
- evm-tests/ethjson/src/spec/ethash.rs
- evm-tests/ethjson/src/test_helpers/tester.rs
- evm-tests/ethjson/src/spec/validator_set.rs
- evm-tests/ethjson/src/spec/account.rs
- evm-tests/ethjson/src/lib.rs
- evm-tests/ethjson/src/spec/builtin.rs
- evm-tests/ethjson/src/spec/instant_seal.rs
- evm-tests/ethjson/src/test_helpers/blockchain/header.rs
- evm-tests/ethjson/src/test_helpers/trie/mod.rs
- evm-tests/EIP-152/Cargo.toml
- evm-tests/ethjson/src/spec/step_duration.rs
- evm-tests/ethjson/src/spec/null_engine.rs
- evm-tests/EIP-152/benches/bench.rs
- evm-tests/ethjson/src/spec/seal.rs
- evm-tests/ethjson/src/state.rs
- evm-tests/ethjson/src/spec/spec.rs
- evm-tests/ethjson/src/spec/genesis.rs
- evm-tests/ethjson/src/test_helpers/skip.rs
- evm-tests/ethjson/src/spec/mod.rs
- evm-tests/ethjson/src/spec/state.rs
- evm-tests/ethjson/src/spec/clique.rs
- evm-tests/ethjson/Cargo.toml
- evm-tests/ethjson/src/spec/basic_authority.rs
- evm-tests/ethjson/src/maybe.rs
- evm-tests/ethjson/src/vm.rs
- evm-tests/ethjson/src/test_helpers/transaction.rs
- evm-tests/ethjson/src/transaction.rs
- evm-tests/EIP-152/src/portable.rs
- evm-tests/EIP-152/LICENSE
- evm-tests/ethjson/src/test_helpers/blockchain/mod.rs
- evm-tests/ethcore-builtin/src/lib.rs
- evm-tests/ethjson/src/spec/authority_round.rs
- evm-tests/ethcore-builtin/Cargo.toml
- evm-tests/EIP-152/src/avx2.rs
- evm-tests/ethjson/src/test_helpers/trie/input.rs
- evm-tests/ethjson/src/uint.rs
- evm-tests/ethjson/src/test_helpers/state.rs
- evm-tests/ethcore-builtin/src/bls.rs
- evm-tests/ethcore-builtin/src/kzg.rs
- evm-tests/ethjson/src/spec/hardcoded_sync.rs
- evm-tests/EIP-152/src/lib.rs
- evm-tests/ethjson/src/hash.rs
🧰 Additional context used
🧬 Code graph analysis (16)
evm-tests/src/vm.rs (5)
evm/src/core/mod.rs (2)
stack(73-75)return_value(131-151)evm-tests/src/execution_results.rs (1)
new(26-32)evm/src/runtime/mod.rs (2)
frontier(270-331)machine(74-76)evm/src/executor/stack/executor.rs (1)
new_with_precompiles(432-442)evm/src/executor/stack/memory.rs (1)
logs(47-49)
evm-tests/src/precompiles.rs (2)
evm/src/executor/stack/executor.rs (1)
precompiles(427-429)evm-tests/src/precompiles/kzg.rs (2)
hash(141-147)execute(252-262)
evm-tests/src/types/spec.rs (1)
evm/src/runtime/mod.rs (1)
istanbul(335-396)
evm-tests/src/types/transaction.rs (5)
evm-tests/src/types/json_utils.rs (9)
deserialize_bytes_from_str_opt(202-211)deserialize_h160_from_str(100-104)deserialize_h160_from_str_opt(252-259)deserialize_h256_from_u256_str_opt(134-140)deserialize_u256_from_str(117-121)deserialize_u256_from_str_opt(145-151)deserialize_u8_from_str_opt(165-171)deserialize_vec_of_hex(185-197)deserialize_vec_u256_from_str(227-234)evm-tests/src/types/blob.rs (1)
new(67-73)evm-tests/src/types/eip_7702.rs (2)
new(31-37)new(67-76)evm/src/gasometer/mod.rs (2)
create_transaction_cost(461-474)call_transaction_cost(440-456)evm-tests/src/types/eip_7623.rs (2)
calc_tx_floor_cost(22-24)get_tokens_in_calldata(14-18)
evm-tests/src/precompiles/kzg.rs (1)
evm-tests/src/precompiles.rs (1)
execute(28-35)
evm-tests/src/types/vm.rs (1)
evm-tests/src/types/json_utils.rs (7)
deserialize_bytes_from_str(175-180)deserialize_bytes_from_str_opt(202-211)deserialize_h160_from_str(100-104)deserialize_h160_from_str_opt(252-259)deserialize_h256_from_u256_str_opt(134-140)deserialize_u256_from_str(117-121)deserialize_u256_from_str_opt(145-151)
evm-tests/src/execution_results.rs (1)
evm-tests/src/precompiles.rs (1)
new(43-57)
evm-tests/src/types/eip_7702.rs (1)
evm-tests/src/types/ecrecover.rs (1)
ecrecover(6-26)
evm-tests/src/state.rs (7)
evm-tests/src/assertions.rs (4)
assert_call_exit_exception(357-362)assert_empty_create_caller(346-354)assert_vicinity_validation(9-194)check_create_exit_reason(365-420)evm/src/executor/stack/executor.rs (3)
precompiles(427-429)new_with_precompiles(432-442)used_gas(837-855)evm-tests/src/types/blob.rs (4)
calc_data_fee(173-188)calc_max_data_fee(158-166)new(67-73)from_env(89-97)evm-tests/src/execution_results.rs (1)
new(26-32)evm-tests/src/precompiles.rs (1)
new(43-57)evm-tests/src/types/account_state.rs (3)
caller_balance(179-183)caller_code(185-189)is_delegated(192-196)evm-tests/src/types/transaction.rs (1)
from_tx_bytes(377-388)
evm-tests/src/state_dump.rs (1)
evm-tests/src/main.rs (2)
serde_json(197-197)serde_json(292-292)
evm-tests/src/main.rs (5)
evm-tests/src/types/spec.rs (1)
from_str(90-112)evm-tests/src/execution_results.rs (1)
new(26-32)evm-tests/src/precompiles.rs (1)
new(43-57)evm-tests/src/state.rs (1)
test(31-44)evm-tests/src/vm.rs (1)
test(12-99)
evm-tests/src/types/json_utils.rs (1)
evm-tests/src/types/account_state.rs (3)
h160_from_str(77-77)deserialize(73-81)from(30-48)
evm-tests/src/types/eip_4844.rs (2)
evm-tests/src/types/blob.rs (6)
calc_excess_blob_gas(105-107)calc_blob_gas_price(114-120)fake_exponential(134-151)get_total_blob_gas(195-197)calc_max_data_fee(158-166)calc_data_fee(173-188)evm-tests/src/types/mod.rs (1)
from(188-205)
evm-tests/src/types/mod.rs (4)
evm-tests/src/types/json_utils.rs (7)
deserialize_bytes_from_str(175-180)deserialize_bytes_from_str_opt(202-211)deserialize_h160_from_str(100-104)deserialize_h256_from_u256_str(108-113)deserialize_h256_from_u256_str_opt(134-140)deserialize_u256_from_str(117-121)deserialize_u64_from_str_opt(155-161)evm-tests/src/types/vm.rs (1)
get_memory_vicinity(76-93)evm-tests/src/types/blob.rs (1)
new(67-73)evm-tests/src/types/account_state.rs (1)
from(30-48)
evm-tests/src/types/blob.rs (1)
evm-tests/src/types/mod.rs (1)
from(188-205)
evm-tests/src/types/account_state.rs (1)
evm-tests/src/types/json_utils.rs (7)
btree_h256_h256_from_str(80-96)deserialize_bytes_from_str_opt(202-211)deserialize_u256_from_str(117-121)h160_from_str(53-60)h160_from_str(103-103)h160_from_str(257-257)strip_0x_prefix(8-10)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: ethereum-tests-32-bit
- GitHub Check: unit-tests-32-bit
- GitHub Check: ethereum-tests
…sputnikvm into feat/ethjson-refactoring
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
evm-tests/src/precompiles.rs (1)
44-56: Fix precompile set composition for pre-Istanbul forks.This is the same critical issue identified in the previous review: routing Frontier through Istanbul to
new_istanbul()causes two spec violations:
- Pre-Byzantium forks (Frontier/Homestead/Tangerine/SpuriousDragon) incorrectly register ModExp and alt_bn128 precompiles (0x05–0x08) that didn't exist until Byzantium.
- Byzantium-era forks (Byzantium/Constantinople/Petersburg) incorrectly use Istanbul gas costs for alt_bn128 operations instead of the higher Byzantium costs.
You need separate constructors:
new_frontier()→ registers only 0x01–0x04new_byzantium()→ extends frontier with ModExp and alt_bn128 using Byzantium type parametersnew_istanbul()→ extends byzantium but replaces alt_bn128 with Istanbul type parameters and adds Blake2F🔎 Proposed fix
pub fn new(spec: &Spec) -> Self { match *spec { Spec::Frontier | Spec::Homestead | Spec::Tangerine - | Spec::SpuriousDragon - | Spec::Byzantium + | Spec::SpuriousDragon => Self::new_frontier(), + Spec::Byzantium | Spec::Constantinople - | Spec::Petersburg - | Spec::Istanbul => Self::new_istanbul(), + | Spec::Petersburg => Self::new_byzantium(), + Spec::Istanbul => Self::new_istanbul(), Spec::Berlin | Spec::London | Spec::Merge | Spec::Shanghai => Self::new_berlin(), Spec::Cancun => Self::new_cancun(), Spec::Prague | Spec::Osaka => Self::new_prague(), } } + + pub fn new_frontier() -> Self { + let mut map = BTreeMap::new(); + map.insert( + ECRecover::ADDRESS.raw(), + Box::new(ECRecover) as Box<dyn Precompile>, + ); + map.insert(SHA256::ADDRESS.raw(), Box::new(SHA256)); + map.insert(RIPEMD160::ADDRESS.raw(), Box::new(RIPEMD160)); + map.insert(Identity::ADDRESS.raw(), Box::new(Identity)); + Self(map) + } + + pub fn new_byzantium() -> Self { + let mut map = Self::new_frontier().0; + map.insert( + ModExp::<Byzantium, AuroraModExp>::ADDRESS.raw(), + Box::new(ModExp::<Byzantium, AuroraModExp>::new()), + ); + map.insert( + Bn256Add::<Byzantium>::ADDRESS.raw(), + Box::new(Bn256Add::<Byzantium>::new()), + ); + map.insert( + Bn256Mul::<Byzantium>::ADDRESS.raw(), + Box::new(Bn256Mul::<Byzantium>::new()), + ); + map.insert( + Bn256Pair::<Byzantium>::ADDRESS.raw(), + Box::new(Bn256Pair::<Byzantium>::new()), + ); + Self(map) + } pub fn new_istanbul() -> Self { - let mut map = BTreeMap::new(); - map.insert( - ECRecover::ADDRESS.raw(), - Box::new(ECRecover) as Box<dyn Precompile>, - ); - map.insert(SHA256::ADDRESS.raw(), Box::new(SHA256)); - map.insert(RIPEMD160::ADDRESS.raw(), Box::new(RIPEMD160)); - map.insert(Identity::ADDRESS.raw(), Box::new(Identity)); - map.insert( - ModExp::<Byzantium, AuroraModExp>::ADDRESS.raw(), - Box::new(ModExp::<Byzantium, AuroraModExp>::new()), - ); + let mut map = Self::new_byzantium().0; + // Replace Byzantium alt_bn128 with Istanbul versions map.insert( Bn256Add::<Istanbul>::ADDRESS.raw(), Box::new(Bn256Add::<Istanbul>::new()),
🧹 Nitpick comments (2)
evm-tests/Cargo.toml (1)
12-15: Consider the impact of denying all Clippy lint levels.Denying
all,pedantic, andnurserylints simultaneously is very strict and may generate warnings for idiomatic code patterns or minor style preferences. While this enforces high code quality, it can also slow down development iteration.evm-tests/src/precompiles.rs (1)
265-269: Consider error handling instead of panics in dump function.The dump function uses
expect(line 266) and explicitpanic!(line 268) for error handling. While this is feature-gated test infrastructure code where panics may be acceptable, consider returning aResultor logging errors instead for more graceful failure handling.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
evm-tests/Cargo.toml(1 hunks)evm-tests/src/precompiles.rs(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
evm-tests/src/precompiles.rs (6)
evm/src/executor/stack/executor.rs (1)
precompiles(427-429)evm-tests/src/precompiles/kzg.rs (2)
hash(141-147)execute(252-262)evm/src/core/mod.rs (1)
stack(73-75)evm-tests/src/execution_results.rs (1)
new(26-32)evm-tests/src/types/spec.rs (1)
from_str(90-112)evm-tests/src/main.rs (2)
serde_json(197-197)serde_json(292-292)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: ethereum-tests
- GitHub Check: unit-tests-32-bit
- GitHub Check: ethereum-tests-32-bit
🔇 Additional comments (3)
evm-tests/src/precompiles.rs (2)
25-40: LGTM!The
Precompileswrapper andPrecompileSettrait implementation are clean and correct. UsingBTreeMapprovides deterministic ordering, and the execution flow properly delegates toprocess_precompileandpost_process.
68-71: No action required. ModExp correctly uses Byzantium gas parameters in Istanbul constructor since EIP-2565 gas cost changes occurred in Berlin, not Istanbul. The code and rationale are accurate.evm-tests/Cargo.toml (1)
18-19: Verify the aurora-engine git tag and check for updates.Both dependencies reference a specific git tag
3.10.0-rc.1from the aurora-engine repository. While using tags is better than branches for stability, ensure this tag exists and consider whether a newer stable release is available, as the tag appears to be a release candidate rather than a stable version.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
evm-tests/src/types/eip_7702.rs (1)
84-91: Fix compilation error:to_big_endianrequires a mutable slice parameter.
U256::to_big_endianexpects a mutable 32-byte slice as a parameter and writes into it. The current code calls it without arguments and tries to use the return value, which will not compile.🔎 Proposed fix
fn vrs_to_arr(v: bool, r: U256, s: U256) -> [u8; 65] { let mut result = [0u8; 65]; // (r, s, v), typed (uint256, uint256, uint8) - result[..32].copy_from_slice(&r.to_big_endian()); - result[32..64].copy_from_slice(&s.to_big_endian()); + r.to_big_endian(&mut result[..32]); + s.to_big_endian(&mut result[32..64]); result[64] = u8::from(v); result }evm-tests/src/types/transaction.rs (2)
106-123: Replaceunwrap()calls withexpect()or returnResult.The function has two
unwrap()calls that will panic if the secret key is missing or fails to parse. While the doc comment mentions panics, usingexpect()with descriptive messages or returning aResultwould provide better error context.🔎 Option 1: Use expect with descriptive messages
pub fn get_caller_from_secret_key(&self) -> H160 { - let hash = self.secret_key.unwrap(); + let hash = self.secret_key.expect("Transaction secret key is required"); let mut secret_key = [0; 32]; secret_key.copy_from_slice(hash.as_bytes()); - let secret = libsecp256k1::SecretKey::parse(&secret_key); - let public = libsecp256k1::PublicKey::from_secret_key(&secret.unwrap()); + let secret = libsecp256k1::SecretKey::parse(&secret_key) + .expect("Failed to parse secret key"); + let public = libsecp256k1::PublicKey::from_secret_key(&secret);
195-238: Replaceexpectwith validation error for missing blob gas price.Line 200 uses
expect("expect blob_gas_price")which will panic when blob gas metadata is intentionally omitted in test fixtures. This should return a validation error instead so negative tests can pass.🔎 Proposed fix
if let Some(max) = self.max_fee_per_blob_gas { + let blob_gas_price = blob_gas_price + .ok_or(InvalidTxReason::MissingBlobGasPrice)?; // ensure that the user was willing to at least pay the current blob gasprice - if U256::from( - blob_gas_price - .expect("expect blob_gas_price") - .blob_gas_price, - ) > max + if U256::from(blob_gas_price.blob_gas_price) > max { return Err(InvalidTxReason::BlobGasPriceGreaterThanMax); }Note: You'll need to add
InvalidTxReason::MissingBlobGasPricevariant to the enum.
🧹 Nitpick comments (2)
evm/src/executor/stack/executor.rs (1)
716-717: Consider simplifying the digest-to-array-to-slice conversion pattern.The explicit
<[u8; 32]>::from(Keccak256::digest(...)).as_slice()conversion is functionally correct but unnecessarily verbose. SinceGenericArrayalready has a.as_slice()method, the pattern can be simplified toKeccak256::digest(...).as_slice()directly. The added conversion step (GenericArray → [u8; 32] → &[u8]) appears driven by generic-array version compatibility constraints (noted in Cargo.toml patch), but once those constraints are relaxed or upgraded, this pattern across lines 716, 891, 898, and 1498 could be streamlined without loss of safety or compatibility.evm-tests/src/types/transaction.rs (1)
236-236: Consider usingexpectinstead ofunwrapfor blob count conversion.The
unwrap()ontry_fromcould be replaced withexpectfor better error context, though the conversion should always succeed for the blob limit constants.🔎 Suggested improvement
- if self.blob_versioned_hashes.len() > usize::try_from(max_blob_len).unwrap() { + if self.blob_versioned_hashes.len() > usize::try_from(max_blob_len) + .expect("blob limit constant should fit in usize") { return Err(InvalidTxReason::TooManyBlobs); }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
Cargo.toml(2 hunks)evm-tests/src/types/account_state.rs(1 hunks)evm-tests/src/types/eip_7702.rs(1 hunks)evm-tests/src/types/transaction.rs(1 hunks)evm/src/executor/stack/executor.rs(5 hunks)evm/src/runtime/eval/system.rs(3 hunks)
✅ Files skipped from review due to trivial changes (1)
- evm/src/runtime/eval/system.rs
🧰 Additional context used
🧬 Code graph analysis (3)
evm-tests/src/types/eip_7702.rs (1)
evm-tests/src/types/ecrecover.rs (1)
ecrecover(6-26)
evm-tests/src/types/account_state.rs (2)
evm-tests/src/types/json_utils.rs (7)
btree_h256_h256_from_str(80-96)deserialize_bytes_from_str_opt(202-211)deserialize_u256_from_str(117-121)h160_from_str(53-60)h160_from_str(103-103)h160_from_str(257-257)strip_0x_prefix(8-10)evm/src/backend/memory.rs (1)
account(218-223)
evm/src/executor/stack/executor.rs (3)
evm/src/gasometer/mod.rs (1)
config(134-136)evm/src/backend/memory.rs (1)
state_mut(95-97)evm/src/core/utils.rs (2)
from(74-82)from(86-95)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: ethereum-tests-32-bit
- GitHub Check: ethereum-tests
- GitHub Check: unit-tests-32-bit
🔇 Additional comments (19)
Cargo.toml (3)
5-5: LGTM! Workspace member update aligns with refactoring.The change from
evm-tests/jsonteststoevm-testscorrectly reflects the consolidation of the test framework described in the PR objectives.
11-11: Version bump is appropriate.The minor version increment (2.1.3 → 2.2.0) is suitable for the large-scale refactoring work described in this PR.
25-25: Workspace serde configuration incomplete for projects using derives.The workspace dependency changed from
serde = { version = "1.0", features = ["derive"] }toserde = { version = "1.0", default-features = false }, removing the derive feature. When workspace members use{ workspace = true }, they inherit all settings includingdefault-features, and cannot independently override these values. Projects using#[derive(Serialize, Deserialize)]will fail to compile since the workspace no longer provides the derive feature. Restorefeatures = ["derive"]in the workspace serde definition, or require members to explicitly opt-in:serde = { workspace = true, features = ["derive"] }.evm/src/executor/stack/executor.rs (2)
448-450: LGTM: Consistent const fn accessor.The change to make
state_muta const fn is consistent with thegasometer_mutchange and follows the pattern established in the codebase (as seen inMemoryBackend::state_mut). This improves API flexibility.
250-252: Add MSRV or document Rust 1.83+ requirement for const fn with mutable references.Making
gasometer_muta const fn is a good improvement, but const functions with mutable reference parameters and return types require Rust 1.83+ (stable, released November 28, 2024). The project has norust-versionfield configured in Cargo.toml. If the project maintains compatibility with older Rust versions, add a rust-version field or confirm this breaking change is acceptable.evm-tests/src/types/eip_7702.rs (3)
1-20: LGTM: Constants and imports are correct.The MAGIC constant and SECP256K1N_HALF are properly defined for EIP-7702 and EIP-2 compliance.
22-53: LGTM: Authorization implementation is correct.The struct and methods correctly implement EIP-7702 authorization hash computation with proper RLP encoding and MAGIC byte prepending.
55-82: LGTM: SignedAuthorization structure is sound.The struct properly encapsulates signed authorization data and the
recover_addressmethod correctly delegates toecrecoverfor address recovery.evm-tests/src/types/transaction.rs (7)
1-68: LGTM: Transaction struct is well-designed.The struct comprehensively covers multiple EIPs (2930, 1559, 4844, 7702) with appropriate field types and deserialization helpers.
70-104: LGTM: Accessor methods are correct.The
get_data,get_gas_limit,get_value, andget_access_listmethods properly extract indexed values from the transaction's multi-value test fields.
125-143: LGTM: Intrinsic gas calculation is correct.The method properly computes intrinsic gas costs for both contract creation and calls, including EIP-7702 authorization list handling.
161-191: LGTM: Basic validation checks are sound.The intrinsic gas, block gas limit, and funds checks properly validate transaction constraints with appropriate error returns.
249-311: LGTM: Prague/EIP-7702 validation is comprehensive.The authorization list validation properly checks chain ID, signature values, EIP-2 constraints, and recovers signer addresses with validity flags.
316-357: LGTM: Type definitions are well-structured.The
AccessListandAuthorizationListtypes properly model EIP-2930 and EIP-7702 data structures with appropriate serialization support.
359-391: Consider the panic behavior for unknown transaction types.Line 386-388 panics for unknown transaction types. While this is reasonable for a test framework where new transaction types would require code updates, consider whether this should be a more graceful error in production-like scenarios.
For a test harness, the current panic is acceptable and provides clear feedback when Ethereum introduces new transaction types. However, if this code might be used outside testing contexts, consider returning a
Resultor anUnknownvariant instead.evm-tests/src/types/account_state.rs (4)
1-49: LGTM: StateAccount conversion is correct.The conversion to
MemoryAccountproperly filters out zero storage values per Ethereum's state model, and provides sensible defaults for missing code.
51-82: LGTM: AccountsState deserialization is well-implemented.The custom
Deserializeimplementation correctly handles hex string addresses (with or without0xprefix) and converts them toH160keys, aligning with JSON test fixture formats.
84-140: LGTM: TrieAccount RLP encoding is backward-compatible.The conditional encoding/decoding logic properly handles both 4-field (legacy) and 5-field (with code_version) account representations, ensuring backward compatibility.
142-198: LGTM: MemoryAccountsState methods are robust.The
check_valid_hashmethod correctly computes Merkle roots for validation, and the accessor methods (caller_balance,caller_code,is_delegated) safely handle missing accounts with appropriate defaults.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (8)
evm-tests/src/precompiles.rs (2)
59-115: Consider extracting common precompile setup logic.
new_istanbul()andnew_berlin()are nearly identical, differing only in the ModExp type parameter (Byzantium vs Berlin). Extracting a shared helper function would reduce the ~50 lines of duplication and make future additions easier to maintain.🔎 Proposed refactor to reduce duplication
+ fn new_with_modexp(modexp: Box<dyn Precompile>) -> Self { + let mut map = BTreeMap::new(); + map.insert( + ECRecover::ADDRESS.raw(), + Box::new(ECRecover) as Box<dyn Precompile>, + ); + map.insert(SHA256::ADDRESS.raw(), Box::new(SHA256)); + map.insert(RIPEMD160::ADDRESS.raw(), Box::new(RIPEMD160)); + map.insert(Identity::ADDRESS.raw(), Box::new(Identity)); + map.insert(ModExp::<Byzantium, AuroraModExp>::ADDRESS.raw(), modexp); + map.insert( + Bn256Add::<Istanbul>::ADDRESS.raw(), + Box::new(Bn256Add::<Istanbul>::new()), + ); + map.insert( + Bn256Mul::<Istanbul>::ADDRESS.raw(), + Box::new(Bn256Mul::<Istanbul>::new()), + ); + map.insert( + Bn256Pair::<Istanbul>::ADDRESS.raw(), + Box::new(Bn256Pair::<Istanbul>::new()), + ); + map.insert(Blake2F::ADDRESS.raw(), Box::new(Blake2F)); + Self(map) + } + pub fn new_istanbul() -> Self { - let mut map = BTreeMap::new(); - map.insert( - ECRecover::ADDRESS.raw(), - Box::new(ECRecover) as Box<dyn Precompile>, - ); - map.insert(SHA256::ADDRESS.raw(), Box::new(SHA256)); - map.insert(RIPEMD160::ADDRESS.raw(), Box::new(RIPEMD160)); - map.insert(Identity::ADDRESS.raw(), Box::new(Identity)); - map.insert( - ModExp::<Byzantium, AuroraModExp>::ADDRESS.raw(), - Box::new(ModExp::<Byzantium, AuroraModExp>::new()), - ); - map.insert( - Bn256Add::<Istanbul>::ADDRESS.raw(), - Box::new(Bn256Add::<Istanbul>::new()), - ); - map.insert( - Bn256Mul::<Istanbul>::ADDRESS.raw(), - Box::new(Bn256Mul::<Istanbul>::new()), - ); - map.insert( - Bn256Pair::<Istanbul>::ADDRESS.raw(), - Box::new(Bn256Pair::<Istanbul>::new()), - ); - map.insert(Blake2F::ADDRESS.raw(), Box::new(Blake2F)); - Self(map) + Self::new_with_modexp(Box::new(ModExp::<Byzantium, AuroraModExp>::new())) } pub fn new_berlin() -> Self { - let mut map = BTreeMap::new(); - map.insert( - ECRecover::ADDRESS.raw(), - Box::new(ECRecover) as Box<dyn Precompile>, - ); - map.insert(SHA256::ADDRESS.raw(), Box::new(SHA256)); - map.insert(RIPEMD160::ADDRESS.raw(), Box::new(RIPEMD160)); - map.insert(Identity::ADDRESS.raw(), Box::new(Identity)); - map.insert( - ModExp::<Berlin, AuroraModExp>::ADDRESS.raw(), - Box::new(ModExp::<Berlin, AuroraModExp>::new()), - ); - map.insert( - Bn256Add::<Istanbul>::ADDRESS.raw(), - Box::new(Bn256Add::<Istanbul>::new()), - ); - map.insert( - Bn256Mul::<Istanbul>::ADDRESS.raw(), - Box::new(Bn256Mul::<Istanbul>::new()), - ); - map.insert( - Bn256Pair::<Istanbul>::ADDRESS.raw(), - Box::new(Bn256Pair::<Istanbul>::new()), - ); - map.insert(Blake2F::ADDRESS.raw(), Box::new(Blake2F)); - Self(map) + Self::new_with_modexp(Box::new(ModExp::<Berlin, AuroraModExp>::new())) }
240-275: Consider clarifying dump function logic and improving robustness.A few optional improvements for this debug utility:
Line 240: The guard
evm_context_address != precompile_addressprevents dumping when addresses differ, but the function is documented as a general precompile dump utility. Consider either removing this check or documenting why it's needed.Lines 244-255: Parse errors are silently ignored, falling back to an empty structure. Consider logging parse failures for debugging purposes.
Lines 260-275: The deduplication check only compares inputs but doesn't verify that outputs match for the same input. If the same input produces different outputs across runs, only the first output is preserved, potentially hiding issues.
evm-tests/src/types/eip_7623.rs (2)
14-18: Consider checked arithmetic for defensive coding.The multiplication
non_zero_data_len * NON_ZERO_BYTE_MULTIPLIERcould theoretically overflow on very large inputs before thetry_fromconversion. While realistic Ethereum transaction sizes won't trigger this, usingchecked_mulwould make the overflow explicit.🔎 Optional defensive approach
pub fn get_tokens_in_calldata(input: &[u8]) -> u64 { let zero_data_len = bytecount::count(input, 0); let non_zero_data_len = input.len() - zero_data_len; - u64::try_from(zero_data_len + non_zero_data_len * NON_ZERO_BYTE_MULTIPLIER).unwrap() + let tokens = zero_data_len + .checked_add(non_zero_data_len.checked_mul(NON_ZERO_BYTE_MULTIPLIER).unwrap()) + .unwrap(); + u64::try_from(tokens).unwrap() }
20-24: Verify const fn overflow behavior for edge cases.The
const fnperforms unchecked multiplication and addition. With very largetokens_in_calldatavalues, the computation could overflow. While realistic values won't trigger this, consider whether saturating arithmetic would be more appropriate for edge cases.🔎 Alternative with saturating arithmetic
pub const fn calc_tx_floor_cost(tokens_in_calldata: u64) -> u64 { - tokens_in_calldata * TOTAL_COST_FLOOR_PER_TOKEN + 21_000 + tokens_in_calldata + .saturating_mul(TOTAL_COST_FLOOR_PER_TOKEN) + .saturating_add(21_000) }evm-tests/src/assertions.rs (4)
7-194: Consider extracting repetitive validation logic.The function contains significant repetition across fork match arms, with similar patterns for checking
PriorityFeeTooLargeandGasPriceLessThenBlockBaseFeeacross multiple specs. While the current structure is clear for debugging, extracting a helper function to validate expected error strings could reduce duplication and improve maintainability.For example, a helper like
validate_error_messages(states, allowed_messages, reason, spec, name, file_name)could consolidate the iteration and assertion logic.
313-319: Consider breaking long line for readability.Line 314 exceeds 150 characters and combines multiple conditions. Consider formatting it across multiple lines to improve readability.
🔎 Suggested formatting
- let check_result = exception == "TransactionException.TYPE_4_EMPTY_AUTHORIZATION_LIST" || exception == "TransactionException.TYPE_4_TX_CONTRACT_CREATION"; + let check_result = exception == "TransactionException.TYPE_4_EMPTY_AUTHORIZATION_LIST" + || exception == "TransactionException.TYPE_4_TX_CONTRACT_CREATION";
383-390: Improve assertion formatting consistency.The assertion formatting on lines 386-388 is inconsistent with similar assertions elsewhere in the file. Consider aligning the formatting style.
🔎 Suggested formatting
ExitError::MaxNonce => { let check_result = exception == "TR_NonceHasMaxValue" || exception == "TransactionException.NONCE_IS_MAX"; - assert!(check_result, - "unexpected exception {exception:?} for MaxNonce error for test: {name}" + assert!( + check_result, + "unexpected exception {exception:?} for MaxNonce error for test: {name}" ); return true; }
391-398: Maintain formatting consistency.Similarly, the assertion on lines 394-396 could be formatted consistently with the rest of the file.
🔎 Suggested formatting
ExitError::OutOfGas => { let check_result = exception == "TransactionException.INTRINSIC_GAS_TOO_LOW"; - assert!(check_result, - "unexpected exception {exception:?} for OutOfGas error for test: {name}" + assert!( + check_result, + "unexpected exception {exception:?} for OutOfGas error for test: {name}" ); return true; }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
evm-tests/src/assertions.rs(1 hunks)evm-tests/src/precompiles.rs(1 hunks)evm-tests/src/types/eip_7623.rs(1 hunks)
🧰 Additional context used
🧠 Learnings (7)
📓 Common learnings
Learnt from: mrLSD
Repo: aurora-is-near/aurora-evm PR: 95
File: evm-tests/src/state_dump.rs:84-92
Timestamp: 2025-12-19T01:14:52.101Z
Learning: In evm-tests/src/state_dump.rs (aurora-evm crate), the dump_to_file method correctly generates filenames using format!("state_test_{spec:?}_{now}.json"). The Spec enum Debug format produces clean variant names (e.g., "Shanghai", "Cancun") that are filesystem-safe and do not contain problematic characters like colons. This is Aurora EVM, not REVM.
Learnt from: mrLSD
Repo: aurora-is-near/aurora-evm PR: 95
File: evm-tests/src/types/transaction.rs:195-238
Timestamp: 2025-12-19T00:45:08.192Z
Learning: In the aurora-evm-jsontests crate (evm-tests), explicit panics with `expect()` or `unwrap()` are preferred over error handling for test debugging purposes, as they provide exact failure locations and stack traces that are more useful during test development and debugging.
Learnt from: mrLSD
Repo: aurora-is-near/aurora-evm PR: 95
File: evm-tests/src/precompiles.rs:45-86
Timestamp: 2025-12-19T01:09:08.271Z
Learning: In the aurora-evm codebase (evm-tests), there are no test cases for pre-Istanbul Ethereum forks (Frontier, Homestead, Tangerine, SpuriousDragon, Byzantium, Constantinople, Petersburg). Therefore, it is acceptable for the precompile set composition in evm-tests/src/precompiles.rs to route all these forks to new_istanbul() even though this technically applies Istanbul-era gas costs to earlier forks, as there are no tests that would be affected by this implementation choice.
📚 Learning: 2025-12-19T00:45:08.192Z
Learnt from: mrLSD
Repo: aurora-is-near/aurora-evm PR: 95
File: evm-tests/src/types/transaction.rs:195-238
Timestamp: 2025-12-19T00:45:08.192Z
Learning: In test code within the evm-tests crate (evm-tests/src/...), prefer using unwrap() or expect() to trigger immediate panics with precise location when a test condition fails. This aids debugging by showing the exact failure point and stack trace. Use this approach only in test code; production/error-handling paths should still use proper error handling rather than unwrapping.
Applied to files:
evm-tests/src/assertions.rsevm-tests/src/types/eip_7623.rsevm-tests/src/precompiles.rs
📚 Learning: 2025-12-19T00:59:13.529Z
Learnt from: mrLSD
Repo: aurora-is-near/aurora-evm PR: 95
File: evm-tests/src/types/ecrecover.rs:6-26
Timestamp: 2025-12-19T00:59:13.529Z
Learning: In evm-tests/src/types/ecrecover.rs (aurora-evm-jsontests crate), panicking on invalid input (such as insufficient signature length) is preferable to adding input validation checks and returning errors, as explicit panics provide clearer failure locations and stack traces that are more useful for test debugging purposes.
Applied to files:
evm-tests/src/assertions.rsevm-tests/src/types/eip_7623.rsevm-tests/src/precompiles.rs
📚 Learning: 2025-12-19T01:14:52.101Z
Learnt from: mrLSD
Repo: aurora-is-near/aurora-evm PR: 95
File: evm-tests/src/state_dump.rs:84-92
Timestamp: 2025-12-19T01:14:52.101Z
Learning: In evm-tests/src/state_dump.rs (aurora-evm crate), the dump_to_file method correctly generates filenames using format!("state_test_{spec:?}_{now}.json"). The Spec enum Debug format produces clean variant names (e.g., "Shanghai", "Cancun") that are filesystem-safe and do not contain problematic characters like colons. This is Aurora EVM, not REVM.
Applied to files:
evm-tests/src/assertions.rsevm-tests/src/types/eip_7623.rsevm-tests/src/precompiles.rs
📚 Learning: 2025-12-19T00:54:36.099Z
Learnt from: mrLSD
Repo: aurora-is-near/aurora-evm PR: 95
File: evm-tests/src/types/transaction.rs:111-123
Timestamp: 2025-12-19T00:54:36.099Z
Learning: In evm-tests/src/types/transaction.rs, the `get_caller_from_secret_key` method appropriately uses `unwrap()` for test purposes, consistent with the crate's preference for explicit panics over error handling.
Applied to files:
evm-tests/src/assertions.rsevm-tests/src/types/eip_7623.rsevm-tests/src/precompiles.rs
📚 Learning: 2025-12-19T01:09:08.271Z
Learnt from: mrLSD
Repo: aurora-is-near/aurora-evm PR: 95
File: evm-tests/src/precompiles.rs:45-86
Timestamp: 2025-12-19T01:09:08.271Z
Learning: In the aurora-evm codebase (evm-tests), there are no test cases for pre-Istanbul Ethereum forks (Frontier, Homestead, Tangerine, SpuriousDragon, Byzantium, Constantinople, Petersburg). Therefore, it is acceptable for the precompile set composition in evm-tests/src/precompiles.rs to route all these forks to new_istanbul() even though this technically applies Istanbul-era gas costs to earlier forks, as there are no tests that would be affected by this implementation choice.
Applied to files:
evm-tests/src/assertions.rsevm-tests/src/types/eip_7623.rsevm-tests/src/precompiles.rs
📚 Learning: 2025-12-19T00:47:22.497Z
Learnt from: mrLSD
Repo: aurora-is-near/aurora-evm PR: 95
File: evm-tests/src/types/eip_7702.rs:84-91
Timestamp: 2025-12-19T00:47:22.497Z
Learning: In the aurora-evm codebase (evm-tests), the version of `primitive-types` being used has `U256::to_big_endian()` return a `[u8; 32]` array value, not require a mutable slice parameter. The pattern `result[..32].copy_from_slice(&r.to_big_endian())` is correct and compiles successfully.
Applied to files:
evm-tests/src/assertions.rsevm-tests/src/types/eip_7623.rsevm-tests/src/precompiles.rs
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: ethereum-tests-32-bit
- GitHub Check: ethereum-tests
- GitHub Check: unit-tests-32-bit
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
evm-tests/src/assertions.rs (2)
36-124: Consider extracting common fork validation logic.The London, Merge, and Shanghai fork handlers (lines 36-124) share nearly identical structure for
PriorityFeeTooLargeandGasPriceLessThenBlockBaseFee, with only the expected exception string values differing. While the current explicit approach aids debugging, consider extracting a helper function that accepts the fork-specific exception strings as parameters to reduce duplication.Example refactor approach
fn validate_priority_fee_too_large( states: &[PostState], expected_strs: &[&str], spec: &Spec, name: &str, file_name: &str, ) { for (i, state) in states.iter().enumerate() { let expected = state.expect_exception.as_deref().unwrap_or_else(|| { panic!("expected error message for test: {reason:?} [{spec:?}] {name}:{i}\n{file_name:?}") }); let is_checked = expected_strs.contains(&expected); assert!( is_checked, "unexpected error message {expected:?} for: PriorityFeeTooLarge [{spec:?}] {name}:{i}\n{file_name:?}", ); } }Then call with fork-specific strings:
Spec::London => match reason { InvalidTxReason::PriorityFeeTooLarge => { validate_priority_fee_too_large(states, &["tipTooHigh", "TR_TipGtFeeCap"], spec, name, file_name); } // ... }
203-342: Return value in success path is alwaystrue.The function returns
boolbut always returnstrueon line 339 whenexpect_exceptionisSome(_), as all code paths either returntrueor panic. Consider whether the return type could be simplified to()or if theboolserves a future API design purpose.evm-tests/src/precompiles.rs (1)
59-115: Consider extracting common precompiles into a helper.The constructors are correctly implemented with appropriate precompiles for each spec. However,
new_istanbul()andnew_berlin()duplicate ~54 lines of code, differing only in theModExpgas schedule (Byzantium vs Berlin). While the current code is clear and functional, extracting the common precompiles (ECRecover, SHA256, RIPEMD160, Identity, Bn256 ops, Blake2F) into a helper function would improve maintainability.🔎 Optional refactor to reduce duplication
fn new_base() -> BTreeMap<H160, Box<dyn Precompile>> { let mut map = BTreeMap::new(); map.insert( ECRecover::ADDRESS.raw(), Box::new(ECRecover) as Box<dyn Precompile>, ); map.insert(SHA256::ADDRESS.raw(), Box::new(SHA256)); map.insert(RIPEMD160::ADDRESS.raw(), Box::new(RIPEMD160)); map.insert(Identity::ADDRESS.raw(), Box::new(Identity)); map.insert( Bn256Add::<Istanbul>::ADDRESS.raw(), Box::new(Bn256Add::<Istanbul>::new()), ); map.insert( Bn256Mul::<Istanbul>::ADDRESS.raw(), Box::new(Bn256Mul::<Istanbul>::new()), ); map.insert( Bn256Pair::<Istanbul>::ADDRESS.raw(), Box::new(Bn256Pair::<Istanbul>::new()), ); map.insert(Blake2F::ADDRESS.raw(), Box::new(Blake2F)); map } pub fn new_istanbul() -> Self { let mut map = Self::new_base(); map.insert( ModExp::<Byzantium, AuroraModExp>::ADDRESS.raw(), Box::new(ModExp::<Byzantium, AuroraModExp>::new()), ); Self(map) } pub fn new_berlin() -> Self { let mut map = Self::new_base(); map.insert( ModExp::<Berlin, AuroraModExp>::ADDRESS.raw(), Box::new(ModExp::<Berlin, AuroraModExp>::new()), ); Self(map) }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
evm-tests/src/assertions.rs(1 hunks)evm-tests/src/precompiles.rs(1 hunks)evm-tests/src/types/eip_7623.rs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- evm-tests/src/types/eip_7623.rs
🧰 Additional context used
🧠 Learnings (7)
📓 Common learnings
Learnt from: mrLSD
Repo: aurora-is-near/aurora-evm PR: 95
File: evm-tests/src/types/transaction.rs:195-238
Timestamp: 2025-12-19T00:45:08.192Z
Learning: In the aurora-evm-jsontests crate (evm-tests), explicit panics with `expect()` or `unwrap()` are preferred over error handling for test debugging purposes, as they provide exact failure locations and stack traces that are more useful during test development and debugging.
Learnt from: mrLSD
Repo: aurora-is-near/aurora-evm PR: 95
File: evm-tests/src/state_dump.rs:84-92
Timestamp: 2025-12-19T01:14:52.101Z
Learning: In evm-tests/src/state_dump.rs (aurora-evm crate), the dump_to_file method correctly generates filenames using format!("state_test_{spec:?}_{now}.json"). The Spec enum Debug format produces clean variant names (e.g., "Shanghai", "Cancun") that are filesystem-safe and do not contain problematic characters like colons. This is Aurora EVM, not REVM.
Learnt from: mrLSD
Repo: aurora-is-near/aurora-evm PR: 95
File: evm-tests/src/precompiles.rs:45-86
Timestamp: 2025-12-19T01:09:08.271Z
Learning: In the aurora-evm codebase (evm-tests), there are no test cases for pre-Istanbul Ethereum forks (Frontier, Homestead, Tangerine, SpuriousDragon, Byzantium, Constantinople, Petersburg). Therefore, it is acceptable for the precompile set composition in evm-tests/src/precompiles.rs to route all these forks to new_istanbul() even though this technically applies Istanbul-era gas costs to earlier forks, as there are no tests that would be affected by this implementation choice.
📚 Learning: 2025-12-19T00:45:08.192Z
Learnt from: mrLSD
Repo: aurora-is-near/aurora-evm PR: 95
File: evm-tests/src/types/transaction.rs:195-238
Timestamp: 2025-12-19T00:45:08.192Z
Learning: In test code within the evm-tests crate (evm-tests/src/...), prefer using unwrap() or expect() to trigger immediate panics with precise location when a test condition fails. This aids debugging by showing the exact failure point and stack trace. Use this approach only in test code; production/error-handling paths should still use proper error handling rather than unwrapping.
Applied to files:
evm-tests/src/assertions.rsevm-tests/src/precompiles.rs
📚 Learning: 2025-12-19T00:59:13.529Z
Learnt from: mrLSD
Repo: aurora-is-near/aurora-evm PR: 95
File: evm-tests/src/types/ecrecover.rs:6-26
Timestamp: 2025-12-19T00:59:13.529Z
Learning: In evm-tests/src/types/ecrecover.rs (aurora-evm-jsontests crate), panicking on invalid input (such as insufficient signature length) is preferable to adding input validation checks and returning errors, as explicit panics provide clearer failure locations and stack traces that are more useful for test debugging purposes.
Applied to files:
evm-tests/src/assertions.rsevm-tests/src/precompiles.rs
📚 Learning: 2025-12-19T01:09:08.271Z
Learnt from: mrLSD
Repo: aurora-is-near/aurora-evm PR: 95
File: evm-tests/src/precompiles.rs:45-86
Timestamp: 2025-12-19T01:09:08.271Z
Learning: In the aurora-evm codebase (evm-tests), there are no test cases for pre-Istanbul Ethereum forks (Frontier, Homestead, Tangerine, SpuriousDragon, Byzantium, Constantinople, Petersburg). Therefore, it is acceptable for the precompile set composition in evm-tests/src/precompiles.rs to route all these forks to new_istanbul() even though this technically applies Istanbul-era gas costs to earlier forks, as there are no tests that would be affected by this implementation choice.
Applied to files:
evm-tests/src/assertions.rsevm-tests/src/precompiles.rs
📚 Learning: 2025-12-19T01:14:52.101Z
Learnt from: mrLSD
Repo: aurora-is-near/aurora-evm PR: 95
File: evm-tests/src/state_dump.rs:84-92
Timestamp: 2025-12-19T01:14:52.101Z
Learning: In evm-tests/src/state_dump.rs (aurora-evm crate), the dump_to_file method correctly generates filenames using format!("state_test_{spec:?}_{now}.json"). The Spec enum Debug format produces clean variant names (e.g., "Shanghai", "Cancun") that are filesystem-safe and do not contain problematic characters like colons. This is Aurora EVM, not REVM.
Applied to files:
evm-tests/src/assertions.rsevm-tests/src/precompiles.rs
📚 Learning: 2025-12-19T00:54:36.099Z
Learnt from: mrLSD
Repo: aurora-is-near/aurora-evm PR: 95
File: evm-tests/src/types/transaction.rs:111-123
Timestamp: 2025-12-19T00:54:36.099Z
Learning: In evm-tests/src/types/transaction.rs, the `get_caller_from_secret_key` method appropriately uses `unwrap()` for test purposes, consistent with the crate's preference for explicit panics over error handling.
Applied to files:
evm-tests/src/assertions.rsevm-tests/src/precompiles.rs
📚 Learning: 2025-12-19T00:47:22.497Z
Learnt from: mrLSD
Repo: aurora-is-near/aurora-evm PR: 95
File: evm-tests/src/types/eip_7702.rs:84-91
Timestamp: 2025-12-19T00:47:22.497Z
Learning: In the aurora-evm codebase (evm-tests), the version of `primitive-types` being used has `U256::to_big_endian()` return a `[u8; 32]` array value, not require a mutable slice parameter. The pattern `result[..32].copy_from_slice(&r.to_big_endian())` is correct and compiles successfully.
Applied to files:
evm-tests/src/assertions.rsevm-tests/src/precompiles.rs
🧬 Code graph analysis (1)
evm-tests/src/precompiles.rs (3)
evm/src/executor/stack/executor.rs (1)
precompiles(427-429)evm/src/core/mod.rs (1)
stack(73-75)evm-tests/src/types/spec.rs (1)
from_str(90-112)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: ethereum-tests
- GitHub Check: unit-tests-32-bit
- GitHub Check: ethereum-tests-32-bit
🔇 Additional comments (12)
evm-tests/src/assertions.rs (5)
1-7: LGTM!Imports are clean and well-organized. The
#[allow]directives on line 7 are appropriate for the large test assertion function that follows.
8-193: Comprehensive fork coverage with helpful diagnostics.The function provides thorough validation across Istanbul through Prague forks with detailed panic messages that include spec, test name, file name, and state index—excellent for debugging test failures.
195-342: Comprehensive exit reason validation.The function thoroughly validates all
InvalidTxReasonvariants against expected exception strings with clear assertion messages. The extensive coverage of blob-related and authorization-related reasons is particularly valuable for Cancun and Prague fork testing.
344-361: LGTM!Both
assert_empty_create_callerandassert_call_exit_exceptionare focused, single-purpose validation helpers with clear semantics. The use ofexpect()and explicit assertions aligns with the crate's test debugging preferences.
363-419: LGTM!The function correctly maps
ExitReasonerror variants to expected exception strings with appropriate return values:truewhen an error matches expectations,falsewhen errors occur without expected exceptions or for successful exits. The control flow, while complex, is clear and handles all relevant cases for create operation validation.evm-tests/src/precompiles.rs (7)
1-40: LGTM! Clean precompile infrastructure.The module structure, imports, and
PrecompileSetimplementation are well-designed. ThePrecompilesstruct appropriately wraps aBTreeMapfor sorted address lookup, and the trait implementation correctly bridges between the EVM handle interface and the aurora_engine_precompiles execution model.
42-57: Spec routing is correct.The fork routing correctly maps specs to their respective precompile sets. Based on learnings, routing pre-Istanbul forks (Frontier through Petersburg) to
new_istanbul()is acceptable for this codebase since no tests exist for those earlier forks.
117-133: Excellent incremental design pattern.The
new_cancun()andnew_prague()constructors correctly build on their predecessors, making the fork progression explicit and avoiding duplication. This is a good pattern that makes it easy to add future forks.
153-186: Processing logic is correct; dump example properly commented.The
process_precompile()function correctly converts the EVM handle context to aurora_engine_precompiles format and executes the precompile. The previously flagged hardcoded dump logic has been appropriately converted to a commented example (lines 174-182), addressing the earlier concern.
188-224: LGTM! Complete error mapping and cost handling.Both
post_process()andget_exit_error()are correctly implemented. The gas cost recording is appropriate, and the error mapping comprehensively handles allExitErrorvariants between the aurora_engine_precompiles and aurora_evm crates.
226-276: Dump helper is well-implemented.The
dump_precompile_state()function correctly handles file I/O for debugging test cases. The guard clause prevents empty inputs and address mismatches, the deduplication logic avoids duplicate entries, and the use ofexpect()andpanic!()is consistent with test code preferences per learnings. Note: The "debbugging" typo mentioned in a past comment has been fixed to "debugging" on line 228.
5-16: These are git dependencies, not registry versions.The dependencies use git with tag
3.10.0-rc.1from the aurora-engine repository, not crates.io registry versions. The RC tag may still warrant verification for stability, but the premise about "registry existence" is not applicable.Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
evm-tests/src/state.rs (1)
60-64: Consider adding documentation to clarify thecalc_data_feecontract.The call on lines 60-64 passes
blob_gas_price.as_ref()which can beNone. The function uses.expect("expect blob_gas_price")and will panic ifblob_gas_priceisNonewhileconfig.has_shard_blob_transactionsistrue. This appears to be intentional design: when blob transactions are enabled, valid test data must provide blob gas price information.To improve clarity, consider:
- Adding a comment at the call site explaining this contract, or
- Improving the
.expect()message to "blob gas price required when has_shard_blob_transactions is true"This matches the codebase's established pattern of using explicit panics in test code to enforce data contracts.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
evm-tests/src/assertions.rsevm-tests/src/main.rsevm-tests/src/precompiles.rsevm-tests/src/precompiles/kzg.rsevm-tests/src/state.rsevm-tests/src/types/account_state.rsevm-tests/src/types/blob.rsevm-tests/src/types/json_utils.rsevm-tests/src/types/mod.rsevm-tests/src/types/transaction.rsevm-tests/src/types/vm.rs
🚧 Files skipped from review as they are similar to previous changes (3)
- evm-tests/src/precompiles/kzg.rs
- evm-tests/src/types/account_state.rs
- evm-tests/src/types/transaction.rs
🧰 Additional context used
🧠 Learnings (7)
📓 Common learnings
Learnt from: mrLSD
Repo: aurora-is-near/aurora-evm PR: 95
File: evm-tests/src/state_dump.rs:84-92
Timestamp: 2025-12-19T01:14:58.120Z
Learning: In evm-tests/src/state_dump.rs (aurora-evm crate), the dump_to_file method correctly generates filenames using format!("state_test_{spec:?}_{now}.json"). The Spec enum Debug format produces clean variant names (e.g., "Shanghai", "Cancun") that are filesystem-safe and do not contain problematic characters like colons. This is Aurora EVM, not REVM.
Learnt from: mrLSD
Repo: aurora-is-near/aurora-evm PR: 95
File: evm-tests/src/types/transaction.rs:195-238
Timestamp: 2025-12-19T00:45:15.604Z
Learning: In the aurora-evm-jsontests crate (evm-tests), explicit panics with `expect()` or `unwrap()` are preferred over error handling for test debugging purposes, as they provide exact failure locations and stack traces that are more useful during test development and debugging.
Learnt from: mrLSD
Repo: aurora-is-near/aurora-evm PR: 95
File: evm-tests/src/precompiles.rs:45-86
Timestamp: 2025-12-19T01:09:14.674Z
Learning: In the aurora-evm codebase (evm-tests), there are no test cases for pre-Istanbul Ethereum forks (Frontier, Homestead, Tangerine, SpuriousDragon, Byzantium, Constantinople, Petersburg). Therefore, it is acceptable for the precompile set composition in evm-tests/src/precompiles.rs to route all these forks to new_istanbul() even though this technically applies Istanbul-era gas costs to earlier forks, as there are no tests that would be affected by this implementation choice.
📚 Learning: 2025-12-19T01:14:58.120Z
Learnt from: mrLSD
Repo: aurora-is-near/aurora-evm PR: 95
File: evm-tests/src/state_dump.rs:84-92
Timestamp: 2025-12-19T01:14:58.120Z
Learning: In evm-tests/src/state_dump.rs (aurora-evm crate), the dump_to_file method correctly generates filenames using format!("state_test_{spec:?}_{now}.json"). The Spec enum Debug format produces clean variant names (e.g., "Shanghai", "Cancun") that are filesystem-safe and do not contain problematic characters like colons. This is Aurora EVM, not REVM.
Applied to files:
evm-tests/src/state.rsevm-tests/src/main.rsevm-tests/src/assertions.rsevm-tests/src/precompiles.rsevm-tests/src/types/vm.rsevm-tests/src/types/mod.rsevm-tests/src/types/json_utils.rs
📚 Learning: 2025-12-19T01:09:14.674Z
Learnt from: mrLSD
Repo: aurora-is-near/aurora-evm PR: 95
File: evm-tests/src/precompiles.rs:45-86
Timestamp: 2025-12-19T01:09:14.674Z
Learning: In the aurora-evm codebase (evm-tests), there are no test cases for pre-Istanbul Ethereum forks (Frontier, Homestead, Tangerine, SpuriousDragon, Byzantium, Constantinople, Petersburg). Therefore, it is acceptable for the precompile set composition in evm-tests/src/precompiles.rs to route all these forks to new_istanbul() even though this technically applies Istanbul-era gas costs to earlier forks, as there are no tests that would be affected by this implementation choice.
Applied to files:
evm-tests/src/state.rsevm-tests/src/main.rsevm-tests/src/assertions.rsevm-tests/src/precompiles.rsevm-tests/src/types/vm.rsevm-tests/src/types/mod.rsevm-tests/src/types/blob.rs
📚 Learning: 2025-12-19T00:45:08.192Z
Learnt from: mrLSD
Repo: aurora-is-near/aurora-evm PR: 95
File: evm-tests/src/types/transaction.rs:195-238
Timestamp: 2025-12-19T00:45:08.192Z
Learning: In test code within the evm-tests crate (evm-tests/src/...), prefer using unwrap() or expect() to trigger immediate panics with precise location when a test condition fails. This aids debugging by showing the exact failure point and stack trace. Use this approach only in test code; production/error-handling paths should still use proper error handling rather than unwrapping.
Applied to files:
evm-tests/src/state.rsevm-tests/src/main.rsevm-tests/src/assertions.rsevm-tests/src/precompiles.rsevm-tests/src/types/vm.rsevm-tests/src/types/mod.rsevm-tests/src/types/blob.rsevm-tests/src/types/json_utils.rs
📚 Learning: 2025-12-19T00:54:43.287Z
Learnt from: mrLSD
Repo: aurora-is-near/aurora-evm PR: 95
File: evm-tests/src/types/transaction.rs:111-123
Timestamp: 2025-12-19T00:54:43.287Z
Learning: In evm-tests/src/types/transaction.rs, the `get_caller_from_secret_key` method appropriately uses `unwrap()` for test purposes, consistent with the crate's preference for explicit panics over error handling.
Applied to files:
evm-tests/src/state.rsevm-tests/src/assertions.rsevm-tests/src/precompiles.rsevm-tests/src/types/vm.rsevm-tests/src/types/mod.rsevm-tests/src/types/blob.rs
📚 Learning: 2025-12-19T00:59:18.510Z
Learnt from: mrLSD
Repo: aurora-is-near/aurora-evm PR: 95
File: evm-tests/src/types/ecrecover.rs:6-26
Timestamp: 2025-12-19T00:59:18.510Z
Learning: In evm-tests/src/types/ecrecover.rs (aurora-evm-jsontests crate), panicking on invalid input (such as insufficient signature length) is preferable to adding input validation checks and returning errors, as explicit panics provide clearer failure locations and stack traces that are more useful for test debugging purposes.
Applied to files:
evm-tests/src/main.rsevm-tests/src/assertions.rsevm-tests/src/precompiles.rsevm-tests/src/types/vm.rsevm-tests/src/types/mod.rsevm-tests/src/types/json_utils.rs
📚 Learning: 2025-12-19T00:47:31.731Z
Learnt from: mrLSD
Repo: aurora-is-near/aurora-evm PR: 95
File: evm-tests/src/types/eip_7702.rs:84-91
Timestamp: 2025-12-19T00:47:31.731Z
Learning: In the aurora-evm codebase (evm-tests), the version of `primitive-types` being used has `U256::to_big_endian()` return a `[u8; 32]` array value, not require a mutable slice parameter. The pattern `result[..32].copy_from_slice(&r.to_big_endian())` is correct and compiles successfully.
Applied to files:
evm-tests/src/assertions.rsevm-tests/src/precompiles.rsevm-tests/src/types/vm.rsevm-tests/src/types/mod.rsevm-tests/src/types/blob.rsevm-tests/src/types/json_utils.rs
🧬 Code graph analysis (5)
evm-tests/src/state.rs (3)
evm-tests/src/assertions.rs (4)
assert_empty_create_caller(345-353)assert_vicinity_validation(8-193)check_create_exit_reason(364-419)check_validate_exit_reason(197-342)evm-tests/src/types/account_state.rs (3)
caller_balance(180-184)caller_code(186-190)is_delegated(193-197)evm-tests/src/types/transaction.rs (1)
from_tx_bytes(379-390)
evm-tests/src/main.rs (4)
evm-tests/src/types/spec.rs (1)
from_str(90-112)evm-tests/src/execution_results.rs (1)
new(26-32)evm-tests/src/state.rs (1)
test(24-37)evm-tests/src/vm.rs (1)
test(12-99)
evm-tests/src/precompiles.rs (2)
evm/src/executor/stack/executor.rs (1)
precompiles(427-429)evm/src/core/mod.rs (1)
stack(73-75)
evm-tests/src/types/mod.rs (1)
evm-tests/src/types/json_utils.rs (7)
deserialize_bytes_from_str(181-186)deserialize_bytes_from_str_opt(208-217)deserialize_h160_from_str(106-110)deserialize_h256_from_u256_str(114-119)deserialize_h256_from_u256_str_opt(140-146)deserialize_u256_from_str(123-127)deserialize_u64_from_str_opt(161-167)
evm-tests/src/types/json_utils.rs (1)
evm-tests/src/types/account_state.rs (3)
h160_from_hex_str(77-77)deserialize(73-81)from(30-48)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: unit-tests-32-bit
- GitHub Check: ethereum-tests
- GitHub Check: ethereum-tests-32-bit
🔇 Additional comments (3)
evm-tests/src/precompiles.rs (1)
1-276: LGTM! Well-structured precompile registry.The precompile registry implementation is clean and comprehensive. All previously identified issues have been addressed:
- Fork routing is appropriate given the test coverage
- Debug dump code is properly moved to a commented example
- Error mapping is exhaustive and correct
The per-fork constructors are well-organized, with later forks building incrementally on earlier ones (e.g.,
new_cancun()extendsnew_berlin(),new_prague()extendsnew_cancun()), which promotes maintainability.evm-tests/src/types/vm.rs (1)
1-156: LGTM! Clean VM test case structures.The VM test data structures are well-defined with appropriate serde deserialization. The use of
unwrap()in methods likeget_output(),get_gas_left(), andvalidate_state()is consistent with the crate's preference for explicit panics in test code, which aids debugging by providing exact failure locations. Based on learnings from this PR, this approach is intentional and appropriate for test harness code.evm-tests/src/types/mod.rs (1)
1-277: LGTM! Comprehensive state test type system.The state test structures are well-designed with proper validation logic. The
get_memory_vicinity()method correctly handles EIP-1559 validation, including:
- Gas price handling for London+ forks vs. pre-London
- Priority fee validation
- Base fee comparison
The use of
expect("expect gas price")at line 80 and the call toget_caller_from_secret_key()at line 107 are consistent with the crate's preference for explicit panics in test code. Based on learnings from this PR, this approach provides better debugging information during test development.The
InvalidTxReasonenum comprehensively covers various transaction validation failure scenarios across different EIPs.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI Agents
In @evm-tests/src/types/mod.rs:
- Around line 255-276: Rename the typo'd enum variant in InvalidTxReason from
GasPriceLessThenBlockBaseFee to GasPriceLessThanBlockBaseFee and update all
references/usages (e.g., match arms, construction sites, tests) to use the new
name; ensure any pattern matches, imports, or string/serialization mappings
referring to GasPriceLessThenBlockBaseFee are updated accordingly so the code
compiles and behavior remains unchanged.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
evm-tests/src/types/eip_7702.rsevm-tests/src/types/mod.rs
🧰 Additional context used
🧠 Learnings (7)
📓 Common learnings
Learnt from: mrLSD
Repo: aurora-is-near/aurora-evm PR: 95
File: evm-tests/src/types/transaction.rs:195-238
Timestamp: 2025-12-19T00:45:15.604Z
Learning: In the aurora-evm-jsontests crate (evm-tests), explicit panics with `expect()` or `unwrap()` are preferred over error handling for test debugging purposes, as they provide exact failure locations and stack traces that are more useful during test development and debugging.
Learnt from: mrLSD
Repo: aurora-is-near/aurora-evm PR: 95
File: evm-tests/src/state_dump.rs:84-92
Timestamp: 2025-12-19T01:14:58.120Z
Learning: In evm-tests/src/state_dump.rs (aurora-evm crate), the dump_to_file method correctly generates filenames using format!("state_test_{spec:?}_{now}.json"). The Spec enum Debug format produces clean variant names (e.g., "Shanghai", "Cancun") that are filesystem-safe and do not contain problematic characters like colons. This is Aurora EVM, not REVM.
Learnt from: mrLSD
Repo: aurora-is-near/aurora-evm PR: 95
File: evm-tests/src/precompiles.rs:45-86
Timestamp: 2025-12-19T01:09:14.674Z
Learning: In the aurora-evm codebase (evm-tests), there are no test cases for pre-Istanbul Ethereum forks (Frontier, Homestead, Tangerine, SpuriousDragon, Byzantium, Constantinople, Petersburg). Therefore, it is acceptable for the precompile set composition in evm-tests/src/precompiles.rs to route all these forks to new_istanbul() even though this technically applies Istanbul-era gas costs to earlier forks, as there are no tests that would be affected by this implementation choice.
Learnt from: mrLSD
Repo: aurora-is-near/aurora-evm PR: 95
File: evm-tests/src/types/eip_7702.rs:84-91
Timestamp: 2025-12-19T00:47:31.731Z
Learning: In the aurora-evm codebase (evm-tests), the version of `primitive-types` being used has `U256::to_big_endian()` return a `[u8; 32]` array value, not require a mutable slice parameter. The pattern `result[..32].copy_from_slice(&r.to_big_endian())` is correct and compiles successfully.
📚 Learning: 2025-12-19T00:47:31.731Z
Learnt from: mrLSD
Repo: aurora-is-near/aurora-evm PR: 95
File: evm-tests/src/types/eip_7702.rs:84-91
Timestamp: 2025-12-19T00:47:31.731Z
Learning: In the aurora-evm codebase (evm-tests), the version of `primitive-types` being used has `U256::to_big_endian()` return a `[u8; 32]` array value, not require a mutable slice parameter. The pattern `result[..32].copy_from_slice(&r.to_big_endian())` is correct and compiles successfully.
Applied to files:
evm-tests/src/types/eip_7702.rsevm-tests/src/types/mod.rs
📚 Learning: 2025-12-19T00:59:18.510Z
Learnt from: mrLSD
Repo: aurora-is-near/aurora-evm PR: 95
File: evm-tests/src/types/ecrecover.rs:6-26
Timestamp: 2025-12-19T00:59:18.510Z
Learning: In evm-tests/src/types/ecrecover.rs (aurora-evm-jsontests crate), panicking on invalid input (such as insufficient signature length) is preferable to adding input validation checks and returning errors, as explicit panics provide clearer failure locations and stack traces that are more useful for test debugging purposes.
Applied to files:
evm-tests/src/types/eip_7702.rsevm-tests/src/types/mod.rs
📚 Learning: 2025-12-19T00:45:08.192Z
Learnt from: mrLSD
Repo: aurora-is-near/aurora-evm PR: 95
File: evm-tests/src/types/transaction.rs:195-238
Timestamp: 2025-12-19T00:45:08.192Z
Learning: In test code within the evm-tests crate (evm-tests/src/...), prefer using unwrap() or expect() to trigger immediate panics with precise location when a test condition fails. This aids debugging by showing the exact failure point and stack trace. Use this approach only in test code; production/error-handling paths should still use proper error handling rather than unwrapping.
Applied to files:
evm-tests/src/types/eip_7702.rsevm-tests/src/types/mod.rs
📚 Learning: 2025-12-19T00:54:43.287Z
Learnt from: mrLSD
Repo: aurora-is-near/aurora-evm PR: 95
File: evm-tests/src/types/transaction.rs:111-123
Timestamp: 2025-12-19T00:54:43.287Z
Learning: In evm-tests/src/types/transaction.rs, the `get_caller_from_secret_key` method appropriately uses `unwrap()` for test purposes, consistent with the crate's preference for explicit panics over error handling.
Applied to files:
evm-tests/src/types/eip_7702.rsevm-tests/src/types/mod.rs
📚 Learning: 2025-12-19T01:14:58.120Z
Learnt from: mrLSD
Repo: aurora-is-near/aurora-evm PR: 95
File: evm-tests/src/state_dump.rs:84-92
Timestamp: 2025-12-19T01:14:58.120Z
Learning: In evm-tests/src/state_dump.rs (aurora-evm crate), the dump_to_file method correctly generates filenames using format!("state_test_{spec:?}_{now}.json"). The Spec enum Debug format produces clean variant names (e.g., "Shanghai", "Cancun") that are filesystem-safe and do not contain problematic characters like colons. This is Aurora EVM, not REVM.
Applied to files:
evm-tests/src/types/eip_7702.rsevm-tests/src/types/mod.rs
📚 Learning: 2025-12-19T01:09:14.674Z
Learnt from: mrLSD
Repo: aurora-is-near/aurora-evm PR: 95
File: evm-tests/src/precompiles.rs:45-86
Timestamp: 2025-12-19T01:09:14.674Z
Learning: In the aurora-evm codebase (evm-tests), there are no test cases for pre-Istanbul Ethereum forks (Frontier, Homestead, Tangerine, SpuriousDragon, Byzantium, Constantinople, Petersburg). Therefore, it is acceptable for the precompile set composition in evm-tests/src/precompiles.rs to route all these forks to new_istanbul() even though this technically applies Istanbul-era gas costs to earlier forks, as there are no tests that would be affected by this implementation choice.
Applied to files:
evm-tests/src/types/mod.rs
🧬 Code graph analysis (1)
evm-tests/src/types/eip_7702.rs (2)
evm-tests/src/precompiles.rs (1)
new(43-57)evm-tests/src/types/mod.rs (1)
from(187-204)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: ethereum-tests
- GitHub Check: unit-tests-32-bit
- GitHub Check: ethereum-tests-32-bit
🔇 Additional comments (11)
evm-tests/src/types/eip_7702.rs (4)
22-53: LGTM!The
Authorizationstruct and itssignature_hashmethod correctly implement EIP-7702 authorization tuple hashing by prepending the MAGIC byte (0x05) and RLP-encoding the chain_id, address, and nonce.
55-82: LGTM!The
SignedAuthorizationstruct correctly delegates address recovery toecrecoverfromaurora_engine_precompiles. Therecover_addressmethod properly constructs the authorization hash and signature array for recovery.
84-91: LGTM!The
vrs_to_arrhelper correctly packs the signature components into a 65-byte array in the expected (r, s, v) format. Based on learnings, theU256::to_big_endian()method in this version ofprimitive-typesreturns a[u8; 32]array value, so thecopy_from_slicepattern is correct.
10-20: No action needed. TheSECP256K1N_HALFconstant is actively used inevm-tests/src/types/transaction.rs(line 284) for EIP-2 signature validation, verifying that the signature's s-value does not exceed the secp256k1 curve order divided by two. The constant serves its intended purpose and is not unused code.Likely an incorrect or invalid review comment.
evm-tests/src/types/mod.rs (7)
1-27: LGTM!Clean module organization with appropriate public/private visibility for submodules and targeted re-exports of
SpecandVmTestCase.
28-58: LGTM!The
StateTestCasestruct is well-documented and properly structured for deserializing Ethereum state test JSON files. TheBTreeMap<Spec, Vec<PostState>>design supports multiple expected outcomes per fork specification.
60-120: LGTM!The
get_memory_vicinitymethod correctly implements EIP-1559 validation logic:
- Validates that
max_fee_per_gasis not set for pre-London forks- Ensures priority fee does not exceed gas price
- Computes effective gas price per EIP-1559
- Validates gas price is not below block base fee
The use of
expect()and potential panics are appropriate for test debugging purposes. Based on learnings, explicit panics provide clearer failure locations and stack traces.
122-184: LGTM!The
StateEnvstruct comprehensively covers block environment parameters including EIP-4844 blob gas fields. The use ofdeny_unknown_fieldsensures strict deserialization validation, and custom deserializers handle hex string conversions appropriately.
186-205: LGTM!The
From<StateEnv>implementation appropriately sets transaction-dependent fields (gas prices, origin, chain_id) to defaults, with clear comments explaining that these values are not used in state tests. This is consistent with the relevant code snippet context.
207-242: LGTM!The
PreStatenewtype andPostStatestruct are well-designed for state test verification.PostStatecaptures all necessary expected outcomes including state hashes, logs, exception expectations, and optional account state comparisons.
244-253: LGTM!Clean struct for indexing into transaction parameter arrays (data, gas, value).
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI Agents
In @evm-tests/src/types/mod.rs:
- Around line 255-276: Enum variant names contain the typo "Then" instead of
"Than"; rename InvalidTxReason::GasFloorMoreThenGasLimit to
GasFloorMoreThanGasLimit and also rename GasPriceLessThenBlockBaseFee to
GasPriceLessThanBlockBaseFee, then update all references/usages (e.g., in
transaction.rs and mod.rs) to use the new variant names to keep the code
consistent and compile.
🧹 Nitpick comments (5)
evm-tests/src/assertions.rs (4)
6-193: Consider extracting common validation logic to reduce duplication.The function contains significant code duplication across fork branches (Istanbul/Berlin, London, Merge, Shanghai, Cancun, Prague), with each branch following an identical pattern: match on
InvalidTxReason, iteratestates, validate expected exception strings. While explicit code aids test clarity, a helper function could reduce the ~180 lines of repetitive logic.Example refactoring approach
Extract the state iteration and exception checking into a helper:
fn validate_expected_exceptions( states: &[PostState], allowed_exceptions: &[&str], reason: &InvalidTxReason, spec: &Spec, name: &str, file_name: &str, ) { for (i, state) in states.iter().enumerate() { let expected = state.expect_exception.as_deref().unwrap_or_else(|| { panic!("expected error message for test: {reason:?} [{spec:?}] {name}:{i}\n{file_name:?}") }); let is_checked = allowed_exceptions.contains(&expected); assert!( is_checked, "unexpected error message {expected:?} for: {reason:?} [{spec:?}] {name}:{i}\n{file_name:?}", ); } }Then simplify each branch:
Spec::Cancun => match reason { InvalidTxReason::PriorityFeeTooLarge => { validate_expected_exceptions( states, &["TR_TipGtFeeCap", "TransactionException.PRIORITY_GREATER_THAN_MAX_FEE_PER_GAS"], reason, spec, name, file_name, ); } // ... }
195-342: Consider changing return type fromboolto()since the function always returns true.The function never returns
false—it either returnstrueat line 339 or panics on validation failures. This makes the boolean return value potentially misleading. If no callers rely on the boolean result, consider changing the return type to()for clarity.🔎 Proposed change
pub fn check_validate_exit_reason( reason: &InvalidTxReason, expect_exception: Option<&String>, name: &str, spec: &Spec, -) -> bool { +) { expect_exception.map_or_else( || { panic!("unexpected validation error reason: {reason:?} {name}"); }, |exception| { match reason { // ... validation logic ... } - true }, ) }
312-318: Break long line for improved readability.Line 313 exceeds typical line length limits. Consider splitting the boolean expression across multiple lines.
🔎 Proposed formatting
InvalidTxReason::AuthorizationListNotExist => { - let check_result = exception == "TransactionException.TYPE_4_EMPTY_AUTHORIZATION_LIST" || exception == "TransactionException.TYPE_4_TX_CONTRACT_CREATION"; + let check_result = exception == "TransactionException.TYPE_4_EMPTY_AUTHORIZATION_LIST" + || exception == "TransactionException.TYPE_4_TX_CONTRACT_CREATION"; assert!( check_result, "unexpected exception {exception:?} for AuthorizationListNotExist for test: [{spec:?}] {name}" ); }
382-396: Standardize assert! formatting for consistency.The
assert!macro formatting is inconsistent—lines 385-387 and 393-395 span three lines while lines 383-384 span two lines. Consider using consistent formatting throughout the function.🔎 Proposed formatting
ExitError::MaxNonce => { let check_result = exception == "TR_NonceHasMaxValue" || exception == "TransactionException.NONCE_IS_MAX"; - assert!(check_result, - "unexpected exception {exception:?} for MaxNonce error for test: {name}" + assert!( + check_result, + "unexpected exception {exception:?} for MaxNonce error for test: {name}" ); return true; } ExitError::OutOfGas => { let check_result = exception == "TransactionException.INTRINSIC_GAS_TOO_LOW"; - assert!(check_result, - "unexpected exception {exception:?} for OutOfGas error for test: {name}" + assert!( + check_result, + "unexpected exception {exception:?} for OutOfGas error for test: {name}" ); return true; }evm-tests/src/types/transaction.rs (1)
374-390: Potential panic on empty input slice.
from_tx_bytesaccessestx_bytes[0]without checking if the slice is empty. While panics are acceptable in this test crate, an explicit check with a descriptive message would improve debugging experience when test fixtures have malformed data.🔎 Optional improvement for clearer panic message
#[must_use] pub const fn from_tx_bytes(tx_bytes: &[u8]) -> Self { + if tx_bytes.is_empty() { + panic!("tx_bytes cannot be empty"); + } match tx_bytes[0] { b if b > 0x7f => Self::Legacy, 1 => Self::AccessList, 2 => Self::DynamicFee, 3 => Self::ShardBlob, 4 => Self::EOAAccountCode, _ => panic!( "Unknown tx type. You may need to update the TxType enum if Ethereum introduced new enveloped transaction types." ), } }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
evm-tests/src/assertions.rsevm-tests/src/types/mod.rsevm-tests/src/types/transaction.rs
🧰 Additional context used
🧠 Learnings (7)
📓 Common learnings
Learnt from: mrLSD
Repo: aurora-is-near/aurora-evm PR: 95
File: evm-tests/src/state_dump.rs:84-92
Timestamp: 2025-12-19T01:14:58.120Z
Learning: In evm-tests/src/state_dump.rs (aurora-evm crate), the dump_to_file method correctly generates filenames using format!("state_test_{spec:?}_{now}.json"). The Spec enum Debug format produces clean variant names (e.g., "Shanghai", "Cancun") that are filesystem-safe and do not contain problematic characters like colons. This is Aurora EVM, not REVM.
Learnt from: mrLSD
Repo: aurora-is-near/aurora-evm PR: 95
File: evm-tests/src/types/transaction.rs:195-238
Timestamp: 2025-12-19T00:45:15.604Z
Learning: In the aurora-evm-jsontests crate (evm-tests), explicit panics with `expect()` or `unwrap()` are preferred over error handling for test debugging purposes, as they provide exact failure locations and stack traces that are more useful during test development and debugging.
Learnt from: mrLSD
Repo: aurora-is-near/aurora-evm PR: 95
File: evm-tests/src/precompiles.rs:45-86
Timestamp: 2025-12-19T01:09:14.674Z
Learning: In the aurora-evm codebase (evm-tests), there are no test cases for pre-Istanbul Ethereum forks (Frontier, Homestead, Tangerine, SpuriousDragon, Byzantium, Constantinople, Petersburg). Therefore, it is acceptable for the precompile set composition in evm-tests/src/precompiles.rs to route all these forks to new_istanbul() even though this technically applies Istanbul-era gas costs to earlier forks, as there are no tests that would be affected by this implementation choice.
📚 Learning: 2025-12-19T00:54:43.287Z
Learnt from: mrLSD
Repo: aurora-is-near/aurora-evm PR: 95
File: evm-tests/src/types/transaction.rs:111-123
Timestamp: 2025-12-19T00:54:43.287Z
Learning: In evm-tests/src/types/transaction.rs, the `get_caller_from_secret_key` method appropriately uses `unwrap()` for test purposes, consistent with the crate's preference for explicit panics over error handling.
Applied to files:
evm-tests/src/types/transaction.rsevm-tests/src/assertions.rsevm-tests/src/types/mod.rs
📚 Learning: 2025-12-19T00:59:18.510Z
Learnt from: mrLSD
Repo: aurora-is-near/aurora-evm PR: 95
File: evm-tests/src/types/ecrecover.rs:6-26
Timestamp: 2025-12-19T00:59:18.510Z
Learning: In evm-tests/src/types/ecrecover.rs (aurora-evm-jsontests crate), panicking on invalid input (such as insufficient signature length) is preferable to adding input validation checks and returning errors, as explicit panics provide clearer failure locations and stack traces that are more useful for test debugging purposes.
Applied to files:
evm-tests/src/types/transaction.rsevm-tests/src/assertions.rsevm-tests/src/types/mod.rs
📚 Learning: 2025-12-19T00:45:08.192Z
Learnt from: mrLSD
Repo: aurora-is-near/aurora-evm PR: 95
File: evm-tests/src/types/transaction.rs:195-238
Timestamp: 2025-12-19T00:45:08.192Z
Learning: In test code within the evm-tests crate (evm-tests/src/...), prefer using unwrap() or expect() to trigger immediate panics with precise location when a test condition fails. This aids debugging by showing the exact failure point and stack trace. Use this approach only in test code; production/error-handling paths should still use proper error handling rather than unwrapping.
Applied to files:
evm-tests/src/types/transaction.rsevm-tests/src/assertions.rsevm-tests/src/types/mod.rs
📚 Learning: 2025-12-19T00:47:31.731Z
Learnt from: mrLSD
Repo: aurora-is-near/aurora-evm PR: 95
File: evm-tests/src/types/eip_7702.rs:84-91
Timestamp: 2025-12-19T00:47:31.731Z
Learning: In the aurora-evm codebase (evm-tests), the version of `primitive-types` being used has `U256::to_big_endian()` return a `[u8; 32]` array value, not require a mutable slice parameter. The pattern `result[..32].copy_from_slice(&r.to_big_endian())` is correct and compiles successfully.
Applied to files:
evm-tests/src/types/transaction.rsevm-tests/src/assertions.rsevm-tests/src/types/mod.rs
📚 Learning: 2025-12-19T01:09:14.674Z
Learnt from: mrLSD
Repo: aurora-is-near/aurora-evm PR: 95
File: evm-tests/src/precompiles.rs:45-86
Timestamp: 2025-12-19T01:09:14.674Z
Learning: In the aurora-evm codebase (evm-tests), there are no test cases for pre-Istanbul Ethereum forks (Frontier, Homestead, Tangerine, SpuriousDragon, Byzantium, Constantinople, Petersburg). Therefore, it is acceptable for the precompile set composition in evm-tests/src/precompiles.rs to route all these forks to new_istanbul() even though this technically applies Istanbul-era gas costs to earlier forks, as there are no tests that would be affected by this implementation choice.
Applied to files:
evm-tests/src/types/transaction.rsevm-tests/src/assertions.rsevm-tests/src/types/mod.rs
📚 Learning: 2025-12-19T01:14:58.120Z
Learnt from: mrLSD
Repo: aurora-is-near/aurora-evm PR: 95
File: evm-tests/src/state_dump.rs:84-92
Timestamp: 2025-12-19T01:14:58.120Z
Learning: In evm-tests/src/state_dump.rs (aurora-evm crate), the dump_to_file method correctly generates filenames using format!("state_test_{spec:?}_{now}.json"). The Spec enum Debug format produces clean variant names (e.g., "Shanghai", "Cancun") that are filesystem-safe and do not contain problematic characters like colons. This is Aurora EVM, not REVM.
Applied to files:
evm-tests/src/types/transaction.rsevm-tests/src/assertions.rsevm-tests/src/types/mod.rs
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: unit-tests-32-bit
- GitHub Check: ethereum-tests
- GitHub Check: ethereum-tests-32-bit
🔇 Additional comments (10)
evm-tests/src/types/transaction.rs (5)
16-68: LGTM!The
Transactionstruct is well-designed with appropriate serde annotations for JSON deserialization. Fields correctly represent EIP-2930 (access lists), EIP-4844 (blob transactions), and EIP-7702 (authorization lists) data.
70-104: LGTM!The accessor methods correctly retrieve transaction data using post-state indexes. The bounds check in
get_access_listbefore theunwrap()ensures safety. Based on learnings, direct indexing that may panic is acceptable in this test crate for debugging purposes.
125-143: LGTM!The
intrinsic_gascalculation correctly distinguishes between contract creation and call transactions, includes EIP-7702 authorization list costs, and properly uses the gasometer to record transaction costs.
150-191: LGTM!The validation logic for intrinsic gas, block gas limit, and required funds is well-implemented with proper overflow protection using
checked_mulandchecked_add.
249-313: LGTM!The Prague/EIP-7702 validation correctly implements:
- EIP-7623 floor gas check
- Authorization list presence requirement for EOAAccountCode transactions
- Chain ID validation (must be 0 or current chain ID)
- Signature validity checks (v ≤ 1, s ≤ SECP256K1N_HALF per EIP-2)
- Address recovery with graceful fallback to invalid state
The
as_u64()calls on nonce values could theoretically panic for extremely large values, but this is acceptable per the crate's testing philosophy.evm-tests/src/types/mod.rs (5)
1-26: LGTM!Module structure is well-organized with appropriate visibility modifiers. Public modules expose the necessary types for external use while keeping internal utilities private.
28-58: LGTM!
StateTestCaseis well-documented and correctly structured for representing Ethereum state transition tests. The use ofBTreeMap<Spec, Vec<PostState>>allows for ordered iteration over forks and multiple expected outcomes per fork.
122-184: LGTM!
StateEnvcomprehensively models block environment parameters with proper EIP-4844 blob gas fields. Thedeny_unknown_fieldsattribute ensures strict deserialization, catching test fixture issues early.
186-205: LGTM!The
From<StateEnv>implementation provides a sensible conversion for cases where transaction-specific vicinity fields aren't needed. Comments clearly document the intentional zero/default values.
207-253: LGTM!The
PreState,PostState, andPostStateIndexestypes are well-structured for state test representation. The newtype pattern forPreStateprovides type safety whileAsRefallows convenient access to the underlying data.
aleksuss
force-pushed
the
feat/ethjson-refactoring
branch
from
9cf4895 to
d98e360
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI Agents
In @evm-tests/src/types/mod.rs:
- Line 61: Fix the typo in the doc comment for the item whose documentation
starts "Get the memory vicinity for the transaction": change "includesState test
data" to "includes state test data" so there is a space between "includes" and
"state". Update the triple-slash comment above the associated function/struct in
evm-tests/src/types/mod.rs accordingly.
🧹 Nitpick comments (3)
evm-tests/src/assertions.rs (1)
66-124: Consider merging identical spec arms.
Spec::MergeandSpec::Shanghaihave identical handling for bothPriorityFeeTooLargeandGasPriceLessThanBlockBaseFee. They could be consolidated using pattern matching.🔎 Proposed refactor
- Spec::Merge => match reason { + Spec::Merge | Spec::Shanghai => match reason { InvalidTxReason::PriorityFeeTooLarge => { // ... existing logic ... } InvalidTxReason::GasPriceLessThanBlockBaseFee => { // ... existing logic ... } _ => { panic!("Unexpected validation reason: {reason:?} [{spec:?}] {name}\n{file_name:?}") } }, - Spec::Shanghai => match reason { - InvalidTxReason::PriorityFeeTooLarge => { - // ... identical logic to Merge ... - } - InvalidTxReason::GasPriceLessThanBlockBaseFee => { - // ... identical logic to Merge ... - } - _ => { - panic!("Unexpected validation reason: {reason:?} [{spec:?}] {name}\n{file_name:?}") - } - },evm-tests/src/types/mod.rs (2)
219-242: Consider documenting the distinction betweenstateandpost_statefields.Both
stateandpost_stateare optionalAccountsStatefields. Adding documentation to clarify when each is used (e.g., different test formats, legacy vs. current) would improve maintainability.
255-276: Consider adding derives for better error handling.The
InvalidTxReasonenum only derivesDebug. AddingClone,PartialEq, andEqwould enable more flexible error comparisons and pattern matching in tests. ADisplayimplementation would also improve error messages.🔎 Proposed enhancement
-#[derive(Debug)] +#[derive(Debug, Clone, PartialEq, Eq)] pub enum InvalidTxReason {
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
evm-tests/src/assertions.rsevm-tests/src/types/mod.rs
🧰 Additional context used
🧠 Learnings (7)
📓 Common learnings
Learnt from: mrLSD
Repo: aurora-is-near/aurora-evm PR: 95
File: evm-tests/src/state_dump.rs:84-92
Timestamp: 2025-12-19T01:14:58.120Z
Learning: In evm-tests/src/state_dump.rs (aurora-evm crate), the dump_to_file method correctly generates filenames using format!("state_test_{spec:?}_{now}.json"). The Spec enum Debug format produces clean variant names (e.g., "Shanghai", "Cancun") that are filesystem-safe and do not contain problematic characters like colons. This is Aurora EVM, not REVM.
Learnt from: mrLSD
Repo: aurora-is-near/aurora-evm PR: 95
File: evm-tests/src/precompiles.rs:45-86
Timestamp: 2025-12-19T01:09:14.674Z
Learning: In the aurora-evm codebase (evm-tests), there are no test cases for pre-Istanbul Ethereum forks (Frontier, Homestead, Tangerine, SpuriousDragon, Byzantium, Constantinople, Petersburg). Therefore, it is acceptable for the precompile set composition in evm-tests/src/precompiles.rs to route all these forks to new_istanbul() even though this technically applies Istanbul-era gas costs to earlier forks, as there are no tests that would be affected by this implementation choice.
Learnt from: mrLSD
Repo: aurora-is-near/aurora-evm PR: 95
File: evm-tests/src/types/transaction.rs:195-238
Timestamp: 2025-12-19T00:45:15.604Z
Learning: In the aurora-evm-jsontests crate (evm-tests), explicit panics with `expect()` or `unwrap()` are preferred over error handling for test debugging purposes, as they provide exact failure locations and stack traces that are more useful during test development and debugging.
📚 Learning: 2025-12-19T00:45:08.192Z
Learnt from: mrLSD
Repo: aurora-is-near/aurora-evm PR: 95
File: evm-tests/src/types/transaction.rs:195-238
Timestamp: 2025-12-19T00:45:08.192Z
Learning: In test code within the evm-tests crate (evm-tests/src/...), prefer using unwrap() or expect() to trigger immediate panics with precise location when a test condition fails. This aids debugging by showing the exact failure point and stack trace. Use this approach only in test code; production/error-handling paths should still use proper error handling rather than unwrapping.
Applied to files:
evm-tests/src/assertions.rsevm-tests/src/types/mod.rs
📚 Learning: 2025-12-19T00:59:18.510Z
Learnt from: mrLSD
Repo: aurora-is-near/aurora-evm PR: 95
File: evm-tests/src/types/ecrecover.rs:6-26
Timestamp: 2025-12-19T00:59:18.510Z
Learning: In evm-tests/src/types/ecrecover.rs (aurora-evm-jsontests crate), panicking on invalid input (such as insufficient signature length) is preferable to adding input validation checks and returning errors, as explicit panics provide clearer failure locations and stack traces that are more useful for test debugging purposes.
Applied to files:
evm-tests/src/assertions.rsevm-tests/src/types/mod.rs
📚 Learning: 2025-12-19T01:14:58.120Z
Learnt from: mrLSD
Repo: aurora-is-near/aurora-evm PR: 95
File: evm-tests/src/state_dump.rs:84-92
Timestamp: 2025-12-19T01:14:58.120Z
Learning: In evm-tests/src/state_dump.rs (aurora-evm crate), the dump_to_file method correctly generates filenames using format!("state_test_{spec:?}_{now}.json"). The Spec enum Debug format produces clean variant names (e.g., "Shanghai", "Cancun") that are filesystem-safe and do not contain problematic characters like colons. This is Aurora EVM, not REVM.
Applied to files:
evm-tests/src/assertions.rsevm-tests/src/types/mod.rs
📚 Learning: 2025-12-19T01:09:14.674Z
Learnt from: mrLSD
Repo: aurora-is-near/aurora-evm PR: 95
File: evm-tests/src/precompiles.rs:45-86
Timestamp: 2025-12-19T01:09:14.674Z
Learning: In the aurora-evm codebase (evm-tests), there are no test cases for pre-Istanbul Ethereum forks (Frontier, Homestead, Tangerine, SpuriousDragon, Byzantium, Constantinople, Petersburg). Therefore, it is acceptable for the precompile set composition in evm-tests/src/precompiles.rs to route all these forks to new_istanbul() even though this technically applies Istanbul-era gas costs to earlier forks, as there are no tests that would be affected by this implementation choice.
Applied to files:
evm-tests/src/assertions.rsevm-tests/src/types/mod.rs
📚 Learning: 2025-12-19T00:54:43.287Z
Learnt from: mrLSD
Repo: aurora-is-near/aurora-evm PR: 95
File: evm-tests/src/types/transaction.rs:111-123
Timestamp: 2025-12-19T00:54:43.287Z
Learning: In evm-tests/src/types/transaction.rs, the `get_caller_from_secret_key` method appropriately uses `unwrap()` for test purposes, consistent with the crate's preference for explicit panics over error handling.
Applied to files:
evm-tests/src/assertions.rsevm-tests/src/types/mod.rs
📚 Learning: 2025-12-19T00:47:31.731Z
Learnt from: mrLSD
Repo: aurora-is-near/aurora-evm PR: 95
File: evm-tests/src/types/eip_7702.rs:84-91
Timestamp: 2025-12-19T00:47:31.731Z
Learning: In the aurora-evm codebase (evm-tests), the version of `primitive-types` being used has `U256::to_big_endian()` return a `[u8; 32]` array value, not require a mutable slice parameter. The pattern `result[..32].copy_from_slice(&r.to_big_endian())` is correct and compiles successfully.
Applied to files:
evm-tests/src/assertions.rsevm-tests/src/types/mod.rs
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: ethereum-tests-32-bit
- GitHub Check: unit-tests-32-bit
- GitHub Check: ethereum-tests
🔇 Additional comments (12)
evm-tests/src/assertions.rs (4)
1-4: LGTM!Clean imports with no redundancy.
197-342: LGTM!The validation logic is comprehensive. Note that this function effectively cannot return
false— it either validates successfully and returnstrue, or panics on mismatch. This is consistent with the test crate's preference for explicit panics to aid debugging. Based on learnings, this approach is appropriate for test code.
344-361: LGTM!Clean assertion helpers. The use of
expect()inassert_empty_create_callerprovides clear failure messages for debugging. Based on learnings, this is the preferred approach for test code.
363-419: LGTM!Well-structured exit reason checking with proper boolean return semantics. The function correctly returns
truewhen an expected exception matches,falsewhen errors occur without expected exceptions (allowing the caller to handle), and panics on fatal or unexpected errors.evm-tests/src/types/mod.rs (8)
1-26: LGTM! Well-organized module structure.The imports and module declarations are clean and logically organized, with appropriate public/private visibility for the test harness.
28-58: LGTM! Well-structured test case definition.The
StateTestCasestruct is well-documented and properly configured for deserialization. The field renaming (pre→pre_state,post→post_states) improves code clarity.
73-99: LGTM! Correct EIP-1559 gas price validation.The gas price validation logic correctly handles:
- London fork detection and EIP-1559 field presence
- Priority fee bounds checking
- Effective gas price calculation per EIP-1559 spec
- Base fee validation
The use of
expect()on line 79 is consistent with the crate's preference for explicit panics in test code.
101-119: LGTM! Proper MemoryVicinity construction.The
MemoryVicinityis correctly populated with transaction and environment data. The hardcodedchain_idof 1 is appropriate for test scenarios.
122-184: LGTM! Comprehensive environment configuration.The
StateEnvstruct properly captures all block environment parameters with appropriate deserialization, including EIP-4844 blob gas fields. The use ofdeny_unknown_fieldshelps catch test format discrepancies.
186-205: LGTM! Appropriate conversion with clear defaults.The
Fromimplementation correctly convertsStateEnvtoMemoryVicinitywith well-documented defaults for transaction-specific fields that aren't part of the environment.
207-217: LGTM! Clean newtype wrapper.The
PreStatenewtype withAsRefimplementation provides type safety while maintaining convenient access to the underlyingAccountsState.
244-253: LGTM! Clear index structure.The
PostStateIndexesstruct clearly represents indexes into transaction parameter sets with appropriate types and documentation.
|
@aleksuss thanks for the contribution, it's valuable! |
2 participants
Description
🛋️ Motivation
🚀 A large-scale refactoring was carried out with the following goals:
json-testsstructure to match the latest official test suiteethjsonandethcore-builtinGPL-3.0licenseWhat's changed
Refactored
aurora-evm-tests:➡️ removed
ethjsonand refactored VM and State tests➡️ refactored
json-testsdeserializers➡️ refactored all EVM tests types
➡️ refactored tests runner
➡️ refactored precompiles
Benchmarks were conducted, and the results showed that despite an extended set of fields, new structures, and a refactored deserialization mechanism, the deserialization speed remained the same down to the millisecond compared to the previous deserializer implementation.
Summary by CodeRabbit
Project Restructuring
New Features
Improvements
Toolchain
Chores
Removals
✏️ Tip: You can customize this high-level summary in your review settings.