feat: add support for verified reg actor in state decode params #5896
Conversation
WalkthroughAdds Verified Registry (VerifReg) actor support to StateDecodeParams across versions v8–v16: new RPC registry module and registrations, comprehensive Lotus JSON mappings for VerifReg method parameters, versioned signature/type Lotus JSON bridges, expanded API tests and snapshots, an optional fvm_actor_utils dependency, and minor unused-import removals. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant MethodRegistry
participant VerifiedRegModule as VerifiedReg (v8–v16)
participant LotusJson as LotusJson Converters
Client->>MethodRegistry: StateDecodeParams(actorCid, methodNum, params, tipset)
MethodRegistry->>VerifiedRegModule: register_actor_methods(actorCid, version)
MethodRegistry->>LotusJson: select param LotusJson type for (version, methodNum)
LotusJson-->>MethodRegistry: deserialize params → versioned struct
MethodRegistry-->>Client: return decoded params (Lotus JSON)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Assessment against linked issues
Possibly related PRs
Suggested reviewers
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ Finishing Touches
🧪 Generate unit tests
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
04acaa2 to
dfd085f
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
src/shim/crypto.rs (2)
128-141: Same unsafe signature conversion issueThis implementation has the same issue with converting
SignatureType::DelegatedtoBLS. See the critical issue raised above.
143-156: Verify fvm_shared4 signature type supportThe comment claims v4 doesn't have Delegated signature type, but this should be verified. If true, the same unsafe conversion issue applies.
#!/bin/bash # Description: Verify if fvm_shared4 supports Delegated signature type # Check fvm_shared4 SignatureType definition fd -e rs . | xargs rg "enum SignatureType" -A 10 | grep -A 10 "fvm_shared4" # Check if there's any Delegated variant in fvm_shared4 rg "Delegated" --type rust | grep "fvm_shared4"
🧹 Nitpick comments (2)
src/rpc/registry/actors/verified_reg.rs (1)
229-251: Efficient consolidation for v12+ versions.The
register_verified_reg_v12_plusmacro effectively handles versions 12-16 which share the same method set. The main dispatch function correctly routes to version-specific implementations.Consider adding a debug log or comment for the empty default case to document that unknown versions are intentionally ignored.
- _ => {} + _ => { + // Unknown versions are ignored - only known actor versions are supported + }src/lotus_json/actor_states/methods/verified_reg_actor.rs (1)
1019-1035: Document version-specific implementationsConsider adding comments to explain:
- Why v8 and v9 are excluded from
impl_verifier_params(line 1020)- The mapping between actor versions and fvm_shared versions (lines 1027-1029)
- Why different claim allocation param structures are used for v9-v11 vs v12+ (lines 1031-1033)
This will help future maintainers understand the version-specific logic.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (15)
Cargo.toml(1 hunks)src/lotus_json/actor_states/methods/mod.rs(2 hunks)src/lotus_json/actor_states/methods/verified_reg_actor.rs(1 hunks)src/rpc/registry/actors/account.rs(0 hunks)src/rpc/registry/actors/evm.rs(0 hunks)src/rpc/registry/actors/init.rs(0 hunks)src/rpc/registry/actors/miner.rs(0 hunks)src/rpc/registry/actors/mod.rs(1 hunks)src/rpc/registry/actors/power.rs(0 hunks)src/rpc/registry/actors/reward.rs(0 hunks)src/rpc/registry/actors/verified_reg.rs(1 hunks)src/rpc/registry/methods_reg.rs(3 hunks)src/shim/crypto.rs(1 hunks)src/tool/subcommands/api_cmd/api_compare_tests.rs(2 hunks)src/tool/subcommands/api_cmd/test_snapshots.txt(1 hunks)
💤 Files with no reviewable changes (6)
- src/rpc/registry/actors/init.rs
- src/rpc/registry/actors/account.rs
- src/rpc/registry/actors/evm.rs
- src/rpc/registry/actors/reward.rs
- src/rpc/registry/actors/power.rs
- src/rpc/registry/actors/miner.rs
⏰ 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: tests-release
- GitHub Check: Build forest binaries on Linux AMD64
- GitHub Check: tests
- GitHub Check: All lint checks
- GitHub Check: cargo-publish-dry-run
- GitHub Check: Build Ubuntu
- GitHub Check: Build MacOS
- GitHub Check: Analyze (rust)
- GitHub Check: Analyze (go)
🔇 Additional comments (19)
Cargo.toml (1)
223-223: fvm_actor_utils dependency usage verifiedThe
fvm_actor_utilscrate is referenced in multiple places in production code, so it must remain a required dependency:• src/tool/subcommands/api_cmd/api_compare_tests.rs
• src/rpc/registry/actors/verified_reg.rs
• src/lotus_json/actor_states/methods/verified_reg_actor.rsSince none of these usages are gated behind a Cargo feature, leaving
fvm_actor_utils = "14.0.0"as a non-optional dependency is correct. Only if you intend to make VerifiedRegistry support truly optional should you addoptional = trueand expose it via a new feature.src/lotus_json/actor_states/methods/mod.rs (2)
3-3: Good formatting improvement.Adding the newline after the SPDX license identifier improves readability and follows standard formatting conventions.
15-15: LGTM: Module declaration follows existing patterns.The
verified_reg_actormodule declaration is consistent with the existing module structure and correctly usespubvisibility to match other actor modules.src/rpc/registry/actors/mod.rs (1)
11-11: LGTM: Module declaration is consistent.The
verified_regmodule declaration follows the established pattern and uses appropriatepub(crate)visibility for the RPC registry actors module.src/tool/subcommands/api_cmd/test_snapshots.txt (1)
146-157: Excellent test coverage for VerifiedRegistry actor.The addition of 12 new snapshot files demonstrates comprehensive test coverage for the VerifiedRegistry actor's
StateDecodeParamsfunctionality. The files follow the established naming convention and are properly sequenced.src/rpc/registry/methods_reg.rs (3)
76-78: LGTM: Clean import organization.The addition of
verified_regto the imports is well-organized and follows the existing alphabetical ordering pattern.
91-93: LGTM: Consistent actor registration pattern.The VerifiedRegistry actor registration follows the established pattern used by other builtin actors and correctly delegates to the version-specific registration function.
135-135: Good error type consistency improvement.Changing the closure return type from
Result<$param_type>toanyhow::Result<$param_type>improves consistency with the rest of the codebase's error handling approach.src/tool/subcommands/api_cmd/api_compare_tests.rs (2)
1899-1988: LGTM! Comprehensive parameter declarations for Verified Registry actor.The parameter structs cover all major Verified Registry operations and use the correct types from
fil_actor_verifreg_state::v16. The test data includes appropriate dummy values for signatures and addresses.
2101-2196: Well-structured test coverage for all Verified Registry methods.The tests correctly use the Method enum for method numbers and appropriately reuse parameter instances for exported method variants. All major Verified Registry operations are covered, including the UniversalReceiverHook.
src/rpc/registry/actors/verified_reg.rs (4)
11-78: Excellent version-aware handling of core methods.The macro correctly handles the evolution of parameter types across versions:
- v8: Uses generic
VerifierParams- v9-v10: Introduces specialized parameter types
- v11+: Full set of typed parameters including
ConstructorParamsThis approach maintains backward compatibility while supporting newer versions.
81-158: Good handling of method evolution across versions.The macros correctly capture the evolution of allocation-related methods:
- v8 has unique UseBytes/RestoreBytes methods
- v9 introduced allocation/claims methods including ClaimAllocations
- v10-v11 temporarily removed ClaimAllocations (no_claim variant)
- v12+ restored ClaimAllocations
This accurately reflects the actor's method evolution.
160-202: Clean implementation of exported methods and universal receiver.The macros effectively handle:
- Registration of exported method variants for public API
- Dynamic version-specific imports using the
paste!macro- Proper use of
fvm_actor_utils::receiver::UniversalReceiverParamsfor cross-actor communication
204-227: Correct version-specific method registration.Each version function accurately registers the appropriate method set:
- v8: Basic verifier management
- v9: Adds allocation/claims functionality
- v10-v11: Adds exported methods API while temporarily removing ClaimAllocations
The registration order and method selection align with the actor's historical evolution.
src/shim/crypto.rs (3)
71-83: LGTM!The conversion from
fvm_shared3::crypto::signature::Signatureto localSignatureis correctly implemented.
85-97: LGTM!The conversion from
fvm_shared4::crypto::signature::Signatureto localSignatureis correctly implemented.
114-126: LGTM!The conversion from
fvm_shared2::crypto::signature::Signatureto localSignatureis correctly implemented.src/lotus_json/actor_states/methods/verified_reg_actor.rs (2)
181-181: PaddedPieceSize usage is consistentAfter inspection, all Lotus JSON structs in
src/lotus_json/(e.g., in bothverified_reg_actor.rsandallocation.rs) declare theirsizefields using thePaddedPieceSizenewtype. There is no occurrence of a rawu64for these fields, so the originally flagged inconsistency does not exist. No changes are needed here.Likely an incorrect or invalid review comment.
551-628: Verify signature conversions inimpl_remove_data_cap_paramsmacroThe
.into()calls in the macro (lines 601, 605, 616, 620) rely on theFromimplementations in thefvm_shared2::crypto,fvm_shared3::crypto, andfvm_shared4::cryptocrates. You should confirm whether these conversions preserve theDelegatedsignature variant (if supported) or erroneously coerce everything to BLS, which would break signature verification for actor versions 8–11.Paths to inspect:
src/lotus_json/actor_states/methods/verified_reg_actor.rsaround theimpl_remove_data_cap_paramsmacro (lines 551–628)- Signature
Fromimpls in thefvm_shared2,fvm_shared3, andfvm_shared4crypto modulesIf
Delegatedsignatures must be supported, update the conversion logic to handle them explicitly; otherwise, verify that only BLS signatures are used here.
Cargo.toml
Outdated
| tikv-jemallocator = { version = "0.6", optional = true } | ||
| tracing-chrome = { version = "0.7", optional = true } | ||
| tracing-loki = { version = "0.2", default-features = false, features = ["compat-0-2-1", "rustls"], optional = true } | ||
| use = "0.0.1-pre.0" |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Clarify the purpose and naming of the use dependency.
The dependency name use is unusual and may cause confusion since use is a Rust keyword. Please clarify:
- What functionality this dependency provides
- Whether this is the correct/intended package name
- Consider if a more descriptive alias should be used
🏁 Script executed:
#!/bin/bash
# Description: Verify the use dependency exists and check its purpose
# Expected: Find usage in codebase and verify package exists
rg -A 3 "extern crate use|use use::" --type rust
curl -s https://crates.io/api/v1/crates/use | jq '.crate.description'Length of output: 142
Clarify and alias the use dependency in Cargo.toml
It looks like you’re pulling in the use crate (description: “plugin based design”) but its name collides with Rust’s use keyword and isn’t self-descriptive. Please:
- Summarize its purpose (e.g. “plugin-based extension framework”) in a comment above the dependency.
- Alias it to something clearer (for example
plugin_frameworkorplugins) by renaming the dependency:
# plugin-based extension framework
[dependencies]
plugin_framework = { package = "use", version = "0.0.1-pre.0" }This makes its intent clear and avoids confusion with the use statement.
🤖 Prompt for AI Agents
In Cargo.toml at line 228, the dependency named "use" conflicts with Rust's
keyword and is unclear. Add a comment above this dependency summarizing its
purpose as a "plugin-based extension framework" and rename the dependency by
aliasing it to a clearer name like "plugin_framework" using the syntax:
plugin_framework = { package = "use", version = "0.0.1-pre.0" }. This clarifies
intent and avoids confusion.
| filecoin_statedecodeparams_1754401651142334.rpcsnap.json.zst | ||
| filecoin_statedecodeparams_1754401651146485.rpcsnap.json.zst | ||
| filecoin_statedecodeparams_1754401651146560.rpcsnap.json.zst | ||
| filecoin_statedecodeparams_1754401651146644.rpcsnap.json.zst | ||
| filecoin_statedecodeparams_1754401651146719.rpcsnap.json.zst | ||
| filecoin_statedecodeparams_1754401651146788.rpcsnap.json.zst | ||
| filecoin_statedecodeparams_1754401651146860.rpcsnap.json.zst | ||
| filecoin_statedecodeparams_1754401651146948.rpcsnap.json.zst | ||
| filecoin_statedecodeparams_1754401651147022.rpcsnap.json.zst | ||
| filecoin_statedecodeparams_1754401651147091.rpcsnap.json.zst | ||
| filecoin_statedecodeparams_1754401651147157.rpcsnap.json.zst | ||
| filecoin_statedecodeparams_1754401651147231.rpcsnap.json.zst |
There was a problem hiding this comment.
let's embed verified registry in the name?
There was a problem hiding this comment.
does filecoin_verifie_reg_statedecodeparams_1754230255606413.rpcsnap.json.zst works ?
There was a problem hiding this comment.
@akaladarshi lets also add method name here, so we know which method is getting tested
filecoin_verifie_reg_constructor_statedecodeparams_1754230255606413.rpcsnap.json.zst
There was a problem hiding this comment.
filecoin_verified_reg_statedecodeparams_1754401651142334.rpcsnap.json.zst
filecoin_verified_reg_statedecodeparams_1754401651146144.rpcsnap.json.zst
filecoin_verified_reg_statedecodeparams_1754401651146246.rpcsnap.json.zst
filecoin_verified_reg_statedecodeparams_1754401651146327.rpcsnap.json.zst
@akaladarshi with the addition of actor name we now know that these are used for testing verifreg state decode params but it would be better to know which method of verifreg actor it's testing like Constructor, AddVerifier, GetClaimsExported etc
There was a problem hiding this comment.
I don't think we really need that, #5904 (comment), it is being done separately.
We just need to include the name of actor in the file and add the details in the PR description about which snap file is tested with method name as mentioned in the comment.
You can check the description of this PR.
There was a problem hiding this comment.
I feel it's helpful but i'll let @elmattic suggest here
| 14 => register_verified_reg_v12_plus!(registry, cid, fil_actor_verifreg_state::v14, v14), | ||
| 15 => register_verified_reg_v12_plus!(registry, cid, fil_actor_verifreg_state::v15, v15), | ||
| 16 => register_verified_reg_v12_plus!(registry, cid, fil_actor_verifreg_state::v16, v16), | ||
| _ => {} |
There was a problem hiding this comment.
it'd be better to actually report some error in this case
There was a problem hiding this comment.
Actually we are not returning anything here, this is just populating the registry with the
codecid -> methodsParams type for a specific version, so if the version that we are not supporting will just not register and when the StateDecodeParams api actually called with unregistered version(actor code cid), we are returning an error if the code_cid is not there.
There was a problem hiding this comment.
Hm, okay. I'd love if we wouldn't use integers here and matching on them. But it doesn't have to be in this PR.
Imagine a scenario that will come in a month or so; a new actors version is released: 17. A nice thing to have is let the compiler tell you where you need to update your code with the new version. A good thing about matching on an enum is that whenever a new variant is introduced, the entire build fails for places where it's matched non-exhaustively.
The alternative is finding the issues in runtime, which is not ideal and brings us closer to Python land (a bad place).
There was a problem hiding this comment.
Got it, I’ll keep this in mind.
I’m planning to open a PR once we’ve implemented all the actor methods. In that PR, we can review the overall design and suggest changes like renaming files or doing cleanups. I’ll introduce this change in that PR as well.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (4)
Cargo.toml(1 hunks)src/lotus_json/actor_states/methods/verified_reg_actor.rs(1 hunks)src/shim/crypto.rs(2 hunks)src/tool/subcommands/api_cmd/test_snapshots.txt(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- Cargo.toml
- src/shim/crypto.rs
- src/tool/subcommands/api_cmd/test_snapshots.txt
⏰ 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: tests
- GitHub Check: tests-release
- GitHub Check: cargo-publish-dry-run
- GitHub Check: Build MacOS
- GitHub Check: Build Ubuntu
- GitHub Check: Build forest binaries on Linux AMD64
- GitHub Check: All lint checks
- GitHub Check: Analyze (rust)
- GitHub Check: Analyze (go)
🔇 Additional comments (2)
src/lotus_json/actor_states/methods/verified_reg_actor.rs (2)
1-14: Well-structured implementation with comprehensive version supportThe file successfully implements Lotus JSON serialization/deserialization for VerifReg actor parameters across multiple versions. The use of macros effectively reduces code duplication while handling version-specific differences.
1019-1025: Confirm version coverage for VerifReg actor methods
It looks like you’ve intentionally omitted versions 8–10 from:
impl_constructor_params(starts at v11)impl_remove_verifier_params(starts at v11)- and omitted v8 from
impl_remove_expired_allocations_params.Because these type definitions live in external crates (not in this repo), I can’t automatically verify which actor versions actually introduced those methods.
Please double-check and document in comments or the PR description that:
- “Constructor” truly only exists from v11 onward
- “RemoveVerifier” only exists from v11 onward
- “RemoveExpiredAllocations” only exists from v9 onward
This will ensure future readers know these gaps are intentional.
a4d9d56 to
30f5dbc
Compare
…4 and fvm_shared3
…nd small refactor
30f5dbc to
b40cd3f
Compare
…iedreg-actor-decode-params
…iedreg-actor-decode-params
There was a problem hiding this comment.
Actionable comments posted: 7
♻️ Duplicate comments (1)
src/rpc/registry/actors/verified_reg.rs (1)
238-251: Prefer a version enum over raw integers for exhaustivenessMatching on raw
u64is fragile. An enum representing actor versions would force exhaustive matches at compile time when v17+ arrive, preventing silent runtime fallthrough.
🧹 Nitpick comments (6)
src/rpc/registry/actors/verified_reg.rs (2)
10-78: Reduce duplication across version arms in core method registrationThe v8/v9/v10 arms largely mirror the generic path used for v11+, with only a few type deltas. Consider consolidating these via a single macro with small overrides (e.g., by passing a flag or a tiny per-version mapping), to reduce maintenance when adding v17+ later.
186-202: Optional dependency fvm_actor_utils used unconditionally
register_universal_receiver_hook!usesfvm_actor_utils::receiver::UniversalReceiverParams. Iffvm_actor_utilsis markedoptionalin Cargo.toml (as indicated in the PR summary), this unconditional use could break builds when the feature isn’t enabled.Either:
- gate the use with a feature, or
- make the dependency non-optional or part of default features.
I can propose a feature-gated variant if desired.
Would you like me to generate a small Cargo.toml feature snippet and cfg-gates here?
src/lotus_json/signature_type.rs (1)
52-69: Minor simplification: avoid untagged single-variant enumsSince these are always integers, an enum with a single
Integervariant and#[serde(untagged)]is unnecessary. A transparent newtype is simpler and more explicit.Example for v2 (apply similarly to v3/v4):
-#[derive(Deserialize, Serialize, JsonSchema)] -#[serde(untagged)] -pub enum SignatureTypeV2LotusJson { - Integer(#[schemars(with = "u8")] fvm_shared2::crypto::signature::SignatureType), -} +#[derive(Deserialize, Serialize, JsonSchema)] +#[serde(transparent)] +pub struct SignatureTypeV2LotusJson( + #[schemars(with = "u8")] fvm_shared2::crypto::signature::SignatureType +);src/tool/subcommands/api_cmd/api_compare_tests.rs (1)
2287-2399: Duplicate LockBalance test — remove one occurrence
LockBalanceis added twice with identical params (Lines 2381–2385 and 2386–2391). It’ll be deduped later, but better to drop the duplicate for clarity.Apply this diff:
RpcTest::identity(StateDecodeParams::request(( Address::new_id(18101), // https://calibration.filscan.io/en/address/t018101/, fil_actor_multisig_state::v16::Method::LockBalance as u64, to_vec(&multisig_lock_bal_params)?, tipset.key().into(), ))?), - 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(), - ))?),src/lotus_json/actor_states/methods/verified_reg_actor.rs (2)
41-52: Schema derivations are inconsistent across v2/v3/v4 RemoveDataCap params.Minor, but for API docs consistency, consider deriving JsonSchema on RemoveDataCapParamsV2LotusJson and V3LotusJson (as done on requests) so all public LotusJson types have schema metadata.
-#[derive(Serialize, Deserialize, Debug, Clone, PartialEq)] +#[derive(Serialize, Deserialize, JsonSchema, Debug, Clone, PartialEq)] pub struct RemoveDataCapParamsV2LotusJson {-#[derive(Serialize, Deserialize, Debug, Clone, PartialEq)] +#[derive(Serialize, Deserialize, JsonSchema, Debug, Clone, PartialEq)] pub struct RemoveDataCapParamsV3LotusJson {
111-120: Validate lotus_json adapters on primitive Vec usage.You're using
#[schemars(with = "LotusJson<Vec<u64>>")]and#[serde(with = "crate::lotus_json")]on Vec fields. If crate::lotus_json doesn't special-case Vec, this indirection is unnecessary and could cause trait bound friction. If it's intentional and supported, ignore this. Otherwise, consider removing the lotus_json adapter and letting serde handle primitives directly.Would you like me to grep the codebase for existing patterns and adjust accordingly?
Also applies to: 173-182, 202-211
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (19)
Cargo.toml(1 hunks)src/lotus_json/actor_states/methods/mod.rs(2 hunks)src/lotus_json/actor_states/methods/verified_reg_actor.rs(1 hunks)src/lotus_json/signature.rs(2 hunks)src/lotus_json/signature_type.rs(1 hunks)src/rpc/registry/actors/account.rs(0 hunks)src/rpc/registry/actors/cron.rs(0 hunks)src/rpc/registry/actors/datacap.rs(0 hunks)src/rpc/registry/actors/evm.rs(0 hunks)src/rpc/registry/actors/init.rs(0 hunks)src/rpc/registry/actors/miner.rs(0 hunks)src/rpc/registry/actors/mod.rs(1 hunks)src/rpc/registry/actors/multisig.rs(0 hunks)src/rpc/registry/actors/power.rs(0 hunks)src/rpc/registry/actors/reward.rs(0 hunks)src/rpc/registry/actors/verified_reg.rs(1 hunks)src/rpc/registry/methods_reg.rs(3 hunks)src/tool/subcommands/api_cmd/api_compare_tests.rs(7 hunks)src/tool/subcommands/api_cmd/test_snapshots.txt(1 hunks)
💤 Files with no reviewable changes (9)
- src/rpc/registry/actors/cron.rs
- src/rpc/registry/actors/datacap.rs
- src/rpc/registry/actors/evm.rs
- src/rpc/registry/actors/power.rs
- src/rpc/registry/actors/multisig.rs
- src/rpc/registry/actors/init.rs
- src/rpc/registry/actors/account.rs
- src/rpc/registry/actors/reward.rs
- src/rpc/registry/actors/miner.rs
🚧 Files skipped from review as they are similar to previous changes (4)
- Cargo.toml
- src/rpc/registry/methods_reg.rs
- src/lotus_json/actor_states/methods/mod.rs
- src/tool/subcommands/api_cmd/test_snapshots.txt
🧰 Additional context used
🧬 Code Graph Analysis (5)
src/rpc/registry/actors/verified_reg.rs (3)
src/rpc/registry/actors/init.rs (1)
register_actor_methods(41-54)src/rpc/registry/actors/power.rs (1)
register_actor_methods(116-129)src/rpc/registry/actors/reward.rs (1)
register_actor_methods(49-73)
src/lotus_json/signature.rs (5)
src/lotus_json/signature_type.rs (12)
snapshots(29-31)snapshots(74-82)snapshots(99-107)snapshots(124-132)into_lotus_json(33-35)into_lotus_json(84-86)into_lotus_json(109-111)into_lotus_json(134-136)from_lotus_json(37-41)from_lotus_json(88-92)from_lotus_json(113-117)from_lotus_json(138-142)src/lotus_json/mod.rs (17)
snapshots(147-147)snapshots(539-541)snapshots(553-555)snapshots(570-572)snapshots(594-596)serde_json(273-273)serde_json(297-297)into_lotus_json(148-148)into_lotus_json(542-544)into_lotus_json(556-558)into_lotus_json(573-579)into_lotus_json(597-604)from_lotus_json(149-149)from_lotus_json(545-547)from_lotus_json(559-564)from_lotus_json(580-586)from_lotus_json(605-612)src/lotus_json/key_info.rs (3)
snapshots(23-34)into_lotus_json(36-42)from_lotus_json(44-50)src/lotus_json/signed_message.rs (3)
snapshots(41-69)into_lotus_json(71-79)from_lotus_json(81-88)src/lotus_json/vec_u8.rs (2)
into_lotus_json(38-43)from_lotus_json(45-50)
src/lotus_json/signature_type.rs (2)
src/lotus_json/signature.rs (12)
snapshots(59-67)snapshots(90-98)snapshots(119-127)snapshots(148-156)into_lotus_json(69-75)into_lotus_json(100-105)into_lotus_json(129-134)into_lotus_json(158-163)from_lotus_json(77-83)from_lotus_json(107-112)from_lotus_json(136-141)from_lotus_json(165-170)src/lotus_json/mod.rs (15)
snapshots(147-147)snapshots(539-541)snapshots(553-555)snapshots(570-572)snapshots(594-596)into_lotus_json(148-148)into_lotus_json(542-544)into_lotus_json(556-558)into_lotus_json(573-579)into_lotus_json(597-604)from_lotus_json(149-149)from_lotus_json(545-547)from_lotus_json(559-564)from_lotus_json(580-586)from_lotus_json(605-612)
src/tool/subcommands/api_cmd/api_compare_tests.rs (3)
src/rpc/reflect/mod.rs (1)
request(250-260)src/rpc/client.rs (2)
request(272-285)client(114-114)src/blocks/tipset.rs (7)
key(335-338)key(529-532)from(104-109)from(159-161)from(165-167)from(171-176)from(180-185)
src/lotus_json/actor_states/methods/verified_reg_actor.rs (5)
src/shim/address.rs (2)
with(72-77)new_id(142-144)src/lotus_json/signature_type.rs (8)
snapshots(29-31)snapshots(74-82)snapshots(99-107)snapshots(124-132)into_lotus_json(33-35)into_lotus_json(84-86)into_lotus_json(109-111)into_lotus_json(134-136)src/lotus_json/signature.rs (8)
snapshots(59-67)snapshots(90-98)snapshots(119-127)snapshots(148-156)into_lotus_json(69-75)into_lotus_json(100-105)into_lotus_json(129-134)into_lotus_json(158-163)src/rpc/client.rs (1)
client(114-114)src/shim/crypto.rs (4)
new(72-74)bytes(186-188)try_from(205-217)try_from(325-332)
⏰ 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: tests-release
- GitHub Check: tests
- GitHub Check: Build MacOS
- GitHub Check: Build Ubuntu
- GitHub Check: cargo-publish-dry-run
- GitHub Check: All lint checks
- GitHub Check: Analyze (go)
- GitHub Check: Analyze (rust)
🔇 Additional comments (16)
src/rpc/registry/actors/mod.rs (1)
14-14: Module wiring LGTMClean addition of the verified_reg actor module alongside existing ones. This keeps registry structure consistent.
src/rpc/registry/actors/verified_reg.rs (1)
215-227: Method coverage across versions looks correctv10/v11 registering allocation methods without ClaimAllocations (no_claim) and exported variants, plus URH, matches actors’ historical method sets.
src/lotus_json/signature_type.rs (2)
52-69: Versioned SignatureType LotusJson bridges are soundGood separation for v2/v3/v4 with schemars using u8 and integer-only JSON, matching Lotus expectations for SigType. This aligns with the new versioned Signature wrappers elsewhere.
70-143: Impls and snapshots look correctRound-trip mappings for Secp256k1 and BLS across versions are covered. No concerns.
src/lotus_json/signature.rs (2)
19-54: Versioned Signature LotusJson structs are correctPer-version typing of the
Typefield via corresponding SignatureType bridges is clean and maintains schema fidelity. Good reuse of the existing Vec LotusJson for Data.
86-171: HasLotusJson impls for fvm_shared{2,3,4}::Signature are consistentThe into/from conversions and test snapshots are straightforward and mirror the generic Signature path. No issues spotted.
src/tool/subcommands/api_cmd/api_compare_tests.rs (7)
1815-1850: Good: keep Cron Constructor test gatedLeaving the Cron Constructor test commented until upstream go-state-types support lands is practical and documented. The EVM and System decode-param tests look fine.
1864-1893: Miner decode-param tests LGTMConstructor and ChangeWorkerAddress cover common paths with realistic params.
1895-1919: Account decode-param tests LGTMConstructor and AuthenticateMessage (frc42 hash) params provide solid coverage.
1921-1957: Init decode-param tests LGTMCovers Constructor, Exec, and Exec4 with minimal raw bytes payloads, which is adequate for schema compatibility.
1959-2003: Reward decode-param tests LGTMAwardBlockReward, UpdateNetworkKPI, and ThisEpochReward cases are well represented. Using Option mirrors Lotus JSONs.
2005-2117: Power decode-param tests LGTM; method spread is sensibleGood balance across constructor, power mutation, cron, and exported views. The two Lotus-blocked cases are correctly left commented with tracking.
2401-2591: Verified Registry decode-param tests: great coverageConstructor, verifier/client ops, allocations/claims, exported variants, and UniversalReceiverHook are all covered. Mixed signature types in RemoveDataCap params meaningfully exercise the new versioned Signature{Type} LotusJson bridges.
src/lotus_json/actor_states/methods/verified_reg_actor.rs (3)
597-680: Nice partitioning of RemoveDataCap across signature versions.The versioned LotusJson request/param types cleanly avoid cross-version signature conversions and the associated panics. Snapshots align with per-version field names after the V2 "Signature" fix.
Also applies to: 682-764, 766-848
850-951: ClaimAllocations mappings look correct across v9–v16 (post expiry fix).The split into v11 flat and v12+ nested shapes is accurate, and the PaddedPieceSize handling in conversions is sound.
Also applies to: 952-1032
501-532: UniversalReceiverParams wiring LGTM (post key rename).Using fvm_actor_utils for UniversalReceiverParams with RawBytes via lotus_json is appropriate. After renaming "Type", the snapshots should pass.
|
no green checkmark, no review! |
|
|
||
| # optional dependencies | ||
| console-subscriber = { version = "0.4", features = ["parking_lot"], optional = true } | ||
| fvm_actor_utils = "14.0.0" |
There was a problem hiding this comment.
Can you please summarise why it is needed?
There was a problem hiding this comment.
There is one method UniversalReceiverHook that uses fvm_actor_utils::receiver::UniversalReceiverParams type as param
so we have to implement the HasLotusJson trait for it :
There was a problem hiding this comment.
It probably should be a re-export in fil-actor-states, then. Not a blocker, obviously.
| filecoin_verified_reg_statedecodeparams_1754401651142334.rpcsnap.json.zst | ||
| filecoin_verified_reg_statedecodeparams_1754492006350827.rpcsnap.json.zst | ||
| filecoin_verified_reg_statedecodeparams_1754401651146246.rpcsnap.json.zst | ||
| filecoin_verified_reg_statedecodeparams_1754401651146327.rpcsnap.json.zst | ||
| filecoin_verified_reg_statedecodeparams_1754401651146402.rpcsnap.json.zst | ||
| filecoin_verified_reg_statedecodeparams_1754401651146485.rpcsnap.json.zst | ||
| filecoin_verified_reg_statedecodeparams_1754401651146560.rpcsnap.json.zst | ||
| filecoin_verified_reg_statedecodeparams_1754401651146644.rpcsnap.json.zst | ||
| filecoin_verified_reg_statedecodeparams_1754401651146719.rpcsnap.json.zst | ||
| filecoin_verified_reg_statedecodeparams_1754401651146788.rpcsnap.json.zst | ||
| filecoin_verified_reg_statedecodeparams_1754401651146860.rpcsnap.json.zst | ||
| filecoin_verified_reg_statedecodeparams_1754401651146948.rpcsnap.json.zst | ||
| filecoin_verified_reg_statedecodeparams_1754401651147022.rpcsnap.json.zst | ||
| filecoin_verified_reg_statedecodeparams_1754401651147091.rpcsnap.json.zst | ||
| filecoin_verified_reg_statedecodeparams_1754401651147157.rpcsnap.json.zst | ||
| filecoin_verified_reg_statedecodeparams_1754401651147231.rpcsnap.json.zst |
There was a problem hiding this comment.
not a blocker, but for future it might make sense to add method names here. This, or we should prioritize #5894 for structured input.
There was a problem hiding this comment.
I have mostly prioritized the #5894, hence update the PR description
| Ok(tests) | ||
| } | ||
|
|
||
| fn multisig_actor_state_decode_params_tests(tipset: &Tipset) -> anyhow::Result<Vec<RpcTest>> { |
There was a problem hiding this comment.
it'd be nice to refactor those to separate files - this file is getting quite large. Not a blocker here, can be a part of future refactor.
There was a problem hiding this comment.
Add this in the checklist of the cleanup issue: #5937
|
LGTM too! |
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/tool/subcommands/api_cmd/api_compare_tests.rs (1)
2278-2278: Acknowledging file size: refactor to separate files is trackedPrior feedback about splitting into separate files is already tracked in #5937. Not blocking here.
🧹 Nitpick comments (6)
src/tool/subcommands/api_cmd/api_compare_tests.rs (6)
1816-1819: Prefer enum method constants over raw numbers for EVM constructorUse the typed method enum to reduce drift and improve readability.
Apply this diff:
- 1, + fil_actor_evm_state::v16::Method::Constructor as u64,
1871-1883: Use miner Method enum instead of raw method numbers (1, 3)This avoids silent breakage if method numbers change between versions and improves clarity.
Apply this diff:
RpcTest::identity(StateDecodeParams::request(( MINER_ADDRESS, - 1, + fil_actor_miner_state::v16::Method::Constructor as u64, to_vec(&miner_constructor_params)?, tipset.key().into(), ))?), RpcTest::identity(StateDecodeParams::request(( MINER_ADDRESS, - 3, + fil_actor_miner_state::v16::Method::ChangeWorkerAddress as u64, to_vec(&miner_change_worker_params)?, tipset.key().into(), ))?),
1904-1906: Compute FRC-42 method number at build-time instead of hardcodingUse the macro to avoid drift and improve readability.
Apply this diff:
- 2643134072, // frc42_dispatch::method_hash!("AuthenticateMessage"), + frc42_dispatch::method_hash!("AuthenticateMessage"),
1918-1926: Consider using a minimal valid CID instead of Cid::default()While decode-only paths often tolerate placeholder CIDs, using an explicit minimal valid CID reduces the chance of cross-implementation quirks during Lotus JSON bridging.
Apply this diff:
- code_cid: Cid::default(), + code_cid: Cid::from_str("bafkqaaa").unwrap(), // minimal valid identity CID @@ - code_cid: Cid::default(), + code_cid: Cid::from_str("bafkqaaa").unwrap(), // minimal valid identity CIDAlso applies to: 1924-1926
2278-2390: Duplicate multisig LockBalance testThe LockBalance decode test is included twice, which adds noise (it’ll be deduped later anyway). Remove one.
Apply this diff to drop the duplicate block:
- 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(), - ))?),
1931-1946: Use typed Init method enums instead of raw 1/2/3This mirrors the approach used elsewhere and reduces ambiguity.
Apply this diff:
- 1, + fil_actor_init_state::v16::Method::Constructor as u64, @@ - 2, + fil_actor_init_state::v16::Method::Exec as u64, @@ - 3, + fil_actor_init_state::v16::Method::Exec4 as u64,
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (1)
src/tool/subcommands/api_cmd/api_compare_tests.rs(7 hunks)
⏰ 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: tests
- GitHub Check: tests-release
- GitHub Check: All lint checks
- GitHub Check: Build Ubuntu
- GitHub Check: Build MacOS
- GitHub Check: cargo-publish-dry-run
- GitHub Check: Analyze (rust)
- GitHub Check: Analyze (go)
🔇 Additional comments (12)
src/tool/subcommands/api_cmd/api_compare_tests.rs (12)
1806-1814: Good call keeping Cron constructor decode test gated until go-state-types supports itThe TODO with a link is clear. Once upstream lands support, we can just uncomment and wire it in.
1814-1841: Base StateDecodeParams tests look solid (EVM constructor, System constructor, Cron EpochTick)Nice coverage and consistent use of v16 Method enums where available (except the EVM constructor; see suggestion below).
1843-1853: Great modularization: delegating per-actor decode tests keeps the top-level readableThis makes future extensions much easier.
1855-1863: Miner params setup is correct and realisticConstructor and ChangeWorker params are well-formed for v16.
1886-1895: Account params look goodUsing v16 types for constructor and AuthenticateMessage is correct.
1969-1993: Reward decode tests cover the main surfaces and use enums consistentlyConstructor, AwardBlockReward, UpdateNetworkKPI, and ThisEpochReward look correct for v16.
1996-2050: Power actor params setup is correct; nice coverage including exported variantsGood use of RegisteredPoStProof, UpdateClaimedPower, EnrollCronEvent, UpdatePledgeTotal, and raw power queries.
2064-2071: Keeping unsupported Lotus methods commented with upstream issue link is appropriateOnce go-state-types issue 401 is resolved, we can enable the test.
2072-2107: Zero-parameter decode tests for Power actor are comprehensiveConstructor, tick end, and read-only exported methods are all covered.
2392-2483: VerifReg params look well-formed across constructors and admin opsConstructor, AddVerifier, RemoveVerifier, AddVerifiedClient, and RemoveDataCap are set up with plausible placeholder data and correct v16 types.
2485-2581: Excellent VerifReg coverage, including exported FRC-42 variants and URHYou’ve covered the main methods plus exported aliases. If scope permits, verify whether any additional exported VerifReg methods (e.g., for AddVerifier/RemoveVerifier) exist in v8–v16 and add corresponding decode tests for completeness.
Would you like me to open a checklist of VerifReg v8–v16 methods vs tests present here to ensure we meet #5720’s completion criteria?
2478-2483: No changes required:fvm_actor_utilsis already enabledThe root
Cargo.tomldeclaresfvm_actor_utils = "14.0.0"as a normal (non-optional) dependency, so
UniversalReceiverParamsis available without any feature flag or fallback.Likely an incorrect or invalid review comment.
Summary of changes
Changes introduced in this pull request:
Reference issue to close (if applicable)
Closes: #5720
Other information and links
@elmattic here are the metadata for the test that
rpcsnapfiles are testing.Change checklist
Summary by CodeRabbit
New Features
Tests
Chores