Skip to content

refactor: prepare for the changes in debug trace transaction#6731

Open
akaladarshi wants to merge 2 commits intomainfrom
akaladarshi/refactor-for-dbg-txn-api
Open

refactor: prepare for the changes in debug trace transaction#6731
akaladarshi wants to merge 2 commits intomainfrom
akaladarshi/refactor-for-dbg-txn-api

Conversation

@akaladarshi
Copy link
Collaborator

@akaladarshi akaladarshi commented Mar 12, 2026

Summary of changes

Changes introduced in this pull request:

  • Refactors couple of components so they can be used by DebugTraceTransaction API implementation

Reference issue to close (if applicable)

Contains the part of this PR: #6685

Other information and links

Change checklist

  • I have performed a self-review of my own code,
  • I have made corresponding changes to the documentation. All new code adheres to the team's documentation standards,
  • I have added tests that prove my fix is effective or that my feature works (if possible),
  • I have made sure the CHANGELOG is up-to-date. All user-facing changes should be reflected in this document.

Outside contributions

  • I have read and agree to the CONTRIBUTING document.
  • I have read and agree to the AI Policy document. I understand that failure to comply with the guidelines will lead to rejection of the pull request.

Summary by CodeRabbit

  • New Features

    • Trace responses now support Parity-style traces and optional state-diff output for richer debug information.
  • Bug Fixes

    • More consistent and accurate EVM trace error reporting and state-diff computation; improved handling of call inputs.
  • Refactor

    • Trace generation and VM/tipset execution consolidated for more reliable, deferred trace materialization.
  • Tests

    • Expanded tests for nonce/bytecode handling across EVM and non‑EVM scenarios.

@akaladarshi akaladarshi requested a review from a team as a code owner March 12, 2026 12:11
@akaladarshi akaladarshi requested review from LesnyRumcajs and sudo-shashank and removed request for a team March 12, 2026 12:11
@akaladarshi akaladarshi added the RPC requires calibnet RPC checks to run on CI label Mar 12, 2026
@coderabbitai
Copy link
Contributor

coderabbitai bot commented Mar 12, 2026

Walkthrough

Refactors Ethereum trace generation to a two-stage flow: execution now returns a StateTree plus TipsetTraceEntry items; parity-format traces are built later from those entries. Adds TipsetExecutor to centralize VM/tipset preparation and moves trace-related types into types.rs with new helpers in trace.rs.

Changes

Cohort / File(s) Summary
Trace Type & API Types
src/rpc/methods/eth/types.rs
Added EthTraceType and EthTraceResults; changed EthCallMessage to separate data and input with effective_input(); flattened EthReplayBlockTransactionTrace to embed EthTraceResults.
RPC Trace Flow
src/rpc/methods/eth.rs
Removed inline EthTrace/EthTraceResults usage; added internal TipsetTraceEntry and build_parity_traces; changed execute_tipset_traces to return (StateTree, Vec<TipsetTraceEntry>); eth_trace_block reworked to materialize parity traces from entries.
Parity Trace Helpers & Errors
src/rpc/methods/eth/trace.rs
Added Parity-format error constants and EVM error strings; introduced actor_nonce() and actor_bytecode() helpers; refactored state-diff computation to use those helpers; extended tests for nonce/bytecode and EVM cases.
State/VM Preparation
src/state_manager/mod.rs
Introduced internal TipsetExecutor struct encapsulating VM creation and parent-state preparation; refactored apply_block_messages to use the executor for cron/migration and VM setup.

Sequence Diagram(s)

sequenceDiagram
    autonumber
    participant RPC as RPC Handler
    participant Exec as execute_tipset_traces
    participant SM as StateManager/TipsetExecutor
    participant Entry as TipsetTraceEntry
    participant Builder as Parity Trace Builder

    RPC->>Exec: request execute_tipset_traces(ctx, ts, ext)
    Exec->>SM: spawn blocking task → run tipset execution
    SM-->>Exec: (StateTree, Vec<TipsetTraceEntry>)
    Exec-->>RPC: return (StateTree, entries)

    loop per entry
        RPC->>Entry: entry.build_parity_traces(&StateTree)
        Entry->>Builder: derive parity-style EthTrace items
        Builder-->>Entry: Vec<EthTrace>
        Entry-->>RPC: EthTrace results
    end

    RPC->>RPC: assemble EthBlockTrace from EthTrace results
    RPC-->>Client: return trace response
Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Possibly related PRs

Suggested reviewers

  • LesnyRumcajs
  • sudo-shashank
🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 inconclusive)

Check name Status Explanation Resolution
Title check ❓ Inconclusive The title is vague and uses non-specific language ('prepare for the changes') that doesn't clearly convey what the main changes actually are. Replace with a more specific title that describes the primary refactoring, such as 'refactor: extract trace execution logic for reuse' or 'refactor: modularize tipset trace generation'.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
  • 📝 Generate docstrings (stacked PR)
  • 📝 Generate docstrings (commit on current branch)
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch akaladarshi/refactor-for-dbg-txn-api

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

🧹 Nitpick comments (2)
src/rpc/methods/eth/trace.rs (1)

1488-1521: Make the helper tests fail when fixture setup is missing.

The if let Some(actor) guards make these tests pass vacuously if the EVM actor bundle metadata or actor-state setup is unavailable. Replacing them with expect(...) keeps the new coverage meaningful.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/rpc/methods/eth/trace.rs` around lines 1488 - 1521, The tests use `if let
Some(actor) = create_evm_actor_with_bytecode(...)` which allows them to pass
vacuously when fixtures are missing; replace those `if let Some(...)` guards
with direct unwraps via `expect("...")` (or `unwrap()` with a clear message) so
the tests fail if the EVM actor fixture or actor-state setup is not created;
update the three tests that call `create_evm_actor_with_bytecode` (and the one
that uses `create_test_actor` if applicable) to call
`create_evm_actor_with_bytecode(...).expect("failed to create EVM actor
fixture")` or `create_test_actor(...).expect("failed to create test actor")`,
leaving assertions that call `actor_bytecode(...)` and `actor_nonce(...)`
unchanged.
src/state_manager/mod.rs (1)

1897-1967: Add phase/tipset context to the new helper's fallible calls.

TipsetExecutor now owns parent-tipset loading, circulating-supply lookup, VM creation, and block-message loading, but those errors still bubble up bare. When this path fails during state replay or tracing, it's hard to tell which stage or epoch failed. Wrapping the new fallible calls with .context(...) would make these failures much easier to diagnose.

♻️ Example tightening
-        let circ_supply = self.genesis_info.get_vm_circulating_supply(
+        let circ_supply = self.genesis_info.get_vm_circulating_supply(
             epoch,
             self.chain_index.db(),
             &state_root,
-        )?;
-        VM::new(
+        )
+        .with_context(|| format!("failed to compute circulating supply for tipset={}, epoch={epoch}", self.tipset.key()))?;
+        VM::new(
             ExecutionContext {
                 heaviest_tipset: self.tipset.clone(),
                 state_tree_root: state_root,
@@
-            trace,
-        )
+            trace,
+        )
+        .with_context(|| format!("failed to create VM for tipset={}, epoch={epoch}", self.tipset.key()))
@@
-        let mut parent_state = *self.tipset.parent_state();
-        let parent_epoch =
-            Tipset::load_required(self.chain_index.db(), self.tipset.parents())?.epoch();
+        let mut parent_state = *self.tipset.parent_state();
+        let parent_epoch = Tipset::load_required(self.chain_index.db(), self.tipset.parents())
+            .with_context(|| format!("failed to load parent tipset for {}", self.tipset.key()))?
+            .epoch();
@@
-        let block_messages = BlockMessages::for_tipset(self.chain_index.db(), &self.tipset)?;
+        let block_messages = BlockMessages::for_tipset(self.chain_index.db(), &self.tipset)
+            .with_context(|| format!("failed to load block messages for {}", self.tipset.key()))?;

As per coding guidelines, "Use anyhow::Result<T> for most operations and add context with .context() when errors occur".

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/state_manager/mod.rs` around lines 1897 - 1967, The prepare_parent_state
path performs several fallible operations without context; wrap each call that
returns anyhow::Result with .context(...) to indicate phase/tipset/epoch (e.g.,
annotate Tipset::load_required(self.chain_index.db(), self.tipset.parents())?
with context like "loading parent tipset for tipset={:?}" including self.tipset,
annotate get_vm_circulating_supply(...) in create_vm, the create_vm(...) call
and vm.run_cron(...) invocation inside the loop (include epoch_i),
run_state_migrations(...)? return, and BlockMessages::for_tipset(...) with clear
messages identifying the phase and epoch/tipset); update prepare_parent_state,
create_vm, and the loop so errors bubble up with .context(...) so failures show
which stage (parent-load, circ-supply, vm-creation, cron, migration, or
block-message load) and the relevant tipset/epoch.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@src/rpc/methods/eth.rs`:
- Around line 3412-3425: The loop uses msg_idx (initialized to 0) but increments
it before creating each TipsetTraceEntry, making msg_position one-based; change
the logic so transaction positions remain zero-based by using the current
msg_idx when constructing TipsetTraceEntry and only incrementing msg_idx after
pushing the entry (or start msg_idx at 0 and increment after push) in the loop
that iterates raw_traces where EthGetTransactionHashByCid::handle(...) is called
and TipsetTraceEntry { tx_hash, msg_position: msg_idx, ... } is created.

In `@src/rpc/methods/eth/trace.rs`:
- Around line 678-689: The function actor_nonce currently masks evm::State::load
failures by returning actor.sequence (producing wrong EVM nonces); change
actor_nonce to return anyhow::Result<EthUint64> and replace the unwrap_or
fallback with propagating the load error using .context("failed to load EVM
state for actor nonce") so genuine read errors surface. Update callers such as
build_account_diff to handle the Result (use ? to propagate) and adjust any
signature to return anyhow::Result as needed, ensuring all paths propagate the
error instead of fabricating a nonce value.

---

Nitpick comments:
In `@src/rpc/methods/eth/trace.rs`:
- Around line 1488-1521: The tests use `if let Some(actor) =
create_evm_actor_with_bytecode(...)` which allows them to pass vacuously when
fixtures are missing; replace those `if let Some(...)` guards with direct
unwraps via `expect("...")` (or `unwrap()` with a clear message) so the tests
fail if the EVM actor fixture or actor-state setup is not created; update the
three tests that call `create_evm_actor_with_bytecode` (and the one that uses
`create_test_actor` if applicable) to call
`create_evm_actor_with_bytecode(...).expect("failed to create EVM actor
fixture")` or `create_test_actor(...).expect("failed to create test actor")`,
leaving assertions that call `actor_bytecode(...)` and `actor_nonce(...)`
unchanged.

In `@src/state_manager/mod.rs`:
- Around line 1897-1967: The prepare_parent_state path performs several fallible
operations without context; wrap each call that returns anyhow::Result with
.context(...) to indicate phase/tipset/epoch (e.g., annotate
Tipset::load_required(self.chain_index.db(), self.tipset.parents())? with
context like "loading parent tipset for tipset={:?}" including self.tipset,
annotate get_vm_circulating_supply(...) in create_vm, the create_vm(...) call
and vm.run_cron(...) invocation inside the loop (include epoch_i),
run_state_migrations(...)? return, and BlockMessages::for_tipset(...) with clear
messages identifying the phase and epoch/tipset); update prepare_parent_state,
create_vm, and the loop so errors bubble up with .context(...) so failures show
which stage (parent-load, circ-supply, vm-creation, cron, migration, or
block-message load) and the relevant tipset/epoch.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

Run ID: 43ffd8a5-45fb-4395-ba7c-2bb99cee88ad

📥 Commits

Reviewing files that changed from the base of the PR and between c44147f and 7d995ce.

⛔ Files ignored due to path filters (3)
  • src/rpc/snapshots/forest__rpc__tests__rpc__v0.snap is excluded by !**/*.snap
  • src/rpc/snapshots/forest__rpc__tests__rpc__v1.snap is excluded by !**/*.snap
  • src/rpc/snapshots/forest__rpc__tests__rpc__v2.snap is excluded by !**/*.snap
📒 Files selected for processing (4)
  • src/rpc/methods/eth.rs
  • src/rpc/methods/eth/trace.rs
  • src/rpc/methods/eth/types.rs
  • src/state_manager/mod.rs

@codecov
Copy link

codecov bot commented Mar 12, 2026

Codecov Report

❌ Patch coverage is 83.98268% with 37 lines in your changes missing coverage. Please review.
✅ Project coverage is 63.49%. Comparing base (c44147f) to head (b58d745).

Files with missing lines Patch % Lines
src/rpc/methods/eth/trace.rs 76.56% 11 Missing and 4 partials ⚠️
src/rpc/methods/eth.rs 80.70% 3 Missing and 8 partials ⚠️
src/state_manager/mod.rs 89.13% 2 Missing and 8 partials ⚠️
src/rpc/methods/eth/types.rs 94.44% 1 Missing ⚠️
Additional details and impacted files
Files with missing lines Coverage Δ
src/rpc/methods/eth/types.rs 76.86% <94.44%> (+0.81%) ⬆️
src/state_manager/mod.rs 70.76% <89.13%> (+0.94%) ⬆️
src/rpc/methods/eth.rs 68.07% <80.70%> (+0.08%) ⬆️
src/rpc/methods/eth/trace.rs 76.27% <76.56%> (+0.39%) ⬆️

... and 3 files with indirect coverage changes


Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c44147f...b58d745. Read the comment docs.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

}
}

/// Returns the effective nonce for an actor: EVM nonce for EVM actors, sequence otherwise.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think some of those methods could be potentially used outside of the trace module; would it make sense to add them to some utils module? Also, it might be slightly nicer to actually add a trait an implement it for ActorState instead of relying on some free functions.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 3

🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@src/rpc/methods/eth/trace.rs`:
- Around line 689-700: Change actor_bytecode to return
anyhow::Result<Option<EthBytes>> and stop collapsing errors into None: call
evm::State::load(store, actor.code, actor.state) and the subsequent
store.get(...) using ? (propagating errors) and add context() messages for both
failure points so callers (e.g., build_account_diff) can distinguish "no code"
(Ok(None)) from IO/deserialization failures; update the other similar helper at
the second occurrence (the lines referenced around 751-752) to the same
signature and error-handling pattern, and adjust callers (build_account_diff) to
handle the Result<Option<...>> accordingly.
- Around line 136-153: The current guard uses trace_is_evm_or_eam(trace) which
in turn treats any actor with invoked_actor.id != ETHEREUM_ACCOUNT_MANAGER_ACTOR
as EVM, causing non-EVM exits to be misclassified; update the guard/helper so it
only returns true for actual EVM or EAM actors (e.g., change trace_is_evm_or_eam
to check invoked_actor.id equals the EVM actor ID or equals
ETHEREUM_ACCOUNT_MANAGER_ACTOR, or better check the invoked actor's code/type
explicitly), then keep the mapping of evm12::EVM_CONTRACT_* codes (the match in
trace.rs that returns PARITY_* constants) so it only runs for true EVM/EAM
actors.
- Around line 1486-1519: The three tests (test_actor_nonce_evm,
test_actor_bytecode_evm, test_actor_bytecode_evm_no_bytecode) currently use
conditional `if let Some(actor) = create_evm_actor_with_bytecode(...)` which
silently skips assertions when the fixture is missing; change each to
unwrap/expect the fixture (e.g., let actor =
create_evm_actor_with_bytecode(...).expect("failed to create EVM actor
fixture")) so the test fails loudly if the EVM code CID/manifest cannot be
created, and update test_actor_bytecode_evm and
test_actor_bytecode_evm_no_bytecode similarly to use expect instead of `if let
Some(...)`.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

Run ID: ad969f4b-6e2e-4353-b3f1-3b26428b4a1a

📥 Commits

Reviewing files that changed from the base of the PR and between 7d995ce and b58d745.

📒 Files selected for processing (1)
  • src/rpc/methods/eth/trace.rs

Comment on lines 136 to 153
if trace_is_evm_or_eam(trace) {
match code.into() {
evm12::EVM_CONTRACT_REVERTED => return Some("Reverted".into()), // capitalized for compatibility
evm12::EVM_CONTRACT_INVALID_INSTRUCTION => return Some("invalid instruction".into()),
evm12::EVM_CONTRACT_REVERTED => return Some(PARITY_TRACE_REVERT_ERROR.into()), // capitalized for compatibility
evm12::EVM_CONTRACT_INVALID_INSTRUCTION => {
return Some(PARITY_EVM_INVALID_INSTRUCTION.into());
}
evm12::EVM_CONTRACT_UNDEFINED_INSTRUCTION => {
return Some("undefined instruction".into());
return Some(PARITY_EVM_UNDEFINED_INSTRUCTION.into());
}
evm12::EVM_CONTRACT_STACK_UNDERFLOW => return Some("stack underflow".into()),
evm12::EVM_CONTRACT_STACK_OVERFLOW => return Some("stack overflow".into()),
evm12::EVM_CONTRACT_STACK_UNDERFLOW => return Some(PARITY_EVM_STACK_UNDERFLOW.into()),
evm12::EVM_CONTRACT_STACK_OVERFLOW => return Some(PARITY_EVM_STACK_OVERFLOW.into()),
evm12::EVM_CONTRACT_ILLEGAL_MEMORY_ACCESS => {
return Some("illegal memory access".into());
return Some(PARITY_EVM_ILLEGAL_MEMORY_ACCESS.into());
}
evm12::EVM_CONTRACT_BAD_JUMPDEST => return Some(PARITY_EVM_BAD_JUMPDEST.into()),
evm12::EVM_CONTRACT_SELFDESTRUCT_FAILED => {
return Some(PARITY_EVM_SELFDESTRUCT_FAILED.into());
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Fix the EVM/EAM guard before extending this mapping.

This branch still relies on trace_is_evm_or_eam(), and that helper currently returns true for every non-EAM actor because it uses invoked_actor.id != ETHEREUM_ACCOUNT_MANAGER_ACTOR. Any unrelated actor exit code that collides numerically with an EVM code can therefore be mislabeled as a Parity EVM error here.

🛠️ Minimal fix
 fn trace_is_evm_or_eam(trace: &ExecutionTrace) -> bool {
     if let Some(invoked_actor) = &trace.invoked_actor {
+        let eam_actor_id = Address::ETHEREUM_ACCOUNT_MANAGER_ACTOR
+            .id()
+            .expect("EAM actor address should be an ID address");
         is_evm_actor(&invoked_actor.state.code)
-            || invoked_actor.id != Address::ETHEREUM_ACCOUNT_MANAGER_ACTOR.id().unwrap()
+            || invoked_actor.id == eam_actor_id
     } else {
         false
     }
 }
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/rpc/methods/eth/trace.rs` around lines 136 - 153, The current guard uses
trace_is_evm_or_eam(trace) which in turn treats any actor with invoked_actor.id
!= ETHEREUM_ACCOUNT_MANAGER_ACTOR as EVM, causing non-EVM exits to be
misclassified; update the guard/helper so it only returns true for actual EVM or
EAM actors (e.g., change trace_is_evm_or_eam to check invoked_actor.id equals
the EVM actor ID or equals ETHEREUM_ACCOUNT_MANAGER_ACTOR, or better check the
invoked actor's code/type explicitly), then keep the mapping of
evm12::EVM_CONTRACT_* codes (the match in trace.rs that returns PARITY_*
constants) so it only runs for true EVM/EAM actors.

Comment on lines +689 to +700
/// Returns the deployed bytecode of an EVM actor, or `None` for non-EVM actors.
fn actor_bytecode<DB: Blockstore>(store: &DB, actor: &ActorState) -> Option<EthBytes> {
if !is_evm_actor(&actor.code) {
return None;
}
let evm_state = evm::State::load(store, actor.code, actor.state).ok()?;
store
.get(&evm_state.bytecode())
.ok()
.flatten()
.map(EthBytes)
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Don’t collapse bytecode/state read failures into “no code”.

actor_bytecode() swallows both evm::State::load() and store.get() errors with ok()?, so build_account_diff() can emit unchanged/added/removed code deltas even when the underlying EVM state or bytecode read failed. This should return anyhow::Result<Option<EthBytes>> and only treat Ok(None) as “no deployed bytecode.”

🛠️ Proposed fix
-fn actor_bytecode<DB: Blockstore>(store: &DB, actor: &ActorState) -> Option<EthBytes> {
+fn actor_bytecode<DB: Blockstore>(
+    store: &DB,
+    actor: &ActorState,
+) -> anyhow::Result<Option<EthBytes>> {
     if !is_evm_actor(&actor.code) {
-        return None;
+        return Ok(None);
     }
-    let evm_state = evm::State::load(store, actor.code, actor.state).ok()?;
-    store
-        .get(&evm_state.bytecode())
-        .ok()
-        .flatten()
-        .map(EthBytes)
+    let evm_state = evm::State::load(store, actor.code, actor.state)
+        .context("failed to load EVM state for bytecode")?;
+    Ok(
+        store
+            .get(&evm_state.bytecode())
+            .context("failed to load EVM bytecode")?
+            .map(EthBytes),
+    )
 }
-    let pre_code = pre_actor.and_then(|a| actor_bytecode(store, a));
-    let post_code = post_actor.and_then(|a| actor_bytecode(store, a));
+    let pre_code = pre_actor.map(|a| actor_bytecode(store, a)).transpose()?.flatten();
+    let post_code = post_actor.map(|a| actor_bytecode(store, a)).transpose()?.flatten();

As per coding guidelines: Use anyhow::Result<T> for most operations and add context with .context() when errors occur.

Also applies to: 751-752

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/rpc/methods/eth/trace.rs` around lines 689 - 700, Change actor_bytecode
to return anyhow::Result<Option<EthBytes>> and stop collapsing errors into None:
call evm::State::load(store, actor.code, actor.state) and the subsequent
store.get(...) using ? (propagating errors) and add context() messages for both
failure points so callers (e.g., build_account_diff) can distinguish "no code"
(Ok(None)) from IO/deserialization failures; update the other similar helper at
the second occurrence (the lines referenced around 751-752) to the same
signature and error-handling pattern, and adjust callers (build_account_diff) to
handle the Result<Option<...>> accordingly.

Comment on lines +1486 to +1519
fn test_actor_nonce_evm() {
let store = Arc::new(MemoryDB::default());
if let Some(actor) = create_evm_actor_with_bytecode(&store, 1000, 0, 7, Some(&[0x60])) {
let nonce = actor_nonce(store.as_ref(), &actor).unwrap();
// EVM actors use the EVM nonce field, not the actor sequence
assert_eq!(nonce.0, 7);
}
}

#[test]
fn test_actor_bytecode_non_evm() {
let store = MemoryDB::default();
let actor = create_test_actor(1000, 0);
assert!(actor_bytecode(&store, &actor).is_none());
}

#[test]
fn test_actor_bytecode_evm() {
let store = Arc::new(MemoryDB::default());
let bytecode = &[0x60, 0x80, 0x60, 0x40, 0x52];
if let Some(actor) = create_evm_actor_with_bytecode(&store, 1000, 0, 1, Some(bytecode)) {
let result = actor_bytecode(store.as_ref(), &actor);
assert_eq!(result, Some(EthBytes(bytecode.to_vec())));
}
}

#[test]
fn test_actor_bytecode_evm_no_bytecode() {
let store = Arc::new(MemoryDB::default());
if let Some(actor) = create_evm_actor_with_bytecode(&store, 1000, 0, 1, None) {
// No bytecode stored => None (Cid::default() won't resolve to raw data)
let result = actor_bytecode(store.as_ref(), &actor);
assert!(result.is_none());
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

Make these tests fail if the EVM fixture can't be created.

Each if let Some(actor) turns a missing EVM code CID/manifest into a vacuous pass, so these tests can stop asserting anything without going red. Please convert fixture creation to expect(...) so bundle regressions fail immediately.

🧪 Tighten the assertions
     let store = Arc::new(MemoryDB::default());
-    if let Some(actor) = create_evm_actor_with_bytecode(&store, 1000, 0, 7, Some(&[0x60])) {
-        let nonce = actor_nonce(store.as_ref(), &actor).unwrap();
-        // EVM actors use the EVM nonce field, not the actor sequence
-        assert_eq!(nonce.0, 7);
-    }
+    let actor = create_evm_actor_with_bytecode(&store, 1000, 0, 7, Some(&[0x60]))
+        .expect("expected EVM actor fixture");
+    let nonce = actor_nonce(store.as_ref(), &actor).unwrap();
+    // EVM actors use the EVM nonce field, not the actor sequence
+    assert_eq!(nonce.0, 7);

Apply the same pattern to test_actor_bytecode_evm and test_actor_bytecode_evm_no_bytecode.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/rpc/methods/eth/trace.rs` around lines 1486 - 1519, The three tests
(test_actor_nonce_evm, test_actor_bytecode_evm,
test_actor_bytecode_evm_no_bytecode) currently use conditional `if let
Some(actor) = create_evm_actor_with_bytecode(...)` which silently skips
assertions when the fixture is missing; change each to unwrap/expect the fixture
(e.g., let actor = create_evm_actor_with_bytecode(...).expect("failed to create
EVM actor fixture")) so the test fails loudly if the EVM code CID/manifest
cannot be created, and update test_actor_bytecode_evm and
test_actor_bytecode_evm_no_bytecode similarly to use expect instead of `if let
Some(...)`.

pub value: Option<EthBigInt>,
// Some clients use `input`, others use `data`. We have to support both.
#[serde(alias = "input", skip_serializing_if = "Option::is_none", default)]
// Some clients use `input`, others use `data`; both accepted, `input` takes precedence.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💩

Comment on lines +686 to +687
TraceResult::Call(r) => r.output.clone(),
TraceResult::Create(r) => r.code.clone(),
Copy link
Member

@LesnyRumcajs LesnyRumcajs Mar 12, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we avoid cloning given we're grabbing traces ownership anyway?

}

async fn eth_trace_block<DB>(
struct TipsetTraceEntry {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we're refactoring things, I think it'd make sense to move trace-related types to trace/types.rs. This module is already huge.

let mut vm = create_vm(parent_state, epoch, tipset.min_timestamp())?;
let mut vm = exec.create_vm(parent_state, epoch, tipset.min_timestamp(), enable_tracing)?;

// step 4: apply tipset messages
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's keep the comments.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

RPC requires calibnet RPC checks to run on CI

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants