src/tool/subcommands/api_cmd/test_snapshots.txt (1)

164-210: Snapshot list growth looks fine; consider ordering and dedup for stability.

The added miner StateDecodeParams snapshots are OK. For long-term maintainability and stable diffs across runs, consider:

• Keeping this list lexicographically ordered (or grouped by API) to minimize churn in future additions.
• Verifying there are no duplicate filenames.

src/tool/subcommands/api_cmd/api_compare_tests.rs (2)

2125-2131: Duplicate LockBalance test entry.

StateDecodeParams for Multisig LockBalance is added twice in a row. It will be de-duplicated at runtime, but we should remove the redundancy.

Apply this diff to remove the duplicate:

-        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(),
-        ))?),

---

2150-2700: Comprehensive Miner decode coverage; minor realism nitpicks only.

The breadth of v16 Miner methods (including exported) covered here is excellent for catching regressions in param decoding. A few minor nits you can consider later:

• Some sizes (e.g., PaddedPieceSize 12/23) are not realistic in the protocol. Decode paths don’t validate semantics, so it’s fine; optional to switch to canonical sizes to aid future readers.
• Several Cid::default() placeholders are acceptable for decode-only tests; if decode ever adds structural validation, swap to well-formed CIDs.

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

258-261: Minor naming consistency nit: register_miner_versions_14.

Function name suggests multiple versions, but handles only v14. Consider renaming to register_miner_version_14 for clarity. No functional impact.

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

src/tool/subcommands/api_cmd/api_compare_tests.rs (3)

2338-2341: Clarify that multiaddr bytes are intentionally arbitrary

new_multi_addrs: vec![BytesDe(vec![1, 2, 3])] is fine for decode-only tests, but the bytes aren’t a real multiaddr which could confuse future readers. Consider a brief inline comment to document intent.

Apply this diff to add a short note:

-    let miner_change_multiaddrs_params = fil_actor_miner_state::v16::ChangeMultiaddrsParams {
-        new_multi_addrs: vec![BytesDe(vec![1, 2, 3])],
-    };
+    let miner_change_multiaddrs_params = fil_actor_miner_state::v16::ChangeMultiaddrsParams {
+        // Arbitrary bytes (not a real multiaddr); sufficient for exercising decode paths.
+        new_multi_addrs: vec![BytesDe(vec![1, 2, 3])],
+    };

---

2369-2375: Use BitField::default() instead of from_bytes(&[])

BitField::from_bytes(&[])? risks a parse error if the encoding rules change and adds an unnecessary fallible path. For an empty set, Default::default() is clearer and infallible.

Apply this diff:

-    let miner_extend_sector_expiration_params =
-        fil_actor_miner_state::v16::ExtendSectorExpirationParams {
-            extensions: vec![fil_actor_miner_state::v16::ExpirationExtension {
-                deadline: 12,
-                partition: 123,
-                sectors: BitField::from_bytes(&[])?,
-                new_expiration: 1000,
-            }],
-        };
+    let miner_extend_sector_expiration_params =
+        fil_actor_miner_state::v16::ExtendSectorExpirationParams {
+            extensions: vec![fil_actor_miner_state::v16::ExpirationExtension {
+                deadline: 12,
+                partition: 123,
+                sectors: BitField::default(),
+                new_expiration: 1000,
+            }],
+        };

---

2569-2869: Reduce duplication with a tiny helper macro

The repeated boilerplate for building RpcTest::identity(StateDecodeParams::request((MINER_ADDRESS, method as u64, params, tipset.key().into()))) makes the test long and harder to maintain. A small macro can reduce noise and the chance of copy-paste mistakes.

Apply this diff to introduce a macro and show representative replacements; follow the same pattern for the rest:

 use fil_actor_miner_state::v16::Method;
-    Ok(vec![
+    macro_rules! miner_req {
+        ($m:expr, $p:expr) => {
+            RpcTest::identity(StateDecodeParams::request((
+                MINER_ADDRESS,
+                $m as u64,
+                $p,
+                tipset.key().into(),
+            ))?)
+        };
+    }
+    Ok(vec![
-        RpcTest::identity(StateDecodeParams::request((
-            MINER_ADDRESS,
-            Method::ControlAddresses as u64,
-            vec![],
-            tipset.key().into(),
-        ))?),
+        miner_req!(Method::ControlAddresses, vec![]),
 
-        RpcTest::identity(StateDecodeParams::request((
-            MINER_ADDRESS,
-            Method::ChangeWorkerAddress as u64,
-            to_vec(&miner_change_worker_params)?,
-            tipset.key().into(),
-        ))?),
+        miner_req!(Method::ChangeWorkerAddress, to_vec(&miner_change_worker_params)?),
 
-        RpcTest::identity(StateDecodeParams::request((
-            MINER_ADDRESS,
-            Method::ProveCommitAggregate as u64,
-            to_vec(&miner_prove_commit_aggregate_params)?,
-            tipset.key().into(),
-        ))?),
+        miner_req!(Method::ProveCommitAggregate, to_vec(&miner_prove_commit_aggregate_params)?),
 
-        RpcTest::identity(StateDecodeParams::request((
-            MINER_ADDRESS,
-            Method::GetVestingFundsExported as u64,
-            vec![],
-            tipset.key().into(),
-        ))?),
+        miner_req!(Method::GetVestingFundsExported, vec![]),
     ])

This cuts hundreds of repeated tokens and keeps the method/param matrix easy to scan.

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

src/tool/subcommands/api_cmd/api_compare_tests.rs (1)

2341-2891: Reduce duplication and future-proof across actor versions.

The test coverage is excellent, but the function is very long, highly repetitive, and hardwired to v16 types. Two improvements:

• Extract a tiny helper to encode params (and to build tests for no-params methods) to cut duplication/noise.
• Allow running the same suite across actor versions (v8–v16) where shapes match, gated by the tipset’s actor code. This better aligns with the PR objective “API compatibility across V8–V16”.

Example refactor directions:

• Introduce a local encode helper: fn encode<T: Serialize>(t: &T) -> anyhow::Result<Vec<u8>> { to_vec(t).map_err(Into::into) }.
• Build a small macro or a helper that takes (method: u64, params: Vec<u8>) -> RpcTest to avoid repeating the StateDecodeParams::request boilerplate.
• Optionally, add a version switch: detect miner actor code CID at tipset and map to a version module (v8…v16) to construct version-correct param structs.

Happy to draft the concrete refactor if you want to pursue this now.

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

💡 Knowledge Base configuration:

• MCP integration is disabled by default for public repositories
• Jira integration is disabled by default for public repositories
• Linear integration is disabled by default for public repositories

You can enable these settings in your CodeRabbit configuration.

src/tool/subcommands/api_cmd/api_compare_tests.rs (3)

2147-2152: Duplicate multisig LockBalance decode test — remove one copy.

There are two identical StateDecodeParams tests for Multisig LockBalance back-to-back. The second is redundant.

Apply this diff to remove the duplicate:

-        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(),
-        ))?),

---

2386-2387: Use 32-byte randomness for realism (optional).

Randomness is typically 32 bytes. Using a fixed-length buffer avoids surprising renderings across nodes.

Apply this diff:

-        chain_commit_rand: Randomness(vec![1, 22, 43]),
+        chain_commit_rand: Randomness(vec![0; 32]), // 32-byte randomness

---

2519-2522: Prefer canonical piece sizes for PaddedPieceSize (optional).

Values 23 and 12 are not typical piece sizes (normally multiples of 128). While decoding doesn’t validate semantics, using canonical sizes improves fidelity.

Example tweak:

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

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

Also applies to: 2541-2544

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

💡 Knowledge Base configuration:

• MCP integration is disabled by default for public repositories
• Jira integration is disabled by default for public repositories
• Linear integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

src/tool/subcommands/api_cmd/api_compare_tests.rs (1)

2412-2416: Good: empty BitFields use Default instead of from_bytes(&[])

Thanks for avoiding BitField::from_bytes(&[]); Default::default() (or BitField::new()) is correct for an empty RLE+. This addresses the earlier concern raised in past review.

Also applies to: 2420-2425, 2471-2474, 2476-2480, 2488-2491

src/tool/subcommands/api_cmd/api_compare_tests.rs (7)

2380-2383: Use a valid multiaddr byte sequence to avoid edge-case decoding quirks

Using [1,2,3] as a multiaddr payload is unlikely to represent a valid multiaddr. Prefer bytes from a real multiaddr (e.g., /ip4/127.0.0.1/tcp/1234) to reduce the chance of implementation-specific handling differences.

---

2399-2407: Randomness length: use 32 bytes to match common expectations

Many components expect 32-byte randomness. Using a short Vec may still decode but risks divergence. Recommend zero-padding to 32 bytes.

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

---

2536-2545: Use valid padded piece sizes

PaddedPieceSize(23) is not a typical valid value and could trigger validation in some stacks. Prefer standard sizes (e.g., 2048, 4096).

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

---

2560-2566: Likewise: prefer a standard piece size

Use a conventional piece size to avoid accidental rejections during decoding/printing.

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

---

2611-2619: Version coupling: v16-only Method enum may fail if MINER_ADDRESS isn’t v16

These tests hard-bind to fil_actor_miner_state::v16::Method. If MINER_ADDRESS at the snapshot tipsets is not v16 (PR promises V8–V16), some method numbers/param shapes may not match and the test will fail early rather than validating decode parity.

Consider either:

• Detecting the miner actor version at runtime (via StateReadState code CID) and choosing the corresponding vX::Method and param types, or
• Pinning the snapshots to tipsets where MINER_ADDRESS is known to be v16 and documenting this invariant.

---

2619-2626: Minor: misleading inline comment

“Methods without parameters” precedes a block where only the first call (ControlAddresses) has empty params; subsequent calls include params. Move the comment above the single no-param call to avoid confusion.

-        // Methods without parameters
-        RpcTest::identity(StateDecodeParams::request((
+        // Methods without parameters
+        RpcTest::identity(StateDecodeParams::request((
             MINER_ADDRESS,
             Method::ControlAddresses as u64,
             vec![],
             tipset.key().into(),
         ))?),
+        // Methods with parameters

---

2361-2370: Reduce boilerplate with a helper to build decode tests

The function is long and repetitive. A tiny helper reduces noise and future churn.

@@
-fn miner_actor_state_decode_params_tests(tipset: &Tipset) -> anyhow::Result<Vec<RpcTest>> {
+fn miner_actor_state_decode_params_tests(tipset: &Tipset) -> anyhow::Result<Vec<RpcTest>> {
+    use fil_actor_miner_state::v16::Method;
+    let mk = |m: Method, p: Vec<u8>| -> anyhow::Result<RpcTest> {
+        Ok(RpcTest::identity(StateDecodeParams::request((
+            MINER_ADDRESS,
+            m as u64,
+            p,
+            tipset.key().into(),
+        ))?))
+    };
@@
-        RpcTest::identity(StateDecodeParams::request((
-            MINER_ADDRESS,
-            Method::Constructor as u64,
-            to_vec(&miner_constructor_params)?,
-            tipset.key().into(),
-        ))?),
+        mk(Method::Constructor, to_vec(&miner_constructor_params)?)?,
@@
-        RpcTest::identity(StateDecodeParams::request((
-            MINER_ADDRESS,
-            Method::ControlAddresses as u64,
-            vec![],
-            tipset.key().into(),
-        ))?),
+        mk(Method::ControlAddresses, vec![])?,
@@
-        RpcTest::identity(StateDecodeParams::request((
-            MINER_ADDRESS,
-            Method::ChangeWorkerAddress as u64,
-            to_vec(&miner_change_worker_params)?,
-            tipset.key().into(),
-        ))?),
+        mk(Method::ChangeWorkerAddress, to_vec(&miner_change_worker_params)?)?,
 /* …apply mk to the remaining calls… */

Also applies to: 2613-2910

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

💡 Knowledge Base configuration:

• MCP integration is disabled by default for public repositories
• Jira integration is disabled by default for public repositories
• Linear integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

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.

src/tool/subcommands/api_cmd/api_compare_tests.rs (1)

2368-2918: Miner StateDecodeParams coverage added — add cross-version checks for ChangeOwnerAddress (v8–v10 vs v11+)

Solid, exhaustive param construction for v16. One gap: for ChangeOwnerAddress and ChangeOwnerAddressExported, v8–v10 expect a bare Address, while v11+ use ChangeOwnerAddressParams (as noted in learnings). Current tests only encode v16::ChangeOwnerAddressParams, so we may miss regressions in older versions.

Use this to confirm test coverage and registry wiring for v8–v10:

#!/bin/bash
# 1) Do we exercise v8–v10 miner param types anywhere?
rg -nP 'fil_actor_miner_state::v(8|9|10)::' -C2

# 2) Check miner registry wires ChangeOwnerAddress/Exported per version
rg -nP 'ChangeOwnerAddress(?:Exported)?' src/rpc/registry/actors/miner.rs -n -C3

# 3) Check tests only use v16 ChangeOwnerAddressParams
rg -n 'ChangeOwnerAddress' src/tool/subcommands/api_cmd/api_compare_tests.rs -n -C3

If missing, consider adding a small branch that, when run against snapshots from epochs where the miner actor code CID maps to v8–v10, encodes a bare Address for these two methods. I can help draft that if desired.

Additionally, a couple of small nits to reduce noise and improve realism:

• Prefer RawBytes::default() over RawBytes::new(vec![]) in a few places.
• Consider using a fixed 32-byte Randomness for chain_commit_rand (closer to production drand size), though not required for decode-only tests.

Apply within this block:

-        chain_commit_rand: Randomness(vec![1, 22, 43]),
+        chain_commit_rand: Randomness(vec![0u8; 32]),
@@
-            aggregate_proof: RawBytes::new(vec![]),
+            aggregate_proof: RawBytes::default(),
@@
-                replica_proof: RawBytes::new(vec![]),
+                replica_proof: RawBytes::default(),
@@
-            sector_proofs: vec![RawBytes::new(vec![])],
-            aggregate_proof: RawBytes::new(vec![]),
+            sector_proofs: vec![RawBytes::default()],
+            aggregate_proof: RawBytes::default(),
@@
-            sector_proofs: vec![RawBytes::new(vec![])],
-            aggregate_proof: RawBytes::new(vec![]),
+            sector_proofs: vec![RawBytes::default()],
+            aggregate_proof: RawBytes::default(),
@@
-            aggregate_proof: RawBytes::new(vec![23, 2, 23]),
+            aggregate_proof: RawBytes::from(vec![23, 2, 23]),

Lastly, nice job switching all empty BitFields to Default::default(); that addresses the prior RLE+ empty encoding issue flagged earlier.

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

💡 Knowledge Base configuration:

• MCP integration is disabled by default for public repositories
• Jira integration is disabled by default for public repositories
• Linear integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

Learnt from: akaladarshi
PR: ChainSafe/forest#5923
File: src/rpc/registry/actors/miner.rs:221-223
Timestamp: 2025-09-02T10:05:34.323Z
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.

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.