Skip to content

Conversation

@akaladarshi
Copy link
Collaborator

@akaladarshi akaladarshi commented Aug 26, 2025

Summary of changes

Changes introduced in this pull request:

  • Adds support for the payment channel actor in state decode params API

Reference issue to close (if applicable)

Closes #5992

Other information and links

File Name Method Name Method Number
filecoin_payment_channel_statedecodeparams_1756194613312797.rpcsnap.json.zst Constructor 1
filecoin_payment_channel_statedecodeparams_1756194613312868.rpcsnap.json.zst UpdateChannelState 2
filecoin_payment_channel_statedecodeparams_1756194613312941.rpcsnap.json.zst Settle 3
filecoin_payment_channel_statedecodeparams_1756194613313007.rpcsnap.json.zst Collect 4

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 Payment Channel actor support in the RPC method registry, registering Constructor, UpdateChannelState, Settle, and Collect across versions v8–v16 with multi-version handling.
    • Added JSON-serializable, versioned representations for payment-channel constructor params, vouchers, merges, and update-channel-state payloads for cross-version encode/decode.
  • Tests

    • Added API state-decode tests and four snapshot entries validating Payment Channel encoding/decoding.

@akaladarshi akaladarshi requested a review from a team as a code owner August 26, 2025 08:42
@akaladarshi akaladarshi requested review from LesnyRumcajs and hanabi1224 and removed request for a team August 26, 2025 08:42
@coderabbitai
Copy link
Contributor

coderabbitai bot commented Aug 26, 2025

Walkthrough

Adds PaymentChannel actor support to StateDecodeParams: new LotusJson param types for payment-channel methods (v8–v16), per-version RPC method registrations for the actor, API compare tests and snapshots for v16 paych decode cases, and a minor changelog cosmetic edit.

Changes

Cohort / File(s) Summary
Lotus JSON: PaymentChannel params
src/lotus_json/actor_states/methods/mod.rs, src/lotus_json/actor_states/methods/paych_params.rs
Add private paych_params submodule and introduce LotusJson structs and HasLotusJson conversions for PaymentChannel constructor and UpdateChannelState params across v8–v16 (versioned SignedVoucher V2/V3/V4, Merge, ModVerify, UpdateChannelState wrappers).
RPC registry: actor registration
src/rpc/registry/actors/mod.rs, src/rpc/registry/actors/payment_channel.rs, src/rpc/registry/methods_reg.rs
Add payment_channel actor module; implement register_actor_methods and macro-driven per-version registration to register Constructor, UpdateChannelState (typed) and Settle/Collect (empty) for actor versions 8..16 and hook into MethodRegistry dispatch.
Tooling: API compare tests & snapshots
src/tool/subcommands/api_cmd/api_compare_tests.rs, src/tool/subcommands/api_cmd/test_snapshots.txt
Add v16 paych state-decode test helper (Constructor, UpdateChannelState, Settle, Collect) and four new snapshot entries.
Changelog
CHANGELOG.md
Remove a prior changelog entry and tighten whitespace (cosmetic).

Sequence Diagram(s)

sequenceDiagram
  autonumber
  actor Node
  participant Registry as MethodRegistry
  participant Paych as rpc::registry::actors::payment_channel
  participant StateAPI as Filecoin.StateDecodeParams

  Note over Node,Registry: Startup
  Node->>Registry: init()
  Registry->>Registry: detect BuiltinActor::PaymentChannel
  Registry->>Paych: register_actor_methods(code_cid, version)
  alt version ∈ [8..16]
    Paych-->>Registry: register Constructor (typed), UpdateChannelState (typed), Settle (empty), Collect (empty)
  else
    Paych-->>Registry: no-op
  end

  Note over Node,StateAPI: Runtime decode request
  Node->>StateAPI: StateDecodeParams(actor=PaymentChannel, method, params_bytes)
  StateAPI->>Registry: lookup method schema & LotusJson mapping
  Registry-->>StateAPI: param schema / (de)serializers
  StateAPI-->>Node: decoded params (LotusJson -> internal)
Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Assessment against linked issues

Objective Addressed Explanation
Add support for PaymentChannel in Filecoin.StateDecodeParams (v8–v16) [#5992]

Out-of-scope changes

Code Change Explanation
CHANGELOG edit — removed changelog entry / whitespace (CHANGELOG.md) Cosmetic documentation edit not required for PaymentChannel StateDecodeParams functionality.

Possibly related PRs

Suggested reviewers

  • LesnyRumcajs
  • sudo-shashank
  • hanabi1224
✨ 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/add-paych-account-state-decode-params

🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.

Support

Need help? Create a ticket on our support page for assistance with any issues or questions.

CodeRabbit Commands (Invoked using PR/Issue comments)

Type @coderabbitai help to get the list of available commands.

Other keywords and placeholders

  • Add @coderabbitai ignore or @coderabbit ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Status, Documentation and Community

  • Visit our Status Page to check the current availability of CodeRabbit.
  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

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: 3

🧹 Nitpick comments (11)
src/lotus_json/actor_states/methods/paych_params.rs (3)

137-143: Align schema derivations and annotations across versions (V2/V3 missing JsonSchema and schemars attr for Secret)

V4 UpdateChannelStateParams derives JsonSchema and annotates Secret with schemars(with = …), V2/V3 do not. For consistency and API schema generation, mirror V4.

-#[derive(Serialize, Deserialize, Debug, Clone, PartialEq)]
+#[derive(Serialize, Deserialize, JsonSchema, Debug, Clone, PartialEq)]
 #[serde(rename_all = "PascalCase")]
 pub struct UpdateChannelStateParamsV2LotusJson {
     pub sv: SignedVoucherV2LotusJson,
-    #[serde(with = "crate::lotus_json")]
+    #[schemars(with = "LotusJson<Vec<u8>>")]
+    #[serde(with = "crate::lotus_json")]
     pub secret: Vec<u8>,
 }
@@
-#[derive(Serialize, Deserialize, Debug, Clone, PartialEq)]
+#[derive(Serialize, Deserialize, JsonSchema, Debug, Clone, PartialEq)]
 #[serde(rename_all = "PascalCase")]
 pub struct UpdateChannelStateParamsV3LotusJson {
     pub sv: SignedVoucherV3LotusJson,
-    #[serde(with = "crate::lotus_json")]
+    #[schemars(with = "LotusJson<Vec<u8>>")]
+    #[serde(with = "crate::lotus_json")]
     pub secret: Vec<u8>,
 }

Also applies to: 145-151


68-70: Comment contradicts behavior and snapshots regarding Merges null vs []

The comment states Lotus returns null when no merges; your snapshots and recommended serialization use []. Since from_lotus_json already tolerates None/null, prefer serializing [] for consistency and update the comment accordingly.

Would you like me to update the comment to: “Lotus may return null (not []), but we serialize empty as [], and accept null during deserialization for compatibility.”?

Also applies to: 99-101, 130-132


515-516: Minor: extra space in field initializer

Double space after colon is inconsistent.

-                                    actor:  e.actor.into(),
+                                    actor: e.actor.into(),
src/rpc/registry/methods_reg.rs (1)

268-275: Add PaymentChannel to supported actors test list

The test enumerates actors expected to have methods registered; adding PaymentChannel will guard against regressions in its registration.

         let supported_actors = vec![
             BuiltinActor::Account,
             BuiltinActor::Miner,
             BuiltinActor::EVM,
             BuiltinActor::Cron,
             BuiltinActor::DataCap,
+            BuiltinActor::PaymentChannel,
         ];

I can also add a focused test asserting that PaymentChannel Settle (3) and Collect (4) are registered as “empty” params and behave per the base64/{} convention.

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

4-5: Avoid macro/function name ambiguity by aliasing or fully-qualifying the macro.

This module defines a function named register_actor_methods while also importing register_actor_methods from methods_reg. Even though macros and functions live in different namespaces, the identical name hurts readability and can mislead IDE tooling. Alias the macro import (or fully-qualify the macro calls) to make intent explicit.

Apply this diff to disambiguate:

-use crate::rpc::registry::methods_reg::{MethodRegistry, register_actor_methods};
+use crate::rpc::registry::methods_reg::{MethodRegistry, register_actor_methods as register_methods_macro};

And update macro invocations:

-        register_actor_methods!(
+        register_methods_macro!(
             $registry,
             $code_cid,
             [
                 (Method::Constructor, ConstructorParams),
                 (Method::UpdateChannelState, UpdateChannelStateParams),
             ]
         );
@@
-        register_actor_methods!(
+        register_methods_macro!(
             $registry,
             $code_cid,
             [(Method::Settle, empty), (Method::Collect, empty),]
         );

8-29: Document the version-registration macro.

Add a short doc comment explaining that the macro wires PaymentChannel methods for a specific actor version, and that empty denotes parameterless methods. This helps future contributors mirror the pattern for other actors.


31-44: Log unsupported versions to aid debugging.

Silently ignoring unknown versions makes troubleshooting harder when new actor versions land. A low-level log on the default arm would help detect gaps without impacting behavior.

Example patch:

+use tracing::warn;
@@
     match version {
         8 => register_payment_channel_reg_versions!(registry, cid, fil_actor_paych_state::v8),
         9 => register_payment_channel_reg_versions!(registry, cid, fil_actor_paych_state::v9),
         10 => register_payment_channel_reg_versions!(registry, cid, fil_actor_paych_state::v10),
         11 => register_payment_channel_reg_versions!(registry, cid, fil_actor_paych_state::v11),
         12 => register_payment_channel_reg_versions!(registry, cid, fil_actor_paych_state::v12),
         13 => register_payment_channel_reg_versions!(registry, cid, fil_actor_paych_state::v13),
         14 => register_payment_channel_reg_versions!(registry, cid, fil_actor_paych_state::v14),
         15 => register_payment_channel_reg_versions!(registry, cid, fil_actor_paych_state::v15),
         16 => register_payment_channel_reg_versions!(registry, cid, fil_actor_paych_state::v16),
-        _ => {}
+        v => warn!("PaymentChannel actor: unsupported version {v}, skipping registration"),
     }
src/tool/subcommands/api_cmd/api_compare_tests.rs (4)

1919-1919: Don’t panic in tests: propagate serialization errors instead of unwrap.

to_vec(&constructor_params).unwrap() will panic on serialization errors and is inconsistent with surrounding tests that use ?. Prefer ? to bubble errors through anyhow::Result.

Apply this diff:

-            to_vec(&constructor_params).unwrap(),
+            to_vec(&constructor_params)?,

1923-1927: Same here: replace unwrap with error propagation.

For consistency and better failure reporting, use ? instead of unwrap().

Apply this diff:

-            to_vec(&update_channel_state).unwrap(),
+            to_vec(&update_channel_state)?,

1884-1893: Optional: factor Paych address into a constant to match existing style.

You already have MINER_ADDRESS, ACCOUNT_ADDRESS, and EVM_ADDRESS constants. Introducing a PAYCH_ADDRESS constant improves discoverability and reuse across tests.

Add near other address constants (outside this hunk):

const PAYCH_ADDRESS: Address = Address::new_id(66116); // payment channel actor t066116

Then update here:

-    let paych_address = Address::new_id(66116);
+    let paych_address = PAYCH_ADDRESS;

1884-1941: Consider adding at least one pre-v16 decode case (optional).

The registry supports v8–v16. While v16 coverage is great, a single additional case (e.g., v12) can catch cross-version param shape drift early. This is optional if snapshot data or Lotus JSON support isn’t readily available.

Would you like me to draft a minimal v12 decode test using the same scaffolding?

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

💡 Knowledge Base configuration:

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

You can enable these sources in your CodeRabbit configuration.

📥 Commits

Reviewing files that changed from the base of the PR and between 81115f8 and 2d41366.

📒 Files selected for processing (7)
  • src/lotus_json/actor_states/methods/mod.rs (1 hunks)
  • src/lotus_json/actor_states/methods/paych_params.rs (1 hunks)
  • src/rpc/registry/actors/mod.rs (1 hunks)
  • src/rpc/registry/actors/payment_channel.rs (1 hunks)
  • src/rpc/registry/methods_reg.rs (2 hunks)
  • src/tool/subcommands/api_cmd/api_compare_tests.rs (1 hunks)
  • src/tool/subcommands/api_cmd/test_snapshots.txt (1 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
src/tool/subcommands/api_cmd/api_compare_tests.rs (2)
src/shim/address.rs (1)
  • new_id (142-144)
src/blocks/tipset.rs (2)
  • key (336-339)
  • key (530-533)
src/rpc/registry/methods_reg.rs (1)
src/rpc/registry/actors/payment_channel.rs (1)
  • register_actor_methods (31-44)
src/lotus_json/actor_states/methods/paych_params.rs (2)
src/shim/address.rs (1)
  • new_id (142-144)
src/shim/econ.rs (1)
  • from_atto (106-108)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (9)
  • GitHub Check: Build forest binaries on Linux AMD64
  • GitHub Check: All lint checks
  • GitHub Check: tests-release
  • GitHub Check: tests
  • GitHub Check: cargo-publish-dry-run
  • GitHub Check: Build MacOS
  • GitHub Check: Build Ubuntu
  • GitHub Check: Analyze (rust)
  • GitHub Check: Analyze (go)
🔇 Additional comments (6)
src/lotus_json/actor_states/methods/paych_params.rs (1)

542-546: Version mapping looks correct (V2: v8–v9, V3: v10–v11, V4: v12–v16)

Registration covers V8–V16 as required by the issue, with appropriate param versions per actor version.

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

11-11: Module wiring added

payment_channel module is correctly exposed crate-privately alongside other actors.

src/lotus_json/actor_states/methods/mod.rs (1)

16-16: Paych params module inclusion

Private module inclusion is appropriate; keeps public API surface stable while enabling registry usage.

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

76-79: Actor imports updated to include payment_channel

Import ordering and names are consistent with existing pattern.


100-102: PaymentChannel dispatch integrated into registry

Actor-specific registration for PaymentChannel is correctly hooked into the central registry.

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

1879-1879: Nice: paych tests integrated into the state decode suite.

Wiring the PaymentChannel decode tests into state_decode_params_api_tests keeps coverage consistent with other actors.

Comment on lines +56 to +58
#[schemars(with = "LotusJson<Vec<u8>>")]
#[serde(with = "crate::lotus_json", rename = "SecretHash")]
pub secret_pre_image: Vec<u8>,
Copy link
Contributor

@coderabbitai coderabbitai bot Aug 26, 2025

Choose a reason for hiding this comment

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

⚠️ Potential issue

SecretPreImage field incorrectly renamed to SecretHash (breaks JSON shape and tests)

The SignedVoucher structs override the PascalCase name to "SecretHash". Lotus and your snapshots expect "SecretPreImage"; this mismatch will cause decoding snapshot tests to fail and diverge from Lotus.

Apply this diff to all three voucher versions:

@@
-    #[schemars(with = "LotusJson<Vec<u8>>")]
-    #[serde(with = "crate::lotus_json", rename = "SecretHash")]
-    pub secret_pre_image: Vec<u8>,
+    #[schemars(with = "LotusJson<Vec<u8>>")]
+    #[serde(with = "crate::lotus_json")]
+    pub secret_pre_image: Vec<u8>,

Also applies to: 87-90, 118-121

🤖 Prompt for AI Agents
In src/lotus_json/actor_states/methods/paych_params.rs around lines 56-58 (and
also update the same pattern at lines 87-90 and 118-121), the serde rename
attribute incorrectly sets the JSON field to "SecretHash" causing snapshots to
diverge from Lotus; change the serde rename value from "SecretHash" to
"SecretPreImage" for the SecretPreImage field in all three SignedVoucher
versions (keep the schemars and serde(with = "crate::lotus_json") attributes
intact) so the JSON shape matches Lotus and tests.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The field names are derived from the builtin-actors crate and the SecretHash added for lotus compatibility

Copy link
Contributor

Choose a reason for hiding this comment

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

.

The serde rename attribute is indeed correct for Lotus compatibility. My apologies for the confusion in the original review comment - the field naming you've implemented matches what Lotus expects in its JSON API format.


✏️ Learnings added
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.311Z
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.

Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.


🧠 Learnings used
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.

Comment on lines +269 to +277
merges: if self.sv.merges.is_empty() {
None
} else {
Some(self.sv.merges.into_iter().map(|m| MergeLotusJson {
lane: m.lane,
nonce: m.nonce,
}).collect())
},
signature: self.sv.signature,
Copy link
Contributor

@coderabbitai coderabbitai bot Aug 26, 2025

Choose a reason for hiding this comment

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

⚠️ Potential issue

Empty merges serialized as null; snapshots (and typical expectations) use []

into_lotus_json currently maps empty merges to None→null. Your snapshots emit "Merges": [], so tests will fail. Also, many clients expect an empty array when no merges are present.

Minimal fix: always serialize an array (possibly empty). Keep from_lotus_json tolerant of null via existing Option handling.

-                                merges: if self.sv.merges.is_empty() {
-                                    None
-                                } else {
-                                    Some(self.sv.merges.into_iter().map(|m| MergeLotusJson {
-                                        lane: m.lane,
-                                        nonce: m.nonce,
-                                    }).collect())
-                                },
+                                merges: Some(
+                                    self.sv
+                                        .merges
+                                        .into_iter()
+                                        .map(|m| MergeLotusJson { lane: m.lane, nonce: m.nonce })
+                                        .collect()
+                                ),

Repeat the same change for the V3 and V4 impls in the indicated ranges.

Also applies to: 381-389, 493-501

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is done for the lotus compatibility, Lotus returns null when the merges is empty.

Copy link
Contributor

Choose a reason for hiding this comment

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

@akaladarshi, thank you for the clarification! You're absolutely right - if Lotus returns null for empty merges, then the current implementation is correct for compatibility reasons.

However, this means the test snapshots in the code should be updated to expect null instead of [] for the Merges field to match the actual Lotus behavior:

"Merges": null

instead of:

"Merges": []

This would ensure the tests accurately reflect the expected Lotus-compatible output.

(\   /)

( .. )
o
(")(")
learning added


✏️ Learnings added
Learnt from: akaladarshi
PR: ChainSafe/forest#5996
File: src/lotus_json/actor_states/methods/paych_params.rs:269-277
Timestamp: 2025-09-02T08:46:04.916Z
Learning: In payment channel actor Lotus JSON serialization, empty merges should be serialized as `null` (not an empty array `[]`) to maintain compatibility with Lotus behavior.

Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.

@akaladarshi akaladarshi added the RPC requires calibnet RPC checks to run on CI label Aug 26, 2025
@LesnyRumcajs
Copy link
Member

no green checkmark, no review!

hanabi1224
hanabi1224 previously approved these changes Sep 2, 2025

pub(crate) fn register_actor_methods(registry: &mut MethodRegistry, cid: Cid, version: u64) {
match version {
8 => register_payment_channel_reg_versions!(registry, cid, fil_actor_paych_state::v8),
Copy link
Contributor

Choose a reason for hiding this comment

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

Better not to duplicate version literals here to avoid typos.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Actually we are planning to use enum tracking issue #5937, so we won't have typos.

Copy link
Contributor

Choose a reason for hiding this comment

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

By typo, I mean the pair of 8 and ::v8, I think it compiles if we write 8 => ::v9 by mistake. My suggested change is in #5996 (comment)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done

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 (2)
src/rpc/registry/methods_reg.rs (1)

266-279: Add PaymentChannel to the “supported actors” smoke-test list.

This keeps the invariant that all registered actors have at least their constructor method present, and guards future regressions.

Apply this diff:

         let supported_actors = vec![
             BuiltinActor::Account,
             BuiltinActor::Miner,
             BuiltinActor::EVM,
             BuiltinActor::Cron,
             BuiltinActor::DataCap,
+            BuiltinActor::PaymentChannel,
         ];
src/tool/subcommands/api_cmd/api_compare_tests.rs (1)

1909-1967: Avoid panics: replace unwrap() with error propagation.

Two to_vec(...).unwrap() calls can panic; use ? for consistent error handling like other actor tests.

Apply this diff:

-            to_vec(&constructor_params).unwrap(),
+            to_vec(&constructor_params)?,
...
-            to_vec(&update_channel_state).unwrap(),
+            to_vec(&update_channel_state)?,
📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

💡 Knowledge Base configuration:

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

You can enable these sources in your CodeRabbit configuration.

📥 Commits

Reviewing files that changed from the base of the PR and between 6f90a71 and a678163.

📒 Files selected for processing (5)
  • CHANGELOG.md (0 hunks)
  • src/rpc/registry/actors/mod.rs (1 hunks)
  • src/rpc/registry/methods_reg.rs (2 hunks)
  • src/tool/subcommands/api_cmd/api_compare_tests.rs (1 hunks)
  • src/tool/subcommands/api_cmd/test_snapshots.txt (1 hunks)
💤 Files with no reviewable changes (1)
  • CHANGELOG.md
🚧 Files skipped from review as they are similar to previous changes (2)
  • src/rpc/registry/actors/mod.rs
  • src/tool/subcommands/api_cmd/test_snapshots.txt
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: akaladarshi
PR: ChainSafe/forest#5996
File: src/lotus_json/actor_states/methods/paych_params.rs:269-277
Timestamp: 2025-09-02T08:46:04.916Z
Learning: In payment channel actor Lotus JSON serialization, empty merges should be serialized as `null` (not an empty array `[]`) to maintain compatibility with Lotus behavior.
🧬 Code graph analysis (2)
src/tool/subcommands/api_cmd/api_compare_tests.rs (2)
src/shim/address.rs (1)
  • new_id (142-144)
src/blocks/tipset.rs (2)
  • key (336-339)
  • key (530-533)
src/rpc/registry/methods_reg.rs (1)
src/rpc/registry/actors/payment_channel.rs (1)
  • register_actor_methods (31-44)
⏰ 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). (10)
  • GitHub Check: All lint checks
  • GitHub Check: Check
  • GitHub Check: tests-release
  • GitHub Check: tests
  • GitHub Check: Build Ubuntu
  • GitHub Check: cargo-publish-dry-run
  • GitHub Check: Build MacOS
  • GitHub Check: Build forest binaries on Linux AMD64
  • GitHub Check: Analyze (rust)
  • GitHub Check: Analyze (go)
🔇 Additional comments (4)
src/rpc/registry/methods_reg.rs (2)

77-79: Good addition: wire PaymentChannel into the registry.

Importing payment_channel alongside the other actors is correct and keeps the registration site cohesive.


101-103: Registration path for PaymentChannel looks correct.

Dispatching to payment_channel::register_actor_methods(self, cid, version) mirrors other actors and should cover v8–v16.

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

1904-1905: Nice: extend StateDecodeParams coverage with Paych.

Hooking Paych tests into the main suite is aligned with the PR goal.


1947-1952: Verified: the paych_params serialization maps empty merges to None → JSON null. The RpcTest::identity test won’t catch this at JSON level. Switching to RpcTest::validate_raw is appropriate to lock in the invariant.

akaladarshi and others added 2 commits September 2, 2025 20:42
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

♻️ Duplicate comments (1)
src/rpc/registry/actors/payment_channel.rs (1)

33-45: Optional: centralize supported versions to avoid drift

The literal list 8..16 is repeated across actor registries. Consider a shared SUPPORTED_PAYCH_VERSIONS (or macro alias) reused here to avoid future typos when bumping versions.

🧹 Nitpick comments (2)
src/tool/subcommands/api_cmd/api_compare_tests.rs (2)

1945-1953: Avoid unwrap in tests; propagate errors instead

Replace unwrap() with ? to keep failure context and avoid panics during test construction.

-            to_vec(&constructor_params).unwrap(),
+            to_vec(&constructor_params)?,
...
-            to_vec(&update_channel_state).unwrap(),
+            to_vec(&update_channel_state)?,

1910-1968: Optional: add one legacy-version case

Consider adding a single v8 UpdateChannelState case to detect version-boundary regressions early (since registry supports v8–v16).

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

💡 Knowledge Base configuration:

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

You can enable these sources in your CodeRabbit configuration.

📥 Commits

Reviewing files that changed from the base of the PR and between a678163 and afb45bf.

📒 Files selected for processing (4)
  • src/lotus_json/actor_states/methods/mod.rs (1 hunks)
  • src/rpc/registry/actors/payment_channel.rs (1 hunks)
  • src/tool/subcommands/api_cmd/api_compare_tests.rs (1 hunks)
  • src/tool/subcommands/api_cmd/test_snapshots.txt (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
  • src/lotus_json/actor_states/methods/mod.rs
  • src/tool/subcommands/api_cmd/test_snapshots.txt
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: akaladarshi
PR: ChainSafe/forest#5996
File: src/lotus_json/actor_states/methods/paych_params.rs:269-277
Timestamp: 2025-09-02T08:46:04.916Z
Learning: In payment channel actor Lotus JSON serialization, empty merges should be serialized as `null` (not an empty array `[]`) to maintain compatibility with Lotus behavior.
🧬 Code graph analysis (1)
src/tool/subcommands/api_cmd/api_compare_tests.rs (2)
src/shim/address.rs (2)
  • new_id (142-144)
  • default (122-124)
src/blocks/tipset.rs (2)
  • key (336-339)
  • key (530-533)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (9)
  • GitHub Check: Build MacOS
  • GitHub Check: cargo-publish-dry-run
  • GitHub Check: Build Ubuntu
  • GitHub Check: tests
  • GitHub Check: tests-release
  • GitHub Check: Build forest binaries on Linux AMD64
  • GitHub Check: All lint checks
  • GitHub Check: Analyze (rust)
  • GitHub Check: Analyze (go)
🔇 Additional comments (3)
src/rpc/registry/actors/payment_channel.rs (1)

8-31: Macro-based per-version registration looks good

Using paste to bind Method/params per version keeps the mapping tight and prevents 8=>v9 style mismatches. LGTM.

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

1905-1906: Good call adding Paych decode-param tests to the suite

This wires the new actor into the existing StateDecodeParams tests. LGTM.


1915-1939: Verify empty merges serialize to null (Lotus parity)

Per our learning for Paych Lotus JSON, empty merges must serialize as null, not []. Your test uses merges: vec![]; please confirm paych_params.rs implements this mapping (it should around lines 269–277).

@akaladarshi akaladarshi requested a review from elmattic September 2, 2025 16:24
@akaladarshi akaladarshi added this pull request to the merge queue Sep 2, 2025
Merged via the queue into main with commit 8025e39 Sep 2, 2025
54 checks passed
@akaladarshi akaladarshi deleted the akaladarshi/add-paych-account-state-decode-params branch September 2, 2025 17:13
@coderabbitai coderabbitai bot mentioned this pull request Oct 22, 2025
4 tasks
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.

Add support for the PaymentChannel Actor state in Filecoin.StateDecodeParams API

5 participants