feat: Add EVM execution frame #1665
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
1 Skipped Deployment
|
|
|
""" WalkthroughThe changes introduce a modular EVM execution framework in Zig by adding a new Changes
Sequence Diagram(s)sequenceDiagram
participant Test as Test Suite
participant EVM as Evm
participant Frame as Frame
participant State as StateManager
Test->>EVM: init(allocator, stateManager)
Test->>EVM: execute(input)
EVM->>State: loadCode(address)
EVM->>Frame: createFrame(input, code, depth)
Frame->>State: checkpoint()
EVM->>Frame: execute(stateManager)
Frame-->>EVM: FrameResult
EVM-->>Test: FrameResult
Poem
Note ⚡️ AI Code Reviews for VS Code, Cursor, WindsurfCodeRabbit now has a plugin for VS Code, Cursor and Windsurf. This brings AI code reviews directly in the code editor. Each commit is reviewed immediately, finding bugs before the PR is raised. Seamless context handoff to your AI code agent ensures that you can easily incorporate review feedback. Note ⚡️ Faster reviews with cachingCodeRabbit now supports caching for code and dependencies, helping speed up reviews. This means quicker feedback, reduced wait times, and a smoother review experience overall. Cached data is encrypted and stored securely. This feature will be automatically enabled for all accounts on May 16th. To opt out, configure 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (6)
✅ Files skipped from review due to trivial changes (3)
⏰ Context from checks skipped due to timeout of 90000ms (6)
🔇 Additional comments (7)
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 (
|
There was a problem hiding this comment.
Actionable comments posted: 10
🧹 Nitpick comments (7)
src/Evm/log_config.zig (2)
20-23:std.time.milliTimestamp()is called twice – minor performance & skew issueYou compute
millisandsecondsfrom two independent timestamp calls. In fast-moving loops that could yield inconsistent values (e.g., seconds incremented but millis from previous second).-const millis = std.time.milliTimestamp() % 1000; -const seconds = @divFloor(std.time.milliTimestamp(), 1000); +const ts = std.time.milliTimestamp(); +const millis = ts % 1000; +const seconds = @divFloor(ts, 1000);
26-35: Fixed-width padding{d:4}.{d:3}may truncate after ~9999 sWhen long-running processes exceed ~2 h 46 m, the seconds component will overflow the
4-digit field and mis-align the log. Consider dynamic width or a larger field.src/Evm/frame.zig (3)
107-114: Out-of-bounds load silently returns empty sliceSilently returning
[]whenoffset >= lenhides logic bugs and diverges from EVM
behaviour (which returns zeroes but still charges gas).
Consider returning a slice of zeroes with the requested length, or at least an error to make bugs visible in tests.
131-137:pushappends after overflow check but does not cap lengthThe check
self.data.items.len >= 1024is correct, yet in case an external caller
manipulatesdata.items.lendirectly the guarantee vanishes.
Prefer encapsulating the stack list (e.g. makedataprivate and expose onlypush/pop)
or assert afterappendthatlen <= 1024.
138-143: Redundant nil-coalescing inpopYou already guard against
len == 0;pop()will therefore never returnnull.
Theorelseis unreachable:- return self.data.pop() orelse return error.StackUnderflow; + return self.data.pop();src/Evm/frame_test.zig (2)
127-140: Memory zero-initialisation not assertedGiven the earlier note about
Memory.store, adding aloadfrom an untouched
offset (e.g.memory.load(10, 4)) and expecting four zero bytes would catch
regressions in zero-fill behaviour.
170-176: Test only checks status, not gas usage nor refundsVerifying
gasUsedandgasRefundedfields would strengthen the regression
suite once gas accounting is implemented.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge Base: Disabled due to data retention organization setting
📒 Files selected for processing (5)
build.zig(2 hunks)src/Evm/evm.zig(2 hunks)src/Evm/frame.zig(1 hunks)src/Evm/frame_test.zig(1 hunks)src/Evm/log_config.zig(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: Nx Cloud Agents (4)
- GitHub Check: Nx Cloud Agents (6)
- GitHub Check: Nx Cloud Agents (5)
- GitHub Check: Nx Cloud Agents (2)
- GitHub Check: Nx Cloud Agents (3)
- GitHub Check: Nx Cloud Agents (1)
🔇 Additional comments (3)
build.zig (1)
38-40: Verify thatexe_modandwasm_modcan resolveAddress
zigevm_modgets theAddressimport, but any code pulled in bymain.zigor the WASM root that directly uses@import("Address")will fail to compile becauseexe_mod/wasm_moddo not know about it.
If those roots transitively depend onAddressonly throughzigevm_mod, you’re fine; otherwise add:exe_mod.addImport("Address", address_mod); wasm_mod.addImport("Address", address_mod);src/Evm/frame.zig (1)
299-301:gasUsedis always zero
state.gas.remainingis never decremented, sogasUsedis constant.
Even a placeholder interpreter should account for at least base cost of the executed
instruction to avoid misleading test coverage.src/Evm/frame_test.zig (1)
102-104: Undeclared identifieru256in test scope
u256is used without anusingnamespace std;or explicit
const u256 = @import("std").meta.Int(.unsigned, 256);.
If another file does not alreadypub-export this alias, the test will fail
to compile.
build.zig
Outdated
| const frame_test_step = b.step("test-frame", "Run EVM frame tests"); | ||
| frame_test_step.dependOn(&run_frame_test.step); | ||
|
|
||
| // Add a test for evm.zig | ||
| const evm_test = b.addTest(.{ | ||
| .name = "evm-test", | ||
| .root_source_file = b.path("src/Evm/evm.zig"), | ||
| .target = target, | ||
| .optimize = optimize, | ||
| }); | ||
|
|
||
| // Add the Address module to evm_test | ||
| evm_test.root_module.addImport("Address", address_mod); | ||
|
|
||
| const run_evm_test = b.addRunArtifact(evm_test); | ||
|
|
||
| // Add a separate step for testing the EVM | ||
| const evm_test_step = b.step("test-evm", "Run EVM tests"); | ||
| evm_test_step.dependOn(&run_evm_test.step); | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
New test steps are not wired into the umbrella test step
You correctly created test-frame and test-evm, but the default zig build test target (the test_step) does not depend on either. CI users running the canonical command will silently skip the new suites.
test_step.dependOn(&run_lib_unit_tests.step);
test_step.dependOn(&run_exe_unit_tests.step);
+test_step.dependOn(&frame_test_step.step);
+test_step.dependOn(&evm_test_step.step);📝 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.
| const frame_test_step = b.step("test-frame", "Run EVM frame tests"); | |
| frame_test_step.dependOn(&run_frame_test.step); | |
| // Add a test for evm.zig | |
| const evm_test = b.addTest(.{ | |
| .name = "evm-test", | |
| .root_source_file = b.path("src/Evm/evm.zig"), | |
| .target = target, | |
| .optimize = optimize, | |
| }); | |
| // Add the Address module to evm_test | |
| evm_test.root_module.addImport("Address", address_mod); | |
| const run_evm_test = b.addRunArtifact(evm_test); | |
| // Add a separate step for testing the EVM | |
| const evm_test_step = b.step("test-evm", "Run EVM tests"); | |
| evm_test_step.dependOn(&run_evm_test.step); | |
| test_step.dependOn(&run_lib_unit_tests.step); | |
| test_step.dependOn(&run_exe_unit_tests.step); | |
| test_step.dependOn(&frame_test_step.step); | |
| test_step.dependOn(&evm_test_step.step); |
🤖 Prompt for AI Agents
In build.zig around lines 128 to 147, the new test steps "test-frame" and
"test-evm" are created but not linked to the main umbrella test step. To fix
this, add dependencies from the main test step to both frame_test_step and
evm_test_step so that running the default "zig build test" will include these
new test suites.
| if (input == .Create) { | ||
| if (frameResult == .Call) { | ||
| log.debug("Converting Call result to Create result", .{}); | ||
| const callResult = frameResult.Call; | ||
| return frame.FrameResult{ .Create = .{ | ||
| .status = callResult.status, | ||
| .returnData = callResult.returnData, | ||
| .gasUsed = callResult.gasUsed, | ||
| .gasRefunded = callResult.gasRefunded, | ||
| .createdAddress = null, | ||
| } }; | ||
| } | ||
| } else if (input == .Call) { | ||
| if (frameResult == .Create) { |
There was a problem hiding this comment.
Illegal union comparisons: input == .Create / frameResult == .Call
Both FrameInput and FrameResult variants carry payloads; direct equality with a tag literal is only allowed for tag-only unions. Use input == .Create only if .Create has no payload.
Correct pattern:
if (input.tag == .Create) { ... }or switch on the tag:
switch (input) {
.Create => |payload| { ... }
.Call => |payload| { ... }
}Same issue exists for frameResult == .Call and callInput == .Call.
🤖 Prompt for AI Agents
In src/Evm/evm.zig around lines 170 to 183, the code incorrectly compares union
values directly to tagged literals like `input == .Create` and `frameResult ==
.Call`, which is invalid for unions with payloads. To fix this, replace these
comparisons with checks on the `.tag` field, such as `input.tag == .Create` and
`frameResult.tag == .Call`, or use a switch statement on the union to handle
each variant properly. This ensures correct and legal union variant checking.
| // Create checkpoint for state changes | ||
| const checkpoint = self.stateManager.checkpoint(); | ||
|
|
||
| // Initialize and return the frame | ||
| return try frame.Frame.init(self.allocator, input, code, depth, checkpoint); | ||
| } |
There was a problem hiding this comment.
StateManager.checkpoint is invoked but not defined
frame.StateManager in the provided context only contains three hash-maps. Calling .checkpoint() will fail at compile time unless an extension method exists elsewhere.
If checkpointing is still on the roadmap, stub it in StateManager:
pub fn checkpoint(self: *StateManager) Checkpoint {
// todo: implement proper snapshotting
return Checkpoint{};
}and ensure Checkpoint is a concrete type accepted by Frame.init.
🤖 Prompt for AI Agents
In src/Evm/evm.zig around lines 119 to 124, the call to
self.stateManager.checkpoint() fails because the checkpoint method is not
defined in StateManager. To fix this, add a stub checkpoint method to the
StateManager struct that returns a concrete Checkpoint type, for example by
defining a pub fn checkpoint(self: *StateManager) Checkpoint that returns an
empty or default Checkpoint instance. Also ensure that the Checkpoint type is
properly defined and compatible with the Frame.init method's expected argument.
| // Mock code for testing | ||
| stateManager.mockCode = &[_]u8{0x00}; // STOP opcode | ||
|
|
There was a problem hiding this comment.
mockCode field not present on StateManager – test won’t compile
The unit test sets stateManager.mockCode = &[_]u8{0x00}; but StateManager does not declare such a member.
Quickest path: extend the struct with a mockCode slice guarded by std.testing or provide a real loadCode implementation that returns hard-coded STOP when empty.
🤖 Prompt for AI Agen E864 ts
In src/Evm/evm.zig around lines 247 to 249, the StateManager struct lacks a
mockCode field, causing the test to fail compilation. To fix this, add a
mockCode slice field to the StateManager struct, but only include it when
compiling tests by guarding it with std.testing. Alternatively, implement the
loadCode method to return a hard-coded STOP opcode when no code is present,
removing the need for mockCode.
| var code: frame.Bytes = undefined; | ||
|
|
||
| // Get the code based on frame input type | ||
| switch (input) { | ||
| .Call => |call| { | ||
| // For a call, load the code from the target address | ||
| log.debug("Loading code for address: {any}", .{call.codeAddress}); | ||
| code = try self.stateManager.loadCode(call.codeAddress); | ||
| log.debug("Call input - Gas Limit: {d}, Code length: {d}", .{ call.gasLimit, code.len }); | ||
| }, | ||
| .Create => |create| { |
There was a problem hiding this comment.
loadCode missing on StateManager
self.stateManager.loadCode(call.codeAddress) assumes a helper that does not exist in the declared struct. Either implement:
pub fn loadCode(self: *StateManager, addr: address.Address) !frame.Bytes { ... }or move code retrieval logic elsewhere.
🤖 Prompt for AI Agents
In src/Evm/evm.zig around lines 130 to 140, the call to
self.stateManager.loadCode does not compile because the loadCode method is
missing from the StateManager struct. To fix this, implement a loadCode method
on StateManager with the signature pub fn loadCode(self: *StateManager, addr:
address.Address) !frame.Bytes that retrieves and returns the code bytes for the
given address. Alternatively, if code retrieval belongs elsewhere, move this
logic out of StateManager and adjust the call accordingly.
| // In a real implementation, we would recursively handle the call | ||
| // but for now we'll just return a success result | ||
| log.debug("Got Call result", .{}); | ||
| if (callInput == .Call) { | ||
| log.debug("Call type: Call", .{}); | ||
| return frame.FrameResult{ .Call = .{ | ||
| .status = .Success, | ||
| .returnData = &[_]u8{}, | ||
| .gasUsed = 0, | ||
| .gasRefunded = 0, | ||
| } }; | ||
| } else { | ||
| log.debug("Call type: Create", .{}); | ||
| return frame.FrameResult{ .Create = .{ | ||
| .status = .Success, | ||
| .returnData = &[_]u8{}, | ||
| .gasUsed = 0, | ||
| .gasRefunded = 0, | ||
| .createdAddress = null, | ||
| } }; | ||
| } | ||
| }, | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Logic inside .Call => |callInput| mis-uses the payload
Inside this branch callInput is already the payload (not the union). Comparing it to .Call or .Create is nonsensical.
Moreover the function currently fabricates a success result instead of performing recursion; consider returning result unchanged until full recursion is implemented, to avoid discarding gas accounting & returndata.
-.Call => |callInput| {
- // placeholder logic
- ...
-}
+else => |nested| {
+ // TODO: recursive execution of nested frames
+ return nested;
+}Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In src/Evm/evm.zig around lines 198 to 220, the code incorrectly compares
callInput, which is already the payload, to .Call or .Create, which is invalid.
Instead of fabricating a success result, modify the logic to return the existing
result unchanged to preserve gas accounting and return data until full recursion
is implemented.
| pub fn store(self: *Memory, offset: usize, value: []const u8) !void { | ||
| // Expand memory if needed | ||
| const requiredSize = offset + value.len; | ||
| if (requiredSize > self.data.items.len) { | ||
| try self.data.resize(requiredSize); | ||
| } | ||
|
|
||
| @memcpy(self.data.items[offset..offset+value.len], value); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Memory expansion leaves bytes un-initialised
ArrayList.resize does not zero-initialise the newly allocated region.
EVM memory, however, must be zero-filled when first expanded (see Yellow-Paper §9.4 Memory).
Reading from an uninitialised segment after a store at a sparse offset will therefore return
garbage instead of 0x00.
- if (requiredSize > self.data.items.len) {
- try self.data.resize(requiredSize);
- }
+ if (requiredSize > self.data.items.len) {
+ const old_len = self.data.items.len;
+ try self.data.resize(requiredSize);
+ // Zero-fill the gap [old_len .. requiredSize)
+ @memset(self.data.items[old_len..requiredSize], 0);
+ }(This uses @memset; alternatively std.mem.set(u8, slice, 0).)
📝 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.
| pub fn store(self: *Memory, offset: usize, value: []const u8) !void { | |
| // Expand memory if needed | |
| const requiredSize = offset + value.len; | |
| if (requiredSize > self.data.items.len) { | |
| try self.data.resize(requiredSize); | |
| } | |
| @memcpy(self.data.items[offset..offset+value.len], value); | |
| } | |
| pub fn store(self: *Memory, offset: usize, value: []const u8) !void { | |
| // Expand memory if needed | |
| const requiredSize = offset + value.len; | |
| if (requiredSize > self.data.items.len) { | |
| const old_len = self.data.items.len; | |
| try self.data.resize(requiredSize); | |
| // Zero-fill the gap [old_len .. requiredSize) | |
| @memset(self.data.items[old_len..requiredSize], 0); | |
| } | |
| @memcpy(self.data.items[offset..offset+value.len], value); | |
| } |
🤖 Prompt for AI Agents
In src/Evm/frame.zig around lines 97 to 105, the memory expansion in the store
function does not zero-initialize the newly allocated bytes, which violates
EVM's requirement for zero-filled memory on expansion. After resizing the data
array, explicitly zero out the newly added region using @memset or std.mem.set
to ensure all new bytes are initialized to 0 before copying the value.
| // Simple implementation for initial tests | ||
| // Just check for basic opcodes and handle them | ||
| log.debug("Starting execution", .{}); | ||
|
|
||
| if (self.code.len == 0) { | ||
| log.debug("Empty code, returning success", .{}); | ||
| // Empty code returns success | ||
| return FrameOrCall{ | ||
| .Result = FrameResult{ | ||
| .Call = .{ | ||
| .status = .Success, | ||
| .returnData = &[_]u8{}, | ||
| .gasUsed = self.input.getGasLimit() - self.state.gas.remaining, | ||
| .gasRefunded = self.state.gas.refunded, | ||
| } | ||
| } | ||
| }; | ||
| } | ||
|
|
||
| log.debug("Code length is {d}, first byte: {x:0>2}", .{self.code.len, self.code[0]}); | ||
|
|
||
| // In a real implementation, we would loop through the bytecode and interpret opcodes | ||
| // but for this simplified version, we'll just check for STOP (0x00) | ||
| if (self.code[0] == 0x00) { | ||
| log.debug("Found STOP opcode, returning success", .{}); | ||
| // STOP opcode | ||
| return FrameOrCall{ | ||
| .Result = FrameResult{ | ||
| .Call = .{ | ||
| .status = .Success, | ||
| .returnData = &[_]u8{}, | ||
| .gasUsed = self.input.getGasLimit() - self.state.gas.remaining, | ||
| .gasRefunded = self.state.gas.refunded, | ||
| } | ||
| } | ||
| }; | ||
| } | ||
|
|
||
| log.debug("Unknown opcode, returning InvalidOpcode", .{}); | ||
| // Default response for other opcodes (not yet implemented) | ||
| return FrameOrCall{ | ||
| .Result = FrameResult{ | ||
| .Call = .{ | ||
| .status = .InvalidOpcode, | ||
| .returnData = &[_]u8{}, | ||
| .gasUsed = self.input.getGasLimit() - self.state.gas.remaining, | ||
| .gasRefunded = self.state.gas.refunded, | ||
| } | ||
| } | ||
| }; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
execute always tags the result as .Call
For FrameInput.Create you still return a .Call result, which makes the caller
mis-interpret the outcome and loses the createdAddress field.
- return FrameOrCall{
- .Result = FrameResult{
- .Call = .{
+ const tag: FrameResult = switch (self.input) {
+ .Create => .{
+ .Create = .{
+ .status = .Success,
+ .returnData = &[_]u8{},
+ .gasUsed = self.input.getGasLimit() - self.state.gas.remaining,
+ .gasRefunded = self.state.gas.refunded,
+ .createdAddress = null, // TODO: compute address
+ },
+ },
+ else => .{
+ .Call = .{
.status = .Success,
.returnData = &[_]u8{},
.gasUsed = self.input.getGasLimit() - self.state.gas.remaining,
.gasRefunded = self.state.gas.refunded,
}
- }
- };
+ },
+ };
+
+ return .{ .Result = tag };Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In src/Evm/frame.zig between lines 268 and 318, the execute function always
returns a result tagged as .Call, even when the FrameInput is of type Create.
This causes the caller to misinterpret the outcome and lose the createdAddress
field. To fix this, modify the return statements to check if the input is a
Create operation and return a FrameResult tagged as .Create with the appropriate
createdAddress field set; otherwise, return .Call as before. Ensure that the
result type matches the input type to preserve all relevant data.
| // Push and pop values | ||
| try stack.push(123); | ||
| try stack.push(456); | ||
|
|
||
| try testing.expectEqual(@as(u256, 456), try stack.pop()); | ||
| try testing.expectEqual(@as(u256, 123), try stack.pop()); | ||
|
|
||
| // Test stack underflow | ||
| try testing.expectError(error.StackUnderflow, stack.pop()); | ||
|
|
||
| // Push many values to test overflow | ||
| var i: usize = 0; | ||
| while (i < 1024) : (i += 1) { | ||
| try stack.push(i); | ||
| } | ||
|
|
||
| // Test stack overflow | ||
| try testing.expectError(error.StackOverflow, stack.push(1025)); | ||
| } |
There was a problem hiding this comment.
usize → u256 conversion required in loop
i is usize; implicit widening to u256 is disallowed for run-time values, causing a
compile error.
- try stack.push(i);
+ try stack.push(@as(u256, i));Alternatively, declare var i: u256 = 0.
📝 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 and pop values | |
| try stack.push(123); | |
| try stack.push(456); | |
| try testing.expectEqual(@as(u256, 456), try stack.pop()); | |
| try testing.expectEqual(@as(u256, 123), try stack.pop()); | |
| // Test stack underflow | |
| try testing.expectError(error.StackUnderflow, stack.pop()); | |
| // Push many values to test overflow | |
| var i: usize = 0; | |
| while (i < 1024) : (i += 1) { | |
| try stack.push(i); | |
| } | |
| // Test stack overflow | |
| try testing.expectError(error.StackOverflow, stack.push(1025)); | |
| } | |
| // Push many values to test overflow | |
| var i: usize = 0; | |
| while (i < 1024) : (i += 1) { | |
| try stack.push(@as(u256, i)); | |
| } | |
| // Test stack overflow | |
| try testing.expectError(error.StackOverflow, stack.push(1025)); | |
| } |
🤖 Prompt for AI Agents
In src/Evm/frame_test.zig around lines 98 to 116, the loop variable i is
declared as usize but is used where a u256 is expected, causing a compile error
due to disallowed implicit widening. Fix this by changing the declaration of i
from usize to u256 to match the expected type and avoid the conversion error.
Description
Implemented the EVM execution frame system, which is a core component of the Ethereum Virtual Machine. This PR adds:
Framemodule that represents a single execution context in the EVMThe implementation supports both contract calls and contract creation, with proper handling of execution results and gas accounting.
Testing
Additional Information
Your ENS/address:
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Chores