Add support for cron actor in StateDecodeParams API#5804
Conversation
|
No parity test yet for this as lotus is missing support for cron actor |
sudo-shashank
force-pushed
the
shashank/cron-state-decode-params
branch
from
a5c7ce3 to
631ce0b
Compare
WalkthroughAdds Cron actor support across versions v8–v16: bumps fil_actor_* and fil_actors_shared dependencies to 22.4, introduces Lotus JSON constructor params for Cron, registers Cron actor methods (Constructor, EpochTick) in the RPC method registry, wires actor dispatch, and adds corresponding API tests and a snapshot. Changes
Sequence Diagram(s)sequenceDiagram
participant Init as Init Code
participant Registry as MethodRegistry
participant CronMod as cron module
Init->>Registry: create registry
Registry->>CronMod: register_actor_methods(cid, version)
CronMod-->>Registry: register Constructor(params v8–v16)
CronMod-->>Registry: register EpochTick(empty)
Registry-->>Init: registry populated
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Assessment against linked issues
Assessment against linked issues: Out-of-scope changes
Possibly related PRs
Suggested reviewers
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (5)
🚧 Files skipped from review as they are similar to previous changes (5)
⏰ 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)
✨ 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 (
|
sudo-shashank
force-pushed
the
shashank/cron-state-decode-params
branch
from
631ce0b to
5faf793
Compare
sudo-shashank
requested review from
akaladarshi and
elmattic
and removed request for
a team
|
@sudo-shashank CI is failing |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
src/rpc/registry/methods_reg.rs (1)
256-263: Add Cron to unit test coverage for registered methodsTo exercise the new Cron registration path and prevent regressions, include Cron in
test_supported_actor_methods_registered()’s supported actors.Apply this diff:
- let supported_actors = vec![ - BuiltinActor::Account, - BuiltinActor::Miner, - BuiltinActor::EVM, - ]; + let supported_actors = vec![ + BuiltinActor::Account, + BuiltinActor::Miner, + BuiltinActor::EVM, + BuiltinActor::Cron, + ];src/tool/subcommands/api_cmd/api_compare_tests.rs (2)
1900-1908: Prefer keeping Cron constructor test compiled but ignored (to avoid bit-rot)Instead of commenting out the Cron constructor params and test, define them and add the test with
.ignore("blocked by go-state-types #396"). This keeps types exercised by CI and eases future enablement.Apply this diff:
- // TODO(go-state-types): https://github.com/filecoin-project/go-state-types/issues/396 - // Enable this test when lotus supports it in go-state-types. - // let cron_constructor_params = fil_actor_cron_state::v16::ConstructorParams { - // entries: vec![fil_actor_cron_state::v16::Entry { - // receiver: Address::new_id(1000).into(), - // method_num: fil_actor_cron_state::v16::Method::EpochTick as u64, - // }], - // }; + // TODO(go-state-types): https://github.com/filecoin-project/go-state-types/issues/396 + // Keep compiled but ignored so CI catches type drift; un-ignore when Lotus supports it. + let cron_constructor_params = fil_actor_cron_state::v16::ConstructorParams { + entries: vec![fil_actor_cron_state::v16::Entry { + receiver: Address::new_id(1000).into(), + method_num: fil_actor_cron_state::v16::Method::EpochTick as u64, + }], + };And add the ignored test in the list below (see next suggestion).
2030-2043: Cron EpochTick test addition looks good; also add ignored constructor testEpochTick is an empty-params method; the identity test is appropriate. Additionally, include an ignored constructor test to be enabled once Lotus parity lands.
Apply this diff to insert the ignored constructor test next to EpochTick:
- // TODO(go-state-types): https://github.com/filecoin-project/go-state-types/issues/396 - // Enable this test when lotus supports it in go-state-types. - // RpcTest::identity(StateDecodeParams::request(( - // Address::CRON_ACTOR, - // fil_actor_cron_state::v16::Method::Constructor as u64, - // to_vec(&cron_constructor_params)?, - // tipset.key().into(), - // ))?), + // Keep this compiled but ignored until Lotus supports Cron Constructor params. + RpcTest::identity(StateDecodeParams::request(( + Address::CRON_ACTOR, + fil_actor_cron_state::v16::Method::Constructor as u64, + to_vec(&cron_constructor_params)?, + tipset.key().into(), + ))?) + .ignore("Blocked by go-state-types: https://github.com/filecoin-project/go-state-types/issues/396"), RpcTest::identity(StateDecodeParams::request(( Address::CRON_ACTOR, fil_actor_cron_state::v16::Method::EpochTick as u64, vec![], tipset.key().into(), ))?),
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/lotus_json/actor_states/methods/mod.rs(1 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(2 hunks)src/tool/subcommands/api_cmd/test_snapshots.txt(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- src/tool/subcommands/api_cmd/test_snapshots.txt
🚧 Files skipped from review as they are similar to previous changes (2)
- src/rpc/registry/actors/mod.rs
- src/lotus_json/actor_states/methods/mod.rs
🧰 Additional context used
🧬 Code Graph Analysis (1)
src/rpc/registry/methods_reg.rs (2)
src/rpc/registry/actors/cron.rs (1)
register_actor_methods(24-37)src/rpc/registry/actors/system.rs (1)
register_actor_methods(22-35)
⏰ 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: Build Ubuntu
- GitHub Check: cargo-publish-dry-run
- GitHub Check: tests-release
- GitHub Check: All lint checks
- GitHub Check: Build MacOS
- GitHub Check: Analyze (rust)
- GitHub Check: Analyze (go)
🔇 Additional comments (2)
src/rpc/registry/methods_reg.rs (2)
77-78: Cron actor module import looks correctImporting
cronalongside existing actor modules is appropriate and consistent with the module structure.
91-92: Registering Cron methods in the actor dispatch is correctDispatching
BuiltinActor::Crontocron::register_actor_methods(self, cid, version)aligns with other actors and enables versioned registration for v8–v16.
There was a problem hiding this comment.
@sudo-shashank rename the file cron_construtor_params -> cron_actor_params or cron_actor.
We are planning to keep a single file for all the params of a particular actor.
Edit: lets go with the cron_actor_params.rs, will refactor in other places as well in later PR's
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/lotus_json/actor_states/methods/cron_actor_params.rs (2)
62-75: Cross-version fallback is pragmatic; consider documenting field-loss semanticsWhen encountering a mismatched
Entryvariant, you normalize viaentry.into_lotus_json()and reconstruct the v$version type. If a future actor version adds fields toEntry, those will be dropped during this fallback. Today (v8–v16) the shape is stable, so this is fine.Add a short comment to clarify this intentional lossy conversion for future readers.
Self { entries: json.entries.into_iter().map(|entry| match entry { Entry::[<V $version>](e) => e, _ => { + // Note: Fallback normalizes via Lotus JSON; if newer versions add fields, + // they may be dropped here when constructing v$version::Entry. let lotus_entry = entry.into_lotus_json(); fil_actor_cron_state::[<v $version>]::Entry { receiver: lotus_entry.receiver.into(), method_num: lotus_entry.method_num, } } }).collect(), }
12-16: Optional: be strict about unknown fields in JSONIf you want to harden decoding, add
deny_unknown_fieldsto fail fast on unexpected JSON keys. This mirrors patterns used elsewhere when strict Lotus compatibility is desired.#[derive(Debug, Serialize, Deserialize, JsonSchema, Clone)] -#[serde(rename_all = "PascalCase")] +#[serde(rename_all = "PascalCase", deny_unknown_fields)] pub struct CronConstructorParamsLotusJson {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/lotus_json/actor_states/methods/cron_actor_params.rs(1 hunks)src/lotus_json/actor_states/methods/mod.rs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/lotus_json/actor_states/methods/mod.rs
🧰 Additional context used
🧬 Code Graph Analysis (1)
src/lotus_json/actor_states/methods/cron_actor_params.rs (1)
src/shim/address.rs (1)
new_id(142-144)
⏰ 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 Ubuntu
- GitHub Check: cargo-publish-dry-run
- GitHub Check: Build MacOS
- GitHub Check: tests-release
- GitHub Check: tests
- GitHub Check: All lint checks
- GitHub Check: Build forest binaries on Linux AMD64
- GitHub Check: Analyze (go)
- GitHub Check: Analyze (rust)
🔇 Additional comments (3)
src/lotus_json/actor_states/methods/cron_actor_params.rs (3)
10-16: Serde/schemars wiring looks correct for Lotus JSON shapeUsing PascalCase with a custom serde module for
Vec<Entry>aligns with existing lotus_json patterns in this codebase. The schema indirection viaLotusJson<Vec<Entry>>is also consistent.
56-61: Version-tagging entries on encode is correct and efficientWrapping versioned entries with the
Entry::V$versionvariant duringinto_lotus_jsonis the right approach for round-trippable, version-aware JSON.
82-82: Coverage for v8–v16 looks completeThe macro invocation registers all required versions per the issue checklist.
| use super::*; | ||
| mod account_authenticate_params; | ||
| mod account_constructor_params; | ||
| mod cron_actor_params; |
There was a problem hiding this comment.
We should determine a unique naming convention for our modules:
Right now we have 3:
- <actor-name>_<method>_params
- <actor-name>_actor
- <actor-name>_methods
At least let not invent a forth one. (<actor-name>_actor_params)
There was a problem hiding this comment.
@elmattic we choose <actor-name>_actor_params as suggested by @akaladarshi #5804 (review)
There was a problem hiding this comment.
@elmattic all the common actor methods will be moved to a single file, earlier all of them were separated, hence the discrepancies.
There was a problem hiding this comment.
@akaladarshi Do we have an issue to track this refactor?
| 14 => register_cron_version!(registry, cid, fil_actor_cron_state::v14), | ||
| 15 => register_cron_version!(registry, cid, fil_actor_cron_state::v15), | ||
| 16 => register_cron_version!(registry, cid, fil_actor_cron_state::v16), | ||
| _ => {} |
There was a problem hiding this comment.
I mean, should we trigger an error instead of performing a no-op?
There was a problem hiding this comment.
@elmattic we should actually match against enum variants and not integer value directly
@akaladarshi has planned to fix this issue in a separate PR #5896 (comment)
There was a problem hiding this comment.
@akaladarshi do we have an issue raised to track these improvements?
3 participants
Summary of changes
Changes introduced in this pull request:
cronactor inFilecoin.StateDecodeParamsAPI.Reference issue to close (if applicable)
Closes #5725
Other information and links
Change checklist
Summary by CodeRabbit