Skip to content

Conversation

@akaladarshi
Copy link
Collaborator

@akaladarshi akaladarshi commented Aug 26, 2025

Summary of changes

Changes introduced in this pull request:

  • Created two new modules inside lotus_json/actors state and params
    • Moved all the actor state related files inside the state module and same for the params
  • Combined the individual actor methods into a common actor file
  • Renamed all the actor params and actor state files inside lotus json:
    • State files: <actor-name_state>
    • Param files: <actor-name_params>
  • Add version 17 support for the actors
  • Use fil_actor_shared::ActorVersion instead of u64 to track actor versions
  • Added Exec4 in v10 for init actor
  • Added remaining methods of version 8 to 10 of account actor
  • Moved the state decode params api snapshot tests into separate module

Reference issue to close (if applicable)

Closes #5937

Other information and links

Change checklist

  • I have performed a self-review of my own code,
  • I have made corresponding changes to the documentation. All new code adheres to the team's documentation standards,
  • I have added tests that prove my fix is effective or that my feature works (if possible),
  • I have made sure the CHANGELOG is up-to-date. All user-facing changes should be reflected in this document.

Summary by CodeRabbit

  • New Features

    • Added support for decoding actor method parameters and extended decoding coverage to actor version 17 across core actors.
  • Refactor

    • Switched to a typed ActorVersion dispatch for safer, exhaustive per-version handling.
  • Tests

    • Consolidated and added extensive state-decode test generators for many actors, centralizing test creation.
  • Chores

    • Bumped several actor dependency versions.
  • Documentation

    • Changelog updated to document the new decoding capability.

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Aug 26, 2025

Walkthrough

Refactors lotus_json into an actors/params layout, moves many Lotus JSON adapters and tests under per-actor modules, expands adapter coverage to v17, converts RPC registries from numeric u64 to ActorVersion enum, adds per-actor StateDecodeParams test generators, removes legacy adapter files, and bumps actor dependency versions and CHANGELOG.

Changes

Cohort / File(s) Summary
Changelog
CHANGELOG.md
Adds an unreleased entry documenting support for Filecoin.StateDecodeParams (PR #6000).
Dependency bumps
Cargo.toml
Bumps fil_actor_* and fil_actors_shared versions (24 → 24.1.2).
Lotus JSON module reorg
src/lotus_json/mod.rs, src/lotus_json/actors/mod.rs, src/lotus_json/actors/params/mod.rs
Replace actor_states with actors; add new actors module and actors/params & actors/states submodules.
Removed legacy adapter files
src/lotus_json/actor_states/methods/*, src/lotus_json/actor_states/methods/mod.rs
Deleted many old lotus_json method adapter files and removed their module exports.
Per-actor params added / moved
src/lotus_json/actors/params/*.rs
Added/moved Lotus JSON adapter types/macros into per-actor param files (account, init, cron, datacap, eam, evm, miner, multisig, paych, power, reward, verified_reg, market, etc.) and extended macro invocations to include v17.
Init & Init-Exec adapters
src/lotus_json/actors/params/init_params.rs
Added InitConstructorParamsLotusJson, InitExecParamsLotusJson, InitExec4ParamsLotusJson and macros to generate HasLotusJson impls across versions (8–17 / 10–17 where applicable).
Account adapter moved
src/lotus_json/actors/params/account_params.rs
Added AccountConstructorParamsLotusJson and macros to generate HasLotusJson impls for account-related types across versions.
Multiple actor param macro expansions
src/lotus_json/actors/params/{cron,datacap,eam,evm,miner,multisig,paych,power,reward,verified_reg}.rs
Updated many macro invocations and/or added v17 coverage; renamed/added some macros and new v17-specific HasLotusJson impls (notably power and miner additions).
Market param path cleanup
src/lotus_json/actors/params/market_params.rs
Shortened fully-qualified paths to local imports; no behavior change.
States module tweak
src/lotus_json/actors/states/mod.rs
Removed mod methods; inclusion.
RPC registries: enum versioning & v17
src/rpc/registry/actors/*.rs, src/rpc/registry/actors_reg.rs
Many register_* functions now take ActorVersion (enum) instead of u64; numeric matches replaced with exhaustive enum variants; ActorRegistry map now stores (BuiltinActor, ActorVersion); added V17 coverage and per-version restructuring (notable changes: account, init, miner, paych, payment_channel refactor).
Payment-channel registry refactor
src/rpc/registry/actors/payment_channel.rs
Replaced paste-based numeric-macro dispatch with explicit ActorVersion match and new register_payment_channel_methods! macro.
Methods registry & tests
src/rpc/registry/methods_reg.rs
Uses Type import; tightened “no deserializer” error path; tests updated to use ActorVersion.
State decode tests: new generator modules
src/tool/subcommands/api_cmd/state_decode_params_tests/*
Added per-actor state decode test generators (account, cron, datacap, eam, ethaccount, evm, init, market, miner, multisig, paych, power, reward, system, verified_reg) and an aggregator create_all_state_decode_params_tests.
API compare tests / test exposure
src/tool/subcommands/api_cmd/api_compare_tests.rs, src/tool/subcommands/api_cmd.rs
Replaced inline state-decode tests with aggregator usage; made RpcTest::identity crate-visible and added mod state_decode_params_tests;.
State migration NV27 update
src/state_migration/nv27/mod.rs
NV27 system states updated to include fil_actor_system_state::v17::State (replacing duplicate v16 entry).

Sequence Diagram(s)

sequenceDiagram
  autonumber
  actor Client
  participant RPC as Forest RPC
  participant AR as ActorRegistry
  participant MR as MethodRegistry
  participant LJ as LotusJson adapters

  Client->>RPC: StateDecodeParams(code_cid, method_num, params, tipset)
  RPC->>AR: get_actor_details_from_code(code_cid)
  AR-->>RPC: (BuiltinActor, ActorVersion)
  RPC->>MR: lookup_decoder(BuiltinActor, ActorVersion, method_num)
  alt decoder found
    RPC->>LJ: deserialize params via HasLotusJson for actor/version
    LJ-->>RPC: JSON params
    RPC-->>Client: Decoded JSON
  else decoder missing / Type::Placeholder
    RPC-->>Client: Error - no deserializer registered
  end
Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~60–90 minutes

Possibly related PRs

Suggested reviewers

  • LesnyRumcajs
  • sudo-shashank
  • hanabi1224

Pre-merge checks (5 passed)

✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title Check ✅ Passed The title "Clean up StateDecodeParams API changes" is concise, a single sentence, and accurately reflects the PR's primary purpose of reorganizing and cleaning the StateDecodeParams-related actor params/states and related API surface.
Linked Issues Check ✅ Passed Based on the provided summaries, the patch implements the linked-issue coding objectives from #5937: it consolidates lotus_json into actors/{state,params} with _state/_params naming, replaces raw u64 versions with fil_actors_shared::ActorVersion throughout registries and ActorRegistry, moves state_decode_params tests into dedicated modules, replaces magic numeric method ids with actor Method enums in tests, updates CHANGELOG.md, adds Exec4 support for init v10, and expands account/actor support (including v8–v10 and v17) and macro coverage as described in the PR metadata and file diffs.
Out of Scope Changes Check ✅ Passed I did not find substantive changes unrelated to the StateDecodeParams cleanup objectives; most edits reorganize lotus_json, extend version support to v17, and migrate registries/tests to ActorVersion. The only notable out-of-objective item is a dependency version bump in Cargo.toml (actor crates moved to 24.1.2), which appears required to enable v17 types rather than an unrelated change.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.

Tip

👮 Agentic pre-merge checks are now available in preview!

Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.

  • Built-in checks – Quickly apply ready-made checks to enforce title conventions, require pull request descriptions that follow templates, validate linked issues for compliance, and more.
  • Custom agentic checks – Define your own rules using CodeRabbit’s advanced agentic capabilities to enforce organization-specific policies and workflows. For example, you can instruct CodeRabbit’s agent to verify that API documentation is updated whenever API schema files are modified in a PR. Note: Upto 5 custom checks are currently allowed during the preview period. Pricing for this feature will be announced in a few weeks.

Please see the documentation for more information.

Example:

reviews:
  pre_merge_checks:
    custom_checks:
      - name: "Undocumented Breaking Changes"
        mode: "warning"
        instructions: |
          Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).

Please share your feedback with us on this Discord post.

✨ Finishing touches
  • 📝 Generate Docstrings
🧪 Generate unit tests
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch akaladarshi/state-decode-param-cleanup

Comment @coderabbitai help to get the list of available commands and usage tips.

@akaladarshi akaladarshi force-pushed the akaladarshi/state-decode-param-cleanup branch 3 times, most recently from a7d1e1e to 7c30481 Compare September 3, 2025 15:12
@akaladarshi akaladarshi force-pushed the akaladarshi/state-decode-param-cleanup branch from 7c30481 to fbbff8b Compare September 9, 2025 17:44
@akaladarshi akaladarshi force-pushed the akaladarshi/state-decode-param-cleanup branch from fbbff8b to c96ab46 Compare September 10, 2025 05:41
@akaladarshi akaladarshi marked this pull request as ready for review September 10, 2025 09:10
@akaladarshi akaladarshi requested a review from a team as a code owner September 10, 2025 09:10
@akaladarshi akaladarshi requested review from hanabi1224 and sudo-shashank and removed request for a team September 10, 2025 09:10
@akaladarshi akaladarshi added the RPC requires calibnet RPC checks to run on CI label Sep 10, 2025
@akaladarshi akaladarshi marked this pull request as draft September 10, 2025 09:13
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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/lotus_json/actors/params/miner_params.rs (1)

3616-3621: from_lotus_json drops require_activation_success (always false)

from_lotus_json for ProveCommitSectorsNIParams hard-codes require_activation_success = false, breaking roundtrip and behavior.

Apply this fix:

-                        require_activation_success: false,
+                        require_activation_success: lotus_json.require_activation_success,
🧹 Nitpick comments (25)
src/rpc/registry/actors/eth_account.rs (1)

24-24: Nit: clarify why V8/V9 are no-ops

Add a brief comment to explain the absence of EthAccount before V10.

Apply this diff:

-        ActorVersion::V8 | ActorVersion::V9 => {}
+        // EthAccount actor was introduced in V10; V8/V9 have no registrations.
+        ActorVersion::V8 | ActorVersion::V9 => {}
src/rpc/registry/actors/datacap.rs (1)

126-142: Exhaustive ActorVersion dispatch looks correct (V8–V17).

No wildcard; V8 is a no-op (Datacap absent pre-V9), and newer versions map to the right modules.

A short comment on Line 132 clarifying “Datacap introduced in V9” would help future readers.

src/rpc/registry/methods_reg.rs (4)

11-11: Avoid version-pinned Type import; match Placeholder via BuiltinActor for consistency.

Prevents coupling to v11 and keeps the match uniform with other arms.

Apply:

-use fil_actors_shared::v11::runtime::builtins::Type;
@@
-                Type::Placeholder => {}
+                BuiltinActor::Placeholder => {}

If BuiltinActor::Placeholder isn’t available in your shim, consider re-exporting it there rather than importing a versioned Type.

Also applies to: 104-105


66-70: Include code_cid in the “no deserializer registered” error.

Improves debugging when multiple bundles or versions are present.

-        bail!(
-            "No deserializer registered for actor type {actor_type:?} ({version}), method {method_num}"
-        );
+        bail!(
+            "No deserializer registered for actor type {actor_type:?} ({version}), method {method_num}, code_cid {code_cid}"
+        );

131-134: Drop unnecessary as_str() when encoding base64.

Slightly cleaner and avoids a transient &str.

-                        Ok(serde_json::json!(BASE64_STANDARD.encode(bytes).as_str()))
+                        Ok(serde_json::json!(BASE64_STANDARD.encode(bytes)))

355-359: Remove redundant hard-coded base64 assert in test.

You already assert equality to the computed expected_base64.

-        assert_eq!(json_value, json!(expected_base64));
-        assert_eq!(json_value.as_str().unwrap(), "ghgqRBI0Vng=");
+        assert_eq!(json_value, json!(expected_base64));
src/rpc/registry/actors/reward.rs (1)

9-28: Rename macro to reflect V17 usage for clarity.

Macro name “11_to_16” is misleading now that it’s used for V17 too.

-macro_rules! register_reward_version_11_to_16 {
+macro_rules! register_reward_version_11_plus {
@@
-            register_reward_version_11_to_16!(registry, cid, fil_actor_reward_state::v11)
+            register_reward_version_11_plus!(registry, cid, fil_actor_reward_state::v11)
@@
-            register_reward_version_11_to_16!(registry, cid, fil_actor_reward_state::v12)
+            register_reward_version_11_plus!(registry, cid, fil_actor_reward_state::v12)
@@
-            register_reward_version_11_to_16!(registry, cid, fil_actor_reward_state::v13)
+            register_reward_version_11_plus!(registry, cid, fil_actor_reward_state::v13)
@@
-            register_reward_version_11_to_16!(registry, cid, fil_actor_reward_state::v14)
+            register_reward_version_11_plus!(registry, cid, fil_actor_reward_state::v14)
@@
-            register_reward_version_11_to_16!(registry, cid, fil_actor_reward_state::v15)
+            register_reward_version_11_plus!(registry, cid, fil_actor_reward_state::v15)
@@
-            register_reward_version_11_to_16!(registry, cid, fil_actor_reward_state::v16)
+            register_reward_version_11_plus!(registry, cid, fil_actor_reward_state::v16)
@@
-            register_reward_version_11_to_16!(registry, cid, fil_actor_reward_state::v17)
+            register_reward_version_11_plus!(registry, cid, fil_actor_reward_state::v17)

Also applies to: 71-90

src/rpc/registry/actors/init.rs (2)

9-23: Macro name/comment drift: used for V17 but labeled “10–16”

Rename to reflect actual coverage and avoid future drift.

-// Macro for versions 10-16 that have Exec4
-macro_rules! register_init_versions_10_to_16 {
+// Macro for versions 10+ that have Exec4
+macro_rules! register_init_versions_10_plus {
     ($registry:expr, $code_cid:expr, $state_version:path) => {{
         use $state_version::{ConstructorParams, Exec4Params, ExecParams, Method};
         register_actor_methods!(
             $registry,
             $code_cid,
             [
                 (Method::Constructor, ConstructorParams),
                 (Method::Exec, ExecParams),
                 (Method::Exec4, Exec4Params)
             ]
         );
     }};
 }

And update call sites below accordingly (see next comment).

Also applies to: 25-40


54-77: Apply the new macro name at all call sites

To keep naming consistent after the rename above.

-            register_init_versions_10_to_16!(registry, cid, fil_actor_init_state::v10)
+            register_init_versions_10_plus!(registry, cid, fil_actor_init_state::v10)
-            register_init_versions_10_to_16!(registry, cid, fil_actor_init_state::v11)
+            register_init_versions_10_plus!(registry, cid, fil_actor_init_state::v11)
-            register_init_versions_10_to_16!(registry, cid, fil_actor_init_state::v12)
+            register_init_versions_10_plus!(registry, cid, fil_actor_init_state::v12)
-            register_init_versions_10_to_16!(registry, cid, fil_actor_init_state::v13)
+            register_init_versions_10_plus!(registry, cid, fil_actor_init_state::v13)
-            register_init_versions_10_to_16!(registry, cid, fil_actor_init_state::v14)
+            register_init_versions_10_plus!(registry, cid, fil_actor_init_state::v14)
-            register_init_versions_10_to_16!(registry, cid, fil_actor_init_state::v15)
+            register_init_versions_10_plus!(registry, cid, fil_actor_init_state::v15)
-            register_init_versions_10_to_16!(registry, cid, fil_actor_init_state::v16)
+            register_init_versions_10_plus!(registry, cid, fil_actor_init_state::v16)
-            register_init_versions_10_to_16!(registry, cid, fil_actor_init_state::v17)
+            register_init_versions_10_plus!(registry, cid, fil_actor_init_state::v17)
src/rpc/registry/actors/power.rs (1)

110-139: Optional: de-duplicate V16/V17 registration

The param’ed method sets are identical; consider a single “16+” macro to reduce duplication.

-macro_rules! register_power_version_17 {
+// Same as v16; keep one shared macro for 16+
+macro_rules! register_power_version_16_plus {
@@
 }
@@
-        ActorVersion::V16 => register_power_version_16!(registry, cid, fil_actor_power_state::v16),
-        ActorVersion::V17 => register_power_version_17!(registry, cid, fil_actor_power_state::v17),
+        ActorVersion::V16 => register_power_version_16_plus!(registry, cid, fil_actor_power_state::v16),
+        ActorVersion::V17 => register_power_version_16_plus!(registry, cid, fil_actor_power_state::v17),
src/rpc/registry/actors/account.rs (3)

28-35: Confirm v9 UniversalReceiverHook on Account actor.

Is Method::UniversalReceiverHook actually exposed by the v9 Account actor? If not, this registration will surface a non-existent method.

If verification shows it’s absent, drop it:

     register_actor_methods!(
         registry,
         cid,
         [
             (Method::PubkeyAddress, empty),
-            (Method::UniversalReceiverHook, empty)
         ]
     );

57-61: Doc nit: include v17 in the “with_types” comment.

The macro is used for V15–V17; the comment mentions only v15, v16.

-    // For versions that use types module (v15, v16)
+    // For versions that use types module (v15–v17)

10-55: Minor duplication across v8/v9/v10 helpers.

You can reduce repetition by introducing a small helper/macro for {Constructor(Address), PubkeyAddress} and param-only deltas.

src/rpc/registry/actors_reg.rs (2)

30-37: Unknown major versions are silently skipped. Add tracing.

If bundles contain a newer major version, entries are dropped without visibility. Emit a warn/debug to aid diagnostics.

-            if let Ok(version_u64) = metadata.actor_major_version() {
-                if let Some(version) = ActorVersion::from_repr(version_u64 as u8) {
+            if let Ok(version_u64) = metadata.actor_major_version() {
+                if let Some(version) = ActorVersion::from_repr(version_u64 as u8) {
                     for (actor_type, cid) in metadata.manifest.builtin_actors() {
                         map.insert(cid, (actor_type, version));
                     }
-                }
+                } else {
+                    tracing::warn!("Skipping actors with unknown major version: {version_u64}");
+                }
             }

70-79: Consider returning version from load path if needed later.

You ignore the resolved ActorVersion here; if future state decoding needs version-specific handling, thread it through now.

src/lotus_json/actors/params/account_params.rs (4)

6-6: Wrong Serialize import; prefer serde or drop it.

#[derive(Serialize)] targets serde::Serialize. The jsonrpsee::core::Serialize import is misleading.

-use jsonrpsee::core::Serialize;
+use serde::Serialize;

Or remove the line entirely.


11-17: Schema parity: add schemars(transparent).

To mirror serde(transparent) in the OpenAPI/JSON Schema, add #[schemars(transparent)].

 #[derive(Serialize, Deserialize, JsonSchema, Debug, Clone, PartialEq)]
 #[serde(transparent)]
+#[schemars(transparent)]
 pub struct AccountConstructorParamsLotusJson {

41-44: Fully qualify json! macro in snapshots.

json! isn’t imported in this module; use serde_json::json! to avoid scope surprises.

-                                json!({
+                                serde_json::json!({
-                                json!("f01234"),
+                                serde_json::json!("f01234"),

Also applies to: 83-83


30-69: Snapshot defaults: null vs base64 for Vec.

LotusJson for Vec<u8> typically encodes bytes as base64 strings. Using null may not reflect real RPC payloads and could hide regressions.

Consider a non-empty example (e.g., "SGVsbG8=") for both fields to match production encoding.

src/rpc/registry/actors/payment_channel.rs (1)

9-11: Update comment to match supported versions

The code handles V8–V17; the comment says V8–V16.

-// Payment channel methods are consistent across all versions V8-V16
+// Payment channel methods are consistent across all versions V8–V17
src/lotus_json/actors/params/power_params.rs (1)

317-339: Deduplicate v16/v17 MinerPowerParams impls

The two impl blocks are identical; consider a small macro to reduce duplication and keep future bumps trivial.

Example:

macro_rules! impl_lotus_json_for_power_miner_power_params {
    ($($ver:literal),+ $(,)?) => {$(
        paste! {
            impl HasLotusJson for fil_actor_power_state::[<v $ver>]::MinerPowerParams {
                type LotusJson = MinerPowerParamsLotusJson;
                #[cfg(test)]
                fn snapshots() -> Vec<(serde_json::Value, Self)> {
                    vec![(json!({"Miner": 1002}), Self { miner: 1002 })]
                }
                fn into_lotus_json(self) -> Self::LotusJson {
                    MinerPowerParamsLotusJson { miner: self.miner }
                }
                fn from_lotus_json(lotus_json: Self::LotusJson) -> Self {
                    Self { miner: lotus_json.miner }
                }
            }
        }
    )+}
}

Then:

impl_lotus_json_for_power_miner_power_params!(16, 17);
src/rpc/registry/actors/miner.rs (2)

335-349: Minor naming nit: singular “version”

Function name reads “versions_14” but registers a single version; consider renaming for consistency with others.

-fn register_miner_versions_14(registry: &mut MethodRegistry, cid: Cid) {
+fn register_miner_version_14(registry: &mut MethodRegistry, cid: Cid) {

And update the call site in the match.


4-4: Potential unused import

MethodNum isn’t referenced in this module; if not needed by macros, consider removing to keep imports lean.

src/lotus_json/actors/params/init_params.rs (1)

7-11: Trim unused/incorrect imports

jsonrpsee::core::Serialize, serde::Deserialize, and std::fmt::Debug aren’t needed for derives and can trigger unused import warnings.

-use jsonrpsee::core::Serialize;
@@
-use serde::Deserialize;
-use std::fmt::Debug;
src/lotus_json/actors/params/miner_params.rs (1)

3713-3715: Macro name vs. usage mismatch (includes v8 in a v9+ macro)

impl_lotus_json_for_miner_terminate_sectors_params_v9_and_above! is invoked with 8 as well. If v8 really shares the same struct semantics, consider renaming the macro; otherwise, drop v8 here and keep a dedicated v8 impl.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 4b4411a and bddc9dc.

📒 Files selected for processing (40)
  • CHANGELOG.md (1 hunks)
  • Cargo.toml (1 hunks)
  • src/lotus_json/actor_states/methods/account_constructor_params.rs (0 hunks)
  • src/lotus_json/actor_states/methods/init_constructor_params.rs (0 hunks)
  • src/lotus_json/actor_states/methods/init_exec4_params.rs (0 hunks)
  • src/lotus_json/actor_states/methods/init_exec_params.rs (0 hunks)
  • src/lotus_json/actor_states/methods/mod.rs (0 hunks)
  • src/lotus_json/actors/mod.rs (1 hunks)
  • src/lotus_json/actors/params/account_params.rs (2 hunks)
  • src/lotus_json/actors/params/cron_params.rs (1 hunks)
  • src/lotus_json/actors/params/datacap_params.rs (4 hunks)
  • src/lotus_json/actors/params/eam_params.rs (1 hunks)
  • src/lotus_json/actors/params/evm_params.rs (2 hunks)
  • src/lotus_json/actors/params/init_params.rs (1 hunks)
  • src/lotus_json/actors/params/miner_params.rs (1 hunks)
  • src/lotus_json/actors/params/mod.rs (1 hunks)
  • src/lotus_json/actors/params/multisig_params.rs (1 hunks)
  • src/lotus_json/actors/params/paych_params.rs (1 hunks)
  • src/lotus_json/actors/params/power_params.rs (2 hunks)
  • src/lotus_json/actors/params/reward_params.rs (1 hunks)
  • src/lotus_json/actors/params/verified_reg_params.rs (1 hunks)
  • src/lotus_json/actors/states/mod.rs (1 hunks)
  • src/lotus_json/mod.rs (1 hunks)
  • src/rpc/registry/actors/account.rs (3 hunks)
  • src/rpc/registry/actors/cron.rs (2 hunks)
  • src/rpc/registry/actors/datacap.rs (2 hunks)
  • src/rpc/registry/actors/eam.rs (2 hunks)
  • src/rpc/registry/actors/eth_account.rs (2 hunks)
  • src/rpc/registry/actors/evm.rs (2 hunks)
  • src/rpc/registry/actors/init.rs (3 hunks)
  • src/rpc/registry/actors/market.rs (2 hunks)
  • src/rpc/registry/actors/miner.rs (14 hunks)
  • src/rpc/registry/actors/multisig.rs (2 hunks)
  • src/rpc/registry/actors/payment_channel.rs (2 hunks)
  • src/rpc/registry/actors/power.rs (2 hunks)
  • src/rpc/registry/actors/reward.rs (2 hunks)
  • src/rpc/registry/actors/system.rs (2 hunks)
  • src/rpc/registry/actors/verified_reg.rs (2 hunks)
  • src/rpc/registry/actors_reg.rs (1 hunks)
  • src/rpc/registry/methods_reg.rs (8 hunks)
💤 Files with no reviewable changes (5)
  • src/lotus_json/actor_states/methods/init_exec_params.rs
  • src/lotus_json/actor_states/methods/mod.rs
  • src/lotus_json/actor_states/methods/init_exec4_params.rs
  • src/lotus_json/actor_states/methods/init_constructor_params.rs
  • src/lotus_json/actor_states/methods/account_constructor_params.rs
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2025-09-02T10:05:34.350Z
Learnt from: akaladarshi
PR: ChainSafe/forest#5923
File: src/rpc/registry/actors/miner.rs:221-223
Timestamp: 2025-09-02T10:05:34.350Z
Learning: For miner actor ChangeOwnerAddress and ChangeOwnerAddressExported methods: versions 8-10 use bare Address as parameter type, while versions 11+ use ChangeOwnerAddressParams. This reflects the evolution of the Filecoin miner actor parameter structures across versions.

Applied to files:

  • src/rpc/registry/actors/eth_account.rs
  • src/rpc/registry/actors/power.rs
  • src/rpc/registry/actors/multisig.rs
  • src/rpc/registry/actors/evm.rs
  • src/rpc/registry/actors/account.rs
  • src/lotus_json/actors/params/power_params.rs
  • src/rpc/registry/actors/miner.rs
  • src/lotus_json/actors/params/miner_params.rs
📚 Learning: 2025-08-18T12:25:29.183Z
Learnt from: akaladarshi
PR: ChainSafe/forest#5896
File: src/lotus_json/actor_states/methods/verified_reg_actor.rs:133-137
Timestamp: 2025-08-18T12:25:29.183Z
Learning: In Lotus JSON for verified registry actor, the SectorAllocationClaimsLotusJson struct uses "SectorExpiry" as the field name for the expiry field, not "Expiry". The current code with #[serde(rename = "SectorExpiry")] is correct.

Applied to files:

  • src/lotus_json/mod.rs
📚 Learning: 2025-08-14T06:46:08.056Z
Learnt from: akaladarshi
PR: ChainSafe/forest#5923
File: src/rpc/registry/actors/miner.rs:0-0
Timestamp: 2025-08-14T06:46:08.056Z
Learning: ProveCommitSectors3 method is available in fil_actor_miner_state::v13 and should be mapped to ProveCommitSectors3Params, not ProveCommitSectorParams.

Applied to files:

  • src/rpc/registry/actors/miner.rs
🧬 Code graph analysis (18)
src/lotus_json/actors/mod.rs (1)
src/shim/actors/builtin/market/mod.rs (1)
  • states (165-218)
src/rpc/registry/actors/cron.rs (3)
src/rpc/registry/actors/eam.rs (1)
  • register_actor_methods (28-46)
src/rpc/registry/actors/power.rs (1)
  • register_actor_methods (141-174)
src/rpc/registry/actors/system.rs (1)
  • register_actor_methods (23-40)
src/rpc/registry/actors/eam.rs (2)
src/rpc/registry/actors/cron.rs (1)
  • register_actor_methods (24-41)
src/rpc/registry/actors/eth_account.rs (1)
  • register_actor_methods (18-50)
src/rpc/registry/methods_reg.rs (1)
src/rpc/registry/actors_reg.rs (1)
  • get_real_actor_cid (138-144)
src/rpc/registry/actors/eth_account.rs (2)
src/rpc/registry/actors/cron.rs (1)
  • register_actor_methods (24-41)
src/rpc/registry/actors/eam.rs (1)
  • register_actor_methods (28-46)
src/rpc/registry/actors/reward.rs (2)
src/rpc/registry/actors/cron.rs (1)
  • register_actor_methods (24-41)
src/rpc/registry/actors/power.rs (1)
  • register_actor_methods (141-174)
src/rpc/registry/actors/system.rs (2)
src/rpc/registry/actors/cron.rs (1)
  • register_actor_methods (24-41)
src/rpc/registry/actors/power.rs (1)
  • register_actor_methods (141-174)
src/rpc/registry/actors/init.rs (8)
src/rpc/registry/actors/cron.rs (1)
  • register_actor_methods (24-41)
src/rpc/registry/actors/eam.rs (1)
  • register_actor_methods (28-46)
src/rpc/registry/actors/eth_account.rs (1)
  • register_actor_methods (18-50)
src/rpc/registry/actors/multisig.rs (1)
  • register_actor_methods (76-109)
src/rpc/registry/actors/power.rs (1)
  • register_actor_methods (141-174)
src/rpc/registry/actors/reward.rs (1)
  • register_actor_methods (50-92)
src/rpc/registry/actors/system.rs (1)
  • register_actor_methods (23-40)
src/rpc/registry/actors/verified_reg.rs (1)
  • register_actor_methods (239-268)
src/rpc/registry/actors/verified_reg.rs (3)
src/rpc/registry/actors/cron.rs (1)
  • register_actor_methods (24-41)
src/rpc/registry/actors/eam.rs (1)
  • register_actor_methods (28-46)
src/rpc/registry/actors/init.rs (1)
  • register_actor_methods (42-79)
src/rpc/registry/actors/power.rs (2)
src/rpc/registry/actors/cron.rs (1)
  • register_actor_methods (24-41)
src/rpc/registry/actors/init.rs (1)
  • register_actor_methods (42-79)
src/lotus_json/actors/params/init_params.rs (1)
src/lotus_json/actors/params/datacap_params.rs (24)
  • snapshots (218-231)
  • snapshots (271-286)
  • snapshots (322-333)
  • snapshots (365-376)
  • snapshots (405-414)
  • snapshots (441-450)
  • snapshots (480-491)
  • snapshots (523-534)
  • into_lotus_json (233-239)
  • into_lotus_json (288-295)
  • into_lotus_json (335-340)
  • into_lotus_json (378-383)
  • into_lotus_json (416-420)
  • into_lotus_json (452-456)
  • into_lotus_json (493-498)
  • into_lotus_json (536-541)
  • from_lotus_json (241-247)
  • from_lotus_json (297-304)
  • from_lotus_json (342-347)
  • from_lotus_json (385-390)
  • from_lotus_json (422-426)
  • from_lotus_json (458-462)
  • from_lotus_json (500-505)
  • from_lotus_json (543-548)
src/rpc/registry/actors/multisig.rs (2)
src/rpc/registry/actors/cron.rs (1)
  • register_actor_methods (24-41)
src/rpc/registry/actors/init.rs (1)
  • register_actor_methods (42-79)
src/rpc/registry/actors/evm.rs (1)
src/rpc/methods/state.rs (16)
  • fil_actors_shared (2098-2101)
  • fil_actors_shared (2110-2113)
  • fil_actors_shared (2122-2125)
  • fil_actors_shared (2134-2137)
  • fil_actors_shared (2146-2149)
  • fil_actors_shared (2158-2161)
  • fil_actors_shared (2240-2243)
  • fil_actors_shared (2252-2255)
  • fil_actors_shared (2264-2267)
  • fil_actors_shared (2276-2279)
  • fil_actors_shared (2288-2291)
  • fil_actors_shared (2300-2303)
  • fil_actors_shared (2727-2727)
  • fil_actors_shared (2735-2735)
  • fil_actors_shared (2742-2742)
  • fil_actors_shared (2752-2752)
src/rpc/registry/actors/account.rs (2)
src/rpc/registry/actors/cron.rs (1)
  • register_actor_methods (24-41)
src/rpc/registry/actors/eth_account.rs (1)
  • register_actor_methods (18-50)
src/lotus_json/actors/params/power_params.rs (3)
src/lotus_json/actors/params/datacap_params.rs (8)
  • snapshots (218-231)
  • snapshots (271-286)
  • snapshots (322-333)
  • snapshots (365-376)
  • snapshots (405-414)
  • snapshots (441-450)
  • snapshots (480-491)
  • snapshots (523-534)
src/lotus_json/actors/params/evm_params.rs (1)
  • snapshots (139-146)
src/lotus_json/actors/params/miner_params.rs (7)
  • snapshots (797-811)
  • snapshots (1152-1186)
  • snapshots (1279-1290)
  • snapshots (1403-1417)
  • snapshots (1532-1546)
  • snapshots (1679-1691)
  • snapshots (1752-1762)
src/rpc/registry/actors/payment_channel.rs (1)
src/rpc/registry/actors/cron.rs (1)
  • register_actor_methods (24-41)
src/rpc/registry/actors/miner.rs (2)
src/rpc/registry/actors/cron.rs (1)
  • register_actor_methods (24-41)
src/rpc/registry/actors/eam.rs (1)
  • register_actor_methods (28-46)
src/lotus_json/actors/params/account_params.rs (3)
src/shim/address.rs (2)
  • with (72-77)
  • new_id (142-144)
src/lotus_json/actors/params/datacap_params.rs (8)
  • snapshots (218-231)
  • snapshots (271-286)
  • snapshots (322-333)
  • snapshots (365-376)
  • snapshots (405-414)
  • snapshots (441-450)
  • snapshots (480-491)
  • snapshots (523-534)
src/lotus_json/actor_states/methods/account_constructor_params.rs (1)
  • AccountConstructorParamsLotusJson (10-14)
⏰ 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). (7)
  • GitHub Check: Build MacOS
  • GitHub Check: Build Ubuntu
  • GitHub Check: cargo-publish-dry-run
  • GitHub Check: Build forest binaries on Linux AMD64
  • GitHub Check: tests
  • GitHub Check: tests-release
  • GitHub Check: All lint checks
🔇 Additional comments (40)
src/lotus_json/actors/params/evm_params.rs (1)

64-64: v17 coverage for EVM params — LGTM

Both macro invocations correctly extend support through version 17 in line with the crate bumps. No further issues spotted here.

Also applies to: 127-127

src/lotus_json/actors/states/mod.rs (1)

3-3: Minor formatting change — OK

No functional impact.

Cargo.toml (1)

71-86: Actor state crates bumped to 24.1.2 — LGTM

Versions are in lockstep across all fil_actor_* crates and fil_actors_shared, matching the added V17 support in this PR.

src/lotus_json/actors/mod.rs (1)

4-6: Module split into params/states — LGTM

Private submodules align with the new actors namespace. No issues.

CHANGELOG.md (1)

33-33: Changelog entry added — LGTM

Entry is concise and correctly references PR #6000.

src/lotus_json/actors/params/cron_params.rs (1)

82-82: Add Cron constructor LotusJson for V17 — LGTM

Macro arity extension is correct; snapshots and conversions remain consistent.

src/lotus_json/actors/params/multisig_params.rs (1)

460-467: Extend multisig params to V17 — LGTM

All multisig param macros consistently include 17. No API surface change.

src/rpc/registry/actors/market.rs (1)

8-8: Switch to ActorVersion enum with exhaustive mapping — solid improvement

Type-safe dispatch without a catch-all is preferable. Mappings for V8–V17 route to the correct registration macros.

Also applies to: 178-214

src/lotus_json/actors/params/eam_params.rs (1)

150-152: V17 EAM params coverage — LGTM

Extending all three EAM param impl macros to include 17 is consistent with the registry changes and keeps conversions symmetric.

src/rpc/registry/actors/eth_account.rs (1)

18-49: Type-safe version dispatch with ActorVersion + V17 — LGTM

Signature change and exhaustive matching per ActorVersion look correct; registrations for V10–V17 are wired to the right state modules.

src/lotus_json/actors/params/paych_params.rs (2)

543-543: Constructor params now include V17 — LGTM

Macro arity update aligns constructor Lotus JSON bridging with supported versions.


546-546: UpdateChannelState v4 extended to V17 — LGTM

Extending the v4 adapter through V17 matches the fvm_shared4 signature usage in this file and other actors in the PR.

src/rpc/registry/actors/datacap.rs (2)

87-124: Generic macro for v11+ is tidy and consistent.

Shared FRC46 param types + BalanceParams/ConstructorParams align with exported methods.


50-50: Ignore v10 ConstructorParams suggestion; Address is correct
The v10 Datacap actor’s Constructor parameter type is Address, so the existing import and mapping are already correct.

Likely an incorrect or invalid review comment.

src/rpc/registry/actors/cron.rs (1)

24-40: Enum-based versioning + V17 support: LGTM.

Clean switch to ActorVersion with exhaustive match and unchanged method surface via macro.

src/rpc/registry/actors/system.rs (1)

23-39: System actor registry update is correct.

Constructor mapped to empty across V8–V17 via macro; exhaustive match is good.

src/rpc/registry/actors/eam.rs (1)

28-45: Enum-based version dispatch for EAM looks correct (V10–V17) with explicit V8/V9 no-ops.

Consistent with other actor modules; constructor and params registration via the macro is clear and maintainable.

src/rpc/registry/actors/reward.rs (1)

50-91: LGTM: ActorVersion dispatch (V8–V17) and method registration are correct.

Reuse of the 11–16 macro for V17 is fine given identical signatures.

src/rpc/registry/actors/init.rs (2)

7-7: LGTM: switch to ActorVersion

Import and enum-based dispatch align with the repo-wide migration.


42-78: Exhaustive match over ActorVersion: good

Explicit arms V8–V17 with no fallback improves safety.

src/rpc/registry/actors/verified_reg.rs (2)

8-8: LGTM: ActorVersion import

Consistent with other actor registries.


239-267: LGTM: enum-based dispatcher

Per-version registrars are clear and exhaustive for V8–V17.

src/rpc/registry/actors/power.rs (3)

7-7: LGTM: ActorVersion import

Matches the project-wide API change.


141-173: LGTM: exhaustive match

Version routing mirrors other actors and includes V17.


101-108: ```shell
#!/bin/bash

Clone the Filecoin builtin-actors repo for version analysis

if [ ! -d builtin-actors ]; then
git clone https://github.com/filecoin-project/builtin-actors.git
fi
cd builtin-actors

List tags for v10 to v17

tags=$(git tag -l | grep -E "^v(10|11|12|13|14|15|16|17)(\.|$)" | sort -V)
echo "Analyzing tags: $tags"
for tag in $tags; do
echo "=== $tag ==="
git checkout -f $tag
echo "- NetworkRawPowerExported:"
rg -n "NetworkRawPowerExported" -g "actors/power/src/" || echo " (not present)"
echo "- MinerCountExported:"
rg -n "MinerCountExported" -g "actors/power/src/
" || echo " (not present)"
echo "- MinerConsensusCountExported:"
rg -n "MinerConsensusCountExported" -g "actors/power/src/**" || echo " (not present)"
echo
done


</blockquote></details>
<details>
<summary>src/rpc/registry/actors/account.rs (1)</summary><blockquote>

`77-95`: **Double-check “AuthenticateMessageExported” for v11–v14.**

Some versions might still expose `AuthenticateMessage` (not `...Exported`). Please confirm against fil_actor_account_state to avoid mismatched method numbers.

</blockquote></details>
<details>
<summary>src/rpc/registry/actors/payment_channel.rs (1)</summary><blockquote>

`33-69`: **Enum-based dispatch across V8–V17 looks solid**

Explicit per-variant registration is clear and consistent with other actors.

</blockquote></details>
<details>
<summary>src/lotus_json/actors/params/power_params.rs (1)</summary><blockquote>

`341-345`: **v17 coverage for power params macros — LGTM**

Macro expansions now include v17 consistently.

</blockquote></details>
<details>
<summary>src/rpc/registry/actors/miner.rs (2)</summary><blockquote>

`136-151`: **v16+ common methods block — LGTM**

Separate macro for v16 onwards keeps new exported calls tidy.

---

`374-386`: **v16 block — LGTM**

Correctly registers ExtendSectorExpiration/ProveCommitAggregate/ProveReplicaUpdates for v16.

</blockquote></details>
<details>
<summary>src/lotus_json/actors/params/init_params.rs (2)</summary><blockquote>

`98-106`: **Sanity-check default CID snapshot**

The snapshot uses `"/": "baeaaaaa"` while the struct uses `Cid::default()`. Please confirm that your `LotusJson<Cid>` encoder renders `Cid::default()` as `"baeaaaaa"`; otherwise tests will fail.



Run to audit usage consistency across the repo:

```shell
#!/bin/bash
# Check how often "CodeCid" vs "CodeCID" appears in Lotus JSON snapshots/structs.
rg -nP --type=rs -C2 '"CodeCid"|CodeCID|Subaddress|SubAddress' src/lotus_json

# Optional: list places where Cid::default() is used in snapshots to compare expected string form.
rg -nP --type=rs -C2 'Cid::default\(\).*json!' src | sed -n '1,200p'

Also applies to: 142-151


21-30: Fix JSON field names to match Lotus conventions ("CodeCid", "Subaddress")

The struct-level serde config would yield "CodeCid" and "Subaddress". Overriding to "CodeCID" and using "sub_address" will break round-trips and the snapshots. Align the field names with Lotus and your snapshots.

Apply:

@@
 pub struct InitExecParamsLotusJson {
     #[schemars(with = "LotusJson<Cid>")]
     #[serde(with = "crate::lotus_json")]
-    #[serde(rename = "CodeCID")]
     pub code_cid: Cid,
@@
 pub struct InitExec4ParamsLotusJson {
     #[schemars(with = "LotusJson<Cid>")]
     #[serde(with = "crate::lotus_json")]
-    #[serde(rename = "CodeCID")]
     pub code_cid: Cid,
@@
-    #[schemars(with = "LotusJson<RawBytes>")]
-    #[serde(with = "crate::lotus_json")]
-    pub sub_address: RawBytes,
+    #[schemars(with = "LotusJson<RawBytes>")]
+    #[serde(with = "crate::lotus_json")]
+    pub subaddress: RawBytes,
 }

Also applies to: 32-47

⛔ Skipped due to learnings
Learnt from: akaladarshi
PR: ChainSafe/forest#5896
File: src/lotus_json/actor_states/methods/verified_reg_actor.rs:133-137
Timestamp: 2025-08-18T12:25:29.183Z
Learning: In Lotus JSON for verified registry actor, the SectorAllocationClaimsLotusJson struct uses "SectorExpiry" as the field name for the expiry field, not "Expiry". The current code with #[serde(rename = "SectorExpiry")] is correct.
Learnt from: akaladarshi
PR: ChainSafe/forest#5996
File: src/lotus_json/actor_states/methods/paych_params.rs:56-58
Timestamp: 2025-09-02T08:44:08.346Z
Learning: In Lotus JSON for payment channel SignedVoucher, the secret_pre_image field should use serde rename to "SecretHash" (not "SecretPreImage") for Lotus API compatibility, even though the internal struct field is named SecretPreimage. This matches the actual JSON format used by Lotus.
src/rpc/registry/actors/multisig.rs (1)

76-108: Enum-based dispatch LGTM

Clean switch to ActorVersion with exhaustive matching. V8 correctly excludes UniversalReceiverHook; V9+ includes it via the dedicated macro. Looks consistent with other actor registries.

src/lotus_json/actors/params/datacap_params.rs (1)

51-51: v17 support added correctly

Extending the macro invocations to include v17 for balance, constructor, destroy, and mint params is consistent with the rest of the PR. No signature mismatches apparent.

Also applies to: 88-88, 138-138, 198-198

src/lotus_json/actors/params/reward_params.rs (1)

176-176: Reward param adapters extended to v17 — looks good

Constructor, AwardBlockReward, and UpdateNetworkKPI now cover 17; types remain aligned with fvm_shared4 bigint paths.

Also applies to: 178-180

src/lotus_json/actors/params/verified_reg_params.rs (1)

1239-1255: Verified-reg v17 extensions are coherent

All affected macros now include 17, matching the cross-repo expansion. No structural changes beyond widening the sets.

src/lotus_json/actors/params/miner_params.rs (1)

3741-3741: ExtendSectorExpirationParams missing v17

Versions 9–16 are covered; v17 should be included for parity with other miner params.

-impl_miner_extend_sector_expiration_params_v9_onwards!(9, 10, 11, 12, 13, 14, 15, 16);
+impl_miner_extend_sector_expiration_params_v9_onwards!(9, 10, 11, 12, 13, 14, 15, 16, 17);
⛔ Skipped due to learnings
Learnt from: akaladarshi
PR: ChainSafe/forest#5923
File: src/rpc/registry/actors/miner.rs:0-0
Timestamp: 2025-08-14T06:46:08.056Z
Learning: ProveCommitSectors3 method is available in fil_actor_miner_state::v13 and should be mapped to ProveCommitSectors3Params, not ProveCommitSectorParams.
src/lotus_json/mod.rs (1)

221-221: Module rename aligns with new layout

Switching to mod actors; matches the folder reorg to actors/{params,states}. No issues.

src/lotus_json/actors/params/mod.rs (1)

4-17: New params module index looks good

Private submodules keep surface area tight and align with the new actors/* organization.

src/rpc/registry/actors/evm.rs (1)

43-58: ActorVersion-based EVM registration LGTM

Enum match is exhaustive; V8/V9 no-op and V10–V17 wired through register_evm_version! consistently.

@akaladarshi akaladarshi marked this pull request as ready for review September 10, 2025 11:36
sudo-shashank
sudo-shashank previously approved these changes Sep 10, 2025
Copy link
Contributor

@sudo-shashank sudo-shashank left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

sudo-shashank
sudo-shashank previously approved these changes Sep 11, 2025
hanabi1224
hanabi1224 previously approved these changes Sep 11, 2025
@akaladarshi akaladarshi added this pull request to the merge queue Sep 11, 2025
@hanabi1224 hanabi1224 removed this pull request from the merge queue due to a manual request Sep 11, 2025
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

♻️ Duplicate comments (1)
src/tool/subcommands/api_cmd/state_decode_params_tests/init.rs (1)

24-43: Good: replaced magic method numbers with typed Method constants.

This aligns with the PR objective and improves readability.

🧹 Nitpick comments (34)
src/state_migration/nv27/mod.rs (1)

15-15: Add trailing comma for cleaner diffs and rustfmt friendliness

Minor style nit; keeps future edits minimal.

-    fil_actor_system_state::v17::State
+    fil_actor_system_state::v17::State,
src/tool/subcommands/api_cmd/state_decode_params_tests/power.rs (4)

10-16: Consider more realistic peer and multiaddrs to exercise decoding paths.

peer: b"miner".to_vec() and empty multiaddrs decode fine but don’t stress non-trivial inputs. A sample PeerId (bytes) and at least one multiaddr would provide better coverage.


28-31: Use a non-empty payload for EnrollCronEvent to cover non-zero RawBytes.

An empty payload is valid but misses the non-empty decode path. Consider a small byte payload to increase coverage.


39-120: Reduce repetition: cache tipset key and/or add a tiny helper for request construction.

You repeat tipset.key().into() and the StateDecodeParams::request wrapper many times. Caching the TSK and/or a small helper cuts noise and future churn.

Example minimal change:

 pub fn create_tests(tipset: &Tipset) -> Result<Vec<RpcTest>> {
+    let tsk = tipset.key().to_owned().into();-    Ok(vec![
-        RpcTest::identity(StateDecodeParams::request((
-            Address::POWER_ACTOR, Method::CreateMiner as u64, to_vec(&power_create_miner_params)?, tipset.key().into(),
-        ))?),
+    Ok(vec![
+        RpcTest::identity(StateDecodeParams::request((
+            Address::POWER_ACTOR, Method::CreateMiner as u64, to_vec(&power_create_miner_params)?, tsk.clone(),
+        ))?),
         …
     ])
 }

Or introduce a local req(method, params) closure to DRY it up.


76-83: Keep TODOs synced with upstream status.

The Lotus/go-state-types note is helpful. Consider linking a short comment on when to re-enable the test (e.g., feature flag or version check) to avoid permanent dead code.

src/tool/subcommands/api_cmd/state_decode_params_tests/init.rs (4)

5-6: Prefer explicit imports to avoid wildcard pollution.

Make the imported items clear and reduce namespace collisions.

-use fil_actor_init_state::v17::*;
+use fil_actor_init_state::v17::{ConstructorParams, Exec4Params, ExecParams, Method};

13-17: Use a realistic 20-byte subaddress for Exec4.

Some decoders validate subaddress length; 3 bytes may be rejected.

     let init_exec4_params = Exec4Params {
         code_cid: Cid::default(),
         constructor_params: fvm_ipld_encoding::RawBytes::new(vec![0x12, 0x34, 0x56]), // dummy bytecode
-        subaddress: fvm_ipld_encoding::RawBytes::new(vec![0x12, 0x34, 0x56]), // dummy bytecode
+        subaddress: fvm_ipld_encoding::RawBytes::new(vec![0u8; 20]), // dummy 20-byte subaddress
     };

If Exec4Params::subaddress is fvm_shared::address::Subaddress in v17, switch to that type accordingly.


13-22: DRY: factor out repeated dummy constructor params.

Reduces duplication and keeps tests tidy.

-    let init_exec4_params = Exec4Params {
+    let dummy_constructor = fvm_ipld_encoding::RawBytes::new(vec![0x12, 0x34, 0x56]);
+    let init_exec4_params = Exec4Params {
         code_cid: Cid::default(),
-        constructor_params: fvm_ipld_encoding::RawBytes::new(vec![0x12, 0x34, 0x56]), // dummy bytecode
+        constructor_params: dummy_constructor.clone(), // dummy bytecode
         subaddress: fvm_ipld_encoding::RawBytes::new(vec![0u8; 20]), // dummy bytecode
     };
 
-    let init_exec_params = ExecParams {
+    let init_exec_params = ExecParams {
         code_cid: Cid::default(),
-        constructor_params: fvm_ipld_encoding::RawBytes::new(vec![0x12, 0x34, 0x56]), // dummy bytecode
+        constructor_params: dummy_constructor, // dummy bytecode
     };

7-8: Clarify doc to note version coverage.

Tiny doc tweak to reflect v17 focus and methods under test.

-/// Creates state decode params tests for the Init actor.
+/// Creates state decode params tests for the Init actor (v17): Constructor, Exec, Exec4.
src/tool/subcommands/api_cmd/state_decode_params_tests/ethaccount.rs (1)

5-5: Narrow the import to what’s used.

Prefer an explicit import to avoid pulling the whole module into scope.

Apply this diff:

-use fil_actor_ethaccount_state::v17::*;
+use fil_actor_ethaccount_state::v17::Method;
src/tool/subcommands/api_cmd/state_decode_params_tests/multisig.rs (5)

115-117: Avoid .unwrap() on Base64 decode; propagate errors instead.

Use ? to keep the function infallible-free and consistent with nearby ? uses.

@@
-    Ok(vec![
+    let urh_payload = BASE64_STANDARD.decode("ghgqRBI0Vng=")?;
+    Ok(vec![
@@
-            Method::UniversalReceiverHook as u64,
-            BASE64_STANDARD.decode("ghgqRBI0Vng=").unwrap(),
+            Method::UniversalReceiverHook as u64,
+            urh_payload,

101-112: Duplicate LockBalance test entry — intentional?

Two identical LockBalance cases add no signal and slow the suite. If unintentional, remove one.

@@
         RpcTest::identity(StateDecodeParams::request((
             Address::new_id(18101), // https://calibration.filscan.io/en/address/t018101/,
             Method::LockBalance as u64,
             to_vec(&multisig_lock_bal_params)?,
             tipset.key().into(),
         ))?),
-        RpcTest::identity(StateDecodeParams::request((
-            Address::new_id(18101), // https://calibration.filscan.io/en/address/t018101/,
-            Method::LockBalance as u64,
-            to_vec(&multisig_lock_bal_params)?,
-            tipset.key().into(),
-        ))?),

17-22: Replace magic number 0 in ProposeParams.method with the METHOD_SEND constant.

This aligns with the PR goal to avoid magic method numbers.

@@
-use base64::{Engine, prelude::BASE64_STANDARD};
+use base64::{Engine, prelude::BASE64_STANDARD};
+use fvm_shared::METHOD_SEND;
@@
-        method: 0,
+        method: METHOD_SEND,

52-118: DRY up repeated target address and tipset key.

Reduce repetition and chance of copy/paste drift by factoring these once.

@@
-    Ok(vec![
+    let target = Address::new_id(18101);
+    let tsk = tipset.key().into();
+    Ok(vec![
@@
-            Address::new_id(18101), // https://calibration.filscan.io/en/address/t018101/,
+            target.clone(), // https://calibration.filscan.io/en/address/t018101/
@@
-            tipset.key().into(),
+            tsk.clone(),
@@
-            Address::new_id(18101), // https://calibration.filscan.io/en/address/t018101/,
+            target.clone(),
@@
-            tipset.key().into(),
+            tsk.clone(),
@@
-            Address::new_id(18101), // https://calibration.filscan.io/en/address/t018101/,
+            target.clone(),
@@
-            tipset.key().into(),
+            tsk.clone(),
@@
-            Address::new_id(18101), // https://calibration.filscan.io/en/address/t018101/,
+            target.clone(),
@@
-            tipset.key().into(),
+            tsk.clone(),
@@
-            Address::new_id(18101), // https://calibration.filscan.io/en/address/t018101/,
+            target.clone(),
@@
-            tipset.key().into(),
+            tsk.clone(),
@@
-            Address::new_id(18101), // https://calibration.filscan.io/en/address/t018101/,
+            target.clone(),
@@
-            tipset.key().into(),
+            tsk.clone(),
@@
-            Address::new_id(18101), // https://calibration.filscan.io/en/address/t018101/,
+            target.clone(),
@@
-            tipset.key().into(),
+            tsk.clone(),
@@
-            Address::new_id(18101), // https://calibration.filscan.io/en/address/t018101/,
+            target.clone(),
@@
-            tipset.key().into(),
+            tsk.clone(),
@@
-            Address::new_id(18101), // https://calibration.filscan.io/en/address/t018101/,
+            target.clone(),
@@
-            tipset.key().into(),
+            tsk.clone(),

24-27: Confirm intended semantics of proposal_hash.

You set proposal_hash to a single zero byte. If an “empty/none” hash was intended, consider Vec::new() instead. If a non-empty placeholder is required, add a brief comment.

src/tool/subcommands/api_cmd/state_decode_params_tests/market.rs (4)

81-83: Use a single Address type for params (avoid unnecessary conversions).

WithdrawBalanceParams already expects an fvm_shared4 Address; using the shim Address then .into() adds noise. Match the style used for AddBalanceParams above.

-    let market_actor_withdraw_balance_params = WithdrawBalanceParams {
-        provider_or_client: Address::new_id(1000).into(),
+    let market_actor_withdraw_balance_params = WithdrawBalanceParams {
+        provider_or_client: fvm_shared4::address::Address::new_id(1000),
         amount: TokenAmount::default().into(),
     };

107-107: Prefer fvm_shared4 Address for exported call params as well.

Keeps Address usage consistent across all params and mirrors other initializations in this file.

-    let market_actor_get_balance_exported_params = Address::new_id(1000);
+    let market_actor_get_balance_exported_params = fvm_shared4::address::Address::new_id(1000);

38-45: Use distinct client and provider IDs for clearer test intent.

Having both set to the same ID is harmless for decode, but using different values avoids accidental conflation in future assertions or logs.

-            fvm_shared4::address::Address::new_id(1000),
+            fvm_shared4::address::Address::new_id(2000),

48-51: Use a 96-byte BLS signature to avoid potential length checks.

Some decoders or future validations may enforce BLS length. A 96-byte placeholder keeps the test resilient.

-            client_signature: fvm_shared4::crypto::signature::Signature::new_bls(
-                b"test_signature".to_vec(),
-            ),
+            client_signature: fvm_shared4::crypto::signature::Signature::new_bls(vec![0u8; 96]),
src/tool/subcommands/api_cmd/state_decode_params_tests/datacap.rs (4)

74-171: Use the canonical Datacap actor address constant and DRY it up.

"Address::DATACAP_TOKEN_ACTOR" may not exist (in fvm_shared constants it's typically DATACAP_TOKEN_ACTOR_ADDR). Define a single local constant and reuse it to avoid future renames and reduce repetition. Please verify which constant your codebase exports and adjust accordingly.

@@
-use super::*;
-use fil_actor_datacap_state::v17::*;
+use super::*;
+use fil_actor_datacap_state::v17::*;
+use fvm_shared::address::DATACAP_TOKEN_ACTOR_ADDR;
+
+// Single source of truth for the Datacap actor address used below.
+const DATACAP: Address = DATACAP_TOKEN_ACTOR_ADDR;
@@
-        RpcTest::identity(StateDecodeParams::request((
-            Address::DATACAP_TOKEN_ACTOR,
+        RpcTest::identity(StateDecodeParams::request((
+            DATACAP,
             Method::Constructor as u64,
@@
-        RpcTest::identity(StateDecodeParams::request((
-            Address::DATACAP_TOKEN_ACTOR,
+        RpcTest::identity(StateDecodeParams::request((
+            DATACAP,
             Method::MintExported as u64,
@@
-        RpcTest::identity(StateDecodeParams::request((
-            Address::DATACAP_TOKEN_ACTOR,
+        RpcTest::identity(StateDecodeParams::request((
+            DATACAP,
             Method::DestroyExported as u64,
@@
-        RpcTest::identity(StateDecodeParams::request((
-            Address::DATACAP_TOKEN_ACTOR,
+        RpcTest::identity(StateDecodeParams::request((
+            DATACAP,
             Method::NameExported as u64,
@@
-        RpcTest::identity(StateDecodeParams::request((
-            Address::DATACAP_TOKEN_ACTOR,
+        RpcTest::identity(StateDecodeParams::request((
+            DATACAP,
             Method::SymbolExported as u64,
@@
-        RpcTest::identity(StateDecodeParams::request((
-            Address::DATACAP_TOKEN_ACTOR,
+        RpcTest::identity(StateDecodeParams::request((
+            DATACAP,
             Method::TotalSupplyExported as u64,
@@
-        RpcTest::identity(StateDecodeParams::request((
-            Address::DATACAP_TOKEN_ACTOR,
+        RpcTest::identity(StateDecodeParams::request((
+            DATACAP,
             Method::BalanceExported as u64,
@@
-        RpcTest::identity(StateDecodeParams::request((
-            Address::DATACAP_TOKEN_ACTOR,
+        RpcTest::identity(StateDecodeParams::request((
+            DATACAP,
             Method::GranularityExported as u64,
@@
-        RpcTest::identity(StateDecodeParams::request((
-            Address::DATACAP_TOKEN_ACTOR,
+        RpcTest::identity(StateDecodeParams::request((
+            DATACAP,
             Method::TransferExported as u64,
@@
-        RpcTest::identity(StateDecodeParams::request((
-            Address::DATACAP_TOKEN_ACTOR,
+        RpcTest::identity(StateDecodeParams::request((
+            DATACAP,
             Method::TransferFromExported as u64,
@@
-        RpcTest::identity(StateDecodeParams::request((
-            Address::DATACAP_TOKEN_ACTOR,
+        RpcTest::identity(StateDecodeParams::request((
+            DATACAP,
             Method::IncreaseAllowanceExported as u64,
@@
-        RpcTest::identity(StateDecodeParams::request((
-            Address::DATACAP_TOKEN_ACTOR,
+        RpcTest::identity(StateDecodeParams::request((
+            DATACAP,
             Method::DecreaseAllowanceExported as u64,
@@
-        RpcTest::identity(StateDecodeParams::request((
-            Address::DATACAP_TOKEN_ACTOR,
+        RpcTest::identity(StateDecodeParams::request((
+            DATACAP,
             Method::RevokeAllowanceExported as u64,
@@
-        RpcTest::identity(StateDecodeParams::request((
-            Address::DATACAP_TOKEN_ACTOR,
+        RpcTest::identity(StateDecodeParams::request((
+            DATACAP,
             Method::BurnExported as u64,
@@
-        RpcTest::identity(StateDecodeParams::request((
-            Address::DATACAP_TOKEN_ACTOR,
+        RpcTest::identity(StateDecodeParams::request((
+            DATACAP,
             Method::BurnFromExported as u64,
@@
-        RpcTest::identity(StateDecodeParams::request((
-            Address::DATACAP_TOKEN_ACTOR,
+        RpcTest::identity(StateDecodeParams::request((
+            DATACAP,
             Method::AllowanceExported as u64,

5-6: Prefer explicit imports over wildcard.

Using v17::* and super::* makes API drift harder to spot during upgrades. Import only the items used (ConstructorParams, MintParams, DestroyParams, BalanceParams, Method, TokenAmount, Address, StateDecodeParams, RpcTest, Tipset, to_vec).


9-27: Consider non-zero TokenAmounts for better coverage.

Zero amounts serialize fine, but using small non-zero values can exercise more code paths in encoders/decoders across implementations.


9-72: Unify type sources where possible.

You mix v17-specific params (Constructor, Mint, Destroy, Balance) with shared FRC-46 types elsewhere. If shared equivalents exist for these, prefer them to reduce version-coupling. Keep v17-only types where no shared equivalent exists.

src/tool/subcommands/api_cmd/state_decode_params_tests/verified_reg.rs (4)

96-193: Qualify Method to avoid shadowing and make the actor/version explicit

Using a bare Method::... after a glob import risks name clashes and reduces clarity across versions. Qualify it with the crate path.

-            Method::Constructor as u64,
+            fil_actor_verifreg_state::v17::Method::Constructor as u64,
@@
-            Method::AddVerifier as u64,
+            fil_actor_verifreg_state::v17::Method::AddVerifier as u64,
@@
-            Method::RemoveVerifier as u64,
+            fil_actor_verifreg_state::v17::Method::RemoveVerifier as u64,
@@
-            Method::AddVerifiedClient as u64,
+            fil_actor_verifreg_state::v17::Method::AddVerifiedClient as u64,
@@
-            Method::RemoveVerifiedClientDataCap as u64,
+            fil_actor_verifreg_state::v17::Method::RemoveVerifiedClientDataCap as u64,
@@
-            Method::RemoveExpiredAllocations as u64,
+            fil_actor_verifreg_state::v17::Method::RemoveExpiredAllocations as u64,
@@
-            Method::ClaimAllocations as u64,
+            fil_actor_verifreg_state::v17::Method::ClaimAllocations as u64,
@@
-            Method::GetClaims as u64,
+            fil_actor_verifreg_state::v17::Method::GetClaims as u64,
@@
-            Method::ExtendClaimTerms as u64,
+            fil_actor_verifreg_state::v17::Method::ExtendClaimTerms as u64,
@@
-            Method::RemoveExpiredClaims as u64,
+            fil_actor_verifreg_state::v17::Method::RemoveExpiredClaims as u64,
@@
-            Method::AddVerifiedClientExported as u64,
+            fil_actor_verifreg_state::v17::Method::AddVerifiedClientExported as u64,
@@
-            Method::RemoveExpiredAllocationsExported as u64,
+            fil_actor_verifreg_state::v17::Method::RemoveExpiredAllocationsExported as u64,
@@
-            Method::GetClaimsExported as u64,
+            fil_actor_verifreg_state::v17::Method::GetClaimsExported as u64,
@@
-            Method::ExtendClaimTermsExported as u64,
+            fil_actor_verifreg_state::v17::Method::ExtendClaimTermsExported as u64,
@@
-            Method::RemoveExpiredClaimsExported as u64,
+            fil_actor_verifreg_state::v17::Method::RemoveExpiredClaimsExported as u64,
@@
-            Method::UniversalReceiverHook as u64,
+            fil_actor_verifreg_state::v17::Method::UniversalReceiverHook as u64,

33-41: Use realistic signature sizes to prevent future validation flakiness

If upstream starts validating lengths, these test vectors could fail. Prefer canonical sizes: 96 bytes (BLS) and 65 bytes (secp256k1).

-            signature: fvm_shared4::crypto::signature::Signature::new_bls(
-                b"test_signature_1".to_vec(),
-            ),
+            signature: fvm_shared4::crypto::signature::Signature::new_bls(vec![0u8; 96]),
@@
-            signature: fvm_shared4::crypto::signature::Signature::new_secp256k1(
-                b"test_signature_2".to_vec(),
-            ),
+            signature: fvm_shared4::crypto::signature::Signature::new_secp256k1(vec![1u8; 65]),

16-16: Improve readability of byte sizes

Use bit shifts for IEC units.

-        allowance: StoragePower::from(1048576u64), // 1MB
+        allowance: StoragePower::from(1u64 << 20), // 1 MiB
@@
-        allowance: types::DataCap::from(2097152u64), // 2MB
+        allowance: types::DataCap::from(2u64 << 20), // 2 MiB
@@
-        data_cap_amount_to_remove: types::DataCap::from(1048576u64),
+        data_cap_amount_to_remove: types::DataCap::from(1u64 << 20),

Also applies to: 25-26, 30-30


14-17: Use types::DataCap for AddVerifierParams.allowance (v17)

v17 declares AddVerifierParams.allowance as types::DataCap (alias for abi::StoragePower). Replace StoragePower::from(1048576u64) with types::DataCap::from(1048576u64) to mirror the client path and keep type consistency — applies to src/tool/subcommands/api_cmd/state_decode_params_tests/verified_reg.rs lines 14–17, 23–26, 29–31.

src/tool/subcommands/api_cmd/state_decode_params_tests/miner.rs (4)

56-56: Use 32-byte randomness to match expected PoSt commitment size.

Future validation could enforce length; use a 32-byte vector now.

-        chain_commit_rand: Randomness(vec![1, 22, 43]),
+        chain_commit_rand: Randomness([0u8; 32].to_vec()),

153-153: Use valid padded piece size values.

23 is not a valid padded piece size. Prefer powers of two (e.g., 2048).

-                size: fvm_shared4::piece::PaddedPieceSize(23),
+                size: fvm_shared4::piece::PaddedPieceSize(2048),

173-173: Ditto: use a valid padded piece size.

12 → 512 (or another valid size).

-                size: fvm_shared4::piece::PaddedPieceSize(12),
+                size: fvm_shared4::piece::PaddedPieceSize(512),

221-505: Reduce boilerplate with a tiny helper for test construction.

A helper keeps the method casting and tipset plumbing in one place, making additions less error-prone.

Add near the top of the module:

fn mk(to: Address, method: Method, params: Vec<u8>, tipset: &Tipset) -> Result<RpcTest> {
    RpcTest::identity(StateDecodeParams::request((to, method as u64, params, tipset.key().into()))?)
}

Then replace repetitive blocks like:

RpcTest::identity(StateDecodeParams::request((
    MINER_ADDRESS, Method::ControlAddresses as u64, vec![], tipset.key().into(),
))?),

with:

mk(MINER_ADDRESS, Method::ControlAddresses, vec![], tipset)?,
src/tool/subcommands/api_cmd/state_decode_params_tests/evm.rs (3)

33-75: Avoid unwraps; parse the address once and reuse.

Prevents panic on bad input and removes repeated parsing work.

Apply:

 pub fn create_tests(tipset: &Tipset) -> Result<Vec<RpcTest>> {
+    let evm_address = Address::from_str(EVM_ADDRESS)?;
@@
-        RpcTest::identity(StateDecodeParams::request((
-            Address::from_str(EVM_ADDRESS).unwrap(),
+        RpcTest::identity(StateDecodeParams::request((
+            evm_address.clone(),
             Method::Constructor as u64,
@@
-        RpcTest::identity(StateDecodeParams::request((
-            Address::from_str(EVM_ADDRESS).unwrap(),
+        RpcTest::identity(StateDecodeParams::request((
+            evm_address.clone(),
             Method::Resurrect as u64,
@@
-        RpcTest::identity(StateDecodeParams::request((
-            Address::from_str(EVM_ADDRESS).unwrap(),
+        RpcTest::identity(StateDecodeParams::request((
+            evm_address.clone(),
             Method::GetBytecode as u64,
@@
-        RpcTest::identity(StateDecodeParams::request((
-            Address::from_str(EVM_ADDRESS).unwrap(),
+        RpcTest::identity(StateDecodeParams::request((
+            evm_address.clone(),
             Method::GetBytecodeHash as u64,
@@
-        RpcTest::identity(StateDecodeParams::request((
-            Address::from_str(EVM_ADDRESS).unwrap(),
+        RpcTest::identity(StateDecodeParams::request((
+            evm_address.clone(),
             Method::InvokeContract as u64,
@@
-        RpcTest::identity(StateDecodeParams::request((
-            Address::from_str(EVM_ADDRESS).unwrap(),
+        RpcTest::identity(StateDecodeParams::request((
+            evm_address.clone(),
             Method::InvokeContractDelegate as u64,
@@
-        RpcTest::identity(StateDecodeParams::request((
-            Address::from_str(EVM_ADDRESS).unwrap(),
+        RpcTest::identity(StateDecodeParams::request((
+            evm_address,
             Method::GetStorageAt as u64,

Additionally, please verify that EVM_ADDRESS resolves to an actual EVM-actor instance on all chains/snapshots you run against; otherwise StateDecodeParams may fail to resolve actor code for decoding. If not guaranteed, consider making the address configurable or discovering an EVM actor address from the state at tipset.


23-28: Double-check DelegateCallParams.value type and units.

Ensure value: TokenAmount::default().into() matches the expected type (e.g., Option<TokenAmount> or a plain TokenAmount) and unit (attoFIL). If it’s a TokenAmount, prefer an explicit zero to avoid ambiguity.

Possible tweak:

-        value: TokenAmount::default().into(),
+        value: TokenAmount::from_atto(0).into(),

Also consider explicitly importing the type for clarity:

+use fvm_shared::econ::TokenAmount;

13-75: Gate these tests on actor version availability.

This module hard-codes v17 types. If the supplied tipset predates actors v17, decoding can diverge. Consider skipping (returning Ok(vec![])) unless v17 is active at tipset.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 6f54f79 and d7a4cb2.

📒 Files selected for processing (17)
  • Cargo.toml (1 hunks)
  • src/state_migration/nv27/mod.rs (1 hunks)
  • src/tool/subcommands/api_cmd/state_decode_params_tests/account.rs (1 hunks)
  • src/tool/subcommands/api_cmd/state_decode_params_tests/cron.rs (1 hunks)
  • src/tool/subcommands/api_cmd/state_decode_params_tests/datacap.rs (1 hunks)
  • src/tool/subcommands/api_cmd/state_decode_params_tests/eam.rs (1 hunks)
  • src/tool/subcommands/api_cmd/state_decode_params_tests/ethaccount.rs (1 hunks)
  • src/tool/subcommands/api_cmd/state_decode_params_tests/evm.rs (1 hunks)
  • src/tool/subcommands/api_cmd/state_decode_params_tests/init.rs (1 hunks)
  • src/tool/subcommands/api_cmd/state_decode_params_tests/market.rs (1 hunks)
  • src/tool/subcommands/api_cmd/state_decode_params_tests/miner.rs (1 hunks)
  • src/tool/subcommands/api_cmd/state_decode_params_tests/multisig.rs (1 hunks)
  • src/tool/subcommands/api_cmd/state_decode_params_tests/paych.rs (1 hunks)
  • src/tool/subcommands/api_cmd/state_decode_params_tests/power.rs (1 hunks)
  • src/tool/subcommands/api_cmd/state_decode_params_tests/reward.rs (1 hunks)
  • src/tool/subcommands/api_cmd/state_decode_params_tests/system.rs (1 hunks)
  • src/tool/subcommands/api_cmd/state_decode_params_tests/verified_reg.rs (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (7)
  • src/tool/subcommands/api_cmd/state_decode_params_tests/reward.rs
  • src/tool/subcommands/api_cmd/state_decode_params_tests/system.rs
  • Cargo.toml
  • src/tool/subcommands/api_cmd/state_decode_params_tests/paych.rs
  • src/tool/subcommands/api_cmd/state_decode_params_tests/eam.rs
  • src/tool/subcommands/api_cmd/state_decode_params_tests/account.rs
  • src/tool/subcommands/api_cmd/state_decode_params_tests/cron.rs
🧰 Additional context used
🧬 Code graph analysis (9)
src/tool/subcommands/api_cmd/state_decode_params_tests/power.rs (1)
src/tool/subcommands/api_cmd/api_compare_tests.rs (4)
  • identity (330-332)
  • request (263-263)
  • request (288-288)
  • power_actor_state_decode_params_tests (2190-2302)
src/tool/subcommands/api_cmd/state_decode_params_tests/evm.rs (2)
src/tool/subcommands/api_cmd/api_compare_tests.rs (4)
  • create_tests (2099-2132)
  • identity (330-332)
  • request (263-263)
  • request (288-288)
src/rpc/methods/eth/types.rs (1)
  • serialize_params (83-87)
src/tool/subcommands/api_cmd/state_decode_params_tests/verified_reg.rs (1)
src/tool/subcommands/api_cmd/api_compare_tests.rs (4)
  • identity (330-332)
  • request (263-263)
  • request (288-288)
  • verified_reg_actor_state_decode_params_tests (3138-3328)
src/tool/subcommands/api_cmd/state_decode_params_tests/datacap.rs (2)
src/tool/subcommands/api_cmd/state_decode_params_tests/account.rs (1)
  • create_tests (8-33)
src/tool/subcommands/api_cmd/api_compare_tests.rs (4)
  • create_tests (2099-2132)
  • identity (330-332)
  • request (263-263)
  • request (288-288)
src/tool/subcommands/api_cmd/state_decode_params_tests/miner.rs (1)
src/tool/subcommands/api_cmd/api_compare_tests.rs (5)
  • create_tests (2099-2132)
  • identity (330-332)
  • request (263-263)
  • request (288-288)
  • miner_actor_state_decode_params_tests (2472-3022)
src/tool/subcommands/api_cmd/state_decode_params_tests/market.rs (4)
src/tool/subcommands/api_cmd/api_compare_tests.rs (3)
  • identity (330-332)
  • request (263-263)
  • request (288-288)
src/blocks/chain4u.rs (1)
  • tipset (183-185)
src/shim/econ.rs (1)
  • from_atto (106-108)
src/shim/address.rs (1)
  • new_id (142-144)
src/tool/subcommands/api_cmd/state_decode_params_tests/init.rs (2)
src/tool/subcommands/api_cmd/state_decode_params_tests/account.rs (1)
  • create_tests (8-33)
src/tool/subcommands/api_cmd/state_decode_params_tests/evm.rs (1)
  • create_tests (13-76)
src/tool/subcommands/api_cmd/state_decode_params_tests/multisig.rs (1)
src/tool/subcommands/api_cmd/api_compare_tests.rs (4)
  • identity (330-332)
  • request (263-263)
  • request (288-288)
  • multisig_actor_state_decode_params_tests (3024-3136)
src/tool/subcommands/api_cmd/state_decode_params_tests/ethaccount.rs (2)
src/tool/subcommands/api_cmd/state_decode_params_tests/eam.rs (1)
  • create_tests (8-47)
src/tool/subcommands/api_cmd/api_compare_tests.rs (4)
  • create_tests (2099-2132)
  • identity (330-332)
  • request (263-263)
  • request (288-288)
⏰ 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). (7)
  • GitHub Check: Build Ubuntu
  • GitHub Check: cargo-publish-dry-run
  • GitHub Check: Build MacOS
  • GitHub Check: tests
  • GitHub Check: tests-release
  • GitHub Check: Build forest binaries on Linux AMD64
  • GitHub Check: All lint checks
🔇 Additional comments (16)
src/state_migration/nv27/mod.rs (2)

13-16: v17 system state added for NV27 migration — LGTM

This removes the duplicate v16 entry and correctly introduces v17. Ordering v16 -> v17 looks right for NV27.


13-16: NV27 → Actors v17 mapping verified

RPC registries, shim, NV27 migration, and state_decode_params tests reference ActorVersion::V17 / fil_actor_system_state::v17 — mapping is consistent and no changes required.

Relevant locations: src/state_migration/nv27/mod.rs, src/rpc/registry/actors/system.rs, src/shim/actors/builtin/system/mod.rs, src/tool/subcommands/api_cmd/state_decode_params_tests/*

src/tool/subcommands/api_cmd/state_decode_params_tests/power.rs (2)

13-16: Correct Window PoSt proof variant for CreateMiner — good fix.

Using RegisteredPoStProof::StackedDRGWindow32GiBV1P1 (Window, not Winning) matches CreateMiner semantics. LGTM.


23-26: Good use of typed deltas (StoragePower) for UpdateClaimedPower.

Typed values avoid unit confusion and match v17 definitions. LGTM.

src/tool/subcommands/api_cmd/state_decode_params_tests/ethaccount.rs (2)

5-5: Good use of v17 types and actor-specific method constants.

Importing the v17 module and using Method::Constructor avoids magic numbers and aligns with the PR’s objectives.

Also applies to: 11-11


10-10: Verify decode works when the delegated address doesn’t exist at the given tipset.

If StateDecodeParams resolves the actor code by looking up to in state, a synthetic f4 (EAM namespace + 20 zero bytes) may not exist and could cause the RPC to fail. Confirm that both Forest and Lotus can decode based purely on the delegated-address namespace without requiring the actor to be present at tipset. If not, consider:

  • Supplying a known existing EthAccount f4 at tipset, or
  • Adding a code-CID–based decode path (if supported), or
  • Marking this test “conditional/skip” when the actor is absent.
src/tool/subcommands/api_cmd/state_decode_params_tests/multisig.rs (1)

8-51: Good coverage and structure for v17 Multisig params.

Param construction looks correct and matches v17 types; this sets up solid parity tests.

src/tool/subcommands/api_cmd/state_decode_params_tests/market.rs (1)

125-275: Good use of typed Method constants instead of magic numbers.

Casting enum variants to u64 aligns with the request API while keeping readability and version-scoping via v17::Method.

src/tool/subcommands/api_cmd/state_decode_params_tests/datacap.rs (3)

28-41: LGTM: FRC-46 transfer params usage is correct.

The shared FRC-46 types and operator_data via RawBytes look good.


42-58: LGTM: Allowance operations are well-covered.

Increase/Decrease/Revoke params match FRC-46 expectations.


59-67: LGTM: Burn/BurnFrom params are correct.

Shapes align with FRC-46; using owner on BurnFrom is appropriate.

src/tool/subcommands/api_cmd/state_decode_params_tests/verified_reg.rs (2)

8-94: LGTM overall — v17 coverage and method constants look good

Test vectors are comprehensive and align with the PR goals (actor-specific method constants, v17 coverage).


158-186: No action — v17 exported method variants are present.
Confirmed fil_actor_verifreg_state v17 exposes AddVerifiedClientExported, RemoveExpiredAllocationsExported, GetClaimsExported, ExtendClaimTermsExported, RemoveExpiredClaimsExported.

src/tool/subcommands/api_cmd/state_decode_params_tests/miner.rs (1)

220-220: Keep constAddress::new_id is a const fn.
src/shim/address.rs defines pub const fn new_id(id: u64) -> Self (line 142), so const MINER_ADDRESS: Address = Address::new_id(78216); is valid; no change required.

Likely an incorrect or invalid review comment.

src/tool/subcommands/api_cmd/state_decode_params_tests/evm.rs (2)

30-31: No change needed — GetStorageAtParams::new accepts shorter inputs and left-pads to 32 bytes

GetStorageAtParams::new left-pads a supplied Vec into a [u8; 32] (GET_STORAGE_AT_PARAMS_ARRAY_LENGTH = 32), so vec![0xa] is valid and the existing test/serialization is correct (see src/rpc/methods/eth/types.rs).

Likely an incorrect or invalid review comment.


39-44: Resurrect uses ConstructorParams — no change required.
ResurrectParams is a type alias for ConstructorParams, so passing ConstructorParams (to_vec(&evm_constructor_params)) matches the actor ABI. (docs.rs)

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

🧹 Nitpick comments (4)
src/tool/subcommands/api_cmd/state_decode_params_tests/multisig.rs (4)

101-112: Duplicate LockBalance test entry — likely accidental.

The two LockBalance tests are identical. If duplication wasn’t intended, drop one.

Apply:

         RpcTest::identity(StateDecodeParams::request((
             Address::new_id(18101), // https://calibration.filscan.io/en/address/t018101/,
             Method::LockBalance as u64,
             to_vec(&multisig_lock_bal_params)?,
             tipset.key().into(),
         ))?),
-        RpcTest::identity(StateDecodeParams::request((
-            Address::new_id(18101), // https://calibration.filscan.io/en/address/t018101/,
-            Method::LockBalance as u64,
-            to_vec(&multisig_lock_bal_params)?,
-            tipset.key().into(),
-        ))?),

54-115: DRY: factor the multisig address into a const.

Avoid repeating Address::new_id(18101) 10×; using a single const makes future updates trivial.

@@
-    Ok(vec![
+    const MSIG_ID: u64 = 18_101;
+    Ok(vec![
         RpcTest::identity(StateDecodeParams::request((
-            Address::new_id(18101), // https://calibration.filscan.io/en/address/t018101/,
+            Address::new_id(MSIG_ID), // https://calibration.filscan.io/en/address/t018101/
             Method::Constructor as u64,
@@
-            Address::new_id(18101), // https://calibration.filscan.io/en/address/t018101/,
+            Address::new_id(MSIG_ID),
             Method::Propose as u64,
@@
-            Address::new_id(18101), // https://calibration.filscan.io/en/address/t018101/,
+            Address::new_id(MSIG_ID),
             Method::Approve as u64,
@@
-            Address::new_id(18101), // https://calibration.filscan.io/en/address/t018101/,
+            Address::new_id(MSIG_ID),
             Method::Cancel as u64,
@@
-            Address::new_id(18101), // https://calibration.filscan.io/en/address/t018101/,
+            Address::new_id(MSIG_ID),
             Method::AddSigner as u64,
@@
-            Address::new_id(18101), // https://calibration.filscan.io/en/address/t018101/,
+            Address::new_id(MSIG_ID),
             Method::RemoveSigner as u64,
@@
-            Address::new_id(18101), // https://calibration.filscan.io/en/address/t018101/,
+            Address::new_id(MSIG_ID),
             Method::SwapSigner as u64,
@@
-            Address::new_id(18101), // https://calibration.filscan.io/en/address/t018101/,
+            Address::new_id(MSIG_ID),
             Method::ChangeNumApprovalsThreshold as u64,
@@
-            Address::new_id(18101), // https://calibration.filscan.io/en/address/t018101/,
+            Address::new_id(MSIG_ID),
             Method::LockBalance as u64,
@@
-            Address::new_id(18101), // https://calibration.filscan.io/en/address/t018101/,
+            Address::new_id(MSIG_ID),
             Method::UniversalReceiverHook as u64,

24-27: Prefer explicit element type for readability.

vec![Default::default()] relies on inference; vec![0u8] is clearer and avoids accidental type changes.

     let multisig_tx_id_params = TxnIDParams {
         id: Default::default(),
-        proposal_hash: vec![Default::default()],
+        proposal_hash: vec![0u8],
     };

20-21: Confirm: ProposeParams.method = 0.

Is 0 intentional for the proposed call? If you meant a specific method, consider setting that method’s number explicitly for a more representative test.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between d7a4cb2 and 252728e.

📒 Files selected for processing (3)
  • src/tool/subcommands/api_cmd/state_decode_params_tests/ethaccount.rs (1 hunks)
  • src/tool/subcommands/api_cmd/state_decode_params_tests/evm.rs (1 hunks)
  • src/tool/subcommands/api_cmd/state_decode_params_tests/multisig.rs (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
  • src/tool/subcommands/api_cmd/state_decode_params_tests/evm.rs
  • src/tool/subcommands/api_cmd/state_decode_params_tests/ethaccount.rs
🧰 Additional context used
🧬 Code graph analysis (1)
src/tool/subcommands/api_cmd/state_decode_params_tests/multisig.rs (1)
src/tool/subcommands/api_cmd/api_compare_tests.rs (4)
  • identity (330-332)
  • request (263-263)
  • request (288-288)
  • multisig_actor_state_decode_params_tests (3024-3136)
⏰ 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). (7)
  • GitHub Check: Build Ubuntu
  • GitHub Check: Build MacOS
  • GitHub Check: cargo-publish-dry-run
  • GitHub Check: tests
  • GitHub Check: tests-release
  • GitHub Check: Build forest binaries on Linux AMD64
  • GitHub Check: All lint checks
🔇 Additional comments (2)
src/tool/subcommands/api_cmd/state_decode_params_tests/multisig.rs (2)

6-7: Good: upgraded to v17 API and actor method constants.

Using fil_actor_multisig_state::v17::* and Method::* as u64 satisfies the “no magic numbers” goal and aligns with the v17 expansion.


115-117: Good: fallible base64 decode with ? instead of unwrap().

This removes a potential panic path in tests.

@akaladarshi akaladarshi added this pull request to the merge queue Sep 11, 2025
Merged via the queue into main with commit 0eaaed1 Sep 11, 2025
48 of 50 checks passed
@akaladarshi akaladarshi deleted the akaladarshi/state-decode-param-cleanup branch September 11, 2025 16:46
Signor1 pushed a commit to Signor1/forest that referenced this pull request Sep 12, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

RPC requires calibnet RPC checks to run on CI

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Cleanup for the StateDecodeParams implementation

4 participants