feat(api): register missing miner actor v18 methods#6950
Conversation
WalkthroughThis PR updates Filecoin actor crate dependencies from version 26.0.0 to 26.1.0 and adds Lotus-JSON serialization support for new miner v18 sector management methods (GenerateSectorLocation, ValidateSectorStatus), registering these methods in the miner actor RPC registry for v18 and onwards. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
✨ Simplify code
Comment |
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/rpc/registry/actors/miner.rs (1)
415-420: Consider a small regression test for v18 method registry completeness.This change fixes a real gap; a focused assertion that v18 includes these exported miner methods would guard against future misses.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/rpc/registry/actors/miner.rs` around lines 415 - 420, Add a small regression test that calls register_miner_version_18(registry, cid) and then asserts the MethodRegistry contains the exported miner methods expected for fil_actor_miner_state::v18; specifically build a test that creates a fresh MethodRegistry, invokes register_miner_version_18, and checks presence (by method name or method ID) of the key exported methods registered by register_miner_common_methods_v10_onwards!, register_miner_common_method_v14_onwards!, register_miner_common_method_v16_onwards!, and register_miner_common_method_v18_onwards! to detect future omissions; place the test alongside other miner registry tests and use the same helper utilities used by existing tests to construct the CID and lookup entries in MethodRegistry.src/lotus_json/actors/params/miner_params.rs (1)
4130-4180: Optional: rename macro for clearer intent.
impl_lotus_json_for_validate_sector_status_change_paramsreads slightly off from the actual type (ValidateSectorStatusParams); a small rename would improve maintainability.♻️ Proposed rename
-macro_rules! impl_lotus_json_for_validate_sector_status_change_params { +macro_rules! impl_lotus_json_for_validate_sector_status_params { @@ -impl_lotus_json_for_validate_sector_status_change_params!(18); +impl_lotus_json_for_validate_sector_status_params!(18);Also applies to: 4243-4243
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lotus_json/actors/params/miner_params.rs` around lines 4130 - 4180, Rename the macro impl_lotus_json_for_validate_sector_status_change_params to more accurately reflect the target type, e.g. impl_lotus_json_for_validate_sector_status_params; update the macro definition and all call sites to the new name, ensuring the inner generated module, the referenced type fil_actor_miner_state::v{n}::ValidateSectorStatusParams, and the HasLotusJson impl remain unchanged (look for occurrences of impl_lotus_json_for_validate_sector_status_change_params and rename them consistently, including the other occurrence noted in the diff).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/lotus_json/actors/params/miner_params.rs`:
- Around line 4130-4180: Rename the macro
impl_lotus_json_for_validate_sector_status_change_params to more accurately
reflect the target type, e.g. impl_lotus_json_for_validate_sector_status_params;
update the macro definition and all call sites to the new name, ensuring the
inner generated module, the referenced type
fil_actor_miner_state::v{n}::ValidateSectorStatusParams, and the HasLotusJson
impl remain unchanged (look for occurrences of
impl_lotus_json_for_validate_sector_status_change_params and rename them
consistently, including the other occurrence noted in the diff).
In `@src/rpc/registry/actors/miner.rs`:
- Around line 415-420: Add a small regression test that calls
register_miner_version_18(registry, cid) and then asserts the MethodRegistry
contains the exported miner methods expected for fil_actor_miner_state::v18;
specifically build a test that creates a fresh MethodRegistry, invokes
register_miner_version_18, and checks presence (by method name or method ID) of
the key exported methods registered by
register_miner_common_methods_v10_onwards!,
register_miner_common_method_v14_onwards!,
register_miner_common_method_v16_onwards!, and
register_miner_common_method_v18_onwards! to detect future omissions; place the
test alongside other miner registry tests and use the same helper utilities used
by existing tests to construct the CID and lookup entries in MethodRegistry.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: efaa9561-24c2-4e93-a12a-1bb1fda48771
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (3)
Cargo.tomlsrc/lotus_json/actors/params/miner_params.rssrc/rpc/registry/actors/miner.rs
Summary of changes
Changes introduced in this pull request:
GenerateSectorLocationExportedValidateSectorStatusExportedGetNominalSectorExpirationExportedReference issue to close (if applicable)
Closes #6920
Other information and links
Change checklist
Outside contributions
Summary by CodeRabbit
New Features
Chores