Draft
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:
WalkthroughThis PR implements the Changes
Sequence DiagramsequenceDiagram
participant Client
participant RPC as RPC Handler<br/>(trace_call)
participant StateManager
participant VM
participant Store as Blockstore
Client->>RPC: call trace_call(message, traceTypes, blockTag)
RPC->>StateManager: Get state for blockTag
StateManager->>Store: Load pre-execution state
Store-->>StateManager: pre_state
RPC->>VM: Invoke transaction with gas
VM->>VM: Execute message
VM-->>RPC: execution result, gas usage
RPC->>StateManager: Get post-execution state
StateManager->>Store: Load post-execution state
Store-->>StateManager: post_state
alt if StateDiff requested
RPC->>RPC: build_state_diff(pre_state, post_state, touched_addresses)
RPC->>Store: Extract EVM storage entries
Store-->>RPC: storage map
RPC->>RPC: Compute Delta for each slot
RPC->>RPC: Compare balance, nonce, code
end
RPC->>RPC: Format traces and state diffs
RPC-->>Client: EthTraceResults (traces + optional stateDiff)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 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.
🧹 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 11 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.
src/rpc/methods/eth/types.rs
Outdated
| } | ||
|
|
||
| impl EthAddress { | ||
| #[allow(clippy::wrong_self_convention)] |
Collaborator
Author
There was a problem hiding this comment.
The linter is because of this, not sure why now and not on main branch.
[lint:clippy] error: methods with the following characteristics: (
to_*andselftype isCopy) usually takeselfby value
[lint:clippy] --> src/rpc/methods/eth/types.rs:134:32
[lint:clippy] |
[lint:clippy] 134 | pub fn to_filecoin_address(&self) -> anyhow::Result {
[lint:clippy] | ^^^^^
[lint:clippy] |
[lint:clippy] = help: consider choosing a less ambiguous name
[lint:clippy] = help: for further information visit https://rust-lang.github.io/rust-clippy/rust-1.93.0/index.html#wrong_self_convention
[lint:clippy] = note:-D clippy::wrong-self-conventionimplied by-D warnings
[lint:clippy] = help: to override-D warningsadd#[allow(clippy::wrong_self_convention)]
[lint:clippy]
| 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?
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/types.rs`:
- Around line 785-844: Add doc comments to all public helpers related to diffs:
document Delta::from_comparison (describe that it compares old/new Option<T> and
returns Added/Removed/Changed/Unchanged), Delta::is_unchanged (explain it
returns true only for Delta::Unchanged), AccountDiff::is_unchanged (explain it
tests all fields and empty storage), StateDiff::new (note it constructs an empty
StateDiff) and StateDiff::insert_if_changed (state that it inserts the
AccountDiff only when not unchanged). Use /// style comments placed immediately
above each function/struct definition, briefly describing inputs, behavior, and
any important side-effects (e.g., that insert_if_changed skips unchanged diffs).
---
Duplicate comments:
In `@src/rpc/methods/eth.rs`:
- Around line 474-501: Add top-level doc comments for the public trace types:
place concise /// doc comments above the EthTraceType enum describing it
represents the two supported trace output modes (structured call graph "Trace"
and state changes "StateDiff") and above the EthTraceResults struct describing
it represents the combined trace result (output bytes, optional state_diff, and
call trace Vec<EthTrace>) so consumers can discover purpose and fields; keep
existing variant/field docs and serde/lifecycle attributes
(lotus_json_with_self!, #[serde(...)] and derives) intact and ensure comments
follow crate style (brief summary sentence and any important usage notes).
- Around line 4078-4091: In get_trace_output, stop swallowing decode errors by
removing the final .ok(); change the function to propagate decode failures
(e.g., change the signature from fn get_trace_output(...) -> Option<EthBytes> to
return Result<Option<EthBytes>, E>), replace decode_payload(&return_data,
CBOR).ok() with a fallible propagation (e.g., decode_payload(&return_data,
CBOR)? wrapped to produce Ok(Some(...))), and convert the early default-return
branches to Ok(Some(EthBytes::default())) or Ok(None) as appropriate so callers
of get_trace_output (and types like ApiInvocResult and EthBytes) must handle
decoding errors instead of silently receiving None.
Comment on lines
+785
to
+844
| impl<T: PartialEq> Delta<T> { | ||
| pub fn from_comparison(old: Option<T>, new: Option<T>) -> Self { | ||
| match (old, new) { | ||
| (None, None) => Delta::Unchanged, | ||
| (None, Some(new_val)) => Delta::Added(new_val), | ||
| (Some(old_val), None) => Delta::Removed(old_val), | ||
| (Some(old_val), Some(new_val)) => { | ||
| if old_val == new_val { | ||
| Delta::Unchanged | ||
| } else { | ||
| Delta::Changed(ChangedType { | ||
| from: old_val, | ||
| to: new_val, | ||
| }) | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
||
| pub fn is_unchanged(&self) -> bool { | ||
| matches!(self, Delta::Unchanged) | ||
| } | ||
| } | ||
|
|
||
| /// Account state diff after transaction execution. | ||
| /// Tracks changes to balance, nonce, code, and storage. | ||
| // Taken from https://github.com/alloy-rs/alloy/blob/v1.5.2/crates/rpc-types-trace/src/parity.rs#L156 | ||
| #[derive(Clone, Debug, Default, PartialEq, Eq, Serialize, Deserialize, JsonSchema)] | ||
| pub struct AccountDiff { | ||
| pub balance: Delta<EthBigInt>, | ||
| pub code: Delta<EthBytes>, | ||
| pub nonce: Delta<EthUint64>, | ||
| /// All touched/changed storage values (key -> delta) | ||
| pub storage: BTreeMap<EthHash, Delta<EthHash>>, | ||
| } | ||
|
|
||
| impl AccountDiff { | ||
| pub fn is_unchanged(&self) -> bool { | ||
| self.balance.is_unchanged() | ||
| && self.code.is_unchanged() | ||
| && self.nonce.is_unchanged() | ||
| && self.storage.is_empty() | ||
| } | ||
| } | ||
|
|
||
| /// State diff containing all account changes from a transaction. | ||
| #[derive(Clone, Debug, Default, PartialEq, Eq, Serialize, Deserialize, JsonSchema)] | ||
| #[serde(transparent)] | ||
| pub struct StateDiff(pub BTreeMap<EthAddress, AccountDiff>); | ||
|
|
||
| impl StateDiff { | ||
| pub fn new() -> Self { | ||
| Self(BTreeMap::new()) | ||
| } | ||
|
|
||
| pub fn insert_if_changed(&mut self, addr: EthAddress, diff: AccountDiff) { | ||
| if !diff.is_unchanged() { | ||
| self.0.insert(addr, diff); | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
Add doc comments for public diff helpers.
Public helper methods were added without doc comments.
✍️ Suggested doc comments
impl<T: PartialEq> Delta<T> {
+ /// Build a delta from optional old/new values.
pub fn from_comparison(old: Option<T>, new: Option<T>) -> Self {
@@
- pub fn is_unchanged(&self) -> bool {
+ /// Returns true when the delta represents no change.
+ pub fn is_unchanged(&self) -> bool {
matches!(self, Delta::Unchanged)
}
}
@@
impl AccountDiff {
+ /// Returns true when all tracked fields are unchanged and storage is empty.
pub fn is_unchanged(&self) -> bool {
self.balance.is_unchanged()
@@
impl StateDiff {
+ /// Create an empty state diff.
pub fn new() -> Self {
Self(BTreeMap::new())
}
+ /// Insert the account diff only when it contains changes.
pub fn insert_if_changed(&mut self, addr: EthAddress, diff: AccountDiff) {
if !diff.is_unchanged() {
self.0.insert(addr, diff);
}
}
}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
| impl<T: PartialEq> Delta<T> { | |
| pub fn from_comparison(old: Option<T>, new: Option<T>) -> Self { | |
| match (old, new) { | |
| (None, None) => Delta::Unchanged, | |
| (None, Some(new_val)) => Delta::Added(new_val), | |
| (Some(old_val), None) => Delta::Removed(old_val), | |
| (Some(old_val), Some(new_val)) => { | |
| if old_val == new_val { | |
| Delta::Unchanged | |
| } else { | |
| Delta::Changed(ChangedType { | |
| from: old_val, | |
| to: new_val, | |
| }) | |
| } | |
| } | |
| } | |
| } | |
| pub fn is_unchanged(&self) -> bool { | |
| matches!(self, Delta::Unchanged) | |
| } | |
| } | |
| /// Account state diff after transaction execution. | |
| /// Tracks changes to balance, nonce, code, and storage. | |
| // Taken from https://github.com/alloy-rs/alloy/blob/v1.5.2/crates/rpc-types-trace/src/parity.rs#L156 | |
| #[derive(Clone, Debug, Default, PartialEq, Eq, Serialize, Deserialize, JsonSchema)] | |
| pub struct AccountDiff { | |
| pub balance: Delta<EthBigInt>, | |
| pub code: Delta<EthBytes>, | |
| pub nonce: Delta<EthUint64>, | |
| /// All touched/changed storage values (key -> delta) | |
| pub storage: BTreeMap<EthHash, Delta<EthHash>>, | |
| } | |
| impl AccountDiff { | |
| pub fn is_unchanged(&self) -> bool { | |
| self.balance.is_unchanged() | |
| && self.code.is_unchanged() | |
| && self.nonce.is_unchanged() | |
| && self.storage.is_empty() | |
| } | |
| } | |
| /// State diff containing all account changes from a transaction. | |
| #[derive(Clone, Debug, Default, PartialEq, Eq, Serialize, Deserialize, JsonSchema)] | |
| #[serde(transparent)] | |
| pub struct StateDiff(pub BTreeMap<EthAddress, AccountDiff>); | |
| impl StateDiff { | |
| pub fn new() -> Self { | |
| Self(BTreeMap::new()) | |
| } | |
| pub fn insert_if_changed(&mut self, addr: EthAddress, diff: AccountDiff) { | |
| if !diff.is_unchanged() { | |
| self.0.insert(addr, diff); | |
| } | |
| } | |
| impl<T: PartialEq> Delta<T> { | |
| /// Build a delta from optional old/new values. | |
| pub fn from_comparison(old: Option<T>, new: Option<T>) -> Self { | |
| match (old, new) { | |
| (None, None) => Delta::Unchanged, | |
| (None, Some(new_val)) => Delta::Added(new_val), | |
| (Some(old_val), None) => Delta::Removed(old_val), | |
| (Some(old_val), Some(new_val)) => { | |
| if old_val == new_val { | |
| Delta::Unchanged | |
| } else { | |
| Delta::Changed(ChangedType { | |
| from: old_val, | |
| to: new_val, | |
| }) | |
| } | |
| } | |
| } | |
| } | |
| /// Returns true when the delta represents no change. | |
| pub fn is_unchanged(&self) -> bool { | |
| matches!(self, Delta::Unchanged) | |
| } | |
| } | |
| /// Account state diff after transaction execution. | |
| /// Tracks changes to balance, nonce, code, and storage. | |
| // Taken from https://github.com/alloy-rs/alloy/blob/v1.5.2/crates/rpc-types-trace/src/parity.rs#L156 | |
| #[derive(Clone, Debug, Default, PartialEq, Eq, Serialize, Deserialize, JsonSchema)] | |
| pub struct AccountDiff { | |
| pub balance: Delta<EthBigInt>, | |
| pub code: Delta<EthBytes>, | |
| pub nonce: Delta<EthUint64>, | |
| /// All touched/changed storage values (key -> delta) | |
| pub storage: BTreeMap<EthHash, Delta<EthHash>>, | |
| } | |
| impl AccountDiff { | |
| /// Returns true when all tracked fields are unchanged and storage is empty. | |
| pub fn is_unchanged(&self) -> bool { | |
| self.balance.is_unchanged() | |
| && self.code.is_unchanged() | |
| && self.nonce.is_unchanged() | |
| && self.storage.is_empty() | |
| } | |
| } | |
| /// State diff containing all account changes from a transaction. | |
| #[derive(Clone, Debug, Default, PartialEq, Eq, Serialize, Deserialize, JsonSchema)] | |
| #[serde(transparent)] | |
| pub struct StateDiff(pub BTreeMap<EthAddress, AccountDiff>); | |
| impl StateDiff { | |
| /// Create an empty state diff. | |
| pub fn new() -> Self { | |
| Self(BTreeMap::new()) | |
| } | |
| /// Insert the account diff only when it contains changes. | |
| pub fn insert_if_changed(&mut self, addr: EthAddress, diff: AccountDiff) { | |
| if !diff.is_unchanged() { | |
| self.0.insert(addr, diff); | |
| } | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/rpc/methods/eth/types.rs` around lines 785 - 844, Add doc comments to all
public helpers related to diffs: document Delta::from_comparison (describe that
it compares old/new Option<T> and returns Added/Removed/Changed/Unchanged),
Delta::is_unchanged (explain it returns true only for Delta::Unchanged),
AccountDiff::is_unchanged (explain it tests all fields and empty storage),
StateDiff::new (note it constructs an empty StateDiff) and
StateDiff::insert_if_changed (state that it inserts the AccountDiff only when
not unchanged). Use /// style comments placed immediately above each
function/struct definition, briefly describing inputs, behavior, and any
important side-effects (e.g., that insert_if_changed skips unchanged diffs).
8b47987 to
ee300c5
Compare
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/rpc/methods/eth.rs (1)
4015-4031: Prefer.context()to wrap state/apply errors.
Keeps error chains consistent with project style.As per coding guidelines: "Use `anyhow::Result` for most operations, adding context with `.context()`".♻️ Suggested tweak
- let (pre_state_root, _) = ctx - .state_manager - .tipset_state(&ts, StateLookupPolicy::Enabled) - .await - .map_err(|e| anyhow::anyhow!("failed to get tipset state: {e}"))?; + let (pre_state_root, _) = ctx + .state_manager + .tipset_state(&ts, StateLookupPolicy::Enabled) + .await + .context("failed to get tipset state")?; @@ - .await - .map_err(|e| anyhow::anyhow!("failed to apply message: {e}"))?; + .await + .context("failed to apply message")?;🤖 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 4015 - 4031, Wrap the error returns with anyhow::Context instead of map_err(anyhow::anyhow!(...)): add use anyhow::Context if missing, change the tipset_state call to use .await.context("failed to get tipset state"), change StateTree::new_from_root(...) to append .context("failed to create StateTree from root"), and change the apply_on_state_with_gas call to .await.context("failed to apply message"); keep the same messages but use .context() for consistent error chaining around tipset_state, StateTree::new_from_root, and apply_on_state_with_gas.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@docs/docs/developers/guides/trace_call_guide.md`:
- Around line 72-75: Replace the unhyphenated compound adjectives "Geth style
tracing" and "Parity style tracing" with hyphenated forms "Geth-style tracing"
and "Parity-style tracing" in the document so the compounds are grammatically
correct; search for the exact phrases "Geth style" and "Parity style" in the
text and update them accordingly.
In `@src/rpc/methods/eth.rs`:
- Around line 474-501: Public API types EthTraceType and EthTraceResults lack
doc comments; add concise top-level Rust doc comments (/// ...) describing each
type's purpose and behavior, plus brief docs for each EthTraceType variant
(Trace, StateDiff) and for EthTraceResults fields (output, state_diff, trace) to
explain when they are populated and their semantics; ensure comments follow
camelCase/serde intent and appear above the enum, its variants, and the
struct/fields (retain existing serde attributes and lotus_json_with_self!
macros).
- Around line 4036-4038: The code currently swallows decoding failures by
calling decode_payload(...).ok() when building EthTraceResults (around the
get_trace_output/invoke_result usage); change that to propagate the decode error
instead of turning it into None — i.e., remove .ok() and use the fallible call
(e.g., decode_payload(...) ? or decode_payload(...).map_err(|e|
convert_to_rpc_error(e))?) so the enclosing function returns an Err on decode
failure; apply the same fix to the other occurrence that mirrors this behavior
later in the file (the second decode_payload(...) call used when constructing
trace output).
In `@src/rpc/methods/eth/types.rs`:
- Around line 785-845: Public API items lack doc comments; add concise doc
comments for Delta::from_comparison, Delta::is_unchanged, the AccountDiff struct
and AccountDiff::is_unchanged, and the StateDiff struct plus StateDiff::new and
StateDiff::insert_if_changed that explain purpose, parameters and return
behavior (e.g., what from_comparison does with old/new Options, what
is_unchanged checks, that StateDiff is a transparent BTreeMap of account ->
AccountDiff, new creates an empty StateDiff, and insert_if_changed only inserts
non-unchanged AccountDiffs). Keep comments short, use /// style and mention any
invariants or important behavior (like storage emptiness meaning unchanged).
---
Nitpick comments:
In `@src/rpc/methods/eth.rs`:
- Around line 4015-4031: Wrap the error returns with anyhow::Context instead of
map_err(anyhow::anyhow!(...)): add use anyhow::Context if missing, change the
tipset_state call to use .await.context("failed to get tipset state"), change
StateTree::new_from_root(...) to append .context("failed to create StateTree
from root"), and change the apply_on_state_with_gas call to
.await.context("failed to apply message"); keep the same messages but use
.context() for consistent error chaining around tipset_state,
StateTree::new_from_root, and apply_on_state_with_gas.
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