Open
Conversation
Contributor
WalkthroughImplements a new Eth trace_call RPC with trace and optional state-diff results, storage/account diffing via KAMT, VM/state_manager plumbing to optionally flush state roots, new types and tests, a Tracer contract and integration script, and user/developer documentation. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client
participant RPC as Forest RPC (EthTraceCall)
participant StateMgr as StateManager
participant VM as VM/Executor
participant Store as Blockstore/KAMT
participant Trace as TraceBuilder
Client->>RPC: Eth_traceCall(params)
RPC->>StateMgr: load pre-state (actors & touched addresses)
StateMgr->>Store: read pre-state actors & storage
RPC->>VM: execute simulated call (no broadcast)
VM-->>RPC: execution trace, logs, return/revert
RPC->>StateMgr: load post-state (if requested)
StateMgr->>Store: read post-state actors & storage
RPC->>Trace: build traces and build_state_diff(pre, post, touched)
Trace-->>RPC: EthTraceResults (trace + optional StateDiff)
RPC-->>Client: response
Estimated code review effort🎯 4 (Complex) | ⏱️ ~70 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Comment |
akaladarshi
changed the title
Forest.EthTraceCallForest.EthTraceCall [Skip CI]
akaladarshi
force-pushed
the
akaladarshi/eth-trace-call
branch
2 times, most recently
from
701d095 to
a69d125
Compare
akaladarshi
force-pushed
the
akaladarshi/eth-trace-call
branch
from
a69d125 to
47b7aa3
Compare
Member
|
As discussed, let's have a short guide in docs outlining its usage. This is important in light of various formats used across the Ethereum ecosystem for the output of this method. |
LesnyRumcajs
reviewed
Feb 5, 2026
Member
There was a problem hiding this comment.
The developers guides are intended for Forest maintainers. This should be in user space. Looking briefly, it's less of a guide more of a something to add to the knowledge base section (perhaps in a /RPC subsection). It's fine but let's rename the title to something along the lines of trace_call usage.
Member
There was a problem hiding this comment.
Okay, going through it all I think it should be split into two parts:
- User guide/knowledge post with basic usage info.
- Dev guide with all of the Anvil stuff and comparing things.
All in all, solid piece of work, seems to work fine.
LesnyRumcajs
reviewed
Feb 5, 2026
LesnyRumcajs
reviewed
Feb 6, 2026
LesnyRumcajs
reviewed
Feb 6, 2026
LesnyRumcajs
reviewed
Feb 6, 2026
LesnyRumcajs
reviewed
Feb 6, 2026
LesnyRumcajs
reviewed
Feb 6, 2026
LesnyRumcajs
reviewed
Feb 6, 2026
LesnyRumcajs
reviewed
Feb 6, 2026
LesnyRumcajs
reviewed
Feb 6, 2026
LesnyRumcajs
reviewed
Feb 6, 2026
LesnyRumcajs
reviewed
Feb 6, 2026
![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: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/state_manager/mod.rs (1)
681-799:⚠️ Potential issue | 🟡 MinorUpdate public API docs for
flushand the optional state root return.These are signature‑level changes and should be reflected in doc comments.
📝 Suggested doc updates
+ /// Applies a message with gas estimation against a tipset/state. + /// If `flush` is true, returns the post-execution state root in the tuple. pub async fn apply_on_state_with_gas( self: &Arc<Self>, tipset: Option<Tipset>, msg: Message, state_lookup: StateLookupPolicy, flush: bool, ) -> anyhow::Result<(ApiInvocResult, Option<Cid>)> { ... } /// Computes message on the given [Tipset] state, after applying other /// messages and returns the values computed in the VM. + /// When `flush` is true, the last tuple element contains the new state root. pub async fn call_with_gas( self: &Arc<Self>, message: &mut ChainMessage, prior_messages: &[ChainMessage], tipset: Option<Tipset>, trace_config: VMTrace, state_lookup: StateLookupPolicy, flush: bool, ) -> Result<(InvocResult, ApplyRet, Duration, Option<Cid>), Error> { ... }As per coding guidelines: Document all public functions and structs with doc comments.
🤖 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 681 - 799, The public API lacks doc comments explaining the flush parameter and the optional state root in apply_on_state_with_gas and call_with_gas; add/update Rust doc comments for the public functions apply_on_state_with_gas and call_with_gas (and any public types used in their signatures such as ApiInvocResult) that clearly state what the flush bool does (i.e., whether it persists VM changes and causes a returned state root) and that the returned Option<Cid> may be None when flush is false or when no state root was produced; keep the docs short, include behavior for error cases and tracing (VMTrace) where relevant, and ensure the comments appear above the function signatures so they are included in generated docs.
🧹 Nitpick comments (1)
docs/docs/users/knowledge_base/trace_call.md (1)
1-13: Consider a slightly more playful intro tone.A small, friendly line up front would better match the docs voice while keeping the content clear.
✏️ Optional tone tweak
-`trace_call` executes an EVM call and returns detailed execution traces without creating a transaction on the blockchain. It's useful for: +`trace_call` executes an EVM call and returns detailed execution traces without creating a transaction on the blockchain. Think of it as a dress rehearsal before you put a transaction on-chain. It's useful for:Based on learnings: Maintain a playful, approachable tone in Forest project documentation (Markdown files) rather than strictly formal/corporate language. Allow humor or light jokes where appropriate, as long as the content remains clear and helpful.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/docs/users/knowledge_base/trace_call.md` around lines 1 - 13, Update the intro of the "trace_call API Guide" to use a slightly more playful, friendly tone: add a short welcoming one-liner before the Overview (e.g., a light, approachable sentence that references the `trace_call` RPC method) and adjust the first paragraph to keep the same technical links (`Parity/OpenEthereum`, `reth`) and facts but with warmer wording; ensure the heading "trace_call API Guide" and the mention of `trace_call` remain intact and do not change any technical details or links.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/docs/developers/guides/trace_call_guide.md`:
- Around line 72-75: The phrase uses unhyphenated compound adjectives; update
the wording to use hyphenated forms by changing "Geth style" to "Geth-style" and
"Parity style" to "Parity-style" in the sentence describing tracing (the parts
mentioning debug_traceCall/prestateTracer and trace_call/stateDiff) so the
compound adjectives are grammatically correct.
In `@scripts/tests/trace_call_integration_test.sh`:
- Around line 53-62: The deployment block may leave ANVIL_CONTRACT empty causing
later steps to run against a null address; after running the forge create
command that sets ANVIL_CONTRACT (using CONTRACT_PATH and the forge create
invocation), check that ANVIL_CONTRACT is non-empty and a valid address (e.g.,
non-zero length or matches 0x... pattern) and if not, print a clear error
including the raw command output and exit non‑zero so the test suite fails fast;
ensure this guard follows the ANVIL_CONTRACT assignment and references
DEPLOY_CONTRACT, ANVIL_CONTRACT and the forge create/jq pipeline so it triggers
only when deployment was attempted.
In `@src/rpc/methods/eth.rs`:
- Around line 3988-3990: The trace result construction currently swallows decode
errors by calling decode_payload(...).ok(); instead, update the code around
EthTraceResults (and the similar block at the other occurrence) to propagate the
decode error back to the RPC caller rather than returning null: replace the
.ok() usage on decode_payload with proper error handling (e.g., use the ?
operator or map_err to convert/return the decode error) so get_trace_output /
EthTraceResults returns an explicit failure when decode_payload fails instead of
a None/serialized null.
- Around line 474-501: Public types EthTraceType and EthTraceResults lack
struct/enum-level doc comments; add concise top-level doc comments above the
enum EthTraceType and the struct EthTraceResults describing their purpose and
when each field is used (e.g., that EthTraceType selects Trace vs StateDiff
behavior and EthTraceResults contains output, optional state_diff, and trace),
so the public API has docs consistent with project guidelines and Rust
doc-comment style (///).
In `@src/shim/actors/builtin/evm/mod.rs`:
- Around line 56-65: The new public accessor methods bytecode, bytecode_hash,
and contract_state lack doc comments; add concise Rust doc comments (/// ...)
above each public fn describing what the method returns, the units/type (e.g.,
Cid or [u8; 32]), and any important semantics (e.g., whether it is the deployed
bytecode, the hash algorithm, or the CID for contract state), so each of
bytecode(), bytecode_hash(), and contract_state() is documented per public API
guidelines.
---
Outside diff comments:
In `@src/state_manager/mod.rs`:
- Around line 681-799: The public API lacks doc comments explaining the flush
parameter and the optional state root in apply_on_state_with_gas and
call_with_gas; add/update Rust doc comments for the public functions
apply_on_state_with_gas and call_with_gas (and any public types used in their
signatures such as ApiInvocResult) that clearly state what the flush bool does
(i.e., whether it persists VM changes and causes a returned state root) and that
the returned Option<Cid> may be None when flush is false or when no state root
was produced; keep the docs short, include behavior for error cases and tracing
(VMTrace) where relevant, and ensure the comments appear above the function
signatures so they are included in generated docs.
---
Nitpick comments:
In `@docs/docs/users/knowledge_base/trace_call.md`:
- Around line 1-13: Update the intro of the "trace_call API Guide" to use a
slightly more playful, friendly tone: add a short welcoming one-liner before the
Overview (e.g., a light, approachable sentence that references the `trace_call`
RPC method) and adjust the first paragraph to keep the same technical links
(`Parity/OpenEthereum`, `reth`) and facts but with warmer wording; ensure the
heading "trace_call API Guide" and the mention of `trace_call` remain intact and
do not change any technical details or links.
Comment on lines
+53
to
+62
| if [ "$DEPLOY_CONTRACT" = true ]; then | ||
| command -v forge &>/dev/null || { echo "Error: forge is required for --deploy"; exit 1; } | ||
| echo -e "${YELLOW}Deploying Tracer contract on Anvil...${NC}" | ||
| CONTRACT_PATH="src/tool/subcommands/api_cmd/contracts/tracer/Tracer.sol" | ||
| ANVIL_CONTRACT=$(forge create "$CONTRACT_PATH:Tracer" \ | ||
| --rpc-url "$ANVIL_RPC_URL" \ | ||
| --private-key "$ANVIL_PRIVATE_KEY" \ | ||
| --broadcast --json 2>/dev/null | jq -r '.deployedTo') | ||
| echo -e "Deployed to: ${GREEN}$ANVIL_CONTRACT${NC}" | ||
| fi |
Contributor
There was a problem hiding this comment.
Fail fast if deployment doesn’t return a contract address.
This avoids running the rest of the suite with a null/empty $ANVIL_CONTRACT.
🛡️ Suggested guard
ANVIL_CONTRACT=$(forge create "$CONTRACT_PATH:Tracer" \
--rpc-url "$ANVIL_RPC_URL" \
--private-key "$ANVIL_PRIVATE_KEY" \
--broadcast --json 2>/dev/null | jq -r '.deployedTo')
+ if [[ -z "$ANVIL_CONTRACT" || "$ANVIL_CONTRACT" == "null" ]]; then
+ echo -e "${RED}Error: deployment failed; no contract address returned${NC}"
+ exit 1
+ fi
echo -e "Deployed to: ${GREEN}$ANVIL_CONTRACT${NC}"
fi🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/tests/trace_call_integration_test.sh` around lines 53 - 62, The
deployment block may leave ANVIL_CONTRACT empty causing later steps to run
against a null address; after running the forge create command that sets
ANVIL_CONTRACT (using CONTRACT_PATH and the forge create invocation), check that
ANVIL_CONTRACT is non-empty and a valid address (e.g., non-zero length or
matches 0x... pattern) and if not, print a clear error including the raw command
output and exit non‑zero so the test suite fails fast; ensure this guard follows
the ANVIL_CONTRACT assignment and references DEPLOY_CONTRACT, ANVIL_CONTRACT and
the forge create/jq pipeline so it triggers only when deployment was attempted.
Contributor
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
Comment on lines
+474
to
+501
| #[derive(PartialEq, Debug, Clone, Serialize, Deserialize, JsonSchema)] | ||
| #[serde(rename_all = "camelCase")] | ||
| pub enum EthTraceType { | ||
| /// Requests a structured call graph, showing the hierarchy of calls (e.g., `call`, `create`, `reward`) | ||
| /// with details like `from`, `to`, `gas`, `input`, `output`, and `subtraces`. | ||
| Trace, | ||
| /// Requests a state difference object, detailing changes to account states (e.g., `balance`, `nonce`, `storage`, `code`) | ||
| /// caused by the simulated transaction. | ||
| /// | ||
| /// It shows `"from"` and `"to"` values for modified fields, using `"+"`, `"-"`, or `"="` for code changes. | ||
| StateDiff, | ||
| } | ||
|
|
||
| lotus_json_with_self!(EthTraceType); | ||
|
|
||
| #[derive(PartialEq, Debug, Clone, Default, Serialize, Deserialize, JsonSchema)] | ||
| #[serde(rename_all = "camelCase")] | ||
| pub struct EthTraceResults { | ||
| /// Output bytes from the transaction execution | ||
| pub output: Option<EthBytes>, | ||
| #[serde(skip_serializing_if = "Option::is_none")] | ||
| /// State diff showing all account changes (only when StateDiff trace type requested) | ||
| pub state_diff: Option<StateDiff>, | ||
| /// Call trace hierarchy (only when Trace trace type requested) | ||
| pub trace: Vec<EthTrace>, | ||
| } | ||
|
|
||
| lotus_json_with_self!(EthTraceResults); |
Contributor
There was a problem hiding this comment.
Add top-level docs for the new public trace result types.
EthTraceResults is public but lacks a struct-level doc comment. Consider adding short type docs (same for EthTraceType) to meet the public docs rule.
✍️ Suggested doc comments
+/// Trace output selector for `trace_call`.
#[derive(PartialEq, Debug, Clone, Serialize, Deserialize, JsonSchema)]
#[serde(rename_all = "camelCase")]
pub enum EthTraceType {
@@
+/// Result payload returned by `trace_call`.
#[derive(PartialEq, Debug, Clone, Default, Serialize, Deserialize, JsonSchema)]
#[serde(rename_all = "camelCase")]
pub struct EthTraceResults {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| #[derive(PartialEq, Debug, Clone, Serialize, Deserialize, JsonSchema)] | |
| #[serde(rename_all = "camelCase")] | |
| pub enum EthTraceType { | |
| /// Requests a structured call graph, showing the hierarchy of calls (e.g., `call`, `create`, `reward`) | |
| /// with details like `from`, `to`, `gas`, `input`, `output`, and `subtraces`. | |
| Trace, | |
| /// Requests a state difference object, detailing changes to account states (e.g., `balance`, `nonce`, `storage`, `code`) | |
| /// caused by the simulated transaction. | |
| /// | |
| /// It shows `"from"` and `"to"` values for modified fields, using `"+"`, `"-"`, or `"="` for code changes. | |
| StateDiff, | |
| } | |
| lotus_json_with_self!(EthTraceType); | |
| #[derive(PartialEq, Debug, Clone, Default, Serialize, Deserialize, JsonSchema)] | |
| #[serde(rename_all = "camelCase")] | |
| pub struct EthTraceResults { | |
| /// Output bytes from the transaction execution | |
| pub output: Option<EthBytes>, | |
| #[serde(skip_serializing_if = "Option::is_none")] | |
| /// State diff showing all account changes (only when StateDiff trace type requested) | |
| pub state_diff: Option<StateDiff>, | |
| /// Call trace hierarchy (only when Trace trace type requested) | |
| pub trace: Vec<EthTrace>, | |
| } | |
| lotus_json_with_self!(EthTraceResults); | |
| /// Trace output selector for `trace_call`. | |
| #[derive(PartialEq, Debug, Clone, Serialize, Deserialize, JsonSchema)] | |
| #[serde(rename_all = "camelCase")] | |
| pub enum EthTraceType { | |
| /// Requests a structured call graph, showing the hierarchy of calls (e.g., `call`, `create`, `reward`) | |
| /// with details like `from`, `to`, `gas`, `input`, `output`, and `subtraces`. | |
| Trace, | |
| /// Requests a state difference object, detailing changes to account states (e.g., `balance`, `nonce`, `storage`, `code`) | |
| /// caused by the simulated transaction. | |
| /// | |
| /// It shows `"from"` and `"to"` values for modified fields, using `"+"`, `"-"`, or `"="` for code changes. | |
| StateDiff, | |
| } | |
| lotus_json_with_self!(EthTraceType); | |
| /// Result payload returned by `trace_call`. | |
| #[derive(PartialEq, Debug, Clone, Default, Serialize, Deserialize, JsonSchema)] | |
| #[serde(rename_all = "camelCase")] | |
| pub struct EthTraceResults { | |
| /// Output bytes from the transaction execution | |
| pub output: Option<EthBytes>, | |
| #[serde(skip_serializing_if = "Option::is_none")] | |
| /// State diff showing all account changes (only when StateDiff trace type requested) | |
| pub state_diff: Option<StateDiff>, | |
| /// Call trace hierarchy (only when Trace trace type requested) | |
| pub trace: Vec<EthTrace>, | |
| } | |
| lotus_json_with_self!(EthTraceResults); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/rpc/methods/eth.rs` around lines 474 - 501, Public types EthTraceType and
EthTraceResults lack struct/enum-level doc comments; add concise top-level doc
comments above the enum EthTraceType and the struct EthTraceResults describing
their purpose and when each field is used (e.g., that EthTraceType selects Trace
vs StateDiff behavior and EthTraceResults contains output, optional state_diff,
and trace), so the public API has docs consistent with project guidelines and
Rust doc-comment style (///).
Comment on lines
+3988
to
+3990
| let mut trace_results = EthTraceResults { | ||
| output: get_trace_output(&msg, &invoke_result), | ||
| ..Default::default() |
Contributor
There was a problem hiding this comment.
Don’t swallow return-data decode errors.
decode_payload(...).ok() turns decode failures into a None output (serialized as null) instead of surfacing an error, diverging from eth_call behavior. Prefer propagating the decode error so clients get a clear failure.
🐛 Suggested fix
- let mut trace_results = EthTraceResults {
- output: get_trace_output(&msg, &invoke_result),
+ let mut trace_results = EthTraceResults {
+ output: get_trace_output(&msg, &invoke_result)?,
..Default::default()
};-fn get_trace_output(msg: &Message, invoke_result: &ApiInvocResult) -> Option<EthBytes> {
+fn get_trace_output(msg: &Message, invoke_result: &ApiInvocResult) -> Result<Option<EthBytes>> {
if msg.to() == FilecoinAddress::ETHEREUM_ACCOUNT_MANAGER_ACTOR {
- return Some(EthBytes::default());
+ return Ok(Some(EthBytes::default()));
}
- let msg_rct = invoke_result.msg_rct.as_ref()?;
+ let msg_rct = invoke_result.msg_rct.as_ref().context("no message receipt")?;
let return_data = msg_rct.return_data();
if return_data.is_empty() {
- return Some(EthBytes::default());
+ return Ok(Some(EthBytes::default()));
}
- decode_payload(&return_data, CBOR).ok()
+ let bytes = decode_payload(&return_data, CBOR)?;
+ Ok(Some(bytes))
}Also applies to: 4030-4044
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/rpc/methods/eth.rs` around lines 3988 - 3990, The trace result
construction currently swallows decode errors by calling
decode_payload(...).ok(); instead, update the code around EthTraceResults (and
the similar block at the other occurrence) to propagate the decode error back to
the RPC caller rather than returning null: replace the .ok() usage on
decode_payload with proper error handling (e.g., use the ? operator or map_err
to convert/return the decode error) so get_trace_output / EthTraceResults
returns an explicit failure when decode_payload fails instead of a
None/serialized null.
Comment on lines
+56
to
+65
| pub fn bytecode(&self) -> Cid { | ||
| delegate_state!(self.bytecode) | ||
| } | ||
|
|
||
| pub fn bytecode_hash(&self) -> [u8; 32] { | ||
| delegate_state!(self.bytecode_hash.into()) | ||
| } | ||
|
|
||
| pub fn contract_state(&self) -> Cid { | ||
| delegate_state!(self.contract_state) |
Contributor
There was a problem hiding this comment.
Add doc comments for the new public accessors.
These are public APIs and need doc comments for clarity and guideline compliance.
📝 Suggested doc comments
+ /// Returns the CID of the actor bytecode.
pub fn bytecode(&self) -> Cid {
delegate_state!(self.bytecode)
}
+ /// Returns the 32-byte hash of the actor bytecode.
pub fn bytecode_hash(&self) -> [u8; 32] {
delegate_state!(self.bytecode_hash.into())
}
+ /// Returns the CID of the contract state root.
pub fn contract_state(&self) -> Cid {
delegate_state!(self.contract_state)
}As per coding guidelines: Document all public functions and structs with doc comments.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| pub fn bytecode(&self) -> Cid { | |
| delegate_state!(self.bytecode) | |
| } | |
| pub fn bytecode_hash(&self) -> [u8; 32] { | |
| delegate_state!(self.bytecode_hash.into()) | |
| } | |
| pub fn contract_state(&self) -> Cid { | |
| delegate_state!(self.contract_state) | |
| /// Returns the CID of the actor bytecode. | |
| pub fn bytecode(&self) -> Cid { | |
| delegate_state!(self.bytecode) | |
| } | |
| /// Returns the 32-byte hash of the actor bytecode. | |
| pub fn bytecode_hash(&self) -> [u8; 32] { | |
| delegate_state!(self.bytecode_hash.into()) | |
| } | |
| /// Returns the CID of the contract state root. | |
| pub fn contract_state(&self) -> Cid { | |
| delegate_state!(self.contract_state) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/shim/actors/builtin/evm/mod.rs` around lines 56 - 65, The new public
accessor methods bytecode, bytecode_hash, and contract_state lack doc comments;
add concise Rust doc comments (/// ...) above each public fn describing what the
method returns, the units/type (e.g., Cid or [u8; 32]), and any important
semantics (e.g., whether it is the deployed bytecode, the hash algorithm, or the
CID for contract state), so each of bytecode(), bytecode_hash(), and
contract_state() is documented per public API guidelines.
dcd901d to
57ad705
Compare
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/rpc/methods/eth.rs (1)
3944-3952: Add documentation forEthTraceCall.The public
EthTraceCallenum lacks a doc comment. Per coding guidelines, all public types should have documentation describing their purpose and usage.+/// Executes a call and returns traces for the transaction. +/// +/// Supports both call traces (showing the hierarchy of calls) and state diffs +/// (showing account state changes) based on the requested trace types. pub enum EthTraceCall {}As per coding guidelines: "Document all public functions and structs with doc comments".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/rpc/methods/eth.rs` around lines 3944 - 3952, Add a doc comment for the public enum EthTraceCall explaining its purpose and usage: describe that EthTraceCall implements RpcMethod for the "Forest.EthTraceCall"/"trace_call" RPC to return transaction traces, summarize expected parameters (tx, traceTypes, blockParam), note permissions (Permission::Read) and any important behavior/return expectations; place the documentation immediately above the `pub enum EthTraceCall {}` declaration so it appears in generated docs and IDE tooltips.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/rpc/methods/eth.rs`:
- Around line 474-501: Add concise top-level documentation comments for the
public enum EthTraceType and the public struct EthTraceResults: place
triple-slash doc comments (///) immediately above the EthTraceType enum
describing its purpose (to select the trace output format returned by
eth_call/trace APIs, e.g., structured call traces vs state diffs) and
immediately above EthTraceResults describing that it encapsulates the returned
trace output (output bytes, optional state_diff, and trace vector) and when each
field is populated; keep wording brief and public-facing so crate docs surface
the intent of these types.
- Around line 3993-3996: The current code silently converts payload decode
failures into None by calling get_trace_output(&msg, &invoke_result) when
building EthTraceResults, which hides decode errors; change get_trace_output to
return a Result<Option<T>, E> (or otherwise surface the decode_payload error)
and update the EthTraceResults construction site to propagate that error instead
of swallowing it—i.e., call get_trace_output(...)? (or handle Err by returning
the error) so decode_payload failures are returned to the eth_call client rather
than converted into null output.
- Around line 4035-4049: The get_trace_output function currently swallows decode
errors by calling decode_payload(...).ok(); change get_trace_output (and its
callers) to return Result<Option<EthBytes>, RpcError> (or the project’s standard
error type) instead of Option<EthBytes>, replace the final .ok() with a
?-propagated decode_payload(...) call so failure returns an Err with the decode
error, and ensure callers of get_trace_output (and any upstream RPC response
builders) are updated to handle and surface that Err to clients; reference
get_trace_output, decode_payload, ApiInvocResult, and EthBytes when making these
changes.
---
Nitpick comments:
In `@src/rpc/methods/eth.rs`:
- Around line 3944-3952: Add a doc comment for the public enum EthTraceCall
explaining its purpose and usage: describe that EthTraceCall implements
RpcMethod for the "Forest.EthTraceCall"/"trace_call" RPC to return transaction
traces, summarize expected parameters (tx, traceTypes, blockParam), note
permissions (Permission::Read) and any important behavior/return expectations;
place the documentation immediately above the `pub enum EthTraceCall {}`
declaration so it appears in generated docs and IDE tooltips.
Forest.EthTraceCall [Skip CI]Forest.EthTraceCall
akaladarshi
force-pushed
the
akaladarshi/eth-trace-call
branch
from
1c0d067 to
812e5e2
Compare
Codecov Report❌ Patch coverage is Additional details and impacted files
... and 6 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
Contributor
There was a problem hiding this comment.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/rpc/methods/eth.rs`:
- Around line 3988-4077: The output decoding in trace_call currently swallows
decode errors via decode_payload(...).ok(); locate get_trace_output (and the
other occurrence around the 4079–4093 area) and replace the .ok() usage so the
decode error is propagated instead: call decode_payload(...) and use ? to return
the error, adding context with .context("failed to decode return data for
trace_call") (or an equivalent descriptive message) so the error flows back
through the existing Result/ServerError handling rather than producing a null
output.
Contributor
|
@akaladarshi enable |
Member
There was a problem hiding this comment.
I can imagine more methods being explained; let's put it into an knowledge base/RPC directory.
Member
There was a problem hiding this comment.
some of the entries don't look like something that should be added; if it's a variable name or a method name, it should be done inline within the doc.
| } | ||
|
|
||
| impl EthAddress { | ||
| #[allow(clippy::wrong_self_convention)] |
| Some(ts.clone()), | ||
| trace_config, | ||
| StateLookupPolicy::Enabled, | ||
| false, |
Member
There was a problem hiding this comment.
I recommend not using booleans as parameters unless it's obvious what they do. When you're reading call_with_gas (and don't recall what's the method signature), you probably can imagine what StateLookupPolicy::Enabled signifies; on the other hand, false doesn't tell you anything.
I'd move it to an enumeration.
Collaborator
Author
There was a problem hiding this comment.
Yeah I total agree here, that's why I suggested to go with the boolean for now as I am going to raise a subsequent PR to refactor the state_manager in that we will be having more structure to the methods and there i can add this as well, WDYT?
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
4 participants
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 of changes
Changes introduced in this pull request:
Forest.EthTraceCall#6195Reference issue to close (if applicable)
Closes #6185
Other information and links
Change checklist
Summary by CodeRabbit
New Features
Documentation
Tests
Chores