feat: add support for ethaccount actor in state decode params API #5995
feat: add support for ethaccount actor in state decode params API #5995akaladarshi merged 14 commits intomainfrom
Conversation
akaladarshi
requested review from
elmattic and
hanabi1224
and removed request for
a team
|
Important Review skippedReview was skipped due to path filters ⛔ Files ignored due to path filters (1)
CodeRabbit blocks several paths by default. You can override this behavior by explicitly including those paths in the path filters. For example, including You can disable this status message by setting the WalkthroughAdds EthAccount actor support to the RPC method registry: new version-aware eth_account registration module (v10–v16), wiring into the registry match on BuiltinActor::EthAccount, dependency Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Client
participant Registry as MethodRegistry
participant Matcher as BuiltinActor matcher
participant EthAcc as eth_account::register_actor_methods
participant StateMod as fil_actor_ethaccount_state (v10..v16)
Client->>Registry: request registration(cid, version, builtin)
Registry->>Matcher: match builtin actor
Matcher-->>Registry: EthAccount
Registry->>EthAcc: register_actor_methods(registry, cid, version)
alt version in 10..16
EthAcc->>StateMod: select v{version} module
Note over EthAcc,StateMod #D6EAF8: register Constructor (no params)
EthAcc-->>Registry: methods registered
else unsupported version
EthAcc-->>Registry: no-op
end
Registry-->>Client: registration complete
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Assessment against linked issues
Possibly related issues
Possibly related PRs
Suggested reviewers
✨ Finishing Touches🧪 Generate unit tests
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (6)
src/tool/subcommands/api_cmd/api_compare_tests.rs (1)
1868-1874: Add EthAccount Constructor decode-params test via delegated addressGood addition; this exercises the new registry path using a delegated address in the EAM namespace and empty params. Minor nit: replace unwrap with expect for clearer failure context.
- RpcTest::identity(StateDecodeParams::request(( - Address::new_delegated(Address::ETHEREUM_ACCOUNT_MANAGER_ACTOR.id()?, &[0; 20]) - .unwrap(), + RpcTest::identity(StateDecodeParams::request(( + Address::new_delegated( + Address::ETHEREUM_ACCOUNT_MANAGER_ACTOR.id()?, + &[0; 20], + ) + .expect("failed to construct delegated EAM address"), fil_actor_ethaccount_state::v16::Method::Constructor as u64, vec![], tipset.key().into(), ))?),src/rpc/registry/actors/eth_account.rs (3)
4-7: Avoid name ambiguity and drop unused import
- The local function is named register_actor_methods, and the macro used is also named register_actor_methods!. While different namespaces prevent a hard conflict, aliasing the macro improves readability and avoids confusion during navigation and tooling.
- MethodNum is imported but unused.
-use crate::rpc::registry::methods_reg::{MethodRegistry, register_actor_methods}; -use crate::shim::message::MethodNum; +use crate::rpc::registry::methods_reg::{ + MethodRegistry, + register_actor_methods as register_actor_methods_macro, +};
8-15: Macro naming and invocation consistencyThe macro name register_eth_account_reg_version! is slightly awkward and inconsistent with similar modules (e.g., account’s register_account_version!). Consider renaming for consistency and updating the invocation of the aliased register_actor_methods macro for clarity.
-macro_rules! register_eth_account_reg_version { +macro_rules! register_eth_account_version { ($registry:expr, $code_cid:expr, $state_version:path) => {{ use $state_version::*; // Constructor has no parameters - register_actor_methods!($registry, $code_cid, [(Method::Constructor, empty),]); + register_actor_methods_macro!($registry, $code_cid, [(Method::Constructor, empty),]); }}; }Note: If you keep the current macro name, still prefer the aliased invocation:
- register_actor_methods!($registry, $code_cid, [(Method::Constructor, empty),]); + register_actor_methods_macro!($registry, $code_cid, [(Method::Constructor, empty),]);
17-28: Version dispatch covers v10–v16; Constructor-only mapping is fineThe version switch is complete for the supported range and correctly registers Constructor with empty params. No issues spotted. If you rename the macro as suggested, update the calls accordingly.
- match version { - 10 => register_eth_account_reg_version!(registry, cid, fil_actor_ethaccount_state::v10), - 11 => register_eth_account_reg_version!(registry, cid, fil_actor_ethaccount_state::v11), - 12 => register_eth_account_reg_version!(registry, cid, fil_actor_ethaccount_state::v12), - 13 => register_eth_account_reg_version!(registry, cid, fil_actor_ethaccount_state::v13), - 14 => register_eth_account_reg_version!(registry, cid, fil_actor_ethaccount_state::v14), - 15 => register_eth_account_reg_version!(registry, cid, fil_actor_ethaccount_state::v15), - 16 => register_eth_account_reg_version!(registry, cid, fil_actor_ethaccount_state::v16), + match version { + 10 => register_eth_account_version!(registry, cid, fil_actor_ethaccount_state::v10), + 11 => register_eth_account_version!(registry, cid, fil_actor_ethaccount_state::v11), + 12 => register_eth_account_version!(registry, cid, fil_actor_ethaccount_state::v12), + 13 => register_eth_account_version!(registry, cid, fil_actor_ethaccount_state::v13), + 14 => register_eth_account_version!(registry, cid, fil_actor_ethaccount_state::v14), + 15 => register_eth_account_version!(registry, cid, fil_actor_ethaccount_state::v15), + 16 => register_eth_account_version!(registry, cid, fil_actor_ethaccount_state::v16), _ => {} }src/rpc/registry/methods_reg.rs (2)
128-131: Minor: avoid an extra String allocation when base64-encoding bytes.
BASE64_STANDARD.encode(bytes).as_str()builds aStringand then converts to&str, whichserde_json::json!will re-allocate into aStringagain. Passing theStringdirectly avoids the extra copy.Apply this diff:
- use base64::{Engine as _, prelude::BASE64_STANDARD}; - // Return bytes as base64 string, matching Lotus behavior - Ok(serde_json::json!(BASE64_STANDARD.encode(bytes).as_str())) + use base64::{Engine as _, prelude::BASE64_STANDARD}; + // Return bytes as base64 string, matching Lotus behavior + Ok(serde_json::json!(BASE64_STANDARD.encode(bytes)))
293-356: Add a targeted test to assert EthAccount Constructor is registered when present.You added EthAccount support, but the local test that checks “supported actors” doesn’t include EthAccount. To prevent regressions (e.g., missing registrations for some versions), add a conditional test that exercises the EthAccount Constructor only when the actor CID exists in the registry.
Add this test near the bottom of the test module:
#[test] fn test_eth_account_constructor_registered_when_present() { if let Some(eth_cid) = get_real_actor_cid(BuiltinActor::EthAccount, V16) { // Constructor is method 1 let result = deserialize_params(ð_cid, 1, &[]); if let Err(e) = result { let msg = e.to_string(); assert!( !msg.contains("No deserializer registered"), "EthAccount present in registry but Constructor not registered: {msg}" ); } } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (6)
Cargo.toml(1 hunks)src/rpc/registry/actors/eth_account.rs(1 hunks)src/rpc/registry/actors/mod.rs(1 hunks)src/rpc/registry/methods_reg.rs(2 hunks)src/tool/subcommands/api_cmd/api_compare_tests.rs(1 hunks)src/tool/subcommands/api_cmd/test_snapshots.txt(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
src/tool/subcommands/api_cmd/api_compare_tests.rs (2)
src/shim/address.rs (1)
new_delegated(158-160)src/blocks/tipset.rs (2)
key(336-339)key(530-533)
src/rpc/registry/actors/eth_account.rs (1)
src/rpc/registry/actors/account.rs (1)
register_account_actor_methods(46-60)
src/rpc/registry/methods_reg.rs (1)
src/rpc/registry/actors/eth_account.rs (1)
register_actor_methods(17-28)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (9)
- GitHub Check: cargo-publish-dry-run
- GitHub Check: Build MacOS
- GitHub Check: Build Ubuntu
- GitHub Check: All lint checks
- GitHub Check: Build forest binaries on Linux AMD64
- GitHub Check: tests
- GitHub Check: tests-release
- GitHub Check: Analyze (go)
- GitHub Check: Analyze (rust)
🔇 Additional comments (4)
Cargo.toml (1)
75-75: Dependency added for fil_actor_ethaccount_state v22.4 — consistent with the actors setVersion aligns with the rest of the actors crates (22.4). No feature flags required for this use. LGTM.
src/rpc/registry/actors/mod.rs (1)
7-7: Expose eth_account module in the registryModule wiring looks correct and consistent with the surrounding modules.
src/rpc/registry/methods_reg.rs (2)
77-79: EthAccount module import is correctly wired into the registry.Bringing
eth_accountinto scope alongside the existing actor modules is the right hook for method registration.
100-101: Match arm for EthAccount registration looks correct.Delegating to
eth_account::register_actor_methods(self, cid, version)mirrors the pattern used by other actors and enables version-aware registration through the new module.
| filecoin_verified_reg_statedecodeparams_1754401651147091.rpcsnap.json.zst | ||
| filecoin_verified_reg_statedecodeparams_1754401651147157.rpcsnap.json.zst | ||
| filecoin_verified_reg_statedecodeparams_1754401651147231.rpcsnap.json.zst | ||
| filecoin_ethaccount_statedecodeparams_1756186350854913.rpcsnap.json.zst |
There was a problem hiding this comment.
💡 Verification agent
❓ Verification inconclusive
Add EthAccount state_decodeparams snapshot — looks good
The new snapshot file entry is correctly appended next to other state_decodeparams snapshots and before statereplay. No issues spotted.
If helpful, verify that the referenced snapshot file exists in the repo:
🏁 Script executed:
#!/bin/bash
set -euo pipefail
fd -a --exact-depth 1 'filecoin_ethaccount_statedecodeparams_*.rpcsnap.json.zst' src/tool/subcommands/api_cmdLength of output: 131
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Search the entire repository for the new EthAccount state_decodeparams snapshot file
fd --type f 'filecoin_ethaccount_statedecodeparams_.*\.rpcsnap\.json\.zst'Length of output: 96
Missing Snapshot File Reference
The test snapshot list references filecoin_ethaccount_statedecodeparams_1756186350854913.rpcsnap.json.zst, but I was unable to locate this file anywhere in the repository. Please ensure that the new snapshot file is added under src/tool/subcommands/api_cmd/ (or the appropriate snapshots directory) before approving this change.
• Affected location:
src/tool/subcommands/api_cmd/test_snapshots.txt(line 204)
Action required:
- Add the actual
filecoin_ethaccount_statedecodeparams_1756186350854913.rpcsnap.json.zstsnapshot file to the repository so that the test can locate it. - Re-run the snapshot discovery (e.g.,
fd -a --exact-depth 1 'filecoin_ethaccount_statedecodeparams_*.rpcsnap.json.zst' src/tool/subcommands/api_cmd) to confirm its presence before merging.
🤖 Prompt for AI Agents
In src/tool/subcommands/api_cmd/test_snapshots.txt around line 204, the snapshot
list references
filecoin_ethaccount_statedecodeparams_1756186350854913.rpcsnap.json.zst which is
missing from the repo; add the actual snapshot file under
src/tool/subcommands/api_cmd/ (or the designated snapshots directory) with that
exact filename, commit it, and re-run snapshot discovery (e.g., fd -a
--exact-depth 1 'filecoin_ethaccount_statedecodeparams_*.rpcsnap.json.zst'
src/tool/subcommands/api_cmd) to verify the file is present before merging.
|
No green checkmark, no review! |
| fil_actor_cron_state = { version = "23" } | ||
| fil_actor_datacap_state = { version = "23" } | ||
| fil_actor_eam_state = { version = "23" } | ||
| fil_actor_ethaccount_state = { version = "23" } |
There was a problem hiding this comment.
Could you create an issue to track updating https://github.com/ChainSafe/fil-actor-states/blob/main/Makefile#L55 in fil-actor-states for fil_actor_ethaccount_state crate?
akaladarshi
deleted the
akaladarshi/add-eth-account-state-decode-params
branch
4 participants
Summary of changes
Changes introduced in this pull request:
Reference issue to close (if applicable)
Closes #5993
Other information and links
Change checklist
Summary by CodeRabbit
New Features
Tests
Chores