Conversation
WalkthroughRefactors eth RPC state access to use a centralized state manager, converts eth_get_balance to async (and await call sites), updates actor lookup/error handling, and adds randomized eth address tests plus expanded RPC snapshot variants. Changes
Sequence Diagram(s)sequenceDiagram
participant RPC as RPC Handler
participant SM as State Manager
participant BS as Blockstore
participant ST as StateTree
rect rgba(220,53,69,0.5)
Note over RPC,BS: Old flow — direct reconstruction
RPC->>BS: StateTree::new_from_root(store, root_cid)
BS-->>ST: reconstruct state tree
ST-->>RPC: StateTree
RPC->>ST: get_actor(address)
end
rect rgba(40,167,69,0.5)
Note over RPC,SM: New flow — manager-mediated + async balance
RPC->>SM: get_state_tree(root_cid)?
SM->>BS: resolve/access state CID
SM-->>RPC: StateTree
RPC->>ST: get_actor(address)
RPC->>RPC: call async eth_get_balance(...).await
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 3❌ Failed checks (1 warning, 2 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/tool/subcommands/api_cmd/test_snapshots.txt (1)
61-111:⚠️ Potential issue | 🟠 MajorFix list ordering to satisfy
lint:lists.The pipeline reports a sorting diff for this file, so the new snapshot entries appear out of order. Please re-run the list-sorting task (or apply the repo’s list-sort convention) and commit the sorted result to clear
lint:lists.
🧹 Nitpick comments (1)
src/rpc/methods/eth.rs (1)
589-590: Add error context to new state-tree lookups.These new
get_state_treecalls are a good place to attach context so failures are easier to diagnose in logs.Suggested change (example)
- let state_tree = ctx.state_manager.get_state_tree(&state_root)?; + let state_tree = ctx + .state_manager + .get_state_tree(&state_root) + .context("failed to load state tree for executed tipset")?;As per coding guidelines: Use
anyhow::Result<T>for most operations and add context with.context()method.Also applies to: 1448-1449, 1703-1704, 2879-2880, 2914-2915, 3896-3897, 4050-4051
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:
|
LesnyRumcajs
left a comment
There was a problem hiding this comment.
Let's have a changelog entry for this fix.
|
Blocked by #6547. Once it's merged, please rebase. |
Summary of changes
Changes introduced in this pull request:
The EVM returns the
0value instead of error for couple of the API's if the actor/address is not found in the state tree (This behaviour was missing in forest),forgeinternally uses these API to create proper transactions.Refactored some of the ETH API's to use the
StateMangerinstead of directly accessing the state tree, so we have single point of contact with the state tree in the API's at least. This will help us in future to make thestateManageras trait with public facing API's.Reference issue to close (if applicable)
Closes #6525
Other information and links
Change checklist
Outside contributions
Summary by CodeRabbit
Improvements
Bug Fixes
Tests