Add support for datacap actor in state decode params API#5871
Add support for datacap actor in state decode params API#5871sudo-shashank merged 24 commits intomainfrom
Conversation
WalkthroughAdds Datacap actor LotusJson parameter wrappers (v9–v16), registers Datacap actor methods in the RPC MethodRegistry for versions 9–16, and extends state_decode_params API tests and snapshots to cover Datacap (notably v16). A new datacap registry module and its module declaration were added. No public API signatures changed. Changes
Sequence Diagram(s)sequenceDiagram
participant Registry as MethodRegistry
participant Datacap as registry::actors::datacap
participant Macro as register_actor_methods!
Registry->>Datacap: register_datacap_actor_methods(cid, version)
Datacap->>Macro: register method deserializers for version
Macro-->>Registry: Methods registered
sequenceDiagram
participant Client
participant API as Filecoin.StateDecodeParams
participant Registry as MethodRegistry
participant Decoder as Datacap LotusJson
Client->>API: stateDecodeParams(address, method, params, tipset)
API->>Registry: lookup actor/version + method decoder
Registry->>Decoder: deserialize params (versioned)
Decoder-->>API: decoded JSON params
API-->>Client: response
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Assessment against linked issues
Possibly related PRs
Suggested reviewers
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ 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)
✨ 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 (
|
sudo-shashank
requested review from
LesnyRumcajs and
hanabi1224
and removed request for
a team
sudo-shashank
force-pushed
the
shashank/datacap-decode-params
branch
from
e33651b to
4679180
Compare
LesnyRumcajs
requested review from
akaladarshi
and removed request for
LesnyRumcajs
|
I'll let the state decode params expert, @akaladarshi, review this one. |
There was a problem hiding this comment.
@sudo-shashank I think this implementation is incomplete. Right now, it only supports ConstructorParams, MintParams, DestroyParams, and BalanceParams, but there are many more methods that are still missing. You can check this in the builtin-actor.
Look at the methods being called and the parameters they accept builtin-actor.
Sometimes, the method parameters are not defined in builtin-actor actors types.rs (so they won’t be in the fil-actor-state crate either). Instead, they’re imported from other crates. Even in that case, we still need to support them in Forest.
For example, in the verified_reg actor, see this code where UniversalReceiverParams is imported from the fvm_actor_utils crate. We also need to implement the HasLotusJson trait for it, as shown here.
Also, if a method doesn’t take any parameters, we still need to handle that. In such cases, use the empty type. You can see an example of this in the power actor here, no need to include this in test as it should not return any result.
Edit: You can cross check if all the methods are implemented or not by checking all the versions of go-state-types, this is what lotus uses.
all methods are now supported |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
src/tool/subcommands/api_cmd/api_compare_tests.rs (2)
2303-2315: Duplicate test: Multisig LockBalance added twiceThe second LockBalance test is a duplicate of the first and can be dropped to keep the suite lean.
Proposed diff:
- RpcTest::identity(StateDecodeParams::request(( - Address::new_id(18101), // https://calibration.filscan.io/en/address/t018101/, - fil_actor_multisig_state::v16::Method::LockBalance as u64, - to_vec(&multisig_lock_bal_params)?, - tipset.key().into(), - ))?),
1810-1916: Optional: Factor Datacap test setup for maintainabilityThe function is very large. Consider extracting a “make_datacap_v16_decode_tests(tipset)” helper that builds datacap params and the 16 test cases. This keeps state_decode_params_api_tests readable and reduces future merge conflicts.
Also applies to: 2056-2151
src/lotus_json/actor_states/methods/datacap_methods.rs (1)
1-14: NIT: small documentation hint for version coverageConsider a short module-level comment documenting which datacap versions are supported per param group (e.g., Balance/Constructor start at v11). Helps future maintainers cross-check against builtin-actors.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
src/lotus_json/actor_states/methods/datacap_methods.rs(1 hunks)src/lotus_json/actor_states/methods/mod.rs(1 hunks)src/rpc/registry/actors/datacap.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(2 hunks)src/tool/subcommands/api_cmd/test_snapshots.txt(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/rpc/registry/actors/datacap.rs
🧰 Additional context used
🧬 Code Graph Analysis (3)
src/rpc/registry/methods_reg.rs (1)
src/rpc/registry/actors/datacap.rs (1)
register_datacap_actor_methods(121-137)
src/tool/subcommands/api_cmd/api_compare_tests.rs (1)
src/shim/address.rs (2)
new_id(141-143)default(121-123)
src/lotus_json/actor_states/methods/datacap_methods.rs (3)
src/shim/address.rs (1)
new_id(141-143)src/tool/subcommands/api_cmd/api_compare_tests.rs (4)
serde_json(262-262)serde_json(292-292)serde_json(301-301)serde_json(302-302)src/shim/econ.rs (1)
from_atto(98-100)
⏰ 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). (3)
- GitHub Check: tests
- GitHub Check: tests-release
- GitHub Check: Build forest binaries on Linux AMD64
🔇 Additional comments (19)
src/rpc/registry/actors/mod.rs (1)
5-5: Module wiring LGTMdatacap module is correctly exposed alongside existing actor modules; ordering remains consistent.
src/lotus_json/actor_states/methods/mod.rs (1)
6-6: LotusJson datacap module inclusion looks goodThe datacap_methods module is properly declared; this aligns with registry wiring and tests.
src/rpc/registry/methods_reg.rs (3)
76-78: Importing datacap actor module into registry is correctConsistent with other actor imports; no concerns.
65-73: Error message remains clear for unregistered methodsThe bail! path still surfaces detailed context (actor type, version, method). Good for debugging.
75-98: Version coverage for DataCap: verify V8 handling per issue #5717register_known_methods handles DataCap versions via datacap::register_datacap_actor_methods (v9–v16 per datacap.rs). Issue #5717 requests V8–V16 support or justification. If V8 DataCap didn’t exist or params were unavailable, please document explicitly in code comments or CHANGELOG.
Would you confirm whether there is any DataCap actor at V8 in the code registry? If not, please add a brief comment near the match to state “DataCap actor exists from v9+” to close the loop with the issue’s acceptance criteria.
src/tool/subcommands/api_cmd/api_compare_tests.rs (2)
1853-1916: Datacap v16 test vectors: types/fields look correctConstructor/Mint/Destroy/Balance and FRC-46 types (Transfer*, Allowance*, Burn*, GetAllowance) are instantiated with appropriate Address/TokenAmount/RawBytes. Using TokenAmount::default() is fine for identity tests.
2056-2151: Comprehensive Datacap StateDecodeParams coverage (v16) — one thing to confirmThe “no-params” methods (Name/Symbol/TotalSupply/Granularity) pass empty bytes. Ensure the datacap registry registers these with the
emptyhandler, so the deserializer returns {} (or base64 when non-empty), matching Lotus behavior.Can you confirm datacap.rs registers these as empty via register_actor_methods!(..., [(Method::X, empty), ...])?
src/lotus_json/actor_states/methods/datacap_methods.rs (12)
16-52: BalanceParams: solid LotusJson wrapper; v11–v16 only — verify earlier versionsThe implementation and snapshot are correct. Coverage starts at v11. If BalanceParams also exists in v9 or v10, consider adding them; otherwise, add a note in datacap.rs explaining earliest supported version for this param.
Would you confirm whether datacap::v9/v10 define BalanceParams? If yes, extend the macro; if not, add a brief comment in datacap.rs to document why v9/v10 aren’t included for BalanceParams.
53-89: ConstructorParams wrapper: v11–v16 covered — confirm need for earlier versionsSame note as BalanceParams. If ConstructorParams exist in earlier versions, extend macro; else, document earliest version.
90-139: DestroyParams: v9–v16 coverage with correct Address/TokenAmount mappingSerde/schemars and tests are correct; using from_atto(1e18) fits i64 range. LGTM.
140-199: MintParams: v9–v16 coverage OK; operators mapped as VecGood use of iterator map for address conversion. LGTM.
200-249: TransferParams: RawBytes lotus_json mapping is correctoperator_data passed through transparently; snapshot encodes as base64 as expected. LGTM.
250-305: TransferFromParams: Address/TokenAmount/RawBytes mappings correctRound-trip snapshots cover base64 semantics. LGTM.
307-349: IncreaseAllowanceParams: correct JSON shape and conversionsNo issues spotted.
350-391: DecreaseAllowanceParams: correct JSON shape and conversionsNo issues spotted.
393-428: RevokeAllowanceParams: correct mapping of operator addressNo issues.
429-464: BurnParams: TokenAmount mapping correctNo issues.
465-506: BurnFromParams: owner + amount mapping correctNo issues.
508-550: GetAllowanceParams: owner/operator mapping correctNo issues.
akaladarshi
left a comment
akaladarshi
left a comment
There was a problem hiding this comment.
Rename the filename datacap_methods -> datacap_actor_params
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/lotus_json/actor_states/methods/datacap_actor_params.rs (1)
114-118: Nit: Prefer wider integer type for TokenAmount literals.The atto values use i64 (1e18 fits but is close to the limit for typical monetary literals growth). Consider i128 to avoid accidental overflow if test values change.
Example:
-amount: TokenAmount::from_atto(1_000_000_000_000_000_000_i64).into(), +amount: TokenAmount::from_atto(1_000_000_000_000_000_000_i128).into(),Also applies to: 168-176, 225-231, 279-285, 327-333, 371-376, 445-450, 486-491
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/lotus_json/actor_states/methods/datacap_actor_params.rs(1 hunks)src/lotus_json/actor_states/methods/mod.rs(1 hunks)src/rpc/registry/actors/datacap.rs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/rpc/registry/actors/datacap.rs
- src/lotus_json/actor_states/methods/mod.rs
🧰 Additional context used
🧬 Code Graph Analysis (1)
src/lotus_json/actor_states/methods/datacap_actor_params.rs (2)
src/shim/address.rs (1)
new_id(142-144)src/shim/econ.rs (1)
from_atto(106-108)
⏰ 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: tests-release
- GitHub Check: tests
- GitHub Check: Build forest binaries on Linux AMD64
- GitHub Check: All lint checks
- GitHub Check: Analyze (go)
- GitHub Check: Analyze (rust)
🔇 Additional comments (2)
src/lotus_json/actor_states/methods/datacap_actor_params.rs (2)
214-248: FRC46 param wrappers and RawBytes handling look solid.
- Using fil_actors_shared FRC46 types directly is correct.
- RawBytes is serialized via the LotusJson codec (base64), matching the snapshots.
- Address and TokenAmount conversions via shim are consistent.
No changes requested here.
Also applies to: 267-305, 318-349, 361-392, 401-428, 437-464, 476-507, 519-549
90-99: Confirm JSON key casing consistency for Datacap paramsThe
#[serde(rename_all = "PascalCase")]on these LotusJson wrapper structs will serialize fields as"Owner","Amount", etc., but your stored snapshots and FRC-46 spec appear to use snake-/lower-case keys (e.g."owner","amount"). Please:
- Verify the expected JSON field casing via the Lotus/go-state-types definitions for FRC-46 Datacap methods.
- If snapshots and spec use snake-case, remove the
#[serde(rename_all = "PascalCase")]lines.- Otherwise update your snapshots (and any downstream tests) to expect PascalCase.
Affected structs (file
src/lotus_json/actor_states/methods/datacap_actor_params.rs):
- DatacapDestroyParamsLotusJson (lines 90–99)
- TransferParamsLotusJson (201–202)
- TransferFromParamsLotusJson (251–252)
- IncreaseAllowanceParamsLotusJson (308–309)
- DecreaseAllowanceParamsLotusJson (351–352)
- RevokeAllowanceParamsLotusJson (394–395)
- BurnParamsLotusJson (466–467)
- BurnFromParamsLotusJson (509–510)
4 participants
Summary of changes
Changes introduced in this pull request:
Reference issue to close (if applicable)
Closes #5717
Other information and links
Change checklist
Summary by CodeRabbit