🏗️ refactor: reorganize EVM files into logical folders and prepare import fixes #1801
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
1 Skipped Deployment
|
|
WalkthroughThis set of changes reorganizes the EVM codebase by updating import paths to reflect a new directory structure, renames stack error variants for clarity, refactors error mapping to use direct try/catch patterns, and updates function signatures to use new types. Documentation is enhanced throughout, with new sections and detailed comments added, and some obsolete files are removed or marked as unused. Changes
Sequence Diagram(s)sequenceDiagram
participant VM
participant Stack
participant Memory
participant Storage
Note over VM: Main interpret loop (simplified)
VM->>Stack: stack_pop()/stack_push()
Note right of Stack: Returns or throws Stack* errors
VM->>Memory: memory_set_data(), memory_get_slice()
Note right of Memory: Returns or throws Memory* errors
VM->>Storage: vm_get_storage()/vm_set_storage()
Note right of Storage: Returns or throws OutOfMemory
Note over VM: Errors are caught and mapped inline
Possibly related PRs
Poem
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
This stack of pull requests is managed by Graphite. Learn more about stacking. |
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (5)
src/evm/execution/storage.zig (2)
50-55: 🛠️ Refactor suggestionHoist
Contractimport out of function scope
ImportingContractinside theif (vm.chain_rules.IsBerlin)block duplicates the import and adds unnecessary overhead. Moveconst Contract = @import("../contract/contract.zig");to the file-level imports above.Proposed diff:
--- a/src/evm/execution/storage.zig +++ b/src/evm/execution/storage.zig @@ const error_mapping = @import("../error_mapping.zig"); +const Contract = @import("../contract/contract.zig"); /// EIP-3529 (London) gas costs for SSTORE const SSTORE_SET_GAS: u64 = 20000; @@ pub fn op_sload(...){ - const Contract = @import("../contract/contract.zig"); + // use top-level Contract import
92-97: 🛠️ Refactor suggestionHoist and consolidate
Contractimport inop_sstore
The sameContractimport is repeated insideop_sstore. Move it to the top-level imports and remove this in-function import to DRY up the code.Proposed diff:
--- a/src/evm/execution/storage.zig +++ b/src/evm/execution/storage.zig @@ pub fn op_sstore(...){ - const Contract = @import("../contract/contract.zig"); + // use top-level Contract importsrc/evm/evm.zig (1)
65-191: 💡 Verification agent🧩 Analysis chain
Verify the new directory structure and import paths.
The reorganization into logical folders looks excellent for maintainability. However, please ensure all import paths reference existing files in the new structure.
🏁 Script executed:
#!/bin/bash # Description: Verify that all import paths in evm.zig reference existing files # Expected: All imports should resolve to valid files # Check each import path exists declare -a imports=( "src/evm/contract/code_analysis.zig" "src/evm/contract/contract.zig" "src/evm/execution/execution_error.zig" "src/evm/hardforks/hardfork.zig" "src/evm/jump_table/jump_table.zig" "src/evm/opcodes/opcode.zig" "src/evm/opcodes/operation.zig" "src/evm/stack/stack.zig" "src/evm/stack/stack_validation.zig" "src/evm/contract/storage_pool.zig" "src/evm/execution/package.zig" "src/evm/contract/bitvec.zig" "src/evm/hardforks/chain_rules.zig" "src/evm/constants/constants.zig" "src/evm/contract/eip_7702_bytecode.zig" "src/evm/constants/gas_constants.zig" "src/evm/constants/memory_limits.zig" "src/evm/interpret_result.zig" "src/evm/access_list/access_list.zig" ) echo "Checking import paths exist..." for import in "${imports[@]}"; do if [[ -f "$import" ]]; then echo "✓ $import" else echo "✗ MISSING: $import" fi done # Also check that InterpretResult type exists and RunResult doesn't echo -e "\nChecking type definitions..." if fd interpret_result.zig --exec rg -q "InterpretResult"; then echo "✓ InterpretResult type found" else echo "✗ InterpretResult type not found" fiLength of output: 3880
🏁 Script executed:
#!/bin/bash # Inspect definitions in src/evm/interpret_result.zig FILE="src/evm/interpret_result.zig" if [[ -f "$FILE" ]]; then echo "=== $FILE ===" sed -n '1,200p' "$FILE" echo -e "\nSearching for 'pub const' definitions:" rg -n "pub const" "$FILE" else echo "✗ File not found: $FILE" fiLength of output: 3675
Import Paths Validated – Update InterpretResult Docs
All reorganized import paths in
src/evm/evm.zigresolve successfully. One remaining change is needed in the newly renamedinterpret_result.zig:• In
src/evm/interpret_result.zig, the top‐level documentation still refers to “RunResult” even though the module (and its alias inevm.zig) is now InterpretResult. Please update these comments for consistency:/// Result of an EVM execution run. -/// -/// RunResult encapsulates the outcome of executing EVM bytecode, including +/// +/// InterpretResult encapsulates the outcome of executing EVM bytecode, includingsrc/evm/contract/eip_7702_bytecode.zig (2)
76-82: 🛠️ Refactor suggestionAdd input validation to prevent potential issues.
The
new_rawfunction doesn't validate the magic bytes or minimum length, which could lead to malformed instances.pub fn new_raw(bytes: []const u8) !Self { + if (bytes.len < 22) { + return error.InvalidBytecodeLength; + } + if (!std.mem.eql(u8, bytes[0..2], &EIP7702_MAGIC_BYTES)) { + return error.InvalidMagicBytes; + } var address: Address.Address = undefined; - if (bytes.len > 20) { - @memcpy(&address, bytes[2..22]); - } + @memcpy(&address, bytes[2..22]); return Self.new(address); }
99-102:⚠️ Potential issueComplete the
rawfunction implementation.The function returns an empty slice instead of the 10000 proper 22-byte bytecode format. This breaks the contract of the function.
pub fn raw(self: *const Self) []const u8 { - _ = self; - return &[_]u8{}; + // TODO: This should return a properly allocated 22-byte array + // For now, this is a placeholder that needs proper memory management + var result = [_]u8{0} ** 22; + result[0] = EIP7702_MAGIC_BYTES[0]; + result[1] = EIP7702_MAGIC_BYTES[1]; + @memcpy(result[2..22], &self.address); + return &result; }Note: This implementation has lifetime issues since it returns a pointer to stack memory. Consider returning an owned array or requiring an allocator parameter.
🧹 Nitpick comments (2)
src/evm/contract/eip_7702_bytecode.zig (1)
1-1: Address the unused file status.The file is marked as "currently unused" but is receiving documentation improvements. Consider either implementing the missing functionality to make it usable or removing it if it's not needed yet.
Would you like me to help implement the missing
rawfunction or create an issue to track completing this module?src/evm/error_mapping.zig (1)
12-19: Consider preserving error specificity in stack operations.The current implementation maps
StackOutOfBoundsandStackInvalidPositiontoStackUnderflow, which loses error specificity. Consider mapping these to their corresponding ExecutionError variants if they exist, or document why this simplification is acceptable.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (36)
src/evm/CLAUDE.md(3 hunks)src/evm/context.zig(2 hunks)src/evm/contract/bitvec.zig(1 hunks)src/evm/contract/contract.zig(1 hunks)src/evm/contract/eip_7702_bytecode.zig(4 hunks)src/evm/error_mapping.zig(1 hunks)src/evm/evm.zig(3 hunks)src/evm/execution/arithmetic.zig(1 hunks)src/evm/execution/bitwise.zig(1 hunks)src/evm/execution/block.zig(1 hunks)src/evm/execution/comparison.zig(1 hunks)src/evm/execution/control.zig(1 hunks)src/evm/execution/crypto.zig(3 hunks)src/evm/execution/environment.zig(3 hunks)src/evm/execution/execution_error.zig(3 hunks)src/evm/execution/log.zig(1 hunks)src/evm/execution/memory.zig(1 hunks)src/evm/execution/package.zig(1 hunks)src/evm/execution/stack.zig(1 hunks)src/evm/execution/storage.zig(3 hunks)src/evm/execution/system.zig(1 hunks)src/evm/fee_market.zig(1 hunks)src/evm/frame.zig(1 hunks)src/evm/hardforks/chain_rules.zig(1 hunks)src/evm/interpret_result.zig(1 hunks)src/evm/jump_table/jump_table.zig(2 hunks)src/evm/jump_table/operation_specs.zig(1 hunks)src/evm/memory_size.zig(0 hunks)src/evm/opcodes/opcode.zig(2 hunks)src/evm/opcodes/operation.zig(2 hunks)src/evm/stack/stack.zig(27 hunks)src/evm/stack/stack_validation.zig(1 hunks)src/evm/stack/validation_patterns.zig(1 hunks)src/evm/state/state.zig(1 hunks)src/evm/vm.zig(7 hunks)src/evm/wasm_stubs.zig(1 hunks)
💤 Files with no reviewable changes (1)
- src/evm/memory_size.zig
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Nx Cloud - Main Job
- GitHub Check: CI Checks
🔇 Additional comments (48)
src/evm/fee_market.zig (1)
1-1: LGTM - Clear organizational marker during refactoring.The TODO comment appropriately marks this module as temporarily unused during the codebase reorganization, which aligns with the PR's restructuring objectives.
src/evm/context.zig (1)
191-191: LGTM - Documentation cleanup.Removing trailing whitespace from documentation comments improves code quality and is appropriate for this organizational refactoring.
Also applies to: 212-212
src/evm/wasm_stubs.zig (2)
1-1: LGTM - Clear integration tracking.The TODO comment appropriately documents that WASM build integration with the real EVM is needed, which is helpful for tracking remaining work during the reorganization.
7-12: LGTM - Consistent formatting.Reformatting the methods to use explicit braces improves code consistency without changing functionality. The no-op behavior is maintained appropriately.
src/evm/opcodes/opcode.zig (2)
37-44: LGTM - Appropriate consolidation of MemorySize struct.Moving the
MemorySizestruct definition inline consolidates memory size representation into the opcode module, which aligns with the reorganization objectives. The struct maintains the same interface withoffsetandsizefields.
328-328: LGTM - Minor whitespace cleanup.Removing the extra blank line improves code formatting consistency.
src/evm/hardforks/chain_rules.zig (1)
3-3: Import path update looks correct
TheLogmodule import has been updated to the parent directory, matching the new project layout.src/evm/state/state.zig (1)
36-36: Import path update looks correct
ChangingLogimport to../log.zigaligns with the reorganized directory structure.src/evm/contract/bitvec.zig (1)
2-2: Import path update looks correct
Theconstantsimport now points to../constants/constants.zigas intended.src/evm/interpret_result.zig (1)
1-1: Import path update looks correct
UpdatedExecutionErrorimport toexecution/execution_error.zigto reflect the new subdirectory.src/evm/stack/validation_patterns.zig (1)
2-2: Import path update looks correct
TheExecutionErrorimport now correctly points to../execution/execution_error.zig.src/evm/frame.zig (1)
3-5: Import paths updated for reorganized directory structure.
UpdatedStack,Contract, andExecutionErrorimports to point to their new subdirectories. Paths are correct relative tosrc/evm/frame.zigand align with the new layout.src/evm/stack/stack_validation.zig (1)
3-4: Import paths adjusted for new module hierarchy.
OperationandExecutionErrorimports now reference the relocatedopcodesandexecutiondirectories. Paths are correct relative to this file.src/evm/execution/stack.zig (1)
2-4: Align import paths with directory restructuring.
Operation,ExecutionError, andStackmodules are now correctly imported from their respective subdirectories (../opcodes, current directory, and../stack). No functional impact.src/evm/execution/arithmetic.zig (1)
55-57: Update imports to reflect new directory layout.
Operation,ExecutionError, andStackare imported from their reorganized paths. Verified consistency with other execution modules.src/evm/opcodes/operation.zig (2)
3-5: Correct relative imports after reorganization.
ExecutionError,Stack, andMemoryimports updated to use../paths. Ensures module resolution aligns with the new structure.
41-41: ImportExecutionResultfrom updated path.
Adjusted to../execution/execution_result.zig, matching the reorganized execution directory.src/evm/execution/comparison.zig (1)
2-4: Import path updates reflect project reorganization.
The new import paths correctly reference the reorganized modules without altering logic.src/evm/execution/bitwise.zig (1)
2-4: Consistent import path realignment across execution modules.
Import statements have been updated to match the new../opcodes,execution_error, and../stacklocations.src/evm/jump_table/operation_specs.zig (1)
2-5: Corrected module import paths for jump table.
Updated imports forexecution/package.zig,constants/gas_constants.zig,stack/stack.zig, andopcodes/operation.zigalign with the new directory structure.src/evm/execution/block.zig (1)
2-4: Import paths updated to reflect directory restructure.
The new imports forOperation,ExecutionError, andStackcorrectly point to their relocated modules.src/evm/contract/contract.zig (1)
41-41: Aligned import paths with new module locations.
Updated imports forconstants,ExecutionError, andLogreflect the reorganized directories without affecting functionality.Also applies to: 44-44, 47-47
src/evm/execution/memory.zig (1)
2-7: LGTM: Import paths correctly updated for directory restructuring.The import paths have been properly updated to reflect the new directory structure with modules organized into logical subdirectories (opcodes/, stack/, constants/). No functional changes detected.
src/evm/jump_table/jump_table.zig (1)
2-13: LGTM: Comprehensive import path updates align with directory restructuring.All import paths have been correctly updated to reflect the new modular directory structure. The jump table functionality and opcode dispatch logic remain unchanged - these are purely organizational improvements.
Also applies to: 137-137
src/evm/execution/system.zig (1)
2-12: LGTM: Import paths properly updated for modular organization.The import paths have been systematically updated to reflect the new directory structure with logical groupings (opcodes/, stack/, contract/, constants/, access_list/). All system operations maintain their existing functionality.
src/evm/execution/crypto.zig (1)
2-4: LGTM: Import paths updated consistently with directory restructuring.The import paths have been properly updated to use the new modular directory structure. The inline import of
memory_limitswithin the function is acceptable as it's only used locally withinop_sha3. No functional changes to the crypto operations.Also applies to: 31-31, 49-49
src/evm/execution/log.zig (1)
2-4: Import paths aligned with new directory layout
All module imports have been updated to reflect the reorganized folder structure. The paths forOperation,ExecutionError,Stack, andgas_constantsnow correctly point to their new locations without altering any logic.Also applies to: 7-7
src/evm/execution/package.zig (1)
19-20: Re-export paths updated for core types
TheOperation,ExecutionError,Stack,Contract, andgas_constantsexports have been adjusted to the new nested directory structure. All re-exports remain accurate and no API surface has changed.Also applies to: 22-22, 25-26
src/evm/execution/storage.zig (1)
2-4: Import paths updated to match new structure
The top-level imports forOperation,ExecutionError,Stack, andgas_constantsnow point to their relocated modules. This is purely organizational.Also applies to: 7-7
src/evm/execution/control.zig (1)
2-5: Control op imports updated for new directory structure
All imports (Operation,ExecutionError,ExecutionResult,Stack,gas_constants,AccessList) have been correctly realigned to their new paths. No behavior changes introduced.Also applies to: 8-9
src/evm/stack/stack.zig (3)
2-2: CorrectedLogimport path
TheLogmodule now imports from the parent directory, matching the reorganized project layout.
50-56: Stack error variants renamed
The error enum entries now include aStackprefix (StackOverflow,StackUnderflow,StackOutOfBounds,StackInvalidPosition), improving clarity and aligning withExecutionErrorvariants.
106-106: Method implementations updated to use renamed errors
All stack methods now return the newly prefixedError.Stack*variants consistently. Usage acrossappend,pop,peek, andbackis correct.Also applies to: 142-142, 178-178, 225-225
src/evm/CLAUDE.md (2)
7-16: Excellent addition of project status clarity.This section provides crucial context for contributors about the freedom to make breaking changes and optimize without backward compatibility constraints. This will help prevent unnecessary conservative design decisions.
211-228: Well-documented performance optimization strategy.The eager operation array analysis description is clear and provides specific implementation guidance with concrete performance benefits (50% improvement claim). The design principles are actionable and well-reasoned.
src/evm/execution/execution_error.zig (3)
4-28: Excellent documentation enhancement.The error categorization and detailed documentation significantly improve code maintainability and developer understanding. The categorization into logical groups (normal termination, resource exhaustion, etc.) is particularly helpful.
153-154: Good descriptive error messages.The descriptions for the new stack error variants are clear and helpful for debugging. They properly explain when each error condition occurs.
45-51:✅ Verification successful
Verify consistent usage of new stack error variants.
The new stack error variants (
StackOutOfBounds,StackInvalidPosition) follow good naming conventions with theStackprefix. Ensure these errors are used consistently throughout the stack implementation.
🏁 Script executed:
#!/bin/bash # Description: Verify that the new stack error variants are used consistently # Expected: Find usage of new error variants in stack-related files echo "Checking usage of new stack error variants..." # Search for the new error variants echo "=== StackOutOfBounds usage ===" rg "StackOutOfBounds" --type zig -A 2 -B 2 echo -e "\n=== StackInvalidPosition usage ===" rg "StackInvalidPosition" --type zig -A 2 -B 2 echo -e "\n=== MemoryLimitExceeded usage ===" rg "MemoryLimitExceeded" --type zig -A 2 -B 2 # Check if old error names are still being used echo -e "\n=== Checking for old error names that should be updated ===" rg "OutOfBounds|InvalidPosition" --type zig src/evm/stack/ || echo "No old error names found in stack module"Length of output: 25045
New stack error variants are used consistently
Verified that
StackOutOfBoundsandStackInvalidPositionare referenced uniformly across:
• src/evm/stack/stack.zig (all boundary checks use the new variants)
• src/evm/execution/execution_error.zig (both variants are listed and have matching messages)
• src/evm/error_mapping.zig (both map correctly)No lingering references to old error names remain.
src/evm/execution/environment.zig (2)
2-11: LGTM!Import path changes correctly reflect the new directory structure.
156-156: Good simplification of memory operations.The direct calls to
frame.memory.set_data_boundedreduce indirection while maintaining proper error handling through thetryexpression.Also applies to: 309-309
src/evm/vm.zig (4)
2-23: LGTM!Import path updates correctly reflect the new directory structure.
50-50: Verify the need for u32 depth.The change from
u16tou32for the depth field increases the maximum depth from 65,535 to over 4 billion. Given that the EVM typically enforces a maximum call depth of 1024, evenu16provides significant headroom. Is there a specific requirement driving this change?
128-128: Good type rename for clarity.The rename from
RunResulttoInterpretResultbetter describes the purpose of the type and improves code readability.Also applies to: 135-135, 142-142, 183-183, 193-193, 196-196, 199-199, 202-202, 214-214, 228-234
220-220: Nice simplification of PC update logic.The conditional expression is more concise while maintaining the same behavior.
src/evm/error_mapping.zig (4)
8-10: Good documentation update.The simplified documentation clearly describes the module's purpose.
32-42: Good improvement to memory error mapping.The function now correctly maps
MemoryLimitExceededto its corresponding ExecutionError variant instead ofOutOfGas, and theanyerrorparameter provides better flexibility.
52-77: Excellent type safety improvement.Replacing
anytypewith concreteAddress.Addresstype in VM storage functions improves type safety and makes the API clearer.
80-105: Consistent error handling pattern.All memory helper functions now use a uniform pattern of inline error mapping, which improves code consistency and maintainability.
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (81)
Cargo.toml(1 hunks)build.zig(29 hunks)src/address/address.zig(1 hunks)src/block/block.zig(1 hunks)src/compilers/rust_build.zig(5 hunks)src/evm/access_list/access_list.zig(1 hunks)src/evm/access_list/access_list_storage_key.zig(1 hunks)src/evm/context.zig(3 hunks)src/evm/contract/contract.zig(1 hunks)src/evm/contract/eip_7702_bytecode.zig(4 hunks)src/evm/create_result.zig(1 hunks)src/evm/error_mapping.zig(1 hunks)src/evm/evm.zig(3 hunks)src/evm/execution/block.zig(1 hunks)src/evm/execution/control.zig(1 hunks)src/evm/execution/environment.zig(3 hunks)src/evm/execution/log.zig(1 hunks)src/evm/execution/storage.zig(3 hunks)src/evm/execution/system.zig(1 hunks)src/evm/jump_table/jump_table.zig(2 hunks)src/evm/state/evm_log.zig(1 hunks)src/evm/state/state.zig(1 hunks)src/evm/state/storage_key.zig(1 hunks)src/evm/vm.zig(7 hunks)src/rlp/rlp.zig(1 hunks)src/root.zig(1 hunks)src/root_wasm.zig(1 hunks)src/solidity/snail_tracer_test.zig(1 hunks)src/trie/hash_builder.zig(1 hunks)src/trie/hash_builder_complete.zig(1 hunks)src/trie/hash_builder_fixed.zig(1 hunks)src/trie/merkle_trie.zig(1 hunks)src/trie/optimized_branch.zig(1 hunks)src/trie/proof.zig(1 hunks)src/trie/trie.zig(1 hunks)test/evm/error_mapping_test.zig(2 hunks)test/evm/gas/gas_accounting_test.zig(1 hunks)test/evm/integration/arithmetic_flow_test.zig(1 hunks)test/evm/integration/arithmetic_sequences_test.zig(1 hunks)test/evm/integration/basic_sequences_test.zig(1 hunks)test/evm/integration/call_environment_test.zig(1 hunks)test/evm/integration/complex_interactions_test.zig(1 hunks)test/evm/integration/complex_scenarios_test.zig(1 hunks)test/evm/integration/comprehensive_test.zig(1 hunks)test/evm/integration/contract_interaction_test.zig(1 hunks)test/evm/integration/control_flow_test.zig(1 hunks)test/evm/integration/crypto_logging_test.zig(1 hunks)test/evm/integration/edge_cases_test.zig(1 hunks)test/evm/integration/environment_system_test.zig(1 hunks)test/evm/integration/event_logging_test.zig(1 hunks)test/evm/integration/memory_storage_test.zig(1 hunks)test/evm/integration/opcode_integration_test.zig(1 hunks)test/evm/jump_table_test.zig(1 hunks)test/evm/memory_comparison_test.zig(1 hunks)test/evm/memory_limit_test.zig(1 hunks)test/evm/memory_stress_test.zig(1 hunks)test/evm/memory_test.zig(1 hunks)test/evm/opcodes/arithmetic_test.zig(1 hunks)test/evm/opcodes/bitwise_test.zig(1 hunks)test/evm/opcodes/block_test.zig(1 hunks)test/evm/opcodes/comparison_comprehensive_test.zig(1 hunks)test/evm/opcodes/comparison_edge_cases_test.zig(1 hunks)test/evm/opcodes/comparison_test.zig(1 hunks)test/evm/opcodes/control_system_comprehensive_test.zig(1 hunks)test/evm/opcodes/control_test.zig(1 hunks)test/evm/opcodes/create_call_comprehensive_test.zig(1 hunks)test/evm/opcodes/crypto_test.zig(1 hunks)test/evm/opcodes/environment_test.zig(1 hunks)test/evm/opcodes/log_test.zig(1 hunks)test/evm/opcodes/memory_test.zig(1 hunks)test/evm/opcodes/stack_test.zig(1 hunks)test/evm/opcodes/storage_comprehensive_test.zig(1 hunks)test/evm/opcodes/storage_test.zig(1 hunks)test/evm/opcodes/system_test.zig(1 hunks)test/evm/opcodes/test_helpers.zig(1 hunks)test/evm/shared_memory_test.zig(1 hunks)test/evm/stack_batched_test.zig(1 hunks)test/evm/stack_test.zig(3 hunks)test/evm/stack_validation_test.zig(1 hunks)test/evm/static_call_protection_test.zig(1 hunks)test/evm/vm_opcode_test.zig(1 hunks)
✅ Files skipped from review due to trivial changes (66)
- src/evm/access_list/access_list.zig
- src/evm/create_result.zig
- src/root_wasm.zig
- src/evm/state/storage_key.zig
- src/block/block.zig
- src/address/address.zig
- src/evm/access_list/access_list_storage_key.zig
- src/trie/hash_builder_fixed.zig
- src/trie/optimized_branch.zig
- test/evm/memory_stress_test.zig
- test/evm/opcodes/stack_test.zig
- src/trie/hash_builder.zig
- test/evm/opcodes/storage_test.zig
- src/solidity/snail_tracer_test.zig
- test/evm/integration/crypto_logging_test.zig
- test/evm/memory_limit_test.zig
- test/evm/integration/edge_cases_test.zig
- test/evm/opcodes/memory_test.zig
- test/evm/integration/arithmetic_flow_test.zig
- test/evm/shared_memory_test.zig
- test/evm/integration/environment_system_test.zig
- test/evm/opcodes/storage_comprehensive_test.zig
- src/trie/merkle_trie.zig
- test/evm/integration/basic_sequences_test.zig
- test/evm/static_call_protection_test.zig
- test/evm/opcodes/control_test.zig
- test/evm/opcodes/create_call_comprehensive_test.zig
- test/evm/opcodes/test_helpers.zig
- src/rlp/rlp.zig
- test/evm/memory_comparison_test.zig
- Cargo.toml
- test/evm/opcodes/comparison_edge_cases_test.zig
- test/evm/integration/control_flow_test.zig
- test/evm/opcodes/environment_test.zig
- test/evm/integration/opcode_integration_test.zig
- test/evm/stack_batched_test.zig
- test/evm/opcodes/block_test.zig
- test/evm/integration/comprehensive_test.zig
- test/evm/opcodes/crypto_test.zig
- src/evm/state/evm_log.zig
- test/evm/integration/event_logging_test.zig
- test/evm/integration/complex_interactions_test.zig
- test/evm/integration/call_environment_test.zig
- src/trie/trie.zig
- test/evm/opcodes/log_test.zig
- src/trie/hash_builder_complete.zig
- src/trie/proof.zig
- test/evm/integration/arithmetic_sequences_test.zig
- test/evm/stack_validation_test.zig
- test/evm/gas/gas_accounting_test.zig
- test/evm/opcodes/bitwise_test.zig
- src/root.zig
- test/evm/opcodes/control_system_comprehensive_test.zig
- test/evm/opcodes/system_test.zig
- src/compilers/rust_build.zig
- test/evm/jump_table_test.zig
- test/evm/integration/contract_interaction_test.zig
- test/evm/opcodes/arithmetic_test.zig
- test/evm/integration/memory_storage_test.zig
- test/evm/opcodes/comparison_test.zig
- test/evm/opcodes/comparison_comprehensive_test.zig
- test/evm/vm_opcode_test.zig
- src/evm/execution/log.zig
- src/evm/evm.zig
- test/evm/memory_test.zig
- build.zig
🚧 Files skipped from review as they are similar to previous changes (11)
- src/evm/context.zig
- src/evm/state/state.zig
- src/evm/execution/block.zig
- src/evm/execution/system.zig
- src/evm/contract/contract.zig
- src/evm/jump_table/jump_table.zig
- src/evm/execution/control.zig
- src/evm/contract/eip_7702_bytecode.zig
- src/evm/execution/storage.zig
- src/evm/execution/environment.zig
- src/evm/vm.zig
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: CI Checks
- GitHub Check: Nx Cloud - Main Job
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (8)
test/evm/integration/complex_scenarios_test.zig (1)
3-3: LGTM! Clean import path update aligns with reorganization.The import path change from
"evm"to"tevm/evm"correctly reflects the new directory structure as part of the codebase reorganization. All subsequent usage of the imported module remains unchanged, maintaining test functionality.test/evm/stack_test.zig (1)
3-3: Import path correctly updated.The import path change from
"evm"to"tevm/evm"aligns with the new directory structure.test/evm/error_mapping_test.zig (1)
3-3: Import path and error variant correctly updated.Both changes are consistent with the refactoring:
- Import path updated to
"tevm/evm"- Error variant renamed to
Stack.Error.StackUnderflowAlso applies to: 120-120
src/evm/error_mapping.zig (5)
2-6: Import paths correctly updated to reflect new directory structure.The imports now properly reference the reorganized module paths under the new directory layout.
11-19: Clean refactoring of stack_pop with inline error mapping.The inline error translation is more concise than separate mapping functions, and the error variants are correctly updated.
31-42: Good defensive error handling with anyerror type.The generalization to handle any error type with a fallback to
OutOfMemoryis a solid defensive programming practice. The newMemoryLimitExceededmapping is correctly added.
79-105: Well-structured memory helper functions with consistent error mapping.All memory helper functions follow a clean pattern of delegating to the underlying Memory methods and consistently mapping errors through
map_memory_error. This provides good abstraction and error handling.
52-63:✅ Verification successful
Good type safety improvement, but verify error handling coverage.
The change from
anytypeto concreteAddress.Addresstype improves type safety. However, the error handling only catchesOutOfMemory- ensure this covers all possible errors from the storage operations.Let me verify what errors the storage operations can throw:
Also applies to: 66-77
🏁 Script executed:
#!/bin/bash # Search for set_storage and set_transient_storage implementations ast-grep --pattern 'pub fn set_storage($$$) $$ { $$$ }' ast-grep --pattern 'pub fn set_transient_storage($$$) $$ { $$$ }' # Also search in the state module rg "fn set_(transient_)?storage" src/evm/state -A 5Length of output: 1059
Error handling for storage operations is exhaustive
The
set_storageandset_transient_storagemethods onEvmStateboth returnstd.mem.Allocator.Error!void, andstd.mem.Allocator.Errorcurrently only definesOutOfMemory. The existingcatch(err) { switch (err) { OutOfMemory => ExecutionError.Error.OutOfMemory } }covers all possible error cases. No additional error branches are needed—code changes can be approved.
test/evm/stack_test.zig
Outdated
| @@ -42,7 +42,7 @@ test "Stack: basic push and pop operations" { | |||
| try testing.expect(stack.isEmpty()); | |||
|
|
|||
| // Pop from empty stack should error | |||
| try testing.expectError(Stack.Error.Underflow, stack.pop()); | |||
| try testing.expectError(Stack.Error.StackUnderflow, stack.pop()); | |||
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Error variant names correctly updated.
The changes from Stack.Error.Underflow to Stack.Error.StackUnderflow are consistent with the renamed stack error variants.
I notice that other stack error variants like Stack.Error.OutOfBounds, Stack.Error.Overflow, and Stack.Error.InvalidPosition haven't been renamed with the Stack prefix. Let me verify if this is intentional:
Also applies to: 457-457
🏁 Script executed:
#!/bin/bash
# Check all stack error variant definitions and usage patterns
ast-grep --pattern 'Error = error {
$$$
}'
# Also search for all Stack.Error usage to see the full pattern
rg "Stack\.Error\." -A 1 -B 1Length of output: 7690
🏁 Script executed:
#!/usr/bin/env bash
# Locate the definition of the Stack.Error enum to confirm which variants exist
set -e
# Search for the Error declaration in Zig files
rg -n --include "*.zig" "pub const Error = error" -A3 -B3
rg -n --include "*.zig" "const Error = error" -A3 -B3Length of output: 510
🏁 Script executed:
#!/usr/bin/env bash
set -e
# Search for all error enum definitions in Zig files
rg -n -g '*.zig' 'Error = error' -A5 -B5Length of output: 13235
Ensure consistent prefixing of all Stack.Error variants
The rename from Underflow to StackUnderflow is correct, but other variants are referenced inconsistently across the codebase. Please align all uses to match the enum declared in src/evm/stack/stack.zig (StackOverflow, StackUnderflow, StackOutOfBounds, StackInvalidPosition):
• src/evm/stack/stack.zig
– Verify the enum reads:
zig pub const Error = error{ StackOverflow, StackUnderflow, StackOutOfBounds, StackInvalidPosition, };
• src/evm/error_mapping.zig
– Already matches the Stack* prefixes. No change needed here.
• test/evm/stack_batched_test.zig
- Change all
Stack.Error.OutOfBounds→Stack.Error.StackOutOfBounds - Change
Stack.Error.Overflow→Stack.Error.StackOverflow
• test/evm/stack_test.zig
- Update
Stack.Error.Overflow→Stack.Error.StackOverflow - Update all
Stack.Error.OutOfBounds→Stack.Error.StackOutOfBounds - Update
Stack.Error.InvalidPosition→Stack.Error.StackInvalidPosition
• test/evm/error_mapping_test.zig
- Change
Stack.Error.Overflow→Stack.Error.StackOverflow - Change
Stack.Error.OutOfBounds→Stack.Error.StackOutOfBounds - Change
Stack.Error.InvalidPosition→Stack.Error.StackInvalidPosition
Aligning these references will ensure the code compiles and that all stack error variants consistently carry the Stack prefix.
🤖 Prompt for AI Agents
In test/evm/stack_test.zig around line 45, update all references to stack error
variants to include the Stack prefix consistently. Change Stack.Error.Overflow
to Stack.Error.StackOverflow, Stack.Error.OutOfBounds to
Stack.Error.StackOutOfBounds, and Stack.Error.InvalidPosition to
Stack.Error.StackInvalidPosition to match the enum declared in
src/evm/stack/stack.zig. Apply similar updates in
test/evm/stack_batched_test.zig and test/evm/error_mapping_test.zig as described
to ensure uniform naming across the codebase.
src/evm/error_mapping.zig
Outdated
| /// Push a value to the stack with proper error mapping | ||
| pub fn stack_push(stack: *Stack, value: u256) ExecutionError.Error!void { | ||
| return stack.append(value) catch |err| switch (err) { | ||
| Stack.Error.StackOverflow => ExecutionError.Error.StackOverflow, | ||
| Stack.Error.StackUnderflow => ExecutionError.Error.StackUnderflow, | ||
| Stack.Error.StackOutOfBounds => ExecutionError.Error.StackUnderflow, | ||
| Stack.Error.StackInvalidPosition => ExecutionError.Error.StackUnderflow, | ||
| }; | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify error handling in stack_push.
The inline error mapping is good, but handling StackUnderflow, StackOutOfBounds, and StackInvalidPosition errors in a push operation seems unnecessary. The append() function should only throw StackOverflow.
Let me verify what errors the Stack.append() function can actually throw:
🏁 Script executed:
#!/bin/bash
# Search for the append function definition in Stack
ast-grep --pattern 'pub fn append($$$) $$ {
$$$
}'
# Also check for append_unsafe and any error returns in stack implementation
rg "fn append" src/evm/stack -A 10Length of output: 1377
Simplify stack_push error mapping to only handle StackOverflow
Stack.append in src/evm/stack/stack.zig only returns Error.StackOverflow. The other cases (StackUnderflow, StackOutOfBounds, StackInvalidPosition) can never occur on a push. Please remove the redundant arms:
• File: src/evm/error_mapping.zig
Lines: ~21–29
Proposed change:
pub fn stack_push(stack: *Stack, value: u256) ExecutionError.Error!void {
- return stack.append(value) catch |err| switch (err) {
- Stack.Error.StackOverflow => ExecutionError.Error.StackOverflow,
- Stack.Error.StackUnderflow => ExecutionError.Error.StackUnderflow,
- Stack.Error.StackOutOfBounds => ExecutionError.Error.StackUnderflow,
- Stack.Error.StackInvalidPosition => ExecutionError.Error.StackUnderflow,
- };
+ return stack.append(value) catch |Stack.Error.StackOverflow| =>
+ ExecutionError.Error.StackOverflow;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| /// Push a value to the stack with proper error mapping | |
| pub fn stack_push(stack: *Stack, value: u256) ExecutionError.Error!void { | |
| return stack.append(value) catch |err| switch (err) { | |
| Stack.Error.StackOverflow => ExecutionError.Error.StackOverflow, | |
| Stack.Error.StackUnderflow => ExecutionError.Error.StackUnderflow, | |
| Stack.Error.StackOutOfBounds => ExecutionError.Error.StackUnderflow, | |
| Stack.Error.StackInvalidPosition => ExecutionError.Error.StackUnderflow, | |
| }; | |
| } | |
| /// Push a value to the stack with proper error mapping | |
| pub fn stack_push(stack: *Stack, value: u256) ExecutionError.Error!void { | |
| return stack.append(value) catch |Stack.Error.StackOverflow| => ExecutionError.Error.StackOverflow; | |
| } |
🤖 Prompt for AI Agents
In src/evm/error_mapping.zig around lines 21 to 29, the stack_push function's
error handling includes cases for StackUnderflow, StackOutOfBounds, and
StackInvalidPosition, which cannot occur during a push operation as Stack.append
only throws StackOverflow. Simplify the error mapping by removing these
redundant error cases and only handle StackOverflow in the catch block.
Description
This PR reorganizes the EVM codebase into a more structured directory layout, grouping related files into logical modules. Key changes include:
/access_listfor access list related files/constantsfor EVM constants/contractfor contract-related code/executionfor opcode implementations (renamed from/opcodes)/hardforksfor hardfork specifications/jump_tablefor opcode mapping/opcodesfor opcode definitions/stackfor stack implementation/statefor EVM state managementRunResulttoInterpretResultfor clarityTesting
All existing tests continue to pass with the new directory structure. The code changes are purely organizational and don't modify any functional behavior.
Additional Information
Your ENS/address:
Summary by CodeRabbit