release/v1.1.0: osaka evm support, bug fixes#373
Merged
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughSwitches EVM spec from PRAGUE to OSAKA, tightens rWasm compile and resume error handling to return Results instead of panicking, adds EXT_CODE_COPY_MAX_COPY_SIZE validation, consolidates memory reads into Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 12
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
crates/runtime/src/context_wrapper.rs (1)
83-93:⚠️ Potential issue | 🟠 MajorAvoid panic path in
resumeerror handling.Line 92 uses
unwrap_exit_code(), and percrates/types/src/exit_code.rs(Lines 151-163) that panics onErr. This can turnExitCode::RootCallOnlyinto a host panic instead of a recoverable return value.Suggested non-panicking mapping
- let (fuel_consumed, fuel_refunded, exit_code) = syscall_resume_impl( + let resume_result = syscall_resume_impl( &mut self.ctx.borrow_mut(), call_id, return_data, exit_code, fuel_consumed, fuel_refunded, 0, - ) - .unwrap_exit_code(); - (fuel_consumed, fuel_refunded, exit_code) + ); + match resume_result { + Ok((next_fuel_consumed, next_fuel_refunded, next_exit_code)) => { + (next_fuel_consumed, next_fuel_refunded, next_exit_code) + } + Err(err) => (fuel_consumed, fuel_refunded, err.into_i32()), + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/runtime/src/context_wrapper.rs` around lines 83 - 93, The code calls syscall_resume_impl(...).unwrap_exit_code(), which can panic on Err and convert recoverable errors like ExitCode::RootCallOnly into a host panic; instead, handle the Result without unwrapping: match on the syscall_resume_impl(...) return value (or use .map_or_else) and on Ok((fuel_consumed, fuel_refunded, exit_code)) return those, and on Err(err) extract the error as an ExitCode (or convert it) and return (fuel_consumed, fuel_refunded, err_exit_code) so you never call unwrap_exit_code(); update the call site in resume (the code using syscall_resume_impl and unwrap_exit_code) to perform this non-panicking match and propagate the error as an ExitCode.evm-e2e/src/tests.rs (1)
20-3227:⚠️ Potential issue | 🟠 MajorRestore or update missing JSON fixtures referenced by evm-e2e/src/tests.rs
evm-e2e/src/tests.rs references a large number of JSON fixtures that are not present at the referenced paths (examples: tests/GeneralStateTests/Cancun/stEIP1153-transientStorage/01_tloadBeginningTxn.json, tests/GeneralStateTests/Pyspecs/cancun/eip4844_blobs/invalid_precompile_calls.json). Restore the missing fixture files under tests/... or update evm-e2e/src/tests.rs to the correct paths/casing so the test suite can locate them.
Location: evm-e2e/src/tests.rs
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@evm-e2e/src/tests.rs` around lines 20 - 3227, The test file evm-e2e/src/tests.rs declares many fixture-based tests via the define_tests! macro but references JSON fixtures that are missing or mis-cased (e.g. fn _01_tload_beginning_txn and fn invalid_precompile_calls); fix by either restoring the missing JSON files under the tests/... tree with the exact filenames/casing used here or update the function entries in evm-e2e/src/tests.rs (inside the define_tests! blocks) to point to the correct existing paths/names for those fixtures (ensure names like _01_tload_beginning_txn, invalid_precompile_calls and other listed fn entries exactly match the restored file names). Ensure consistent casing and that every fn "<name>("<path>")" entry corresponds to an actual file on disk.
🧹 Nitpick comments (9)
e2e/src/bridge.rs (1)
452-468: Consider extracting bytecode generation into a helper function.The log-emitting bytecode construction pattern is duplicated across multiple tests (this test, lines 396-412, lines 508-524, etc.). A helper function like
fn build_log1_emit_bytecode(log_data: &[u8], signature_hash: &[u8]) -> Vec<u8>could reduce duplication and make tests easier to maintain.♻️ Example helper extraction
fn build_log1_emit_bytecode(log_data: Vec<u8>, signature_hash: &[u8; 32]) -> Vec<u8> { let mut bytecode = Vec::new(); let log_data_offset: u8 = 6 + 2 + 32 + 3 + 1; // CODECOPY + LOG1 setup + STOP // CODECOPY: copy log_data to memory bytecode.push(opcode::PUSH1); bytecode.push(u8::try_from(log_data.len()).unwrap()); bytecode.push(opcode::PUSH1); bytecode.push(log_data_offset); bytecode.push(opcode::PUSH0); bytecode.push(opcode::CODECOPY); // LOG1: emit event bytecode.push(opcode::PUSH32); bytecode.extend_from_slice(signature_hash); bytecode.push(opcode::PUSH1); bytecode.push(u8::try_from(log_data.len()).unwrap()); bytecode.push(opcode::PUSH0); bytecode.push(opcode::LOG1); bytecode.push(opcode::STOP); assert_eq!(bytecode.len(), log_data_offset as usize); bytecode.extend(log_data); bytecode }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e/src/bridge.rs` around lines 452 - 468, Extract the duplicated log-emitting bytecode construction into a helper fn build_log1_emit_bytecode(log_data: &[u8], signature_hash: &[u8]) -> Vec<u8> and replace the inline sequences (the block that builds bytecode using LOG_DATA_OFFSET, opcode::PUSH1, opcode::CODECOPY, opcode::PUSH32, ReceivedMessage::SIGNATURE_HASH, opcode::LOG1, opcode::STOP, and the final extend(log_data)) with calls to that helper; inside the helper compute the same offset (6 + 2 + 32 + 3 + 1), perform the same PUSH/CODECOPY/LOG1/STOP sequence, assert the header length equals the offset, then extend the payload and return the Vec<u8>, and update all duplicate sites (including where ReceivedMessage::SIGNATURE_HASH is used) to call this function.evm-e2e/Makefile (1)
3-6: Git checkout sequence may fail on fresh clone.The
git pullbeforegit checkoutto a specific commit may fail if the default branch doesn't contain that commit, or if there are merge conflicts. Consider reordering:Proposed fix
if [ ! -d tests ]; then \ git clone https://github.com/ethereum/tests; \ - cd tests; git pull; git checkout faf33b471465d3c6cdc3d04fbd690895f78d33f2; \ + cd tests && git fetch origin && git checkout faf33b471465d3c6cdc3d04fbd690895f78d33f2; \ fi🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@evm-e2e/Makefile` around lines 3 - 6, The clone/checkout sequence in the Makefile can fail because it runs git pull before checking out a specific commit; change the shell block so after cloning you cd into the tests repo, run git fetch --all --tags (or git fetch origin) and then git checkout faf33b471465d3c6cdc3d04fbd690895f78d33f2; remove the unconditional git pull or only run it when the repo already exists, ensuring the checkout happens from the fetched refs so the specific commit is present even on a fresh clone.contracts/wasm/lib.rs (1)
13-14: Good improvement: panic replaced with proper error handling.Converting compilation failures from
.unwrap()to a properExitCodeis the right approach. The mapping toMalformedBuiltinParamsaligns with the downstream conversion ininstruction_result_from_exit_code.Consider logging the original error before discarding it, which would help with debugging malformed WASM inputs in non-production environments:
Optional: preserve error details for debugging
- let (result, constructor_params) = - RwasmModule::compile(config, &wasm_binary).map_err(|_| ExitCode::MalformedBuiltinParams)?; + let (result, constructor_params) = RwasmModule::compile(config, &wasm_binary).map_err(|e| { + #[cfg(feature = "debug-print")] + sdk.debug_log(&alloc::format!("WASM compilation failed: {:?}", e)); + ExitCode::MalformedBuiltinParams + })?;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@contracts/wasm/lib.rs` around lines 13 - 14, The compile error from RwasmModule::compile is being mapped to ExitCode::MalformedBuiltinParams and the original error is discarded; change the call to capture the error (e.g., let Err(e) = ...) and log the error before returning the ExitCode so debugging info for malformed WASM is preserved; specifically update the RwasmModule::compile invocation that produces (result, constructor_params) to log the captured error (via your project's logger/tracing) and then map or convert it to ExitCode::MalformedBuiltinParams as currently done.evm-e2e/README.md (1)
1-1: Consider expanding the README with usage instructions.This placeholder README could benefit from basic documentation about:
- What the evm-e2e suite tests
- How to run the tests (e.g.,
make sync_tests,make run_general_state_tests)- Any prerequisites or setup requirements
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@evm-e2e/README.md` at line 1, The README currently only contains a header "# evm-e2e"; expand it to include a short overview of what the evm-e2e suite tests, a Usage section with example commands (e.g., make sync_tests, make run_general_state_tests), and a Prerequisites/Setup section listing required dependencies and any setup steps (e.g., installing toolchains, environment variables, Docker or local node requirements); update the header "# evm-e2e" and add these sections so contributors can quickly understand and run the tests.evm-e2e/src/inspector.rs (2)
196-197: Debugprintln!statements left in code.Multiple
println!calls (lines 196, 229, 262, 267-270) are scattered throughout the function. Since this is marked#[allow(dead_code)]and appears to be for debugging, this may be intentional, but consider usingdebug!ortrace!logging macros if the function is kept long-term.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@evm-e2e/src/inspector.rs` around lines 196 - 197, The code contains leftover debug println! calls in the function annotated with #[allow(dead_code)] (e.g., the println!("Call == Call") and other println! usages around lines shown) which should be replaced with structured logging; update those println! invocations to use the log crate macros (debug! or trace!) instead, ensuring the module imports the log macros (or use tracing's debug!/trace! if project uses tracing) and that the function (and file) initializes/assumes a logger is configured so runtime logging will be controlled by level rather than printing to stdout.
273-276:unreachable!()panics instead of returningErr.The function returns
Result<(), TestError>but usesunreachable!()for mismatched events, causing a panic instead of a proper error return. This makes theResultreturn type misleading.Since the function is marked
#[allow(dead_code)], this may be intentional for debugging purposes, but consider returning a proper error if this function will be used in production.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@evm-e2e/src/inspector.rs` around lines 273 - 276, The match arm currently prints the two events and calls unreachable!(), which panics instead of returning a Result::Err; replace the panic with an appropriate Err(TestError::...) return that contains the mismatch details (include e1 and e2 in the message) so the function returns Err rather than panicking—locate the (_, _) => { eprintln!("\n{:?} == {:?}", e1, e2); unreachable!() } branch in inspector.rs and change it to return Err with a descriptive TestError (e.g., TestError::EventMismatch or another suitable variant), or construct a new TestError variant if none exists.crates/runtime/src/syscall_handler/host/debug_log.rs (1)
18-19: Avoid copying the full debug payload up front.Line 18 still allocates/copies
message_lenbytes before a sink that either truncates to 1,000 bytes or does nothing whendebug-printis off. Consider short-circuiting the handler when debug logging is disabled, or capping the read to the printable prefix, so guest-controlled debug messages do not drive unbounded host work.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/runtime/src/syscall_handler/host/debug_log.rs` around lines 18 - 19, The handler currently calls caller.memory_read_into_vec(message_ptr, message_len) and then syscall_debug_log_impl(&buffer), which copies the entire guest-controlled payload; change this to avoid unbounded reads by first checking whether debug-print is enabled and short-circuiting if not, or by capping the read length to the printable prefix (e.g., max 1000 bytes) before calling memory_read_into_vec; update the code around syscall_debug_log_impl, caller.memory_read_into_vec, message_ptr, and message_len so you only allocate/read the bounded size (or skip reading entirely) and then pass that bounded slice to syscall_debug_log_impl.e2e/src/wasm.rs (1)
454-478: Good coverage for invalid contract interface handling.The test verifies that contracts with malformed interfaces (e.g.,
mainexpects params butdeployhas none) halt withMalformedBuiltinParamsrather than panicking.Consider extracting the magic number
100_000_000into a named constant to clarify it's the default gas limit and improve maintainability if this value changes.#!/bin/bash # Check if there's a default gas limit constant defined rg -n "100_000_000|DEFAULT.*GAS" --type rust -g '!target/*' | head -20🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e/src/wasm.rs` around lines 454 - 478, The test test_wasm_should_not_panic_on_invalid_contract_interface uses the magic number 100_000_000 for gas_used; replace that literal with a named constant (e.g., DEFAULT_GAS_LIMIT or DEFAULT_EXECUTION_GAS) defined where default gas is maintained so the assertion reads assert_eq!(result, ExecutionResult::Halt { reason: HaltReason::MalformedBuiltinParams, gas_used: DEFAULT_GAS_LIMIT }) and update imports/visibility as needed to reference the constant from the module that defines the runtime default gas limit.docs/04-gas-and-fuel.md (1)
74-79: Make calldata surcharge section explicit with threshold/formula reference.Consider adding the exact threshold/formula (or the constant/source path) so this remains actionable and less ambiguous.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/04-gas-and-fuel.md` around lines 74 - 79, Under the "Calldata surcharge" section, add the exact threshold and the surcharge formula (or a direct reference to the constant or function that defines it) so readers can reproduce the calculation; e.g., state the threshold value name and path (the constant or config that sets the cutoff) and the mathematical formula for the quadratic surcharge, or link to the source function (e.g., computeCalldataSurcharge or CALldata_SURCHARGE_THRESHOLD) that implements it so the policy is explicit and actionable.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crates/evm/src/opcodes.rs`:
- Around line 150-154: The code unconditionally calls
as_usize_or_fail!(context.interpreter, memory_offset) before calling
context.interpreter.memory.set_data, which incorrectly validates memory_offset
even when len == 0; change the logic in the EXTCODECOPY resume path to skip the
as_usize_or_fail! cast when len == 0 (matching the initial path at lines
165-176) so zero-length copies are treated as no-ops and do not fail on large
offsets—i.e., guard the as_usize_or_fail! and the use of the resulting
memory_offset_usize behind a len != 0 check and ensure the zero-length branch
performs the same no-op behavior as the initial path (not attempting any
validation).
In `@crates/runtime/src/syscall_handler/host/resume.rs`:
- Around line 81-82: The inline comment is stale: it claims `call_id` is
returned but the code actually returns `result.exit_code`; update the comment
near the return to accurately describe that `result.exit_code` is returned (or
change the return value if `call_id` was intended). Locate the return that
references `result.exit_code` (and any nearby mention of `call_id`) and replace
the misleading text with a brief accurate comment stating that the function
returns `result.exit_code` as the exit code (or adjust code to return `call_id`
if that was the real intent).
In `@crates/testing/src/store.rs`:
- Around line 20-26: The bounds calculation in memory_read_into_vec (and
similarly in memory_read) can overflow when computing offset + length; fix by
computing the end index with checked_add (e.g., let end =
offset.checked_add(length).ok_or(TrapCode::MemoryOutOfBounds)?), then use
memory.get(offset..end).ok_or(TrapCode::MemoryOutOfBounds)? to avoid overflow
and preserve the existing error path.
In `@e2e/src/oom.rs`:
- Line 3: Remove the unused import variant ExitCode::MalformedBuiltinParams from
the use statement that currently reads use fluentbase_sdk::{Address, Bytes,
ExitCode::MalformedBuiltinParams}; — edit the use declaration to only import the
symbols actually used (e.g., Address and Bytes) so the compiler warning is
resolved and no unused enum variant is imported.
In `@evm-e2e/gen_fixture.mjs`:
- Around line 134-136: The unconditional deletes remove legitimate pre/post
state for the fee-manager address 0x0000000000000000000000000000000000520fee;
change the logic so you only remove that account when it is safe: check
preState['0x0000000000000000000000000000000000520fee'] and postState[...] and
only delete if neither contains code/bytecode, storage differences, or any
transaction/runtime-upgrade in the fixture references that address (e.g.,
inspect fixture metadata or tx traces for runtime-upgrade flags); otherwise
leave the account intact so runtime-upgrade flows and legitimate fixtures keep
their expected pre/post state.
- Around line 208-220: The emitted fixture's transaction object omits
blob-specific fields that evm-e2e/src/state.rs expects
(unit.transaction.blob_versioned_hashes and
unit.transaction.max_fee_per_blob_gas); update the generator in gen_fixture.mjs
where the transaction object is built (the block containing transaction: { ...
}) to include these keys when present on the original tx (e.g., if
tx.blobVersionedHashes or tx.blob_versioned_hashes exists, set
transaction.blob_versioned_hashes to that value; if tx.maxFeePerBlobGas or
tx.max_fee_per_blob_gas exists, set transaction.max_fee_per_blob_gas
accordingly), and only add them conditionally so non-blob transactions remain
unchanged.
In `@evm-e2e/src/inspector.rs`:
- Around line 182-183: The code currently calls it1.next().unwrap() and
it2.next().unwrap() which will panic if either iterator is empty; change these
unwraps to safe handling by matching the Option results (e.g., use if let
Some(e1) = it1.next() { ... } else { return Err(...) / break } and similarly for
it2.next()) or propagate a descriptive error/Result indicating missing events;
update any variables e1 and e2 usage accordingly so the function (the iterator
consumption block using it1 and it2) gracefully handles empty iterators instead
of panicking.
In `@evm-e2e/src/runner.rs`:
- Around line 923-930: The runner currently guesses a chain ID from the fixture
path and assigns it to cfg_env.chain_id (and later tx_env.chain_id); instead,
read the serialized fixture config.chainid produced by evm-e2e/gen_fixture.mjs
and use that value if present. Update the code that sets cfg_env.chain_id to
first check the parsed fixture object for a config.chainid field and assign that
(with proper type/parse), falling back to the existing path-based logic only if
the fixture value is missing; ensure tx_env.chain_id continues to get its value
from cfg_env.chain_id so CHAINID-dependent validation uses the fixture-provided
chain ID.
- Around line 653-668: The post-state check falsely passes when contract code is
missing and also unconditionally unwraps the loaded cached account; change the
account handling to return the MissingAccount error if the loaded account is
None (replace the .unwrap() on actual_account.account with a match or
.ok_or(...) that maps to TestErrorKind::MissingAccount), and tighten the code
comparison so you explicitly compare actual_account.code to
account_should_be.code: if account_should_be.code is Some(expected), assert
actual_account.code is Some and compare expected to code.original_bytes(); if
account_should_be.code is None, assert actual_account.code.is_none(). Ensure
these checks reference the existing variables (test.state loop,
db.load_cache_account, actual_account, account_should_be.code,
code.original_bytes()) so missing bytecode no longer false-pass.
In `@evm-e2e/src/state.rs`:
- Around line 166-173: The code currently hardcodes tx_env.tx_type = Eip1559 in
fill_tx_env which breaks replay of legacy, access-list and blob txns; update the
fixture generation (gen_fixture.mjs) to emit the transaction type
(transaction.type) with each fixture and change fill_tx_env to read and set
tx_env.tx_type from that field (use the same numeric values REVM expects); if
fixtures cannot be changed, implement a robust fallback inference in fill_tx_env
that maps presence/absence of fields to types (e.g., blob hashes → type 3,
maxFeePerGas/priorityFee → type 2, access list present but no EIP-1559 fields →
type 1, otherwise type 0) rather than always assigning Eip1559.
In `@scripts/check-node-transition.bash`:
- Line 104: The script writes build output to two different locations (one
redirect to datadir/build.log and another to build.log); update the command that
currently uses "2> build.log" (the line invoking
--manifest-path=./bins/fluent/Cargo.toml) to redirect to the same datadir path
(e.g., "2> \"$datadir/build.log\"" or the script's canonical log variable) so
both writes use the unified build log location referenced elsewhere in the
script.
---
Outside diff comments:
In `@crates/runtime/src/context_wrapper.rs`:
- Around line 83-93: The code calls syscall_resume_impl(...).unwrap_exit_code(),
which can panic on Err and convert recoverable errors like
ExitCode::RootCallOnly into a host panic; instead, handle the Result without
unwrapping: match on the syscall_resume_impl(...) return value (or use
.map_or_else) and on Ok((fuel_consumed, fuel_refunded, exit_code)) return those,
and on Err(err) extract the error as an ExitCode (or convert it) and return
(fuel_consumed, fuel_refunded, err_exit_code) so you never call
unwrap_exit_code(); update the call site in resume (the code using
syscall_resume_impl and unwrap_exit_code) to perform this non-panicking match
and propagate the error as an ExitCode.
In `@evm-e2e/src/tests.rs`:
- Around line 20-3227: The test file evm-e2e/src/tests.rs declares many
fixture-based tests via the define_tests! macro but references JSON fixtures
that are missing or mis-cased (e.g. fn _01_tload_beginning_txn and fn
invalid_precompile_calls); fix by either restoring the missing JSON files under
the tests/... tree with the exact filenames/casing used here or update the
function entries in evm-e2e/src/tests.rs (inside the define_tests! blocks) to
point to the correct existing paths/names for those fixtures (ensure names like
_01_tload_beginning_txn, invalid_precompile_calls and other listed fn entries
exactly match the restored file names). Ensure consistent casing and that every
fn "<name>("<path>")" entry corresponds to an actual file on disk.
---
Nitpick comments:
In `@contracts/wasm/lib.rs`:
- Around line 13-14: The compile error from RwasmModule::compile is being mapped
to ExitCode::MalformedBuiltinParams and the original error is discarded; change
the call to capture the error (e.g., let Err(e) = ...) and log the error before
returning the ExitCode so debugging info for malformed WASM is preserved;
specifically update the RwasmModule::compile invocation that produces (result,
constructor_params) to log the captured error (via your project's
logger/tracing) and then map or convert it to ExitCode::MalformedBuiltinParams
as currently done.
In `@crates/runtime/src/syscall_handler/host/debug_log.rs`:
- Around line 18-19: The handler currently calls
caller.memory_read_into_vec(message_ptr, message_len) and then
syscall_debug_log_impl(&buffer), which copies the entire guest-controlled
payload; change this to avoid unbounded reads by first checking whether
debug-print is enabled and short-circuiting if not, or by capping the read
length to the printable prefix (e.g., max 1000 bytes) before calling
memory_read_into_vec; update the code around syscall_debug_log_impl,
caller.memory_read_into_vec, message_ptr, and message_len so you only
allocate/read the bounded size (or skip reading entirely) and then pass that
bounded slice to syscall_debug_log_impl.
In `@docs/04-gas-and-fuel.md`:
- Around line 74-79: Under the "Calldata surcharge" section, add the exact
threshold and the surcharge formula (or a direct reference to the constant or
function that defines it) so readers can reproduce the calculation; e.g., state
the threshold value name and path (the constant or config that sets the cutoff)
and the mathematical formula for the quadratic surcharge, or link to the source
function (e.g., computeCalldataSurcharge or CALldata_SURCHARGE_THRESHOLD) that
implements it so the policy is explicit and actionable.
In `@e2e/src/bridge.rs`:
- Around line 452-468: Extract the duplicated log-emitting bytecode construction
into a helper fn build_log1_emit_bytecode(log_data: &[u8], signature_hash:
&[u8]) -> Vec<u8> and replace the inline sequences (the block that builds
bytecode using LOG_DATA_OFFSET, opcode::PUSH1, opcode::CODECOPY, opcode::PUSH32,
ReceivedMessage::SIGNATURE_HASH, opcode::LOG1, opcode::STOP, and the final
extend(log_data)) with calls to that helper; inside the helper compute the same
offset (6 + 2 + 32 + 3 + 1), perform the same PUSH/CODECOPY/LOG1/STOP sequence,
assert the header length equals the offset, then extend the payload and return
the Vec<u8>, and update all duplicate sites (including where
ReceivedMessage::SIGNATURE_HASH is used) to call this function.
In `@e2e/src/wasm.rs`:
- Around line 454-478: The test
test_wasm_should_not_panic_on_invalid_contract_interface uses the magic number
100_000_000 for gas_used; replace that literal with a named constant (e.g.,
DEFAULT_GAS_LIMIT or DEFAULT_EXECUTION_GAS) defined where default gas is
maintained so the assertion reads assert_eq!(result, ExecutionResult::Halt {
reason: HaltReason::MalformedBuiltinParams, gas_used: DEFAULT_GAS_LIMIT }) and
update imports/visibility as needed to reference the constant from the module
that defines the runtime default gas limit.
In `@evm-e2e/Makefile`:
- Around line 3-6: The clone/checkout sequence in the Makefile can fail because
it runs git pull before checking out a specific commit; change the shell block
so after cloning you cd into the tests repo, run git fetch --all --tags (or git
fetch origin) and then git checkout faf33b471465d3c6cdc3d04fbd690895f78d33f2;
remove the unconditional git pull or only run it when the repo already exists,
ensuring the checkout happens from the fetched refs so the specific commit is
present even on a fresh clone.
In `@evm-e2e/README.md`:
- Line 1: The README currently only contains a header "# evm-e2e"; expand it to
include a short overview of what the evm-e2e suite tests, a Usage section with
example commands (e.g., make sync_tests, make run_general_state_tests), and a
Prerequisites/Setup section listing required dependencies and any setup steps
(e.g., installing toolchains, environment variables, Docker or local node
requirements); update the header "# evm-e2e" and add these sections so
contributors can quickly understand and run the tests.
In `@evm-e2e/src/inspector.rs`:
- Around line 196-197: The code contains leftover debug println! calls in the
function annotated with #[allow(dead_code)] (e.g., the println!("Call == Call")
and other println! usages around lines shown) which should be replaced with
structured logging; update those println! invocations to use the log crate
macros (debug! or trace!) instead, ensuring the module imports the log macros
(or use tracing's debug!/trace! if project uses tracing) and that the function
(and file) initializes/assumes a logger is configured so runtime logging will be
controlled by level rather than printing to stdout.
- Around line 273-276: The match arm currently prints the two events and calls
unreachable!(), which panics instead of returning a Result::Err; replace the
panic with an appropriate Err(TestError::...) return that contains the mismatch
details (include e1 and e2 in the message) so the function returns Err rather
than panicking—locate the (_, _) => { eprintln!("\n{:?} == {:?}", e1, e2);
unreachable!() } branch in inspector.rs and change it to return Err with a
descriptive TestError (e.g., TestError::EventMismatch or another suitable
variant), or construct a new TestError variant if none exists.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ae6c8e3b-9d6b-47bd-8715-91bca560eacf
⛔ Files ignored due to path filters (5)
Cargo.lockis excluded by!**/*.lockcontracts/Cargo.lockis excluded by!**/*.lockevm-e2e/Cargo.lockis excluded by!**/*.lockevm-e2e/package-lock.jsonis excluded by!**/package-lock.jsonexamples/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (59)
.github/workflows/ci.ymlCargo.tomlMakefileREADME.mdcontracts/Cargo.tomlcontracts/wasm/lib.rscrates/evm/src/evm.rscrates/evm/src/opcodes.rscrates/evm/src/utils.rscrates/revm/src/bridge.rscrates/revm/src/executor.rscrates/revm/src/syscall.rscrates/runtime/src/context_wrapper.rscrates/runtime/src/syscall_handler/hashing/blake3.rscrates/runtime/src/syscall_handler/hashing/keccak256.rscrates/runtime/src/syscall_handler/hashing/sha256.rscrates/runtime/src/syscall_handler/host/debug_log.rscrates/runtime/src/syscall_handler/host/resume.rscrates/runtime/src/syscall_handler/host/write_output.rscrates/testing/src/store.rscrates/types/src/lib.rsdocs/00-system-overview.mddocs/01-runtime-routing-and-ownable-accounts.mddocs/02-interruption-protocol.mddocs/03-syscall-reference-core.mddocs/04-gas-and-fuel.mddocs/05-security-invariants.mddocs/06-runtime-upgrade.mddocs/07-rwasm-integration.mddocs/08-universal-token.mddocs/README.mddocs/create2-factory.mddocs/universal-token.mde2e/src/bridge.rse2e/src/oom.rse2e/src/wasm.rsevm-e2e/.gitignoreevm-e2e/Cargo.tomlevm-e2e/Makefileevm-e2e/README.mdevm-e2e/fixtures/testnet_20986140_legacy_ust20_params.jsonevm-e2e/fixtures/testnet_20987069_rwasm_gas_mismatch.jsonevm-e2e/fixtures/testnet_22882338_hello_world.jsonevm-e2e/gen_fixture.mjsevm-e2e/package.jsonevm-e2e/src/fixtures.rsevm-e2e/src/inspector.rsevm-e2e/src/main.rsevm-e2e/src/merkle_trie.rsevm-e2e/src/models/deserializer.rsevm-e2e/src/models/mod.rsevm-e2e/src/models/spec.rsevm-e2e/src/runner.rsevm-e2e/src/short_tests.rsevm-e2e/src/state.rsevm-e2e/src/tests.rsevm-e2e/src/utils.rsexamples/Cargo.tomlscripts/check-node-transition.bash
💤 Files with no reviewable changes (5)
- docs/create2-factory.md
- evm-e2e/src/models/spec.rs
- evm-e2e/src/models/deserializer.rs
- docs/universal-token.md
- evm-e2e/src/models/mod.rs
Comment on lines
+134
to
+136
| // Remove fee manager (it's bytecode is not executed unless runtime upgrade) | ||
| delete preState['0x0000000000000000000000000000000000520fee']; | ||
| delete postState['0x0000000000000000000000000000000000520fee']; |
Contributor
There was a problem hiding this comment.
Don't strip the fee-manager account unconditionally.
This removes expected pre/post state for any fixture that legitimately touches 0x0000000000000000000000000000000000520fee—including runtime-upgrade flows. The comment already calls out the exception, but the code erases that account for every generated fixture.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@evm-e2e/gen_fixture.mjs` around lines 134 - 136, The unconditional deletes
remove legitimate pre/post state for the fee-manager address
0x0000000000000000000000000000000000520fee; change the logic so you only remove
that account when it is safe: check
preState['0x0000000000000000000000000000000000520fee'] and postState[...] and
only delete if neither contains code/bytecode, storage differences, or any
transaction/runtime-upgrade in the fixture references that address (e.g.,
inspect fixture metadata or tx traces for runtime-upgrade flags); otherwise
leave the account intact so runtime-upgrade flows and legitimate fixtures keep
their expected pre/post state.
Comment on lines
+923
to
+930
| // TODO(dmitry123): Once revm testing unit has config use it instead of path check | ||
| if path.to_str().unwrap().contains("testnet") { | ||
| cfg_env.chain_id = 20994; | ||
| } else if path.to_str().unwrap().contains("devnet") { | ||
| cfg_env.chain_id = 20993; | ||
| } else { | ||
| cfg_env.chain_id = 25363; | ||
| } |
Contributor
There was a problem hiding this comment.
Use the fixture's chain ID instead of guessing from the path.
evm-e2e/gen_fixture.mjs already serializes config.chainid on Lines 176-178, but this runner ignores it and infers the network from directory names. Line 950 then feeds that guess into tx_env.chain_id, so moving or renaming a fixture can silently change validation and CHAINID-dependent behavior.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@evm-e2e/src/runner.rs` around lines 923 - 930, The runner currently guesses a
chain ID from the fixture path and assigns it to cfg_env.chain_id (and later
tx_env.chain_id); instead, read the serialized fixture config.chainid produced
by evm-e2e/gen_fixture.mjs and use that value if present. Update the code that
sets cfg_env.chain_id to first check the parsed fixture object for a
config.chainid field and assign that (with proper type/parse), falling back to
the existing path-based logic only if the fixture value is missing; ensure
tx_env.chain_id continues to get its value from cfg_env.chain_id so
CHAINID-dependent validation uses the fixture-provided chain ID.
Comment on lines
+166
to
+173
| tx_env.kind = match transaction.to { | ||
| Some(add) => TransactTo::Call(add), | ||
| None => TransactTo::Create, | ||
| }; | ||
|
|
||
| tx_env.tx_type = Eip1559 as u8; | ||
| tx_env.nonce = transaction.nonce.to(); | ||
| } |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In revm, how does TxEnv.tx_typeaffect validation and execution, and whattx_type should be used for legacy, EIP-2930, EIP-1559, and EIP-4844/blob transactions?
💡 Result:
In revm, TxEnv.tx_type is a u8 field that specifies the transaction type and is accessed via the Transaction trait method tx_type. It determines how the transaction is validated and executed by affecting gas calculations, fee handling, opcode availability (e.g., BLOBHASH for EIP-4844), and specific validations. Validation occurs in phases via the Handler trait (e.g., MainnetHandler): - validate_env: Checks block/tx/cfg fields without state, including tx_type-dependent rules like gas_price semantics (legacy/EIP-2930 use gas_price directly; EIP-1559 uses it as max_fee). - validate_initial_tx_gas: Computes initial gas costs based on tx_type, data length, access_list (EIP-2930+), blob fields (EIP-4844), etc. - pre_execution (e.g., validate_against_state_and_deduct_caller): Validates nonce/code against loaded account state and deducts caller fees using effective_gas_price, which depends on tx_type and basefee. - Recent updates allow configurable validation by tx_type (e.g., basefee, auth list, blob fee checks). Execution uses tx_type for: - effective_gas_price: Varies by type (gas_price for legacy/2930; min(gas_price, max_fee) for 1559; blob fees for 4844). - Context methods like blob_hash: Only returns values if tx_type == 3 (EIP4844). - Fee deduction, gas refunds, etc., in post_execution. TxEnv::derive_tx_type infers tx_type from fields (e.g., presence of gas_priority_fee suggests EIP-1559) if not set. Recommended tx_type values: - Legacy: 0 - EIP-2930: 1 - EIP-1559: 2 - EIP-4844/blob: 3
Citations:
- 1: https://reth.rs/docs/reth/revm/revm/context/struct.TxEnv.html
- 2: https://docs.rs/revm/latest/revm/context/enum.TransactionType.html
- 3: https://reth.rs/docs/reth_evm/revm/context/tx/struct.TxEnv.html
- 4: https://reth.rs/docs/reth/revm/revm/context/tx/struct.TxEnv.html
- 5: https://docs.rs/revm-context/latest/aarch64-unknown-linux-gnu/src/revm_context/context.rs.html
- 6: Configurable tx validation bluealloy/revm#3215
- 7: https://docs.rs/crate/revm/19.5.0/source/src/evm.rs
- 8: https://reth.rs/docs/reth/revm/revm/handler/trait.Handler.html
🏁 Script executed:
# First, let's examine the full context of the fill_tx_env function
cd evm-e2e && grep -B 20 -A 10 "tx_env.tx_type = Eip1559" src/state.rs | head -60Repository: fluentlabs-xyz/fluentbase
Length of output: 781
🏁 Script executed:
# Check the transaction structure and what fields are available
rg "struct Transaction|enum TransactTo" evm-e2e/src/ -A 5Repository: fluentlabs-xyz/fluentbase
Length of output: 51
🏁 Script executed:
# Look at the gen_fixture.mjs to understand what transaction types are preserved
cat evm-e2e/gen_fixture.mjs | head -250Repository: fluentlabs-xyz/fluentbase
Length of output: 7072
🏁 Script executed:
# Find the TransactionParts struct definition
rg "struct TransactionParts" evm-e2e/src/ -A 20Repository: fluentlabs-xyz/fluentbase
Length of output: 51
🏁 Script executed:
# Check if there's a type field or method to derive it
rg "tx_type|derive.*type|TransactionType" evm-e2e/src/ -B 2 -A 2Repository: fluentlabs-xyz/fluentbase
Length of output: 594
🏁 Script executed:
# Look at the full transaction block in gen_fixture.mjs to see what's actually being saved
sed -n '200,240p' evm-e2e/gen_fixture.mjsRepository: fluentlabs-xyz/fluentbase
Length of output: 1010
🏁 Script executed:
# Find all struct definitions in state.rs
rg "^(pub\s+)?(struct|enum)" evm-e2e/src/state.rsRepository: fluentlabs-xyz/fluentbase
Length of output: 51
🏁 Script executed:
# Look for TransactionParts in the entire evm-e2e directory
fd . evm-e2e/src --type f -name "*.rs" | xargs rg "struct TransactionParts" -lRepository: fluentlabs-xyz/fluentbase
Length of output: 301
🏁 Script executed:
# Check the full context around fill_tx_env function to understand TransactionParts better
sed -n '140,180p' evm-e2e/src/state.rsRepository: fluentlabs-xyz/fluentbase
Length of output: 1160
🏁 Script executed:
# Search for TransactionParts definition across all files
rg "TransactionParts" evm-e2e/ --type rust -B 2 -A 5Repository: fluentlabs-xyz/fluentbase
Length of output: 980
🏁 Script executed:
# Check imports in state.rs to see where TransactionParts comes from
head -50 evm-e2e/src/state.rsRepository: fluentlabs-xyz/fluentbase
Length of output: 1902
🏁 Script executed:
# Look for any fixture or test case files that might define transaction structure
fd . evm-e2e/ --type f \( -name "*.json" -o -name "*.mjs" -o -name "*.rs" \) | head -20Repository: fluentlabs-xyz/fluentbase
Length of output: 299
🏁 Script executed:
# Find Cargo.toml to see revm_statetest_types version
cat evm-e2e/Cargo.toml | grep -A 2 revm_statetest_typesRepository: fluentlabs-xyz/fluentbase
Length of output: 51
🏁 Script executed:
# Search for how TransactionParts is used elsewhere in the codebase
rg "TransactionParts" evm-e2e/src/ -B 1 -A 3Repository: fluentlabs-xyz/fluentbase
Length of output: 651
🏁 Script executed:
# Check if there are test fixture files showing the structure
find evm-e2e -type f -name "*.json" | head -5Repository: fluentlabs-xyz/fluentbase
Length of output: 283
🏁 Script executed:
# Examine one of the fixture files to see the transaction structure
cat "evm-e2e/fixtures/testnet_20986140_legacy_ust20_params.json" | head -100Repository: fluentlabs-xyz/fluentbase
Length of output: 50383
🏁 Script executed:
# Check the full Cargo.toml for dependencies
cat evm-e2e/Cargo.tomlRepository: fluentlabs-xyz/fluentbase
Length of output: 1643
🏁 Script executed:
# Search for documentation or source of revm_statetest_types online
web_search TransactionParts revm_statetest_types structure fieldsRepository: fluentlabs-xyz/fluentbase
Length of output: 119
🏁 Script executed:
# Search for derive_tx_type or type inference in REVM docs/code
rg "derive.*type|infer.*type" evm-e2e/ --type rustRepository: fluentlabs-xyz/fluentbase
Length of output: 51
🏁 Script executed:
# Check if there's any logic to determine tx_type from fields in the codebase
rg "maxFeePerGas|access_list|blob" evm-e2e/src/ --type rust -B 2 -A 2Repository: fluentlabs-xyz/fluentbase
Length of output: 14213
Hardcoding all replayed transactions to EIP-1559 breaks legacy, access-list, and blob transaction execution.
This forces type-2 validation rules onto transactions that may be type 0 (legacy), type 1 (access-list), or type 3 (blob). In REVM, tx_type determines critical execution semantics:
- Gas price calculation (
effective_gas_price) - Validation rules (different rules per type)
- Opcode availability (e.g.,
BLOBHASHonly works whentx_type == 3) - Fee deduction logic
The gen_fixture.mjs generator preserves type-specific fields in fixtures (access lists, max fees, blob hashes), but it does not store the transaction type field itself, leaving fill_tx_env with no way to determine the correct type. Simply inferring from field presence (e.g., maxFeePerGas implies type 2) is insufficient for proper replay.
Add the transaction type to the fixture output so it can be correctly applied during replay. If type information cannot be stored, implement type inference from available fields in fill_tx_env as a fallback.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@evm-e2e/src/state.rs` around lines 166 - 173, The code currently hardcodes
tx_env.tx_type = Eip1559 in fill_tx_env which breaks replay of legacy,
access-list and blob txns; update the fixture generation (gen_fixture.mjs) to
emit the transaction type (transaction.type) with each fixture and change
fill_tx_env to read and set tx_env.tx_type from that field (use the same numeric
values REVM expects); if fixtures cannot be changed, implement a robust fallback
inference in fill_tx_env that maps presence/absence of fields to types (e.g.,
blob hashes → type 3, maxFeePerGas/priorityFee → type 2, access list present but
no EIP-1559 fields → type 1, otherwise type 0) rather than always assigning
Eip1559.
Criterion results (vs baseline)Heads-up: runner perf is noisy; treat deltas as a smoke check. |
dmitry123
force-pushed
the
release/v1.1.0
branch
from
d8ed053 to
bd8b0e1
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
Makefile (2)
22-29: Consider reducing duplication in dependency update commands.Lines 22-29 are correct, but this is repetitive and easy to drift when adding/removing manifests.
Optional refactor
update-rwasm-deps: - cargo update --manifest-path=./contracts/Cargo.toml revm - cargo update --manifest-path=./contracts/Cargo.toml rwasm - cargo update --manifest-path=./examples/Cargo.toml revm - cargo update --manifest-path=./examples/Cargo.toml rwasm - cargo update revm - cargo update rwasm - cargo update --manifest-path=./evm-e2e/Cargo.toml revm - cargo update --manifest-path=./evm-e2e/Cargo.toml rwasm + for manifest in ./contracts/Cargo.toml ./examples/Cargo.toml ./evm-e2e/Cargo.toml; do \ + cargo update --manifest-path=$$manifest revm; \ + cargo update --manifest-path=$$manifest rwasm; \ + done + cargo update revm + cargo update rwasm🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Makefile` around lines 22 - 29, The repeated cargo update lines for multiple manifests and features (e.g., the literal commands "cargo update --manifest-path=./contracts/Cargo.toml revm", "cargo update --manifest-path=./examples/Cargo.toml rwasm", and the plain "cargo update revm"/"cargo update rwasm") should be consolidated: introduce a Makefile variable (e.g., UPDATE_MANIFESTS or UPDATE_TARGETS) listing the manifest paths and a list of features (revm, rwasm), then replace the repeated lines with a small loop or $(foreach) invocation that calls "cargo update" once per manifest and feature and also once for global features, so you update each manifest/feature combination without duplicating lines.
20-21: Consider adding a compatibility alias if this is a rename of an existing target.No references to
update-depswere found in the repository. If this target was previously namedupdate-deps, a compatibility alias would help external automation that depends on the old name. If this is a new target, the alias is unnecessary.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Makefile` around lines 20 - 21, If this new Makefile target update-rwasm-deps is meant to replace an older target name, add a compatibility alias so external automation still works: create a phony target named update-deps that depends on update-rwasm-deps (i.e., declare .PHONY: update-deps and make update-deps: update-rwasm-deps) so invoking update-deps runs the new update-rwasm-deps; if it is truly new, no change is needed.evm-e2e/src/runner.rs (1)
496-502: Use idiomatic!is_empty()instead oflen() > 0.Suggested fix
- if error_list.len() > 0 { + if !error_list.is_empty() { assert!( false,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@evm-e2e/src/runner.rs` around lines 496 - 502, Replace the explicit length check error_list.len() > 0 with the idiomatic !error_list.is_empty() in the assertion block that checks error_list before failing (the code using assert!(false, "...", error_list.join("\n"))). Update the if condition to use !error_list.is_empty() to improve readability and follow Rust conventions while leaving the rest of the assertion logic (error_list.join and the formatted message) unchanged.evm-e2e/src/state.rs (1)
56-61: Assertion on balance mismatch will panic and abort test runs.The
assert_eq!will panic if a genesis account has a non-zero balance that differs from the test'sprestate. This makes the test fail hard rather than providing a recoverable error.Consider returning a
Resulterror or using a softer warning approach consistent with the code collision warning on line 66-68.Suggested approach
if !acc_info.balance.is_zero() && !info.balance.is_zero() { - assert_eq!( - acc_info.balance, info.balance, - "genesis account balance mismatch, this test won't work" - ); + if acc_info.balance != info.balance { + println!( + "WARN: genesis account balance mismatch for {address}, this test might not work" + ); + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@evm-e2e/src/state.rs` around lines 56 - 61, The assert_eq! on acc_info.balance vs info.balance should not panic; replace it with a non-panicking error flow consistent with the collision warning used nearby (lines that emit the collision warning for duplicate accounts). Specifically, change the assertion to return a Result::Err (or propagate a custom error) when balances mismatch, or log a warning and continue if that matches caller semantics; update the surrounding function signature to return Result if needed and include clear context in the error message referencing acc_info.balance and info.balance so callers can handle the mismatch instead of aborting the test run.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@evm-e2e/src/runner.rs`:
- Line 986: Remove the debug println!("result: {:?}", result); from production
code (it prints every test transaction); either delete that line or replace it
with a conditional/levels-based log (e.g., log::debug! or tracing::debug!) so
output is only emitted when debug/trace logging is enabled; ensure to reference
the existing `result` variable and the `println!` call when making the change.
---
Nitpick comments:
In `@evm-e2e/src/runner.rs`:
- Around line 496-502: Replace the explicit length check error_list.len() > 0
with the idiomatic !error_list.is_empty() in the assertion block that checks
error_list before failing (the code using assert!(false, "...",
error_list.join("\n"))). Update the if condition to use !error_list.is_empty()
to improve readability and follow Rust conventions while leaving the rest of the
assertion logic (error_list.join and the formatted message) unchanged.
In `@evm-e2e/src/state.rs`:
- Around line 56-61: The assert_eq! on acc_info.balance vs info.balance should
not panic; replace it with a non-panicking error flow consistent with the
collision warning used nearby (lines that emit the collision warning for
duplicate accounts). Specifically, change the assertion to return a Result::Err
(or propagate a custom error) when balances mismatch, or log a warning and
continue if that matches caller semantics; update the surrounding function
signature to return Result if needed and include clear context in the error
message referencing acc_info.balance and info.balance so callers can handle the
mismatch instead of aborting the test run.
In `@Makefile`:
- Around line 22-29: The repeated cargo update lines for multiple manifests and
features (e.g., the literal commands "cargo update
--manifest-path=./contracts/Cargo.toml revm", "cargo update
--manifest-path=./examples/Cargo.toml rwasm", and the plain "cargo update
revm"/"cargo update rwasm") should be consolidated: introduce a Makefile
variable (e.g., UPDATE_MANIFESTS or UPDATE_TARGETS) listing the manifest paths
and a list of features (revm, rwasm), then replace the repeated lines with a
small loop or $(foreach) invocation that calls "cargo update" once per manifest
and feature and also once for global features, so you update each
manifest/feature combination without duplicating lines.
- Around line 20-21: If this new Makefile target update-rwasm-deps is meant to
replace an older target name, add a compatibility alias so external automation
still works: create a phony target named update-deps that depends on
update-rwasm-deps (i.e., declare .PHONY: update-deps and make update-deps:
update-rwasm-deps) so invoking update-deps runs the new update-rwasm-deps; if it
is truly new, no change is needed.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 0a2c7cb1-0f6e-44a0-aa7c-bc6180d8acca
⛔ Files ignored due to path filters (3)
Cargo.lockis excluded by!**/*.lockevm-e2e/Cargo.lockis excluded by!**/*.lockevm-e2e/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (57)
.github/workflows/ci.ymlCargo.tomlMakefileREADME.mdcontracts/wasm/lib.rscrates/evm/src/evm.rscrates/evm/src/opcodes.rscrates/evm/src/utils.rscrates/revm/src/bridge.rscrates/revm/src/executor.rscrates/revm/src/syscall.rscrates/runtime/src/context_wrapper.rscrates/runtime/src/syscall_handler/hashing/blake3.rscrates/runtime/src/syscall_handler/hashing/keccak256.rscrates/runtime/src/syscall_handler/hashing/sha256.rscrates/runtime/src/syscall_handler/host/debug_log.rscrates/runtime/src/syscall_handler/host/resume.rscrates/runtime/src/syscall_handler/host/write_output.rscrates/testing/src/store.rscrates/types/src/lib.rsdocs/00-system-overview.mddocs/01-runtime-routing-and-ownable-accounts.mddocs/02-interruption-protocol.mddocs/03-syscall-reference-core.mddocs/04-gas-and-fuel.mddocs/05-security-invariants.mddocs/06-runtime-upgrade.mddocs/07-rwasm-integration.mddocs/08-universal-token.mddocs/README.mddocs/create2-factory.mddocs/universal-token.mde2e/src/bridge.rse2e/src/oom.rse2e/src/wasm.rsevm-e2e/.gitignoreevm-e2e/Cargo.tomlevm-e2e/Makefileevm-e2e/README.mdevm-e2e/fixtures/testnet_20986140_legacy_ust20_params.jsonevm-e2e/fixtures/testnet_20987069_rwasm_gas_mismatch.jsonevm-e2e/fixtures/testnet_22882338_hello_world.jsonevm-e2e/gen_fixture.mjsevm-e2e/package.jsonevm-e2e/src/fixtures.rsevm-e2e/src/inspector.rsevm-e2e/src/main.rsevm-e2e/src/merkle_trie.rsevm-e2e/src/models/deserializer.rsevm-e2e/src/models/mod.rsevm-e2e/src/models/spec.rsevm-e2e/src/runner.rsevm-e2e/src/short_tests.rsevm-e2e/src/state.rsevm-e2e/src/tests.rsevm-e2e/src/utils.rsscripts/check-node-transition.bash
💤 Files with no reviewable changes (5)
- evm-e2e/src/models/spec.rs
- docs/universal-token.md
- docs/create2-factory.md
- evm-e2e/src/models/deserializer.rs
- evm-e2e/src/models/mod.rs
✅ Files skipped from review due to trivial changes (21)
- evm-e2e/.gitignore
- evm-e2e/README.md
- crates/runtime/src/syscall_handler/host/write_output.rs
- evm-e2e/Makefile
- crates/runtime/src/syscall_handler/hashing/sha256.rs
- evm-e2e/package.json
- crates/evm/src/evm.rs
- docs/README.md
- crates/types/src/lib.rs
- evm-e2e/src/main.rs
- crates/runtime/src/syscall_handler/hashing/keccak256.rs
- docs/05-security-invariants.md
- scripts/check-node-transition.bash
- docs/07-rwasm-integration.md
- docs/00-system-overview.md
- docs/06-runtime-upgrade.md
- docs/08-universal-token.md
- docs/04-gas-and-fuel.md
- docs/01-runtime-routing-and-ownable-accounts.md
- evm-e2e/Cargo.toml
- README.md
🚧 Files skipped from review as they are similar to previous changes (18)
- crates/runtime/src/syscall_handler/hashing/blake3.rs
- crates/revm/src/executor.rs
- contracts/wasm/lib.rs
- .github/workflows/ci.yml
- e2e/src/wasm.rs
- crates/runtime/src/syscall_handler/host/debug_log.rs
- evm-e2e/src/fixtures.rs
- crates/testing/src/store.rs
- evm-e2e/src/merkle_trie.rs
- Cargo.toml
- crates/evm/src/opcodes.rs
- crates/revm/src/bridge.rs
- docs/03-syscall-reference-core.md
- e2e/src/oom.rs
- crates/runtime/src/syscall_handler/host/resume.rs
- evm-e2e/src/inspector.rs
- e2e/src/bridge.rs
- evm-e2e/src/tests.rs
dmitry123
force-pushed
the
release/v1.1.0
branch
from
bd8b0e1 to
5a30291
Compare
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
evm-e2e/src/tests.rs (1)
20-3227:⚠️ Potential issue | 🔴 CriticalResolve missing test JSON paths referenced in evm-e2e/src/tests.rs
- Issue: 2,772 JSON paths referenced from evm-e2e/src/tests.rs are missing under evm-e2e/ (checked modules: 80).
- Issue: No duplicate fn names were found across modules.
- Actionable fix: restore the missing JSON files to the referenced paths OR remove/update the corresponding fn entries in evm-e2e/src/tests.rs; re-run the path-integrity check afterwards.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@evm-e2e/src/tests.rs` around lines 20 - 3227, The tests file evm-e2e/src/tests.rs references many JSON fixtures that are missing (2,772 paths across modules), causing path-integrity failures; fix by either restoring the missing JSON files under evm-e2e/ to match the referenced paths or removing/updating the corresponding test declarations in tests.rs (look for the define_tests! blocks and the fn entries such as trans_storage_ok, mcopy, access_list, contract_creation, invalid_precompile_calls, etc.), ensure each fn("... .json") points to an existing file, and then re-run the path-integrity check to verify all referenced JSON paths exist.
♻️ Duplicate comments (1)
e2e/src/oom.rs (1)
1-5:⚠️ Potential issue | 🟡 MinorUnused imports:
EvmTestingContextWithGenesisandMalformedBuiltinParams.
- Line 1:
EvmTestingContextWithGenesisis imported but not used. Line 47 usesEvmTestingContext::default().with_full_genesis()instead.- Line 3:
ExitCode::MalformedBuiltinParamsis imported but not used anywhere in this file.,
Proposed fix
-use crate::EvmTestingContextWithGenesis; use fluentbase_contracts::FLUENTBASE_EXAMPLES_MEMORY_OOM; -use fluentbase_sdk::{Address, Bytes, ExitCode::MalformedBuiltinParams}; +use fluentbase_sdk::{Address, Bytes}; use fluentbase_testing::{EvmTestingContext, TxBuilder}; use revm::context::result::{ExecutionResult, HaltReason};🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e/src/oom.rs` around lines 1 - 5, Remove the two unused imports to clean up warnings: delete EvmTestingContextWithGenesis from the first use statement and remove MalformedBuiltinParams (ExitCode::MalformedBuiltinParams) from the fluentbase_sdk import; leave the other imports intact (e.g., EvmTestingContext::default().with_full_genesis() usage remains unchanged).
🧹 Nitpick comments (1)
evm-e2e/src/tests.rs (1)
1017-1018: Consider normalizing misspelled generated test identifiers.
transaction_intinsic_bug,extcodesize_to_epmty,static_internl_call_store_clears_oog, andinternl_call_store_clears_succesmake filtering/debug output harder. Prefer fixing transliteration/normalization ingen_tests.jsand regenerating.Also applies to: 1030-1031, 2474-2474, 2796-2797, 2805-2811
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@evm-e2e/src/tests.rs` around lines 1017 - 1018, Several generated test identifiers contain misspellings (e.g., transaction_intinsic_bug, transaction_intinsic_bug_paris, extcodesize_to_epmty, static_internl_call_store_clears_oog, internl_call_store_clears_succes); update the normalization/transliteration logic in gen_tests.js to correct common typos (intrinsic → intrinsic, empty → empty, internal → internal, success → success), ensure generated function names are sanitized to a consistent pattern (e.g., lowercase_snake_case with non-alphanumerics removed), then regenerate the tests so the functions (transaction_intinsic_bug, transaction_intinsic_bug_paris, extcodesize_to_epmty, static_internl_call_store_clears_oog, internl_call_store_clears_succes and the other instances noted) are rewritten with the corrected identifiers.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@evm-e2e/src/tests.rs`:
- Around line 20-3227: The tests file evm-e2e/src/tests.rs references many JSON
fixtures that are missing (2,772 paths across modules), causing path-integrity
failures; fix by either restoring the missing JSON files under evm-e2e/ to match
the referenced paths or removing/updating the corresponding test declarations in
tests.rs (look for the define_tests! blocks and the fn entries such as
trans_storage_ok, mcopy, access_list, contract_creation,
invalid_precompile_calls, etc.), ensure each fn("... .json") points to an
existing file, and then re-run the path-integrity check to verify all referenced
JSON paths exist.
---
Duplicate comments:
In `@e2e/src/oom.rs`:
- Around line 1-5: Remove the two unused imports to clean up warnings: delete
EvmTestingContextWithGenesis from the first use statement and remove
MalformedBuiltinParams (ExitCode::MalformedBuiltinParams) from the
fluentbase_sdk import; leave the other imports intact (e.g.,
EvmTestingContext::default().with_full_genesis() usage remains unchanged).
---
Nitpick comments:
In `@evm-e2e/src/tests.rs`:
- Around line 1017-1018: Several generated test identifiers contain misspellings
(e.g., transaction_intinsic_bug, transaction_intinsic_bug_paris,
extcodesize_to_epmty, static_internl_call_store_clears_oog,
internl_call_store_clears_succes); update the normalization/transliteration
logic in gen_tests.js to correct common typos (intrinsic → intrinsic, empty →
empty, internal → internal, success → success), ensure generated function names
are sanitized to a consistent pattern (e.g., lowercase_snake_case with
non-alphanumerics removed), then regenerate the tests so the functions
(transaction_intinsic_bug, transaction_intinsic_bug_paris, extcodesize_to_epmty,
static_internl_call_store_clears_oog, internl_call_store_clears_succes and the
other instances noted) are rewritten with the corrected identifiers.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7ed81a57-abbf-41e1-a421-6ae64de2bdb5
⛔ Files ignored due to path filters (2)
Cargo.lockis excluded by!**/*.lockevm-e2e/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (23)
Makefilecontracts/wasm/lib.rscrates/evm/src/evm.rscrates/evm/src/opcodes.rscrates/evm/src/utils.rscrates/revm/src/executor.rscrates/revm/src/syscall.rscrates/runtime/src/context_wrapper.rscrates/runtime/src/syscall_handler/hashing/blake3.rscrates/runtime/src/syscall_handler/hashing/keccak256.rscrates/runtime/src/syscall_handler/hashing/sha256.rscrates/runtime/src/syscall_handler/host/debug_log.rscrates/runtime/src/syscall_handler/host/resume.rscrates/runtime/src/syscall_handler/host/write_output.rscrates/testing/src/store.rscrates/types/src/lib.rsdocs/08-universal-token.mde2e/src/oom.rse2e/src/wasm.rsevm-e2e/Makefileevm-e2e/src/short_tests.rsevm-e2e/src/tests.rsscripts/check-node-transition.bash
💤 Files with no reviewable changes (1)
- evm-e2e/src/short_tests.rs
✅ Files skipped from review due to trivial changes (6)
- evm-e2e/Makefile
- crates/runtime/src/syscall_handler/host/write_output.rs
- crates/testing/src/store.rs
- crates/runtime/src/syscall_handler/hashing/sha256.rs
- crates/evm/src/evm.rs
- scripts/check-node-transition.bash
🚧 Files skipped from review as they are similar to previous changes (12)
- crates/runtime/src/context_wrapper.rs
- crates/revm/src/executor.rs
- crates/runtime/src/syscall_handler/hashing/keccak256.rs
- crates/runtime/src/syscall_handler/host/debug_log.rs
- crates/runtime/src/syscall_handler/hashing/blake3.rs
- crates/evm/src/opcodes.rs
- e2e/src/wasm.rs
- Makefile
- contracts/wasm/lib.rs
- crates/evm/src/utils.rs
- crates/runtime/src/syscall_handler/host/resume.rs
- docs/08-universal-token.md
dmitry123
force-pushed
the
release/v1.1.0
branch
2 times, most recently
from
be61099 to
e60e034
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/runtime/src/syscall_handler/host/resume.rs (1)
26-45:⚠️ Potential issue | 🟡 MinorFail the root-only path before touching guest memory.
Now that
syscall_resume_implexposesExitCode::RootCallOnlyexplicitly, the handler should reject nested callers beforememory_read_into_vec(...)and the fuel-buffer read. As written, a non-root call still copiesreturn_dataand can even surface a memory trap first if the pointers are bad, which makes the new error path both more expensive and less deterministic than it needs to be.Also applies to: 63-67
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/runtime/src/syscall_handler/host/resume.rs` around lines 26 - 45, The handler currently reads guest memory (caller.memory_read_into_vec and fuel buffer) before checking for the RootCallOnly error from syscall_resume_impl; change the flow to detect and return the RootCallOnly path before any guest memory access. Concretely, perform an early call/check using syscall_resume_impl (or a new small helper that queries the same root-only logic) with minimal/no memory-dependent arguments to detect ExitCode::RootCallOnly, returning that error immediately; only if that check passes, proceed to call caller.memory_read_into_vec(return_data_ptr, return_data_len) and the fuel16_ptr memory_read, then call syscall_resume_impl with the real return_data, fuel_consumed, fuel_refunded and fuel16_ptr. Ensure the same error mapping via syscall_process_exit_code(caller, ...) is used for the early check.
🧹 Nitpick comments (1)
evm-e2e/src/tests.rs (1)
1017-1035: Normalize emitted Rust test names instead of mirroring upstream filename typos.Identifiers like
transaction_intinsic_bug,extcodesize_to_epmty,static_internl_call_store_clears_oog, andinternl_call_store_clears_succesmakecargo test <filter>and grep-based triage harder than they need to be. Since the JSON literal already preserves the exact fixture name, I'd normalize the Rust identifier ingen_tests.jsand keep only the path typo-preserving.Also applies to: 2474-2510, 2796-2811
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@evm-e2e/src/tests.rs` around lines 1017 - 1035, The generated Rust test identifiers are mirroring typos from upstream filenames (e.g., transaction_intinsic_bug, extcodesize_to_epmty, static_internl_call_store_clears_oog, internl_call_store_clears_succes); update the generator (gen_tests.js) to emit normalized snake_case Rust identifiers instead of raw filenames: derive the identifier from the fixture path but sanitize non-alphanumerics to underscores, collapse duplicate underscores, ensure it starts with a letter, and apply simple typo-normalizations (e.g., "intinsic"→"intrinsic", "epmty"→"empty", "internl"→"internal", "succes"→"success") so the JSON string literal still contains the original path but the Rust symbol names (as in define_tests! invocations) are corrected for easier cargo test/grep targeting.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crates/revm/src/syscall.rs`:
- Around line 857-862: Add the same copy-size cap check for EXTCODECOPY in the
EVM opcode path before memory is resized: in the extcodecopy opcode handler (the
code that emits SYSCALL_ID_CODE_COPY and currently calls resize_memory!), assert
that the requested len is <= EXT_CODE_COPY_MAX_COPY_SIZE (use the same
assert_halt! / MalformedBuiltinParams semantics) and reject oversized requests
prior to calling resize_memory! so the large pre-interrupt allocation path is
avoided.
In `@evm-e2e/src/tests.rs`:
- Around line 20-42: The generated test registry in evm-e2e/src/tests.rs
contains many paths that do not exist and will cause a panic when runner.rs
calls std::fs::read_to_string(path).unwrap(); fix by either (A) regenerating
tests.rs to only register existing JSON files, or (B) making the runner robust:
replace the unwrap() in the function that calls std::fs::read_to_string
(referenced as read_to_string(path)) with proper error handling—e.g., check
std::fs::metadata(path) or use read_to_string(path).map_err(...) and skip or
report missing files instead of panicking—so tests are filtered/validated at
runtime and the harness never unwraps non-existent paths. Ensure the change
references the read_to_string call in runner.rs and the test registration
functions in tests.rs so missing paths are handled or omitted.
---
Outside diff comments:
In `@crates/runtime/src/syscall_handler/host/resume.rs`:
- Around line 26-45: The handler currently reads guest memory
(caller.memory_read_into_vec and fuel buffer) before checking for the
RootCallOnly error from syscall_resume_impl; change the flow to detect and
return the RootCallOnly path before any guest memory access. Concretely, perform
an early call/check using syscall_resume_impl (or a new small helper that
queries the same root-only logic) with minimal/no memory-dependent arguments to
detect ExitCode::RootCallOnly, returning that error immediately; only if that
check passes, proceed to call caller.memory_read_into_vec(return_data_ptr,
return_data_len) and the fuel16_ptr memory_read, then call syscall_resume_impl
with the real return_data, fuel_consumed, fuel_refunded and fuel16_ptr. Ensure
the same error mapping via syscall_process_exit_code(caller, ...) is used for
the early check.
---
Nitpick comments:
In `@evm-e2e/src/tests.rs`:
- Around line 1017-1035: The generated Rust test identifiers are mirroring typos
from upstream filenames (e.g., transaction_intinsic_bug, extcodesize_to_epmty,
static_internl_call_store_clears_oog, internl_call_store_clears_succes); update
the generator (gen_tests.js) to emit normalized snake_case Rust identifiers
instead of raw filenames: derive the identifier from the fixture path but
sanitize non-alphanumerics to underscores, collapse duplicate underscores,
ensure it starts with a letter, and apply simple typo-normalizations (e.g.,
"intinsic"→"intrinsic", "epmty"→"empty", "internl"→"internal",
"succes"→"success") so the JSON string literal still contains the original path
but the Rust symbol names (as in define_tests! invocations) are corrected for
easier cargo test/grep targeting.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 3cbd2ce8-77d6-4a64-b4cf-1fddc4cb6f07
⛔ Files ignored due to path filters (1)
evm-e2e/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (20)
contracts/wasm/lib.rscrates/evm/src/evm.rscrates/evm/src/opcodes.rscrates/evm/src/utils.rscrates/revm/src/executor.rscrates/revm/src/syscall.rscrates/runtime/src/context_wrapper.rscrates/runtime/src/syscall_handler/hashing/blake3.rscrates/runtime/src/syscall_handler/hashing/keccak256.rscrates/runtime/src/syscall_handler/hashing/sha256.rscrates/runtime/src/syscall_handler/host/debug_log.rscrates/runtime/src/syscall_handler/host/resume.rscrates/runtime/src/syscall_handler/host/write_output.rscrates/testing/src/store.rscrates/types/src/lib.rse2e/src/oom.rse2e/src/wasm.rsevm-e2e/Makefileevm-e2e/src/short_tests.rsevm-e2e/src/tests.rs
💤 Files with no reviewable changes (1)
- evm-e2e/src/short_tests.rs
✅ Files skipped from review due to trivial changes (6)
- crates/runtime/src/syscall_handler/host/write_output.rs
- evm-e2e/Makefile
- crates/evm/src/evm.rs
- crates/types/src/lib.rs
- crates/runtime/src/syscall_handler/hashing/sha256.rs
- crates/runtime/src/syscall_handler/hashing/blake3.rs
🚧 Files skipped from review as they are similar to previous changes (10)
- crates/runtime/src/context_wrapper.rs
- crates/revm/src/executor.rs
- crates/testing/src/store.rs
- crates/runtime/src/syscall_handler/host/debug_log.rs
- crates/evm/src/utils.rs
- crates/runtime/src/syscall_handler/hashing/keccak256.rs
- contracts/wasm/lib.rs
- crates/evm/src/opcodes.rs
- e2e/src/wasm.rs
- e2e/src/oom.rs
* fix(runtime): fix potential extra memory allocations before `MemoryOutOfBounds` checks, finalize rwasm at version v0.4.1, don't panic inside wasm runtime in case of malicious bytecode passed Signed-off-by: Dmitry Savonin <dmitry@fluentlabs.xyz> * fix(evm): fix potential extra memory allocation before gas check for EXTCODECOPY instruction Signed-off-by: Dmitry Savonin <dmitry@fluentlabs.xyz> * fix(runtime): update rwasm to the latest v0.4.1 with correct `read_memory_into_vec` handling Signed-off-by: Dmitry Savonin <dmitry@fluentlabs.xyz> * chore: disable debug print feature Signed-off-by: Dmitry Savonin <dmitry@fluentlabs.xyz> * chore: rename wasm test Signed-off-by: Dmitry Savonin <dmitry@fluentlabs.xyz> * fix: missing trait impl Signed-off-by: Dmitry Savonin <dmitry@fluentlabs.xyz> * fix(runtime): add EXTCODECOPY hard car check Signed-off-by: Dmitry Savonin <dmitry@fluentlabs.xyz> * fix(runtime): use inclusive check for EXTCODECOPY hard cap Signed-off-by: Dmitry Savonin <dmitry@fluentlabs.xyz> * fix(runtime): fix potential crash in metadata copy handler Signed-off-by: Dmitry Savonin <dmitry@fluentlabs.xyz> * fix(runtime): return correct flag for metadata size handler Signed-off-by: Dmitry Savonin <dmitry@fluentlabs.xyz> * fix(runtime): add tests for calling resume inside guest program, fix handling of resume function error Signed-off-by: Dmitry Savonin <dmitry@fluentlabs.xyz> * chore: remove outdated `create2-factory.md` doc Signed-off-by: Dmitry Savonin <dmitry@fluentlabs.xyz> --------- Signed-off-by: Dmitry Savonin <dmitry@fluentlabs.xyz>
* feat(evm)!: enable osaka for evm runtime, update tests for osaka compatibility Signed-off-by: Dmitry Savonin <dmitry@fluentlabs.xyz> * chore: slightly update check node transition script Signed-off-by: Dmitry Savonin <dmitry@fluentlabs.xyz> --------- Signed-off-by: Dmitry Savonin <dmitry@fluentlabs.xyz>
…evm testing suite (#367) Signed-off-by: Dmitry Savonin <dmitry@fluentlabs.xyz>
dmitry123
force-pushed
the
release/v1.1.0
branch
from
e60e034 to
809fbe8
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
e2e/src/wasm.rs (1)
480-513: Test covers only the WASM syscall path; consider adding test for NativeAPI resume path.This test correctly validates the WASM syscall path via
syscall_resume_handler, which properly handlesRootCallOnlyerrors throughsyscall_process_exit_code(). However, the NativeAPI implementation inRuntimeContextWrapper::resume()(line 75-94 incontext_wrapper.rs) calls.unwrap_exit_code()on the result, meaning it would panic ifsyscall_resume_implreturns an error—which occurs when called from a non-root context.The two code paths have inconsistent error handling: the WASM syscall path gracefully converts errors to trap codes, while the NativeAPI path panics. Consider adding a test that directly exercises the NativeAPI resume path to ensure both code paths handle
RootCallOnlyconsistently.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e/src/wasm.rs` around lines 480 - 513, The WASM syscall path is tested but the NativeAPI path (RuntimeContextWrapper::resume which currently calls .unwrap_exit_code()) can panic on non-root calls; add a unit/integration test that exercises the NativeAPI resume path (call RuntimeContextWrapper::resume or directly invoke syscall_resume_impl on a non-root context) and assert it returns an error/trap corresponding to HaltReason::RootCallOnly (or the same error conversion as syscall_process_exit_code) instead of panicking, so both paths handle RootCallOnly consistently.evm-e2e/src/tests.rs (1)
2474-2474: Normalize Rust test identifiers instead of mirroring fixture typos.Lines 2474 and 2796-2811 leak
internlandsuccestypos into the actual#[test]names. While fixture filenames must preserve these typos, there's no need for the Rust identifier itself to repeat them. Normalize the emitted test identifiers ingen_tests.jsand keep the exact typo only in the file path literal—this makescargo testfilters and scripted selectors more robust.No external code references these test identifiers, so the change is safe.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@evm-e2e/src/tests.rs` at line 2474, The generated Rust test identifier mirrors fixture typos (e.g. static_internl_call_store_clears_oog) — update the test name generation logic in gen_tests.js so it normalizes identifiers (fix common misspellings like "internl"→"internal", "succes"→"success"), strips/normalizes non‑identifier chars and builds a valid Rust identifier while leaving the original fixture path string literal unchanged; locate the code that emits identifiers (the branch that produces names like static_internl_call_store_clears_oog) and replace it with a sanitizer/mapping step that returns corrected identifiers but still uses the unchanged filename in the test body.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crates/runtime/src/syscall_handler/host/resume.rs`:
- Around line 63-66: The early return on the call-depth check currently returns
Err(ExitCode::RootCallOnly) which causes context_wrapper.rs's
.unwrap_exit_code() to panic; instead return a successful Result containing the
canonical error tuple so the wrapper can handle it without panicking. Replace
the Err(ExitCode::RootCallOnly) with an Ok((0_u64, -1_i64,
ExitCode::RootCallOnly as i32)) (or use the crate's helper to convert ExitCode
to i32) in the function in resume.rs (the function that currently has the
ctx.call_depth > 0 check) so callers expecting a (u64,i64,i32) tuple receive the
error encoded in the tuple rather than an Err.
---
Nitpick comments:
In `@e2e/src/wasm.rs`:
- Around line 480-513: The WASM syscall path is tested but the NativeAPI path
(RuntimeContextWrapper::resume which currently calls .unwrap_exit_code()) can
panic on non-root calls; add a unit/integration test that exercises the
NativeAPI resume path (call RuntimeContextWrapper::resume or directly invoke
syscall_resume_impl on a non-root context) and assert it returns an error/trap
corresponding to HaltReason::RootCallOnly (or the same error conversion as
syscall_process_exit_code) instead of panicking, so both paths handle
RootCallOnly consistently.
In `@evm-e2e/src/tests.rs`:
- Line 2474: The generated Rust test identifier mirrors fixture typos (e.g.
static_internl_call_store_clears_oog) — update the test name generation logic in
gen_tests.js so it normalizes identifiers (fix common misspellings like
"internl"→"internal", "succes"→"success"), strips/normalizes non‑identifier
chars and builds a valid Rust identifier while leaving the original fixture path
string literal unchanged; locate the code that emits identifiers (the branch
that produces names like static_internl_call_store_clears_oog) and replace it
with a sanitizer/mapping step that returns corrected identifiers but still uses
the unchanged filename in the test body.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 27e07dbe-3707-420d-8b30-30ea3633ca6e
⛔ Files ignored due to path filters (1)
evm-e2e/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (20)
contracts/wasm/lib.rscrates/evm/src/evm.rscrates/evm/src/opcodes.rscrates/evm/src/utils.rscrates/revm/src/executor.rscrates/revm/src/syscall.rscrates/runtime/src/context_wrapper.rscrates/runtime/src/syscall_handler/hashing/blake3.rscrates/runtime/src/syscall_handler/hashing/keccak256.rscrates/runtime/src/syscall_handler/hashing/sha256.rscrates/runtime/src/syscall_handler/host/debug_log.rscrates/runtime/src/syscall_handler/host/resume.rscrates/runtime/src/syscall_handler/host/write_output.rscrates/testing/src/store.rscrates/types/src/lib.rse2e/src/oom.rse2e/src/wasm.rsevm-e2e/Makefileevm-e2e/src/short_tests.rsevm-e2e/src/tests.rs
💤 Files with no reviewable changes (1)
- evm-e2e/src/short_tests.rs
✅ Files skipped from review due to trivial changes (5)
- crates/types/src/lib.rs
- crates/evm/src/evm.rs
- crates/runtime/src/syscall_handler/host/write_output.rs
- crates/runtime/src/syscall_handler/hashing/keccak256.rs
- crates/evm/src/opcodes.rs
🚧 Files skipped from review as they are similar to previous changes (10)
- crates/runtime/src/context_wrapper.rs
- crates/runtime/src/syscall_handler/hashing/sha256.rs
- crates/testing/src/store.rs
- crates/revm/src/executor.rs
- contracts/wasm/lib.rs
- crates/runtime/src/syscall_handler/host/debug_log.rs
- crates/evm/src/utils.rs
- evm-e2e/Makefile
- crates/runtime/src/syscall_handler/hashing/blake3.rs
- e2e/src/oom.rs
Signed-off-by: Dmitry Savonin <dmitry@fluentlabs.xyz>
Signed-off-by: Dmitry Savonin <dmitry@fluentlabs.xyz>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Bug Fixes
Chores