refactor(actor_state): allow easy version upgrades #6087
Conversation
WalkthroughDeserialization now normalizes Lotus JSON to the latest actor-state versions (V17) via new default_latest_version helpers; many HasLotusJson impls were simplified or macro-generated (paste); several LotusJson structs and field shapes changed (EVM, transient/tombstone, bytecode hash, filter estimates, vesting funds); shim modules gained V17 factory constructors. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Caller
participant LotusJSON as LotusJson payload
participant HasLotusJson as HasLotusJson impl (macro)
participant Shim as Shim latest-version factory
Caller->>HasLotusJson: from_lotus_json(LotusJSON)
note right of HasLotusJson #DDEBF7: per-version parsing via macro\nmap/convert subcomponents
HasLotusJson->>Shim: State::default_latest_version(mapped fields)
note right of Shim #E6F4EA: constructs V17 variant
Shim-->>Caller: State (V17)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ 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). (7)
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. Comment |
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
src/lotus_json/tombstone.rs (1)
21-31: Serialize TombstoneLotusJson.orign as "Origin"Add a per-field serde rename so the misspelled field doesn't serialize as "Orign" and break round‑trips.
File: src/lotus_json/tombstone.rs
pub struct TombstoneLotusJson { - pub orign: ActorID, + #[serde(rename = "Origin")] + pub orign: ActorID, pub nonce: u64, }src/shim/actors/builtin/market/mod.rs (3)
139-141: Bug: V17 escrow_table arm constructs a V16BalanceTableThis will load the wrong table type for V17 states.
Apply this diff:
- Self::V17(s) => { - V16BalanceTable::from_root(store, &s.escrow_table, "escrow table")?.into() - } + Self::V17(s) => { + V17BalanceTable::from_root(store, &s.escrow_table, "escrow table")?.into() + }
245-247: Wrong error adapter used for V17 deal statesV17 arm wraps with V16AsActorError instead of V17AsActorError.
Apply this diff:
- State::V17(st) => Ok(DealStates::V17(V16AsActorError::context_code( + State::V17(st) => Ok(DealStates::V17(V17AsActorError::context_code( V17DealMetaArray::load(&st.states, store), FVM4ExitCode::USR_ILLEGAL_STATE, "failed to load deal state array", )?)),
114-174: Critical: fix V17 copy/paste errors in market actor
- src/shim/actors/builtin/market/mod.rs — escrow_table (lines ~114–174): Self::V17(s) calls V16BalanceTable::from_root; change to V17BalanceTable::from_root.
- src/shim/actors/builtin/market/mod.rs:245 — State::V17(...) wraps V17DealMetaArray with V16AsActorError::context_code; change to V17AsActorError::context_code.
Run a repo-wide search for other V17 arms referencing V16 types/errors and correct them.
src/lotus_json/filter_estimate.rs (1)
81-81: Fix typo inoriginfield nameThere's a typo in the field name -
orignshould beorigin.- origin: tombstone_lotus.orign.into(), + origin: tombstone_lotus.origin.into(),
🧹 Nitpick comments (23)
src/lotus_json/bytecode_hash.rs (2)
7-10: Solid macroization; add Copy/Clone and a minimal snapshot for stability.
- Add Copy/Clone to the JSON wrapper (cheap 32-byte copy).
- Provide one snapshot to guard the codec.
-#[derive(Debug, Serialize, Deserialize, JsonSchema)] +#[derive(Debug, Serialize, Deserialize, JsonSchema, Clone, Copy)] #[serde(rename_all = "PascalCase")] #[schemars(rename = "BytecodeHash")] pub struct BytecodeHashLotusJson([u8; 32]);- #[cfg(test)] - fn snapshots() -> Vec<(serde_json::Value, Self)> { - vec![] - } + #[cfg(test)] + fn snapshots() -> Vec<(serde_json::Value, Self)> { + let bytes = [0u8; 32]; + let json = serde_json::Value::Array( + std::iter::repeat(serde_json::Value::from(0u8)) + .take(32) + .collect() + ); + vec![(json, Self::from(bytes))] + }Also applies to: 12-35
37-37: Centralize version lists to avoid drift.Consider a shared “version-list” macro/const used across modules so adding V18 is a one-liner.
src/shim/actors/builtin/system/mod.rs (1)
29-33: LGTM: consistent latest-version factory.Matches the pattern used elsewhere; straightforward and safe.
Optionally define a single LATEST = V17 constant or helper to reduce edits when bumping to V18.
src/shim/actors/builtin/datacap/mod.rs (1)
36-41: LGTM; tiny readability nit on Address types.Signature mixes fvm_shared2::address::Address (in scope) with fvm_shared4::address::Address (fully qualified). Consider aliasing v4 for clarity.
+use fvm_shared4::address::Address as AddressV4; ... - pub fn default_latest_version( - governor: fvm_shared4::address::Address, + pub fn default_latest_version( + governor: AddressV4, token: TokenState, ) -> Self {src/shim/actors/builtin/init/mod.rs (1)
31-41: LGTM: latest-version factory added.Optional: accept network_name: impl Into to reduce caller allocations.
- pub fn default_latest_version( + pub fn default_latest_version( address_map: ::cid::Cid, next_id: u64, - network_name: String, + network_name: impl Into<String>, ) -> Self { - State::V17(fil_actor_init_state::v17::State { + State::V17(fil_actor_init_state::v17::State { address_map, next_id, - network_name, + network_name: network_name.into(), }) }src/lotus_json/actors/states/payment_channel_state.rs (1)
47-66: Snapshot looks good; drop redundant import in function scope.
Addressis already imported at module scope; the inneruse crate::shim::address::Address;is unnecessary.src/shim/actors/builtin/paymentchannel/mod.rs (1)
4-4: Avoid mixing fvm_shared2 and fvm_shared4; use v4 for ChainEpoch here.Keeps types consistent with the V17 state you construct below.
Apply this diff:
-use fvm_shared2::{address::Address, clock::ChainEpoch, econ::TokenAmount}; +use fvm_shared4::clock::ChainEpoch;src/lotus_json/actors/states/init_state.rs (1)
53-60: Deserializing to latest version: good, but call out behavior.Behavioral change is explicit; deserialization will always yield the latest
State. Consider a short rustdoc onfrom_lotus_jsonor module‑level note to help future readers.src/shim/actors/builtin/account/mod.rs (1)
5-5: Remove unused import.
use crate::shim;appears unused in this file.-use crate::shim;src/shim/actors/builtin/evm/mod.rs (2)
28-44: Constructor API: accept BytecodeHash directly to avoid misuse.Taking
[u8; 32]invites accidental non‑keccak inputs; preferBytecodeHashto make the contract explicit. You can keep a helper that converts from[u8;32]if needed.- pub fn default_latest_version( - bytecode: Cid, - bytecode_hash: [u8; 32], + pub fn default_latest_version( + bytecode: Cid, + bytecode_hash: BytecodeHash, contract_state: Cid, transient_data: Option<TransientData>, nonce: u64, tombstone: Option<Tombstone>, ) -> Self { State::V17(fil_actor_evm_state::v17::State { bytecode, - bytecode_hash: BytecodeHash::from(bytecode_hash), + bytecode_hash, contract_state, transient_data, nonce, tombstone, }) }
4-8: Prune unused imports.
HasLotusJsonandfvm_shared2::address::Addressappear unused here.-use crate::lotus_json::HasLotusJson; - use ::cid::Cid; - use fil_actor_evm_state::v17::{BytecodeHash, Tombstone, TransientData}; - use fvm_shared2::address::Address; +use ::cid::Cid; +use fil_actor_evm_state::v17::{BytecodeHash, Tombstone, TransientData};src/shim/actors/builtin/multisig/mod.rs (1)
45-64: Prefer v4 ChainEpoch in latest-version ctor.Verified: the ctor mixes fvm_shared4 address/TokenAmount with the crate-imported (v2) ChainEpoch — switch the two params to fvm_shared4::clock::ChainEpoch for clarity.
- pub fn default_latest_version( - signers: Vec<fvm_shared4::address::Address>, - num_approvals_threshold: u64, - next_tx_id: i64, - initial_balance: fvm_shared4::econ::TokenAmount, - start_epoch: ChainEpoch, - unlock_duration: ChainEpoch, - pending_txs: cid::Cid, - ) -> Self { + pub fn default_latest_version( + signers: Vec<fvm_shared4::address::Address>, + num_approvals_threshold: u64, + next_tx_id: i64, + initial_balance: fvm_shared4::econ::TokenAmount, + start_epoch: fvm_shared4::clock::ChainEpoch, + unlock_duration: fvm_shared4::clock::ChainEpoch, + pending_txs: cid::Cid, + ) -> Self {src/shim/actors/builtin/reward/mod.rs (1)
58-85: Future‑proof the “latest” constructor.Optional: add a small doc note or an associated const to flag bump work when V18 lands, to avoid missing this spot during version upgrades.
impl State { + /// Construct the latest known actor state (update when a new version is added). #[allow(clippy::too_many_arguments)] pub fn default_latest_version(src/shim/actors/builtin/power/mod.rs (1)
39-78: Tame the long parameter list (optional).Consider a Params struct or builder to reduce call‑site churn and ease version bumps.
impl State { - #[allow(clippy::too_many_arguments)] - pub fn default_latest_version( - total_raw_byte_power: StoragePower, - total_bytes_committed: StoragePower, - total_quality_adj_power: StoragePower, - total_qa_bytes_committed: StoragePower, - total_pledge_collateral: fvm_shared4::econ::TokenAmount, - this_epoch_raw_byte_power: StoragePower, - this_epoch_quality_adj_power: StoragePower, - this_epoch_pledge_collateral: fvm_shared4::econ::TokenAmount, - this_epoch_qa_power_smoothed: fil_actors_shared::v17::builtin::reward::smooth::FilterEstimate, - miner_count: i64, - miner_above_min_power_count: i64, - cron_event_queue: cid::Cid, - first_cron_epoch: i64, - claims: cid::Cid, - proof_validation_batch: Option<cid::Cid>, - ramp_start_epoch: i64, - ramp_duration_epochs: u64, - ) -> Self { + pub fn default_latest_version(p: PowerV17Params) -> Self { State::V17(fil_actor_power_state::v17::State { - total_raw_byte_power, - total_bytes_committed, - total_quality_adj_power, - total_qa_bytes_committed, - total_pledge_collateral, - this_epoch_raw_byte_power, - this_epoch_quality_adj_power, - this_epoch_pledge_collateral, - this_epoch_qa_power_smoothed, - miner_count, - miner_above_min_power_count, - cron_event_queue, - first_cron_epoch, - claims, - proof_validation_batch, - ramp_start_epoch, - ramp_duration_epochs, + total_raw_byte_power: p.total_raw_byte_power, + total_bytes_committed: p.total_bytes_committed, + total_quality_adj_power: p.total_quality_adj_power, + total_qa_bytes_committed: p.total_qa_bytes_committed, + total_pledge_collateral: p.total_pledge_collateral, + this_epoch_raw_byte_power: p.this_epoch_raw_byte_power, + this_epoch_quality_adj_power: p.this_epoch_quality_adj_power, + this_epoch_pledge_collateral: p.this_epoch_pledge_collateral, + this_epoch_qa_power_smoothed: p.this_epoch_qa_power_smoothed, + miner_count: p.miner_count, + miner_above_min_power_count: p.miner_above_min_power_count, + cron_event_queue: p.cron_event_queue, + first_cron_epoch: p.first_cron_epoch, + claims: p.claims, + proof_validation_batch: p.proof_validation_batch, + ramp_start_epoch: p.ramp_start_epoch, + ramp_duration_epochs: p.ramp_duration_epochs, }) }And define PowerV17Params alongside (non‑blocking).
src/lotus_json/actors/states/market_state.rs (1)
65-84: Avoid setting fields incommon_*that specialized macros immediately override (nit).Keeps the update syntax minimal and avoids constructing throwaway values.
macro_rules! common_market_state_fields { ($state:expr) => {{ MarketStateLotusJson { proposals: $state.proposals, states: $state.states, pending_proposals: $state.pending_proposals, escrow_table: $state.escrow_table, locked_table: $state.locked_table, next_id: $state.next_id, deal_ops_by_epoch: $state.deal_ops_by_epoch, last_cron: $state.last_cron, total_client_locked_collateral: $state.total_client_locked_collateral.into(), total_provider_locked_collateral: $state.total_provider_locked_collateral.into(), total_client_storage_fee: $state.total_client_storage_fee.into(), - pending_deal_allocation_ids: None, - provider_sectors: None, } }}; }And explicitly set them in v8:
macro_rules! v8_market_state_fields { ($state:expr) => {{ MarketStateLotusJson { + pending_deal_allocation_ids: None, + provider_sectors: None, ..common_market_state_fields!($state) } }}; }src/shim/actors/builtin/market/mod.rs (1)
743-754: Inconsistent enum typing for V8 deal meta array
DealStates::V8(V9DealMetaArray<...>)is misleading since V8 path is unimplemented above. Consider removing the V8 variant or annotating it as intentionally unused to avoid confusion.src/shim/actors/builtin/cron/mod.rs (1)
4-5: Remove unused import
use crate::shim;appears unused.use crate::lotus_json::HasLotusJson; -use crate::shim;src/lotus_json/actors/states/cron_state.rs (1)
60-72: Avoid double conversion when deserializingCurrent code builds Entry::V17 then re-converts to v17::Entry. Map directly to v17 entries and call the latest constructor.
- fn from_lotus_json(lotus_json: Self::LotusJson) -> Self { - let entries = lotus_json - .entries - .into_iter() - .map(|entry| { - let lotus_entry = entry.into_lotus_json(); - Entry::default_latest_version(lotus_entry.receiver.into(), lotus_entry.method_num) - }) - .collect(); - - State::default_latest_version_from_entries(entries) - } + fn from_lotus_json(lotus_json: Self::LotusJson) -> Self { + let entries_v17: Vec<fil_actor_cron_state::v17::Entry> = lotus_json + .entries + .into_iter() + .map(|entry| { + let e = entry.into_lotus_json(); + fil_actor_cron_state::v17::Entry { + receiver: e.receiver.into(), + method_num: e.method_num, + } + }) + .collect(); + State::default_latest_version(entries_v17) + }src/lotus_json/transient_data.rs (2)
9-17: Derives: add Clone/PartialEq/Eq for ergonomics and consistencyThese JSON DTOs are frequently cloned/compared in tests and conversions elsewhere in this crate. Recommend adding Clone, PartialEq, and Eq to TransientDataLotusJson.
-#[derive(Debug, Serialize, Deserialize, JsonSchema)] +#[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize, JsonSchema)] pub struct TransientDataLotusJson {
19-25: Derives: add Eq (you already have PartialEq/Clone)All fields implement Eq; deriving Eq helps equality in tests and avoids surprises.
-#[derive(Debug, Clone, PartialEq, Serialize, Deserialize, JsonSchema)] +#[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize, JsonSchema)] pub struct TransientDataLifespanLotusJson {src/lotus_json/actors/states/vesting_funds.rs (3)
9-18: Derives: include PartialEq/Eq for stable testing and comparisonsThese DTOs appear in snapshots; deriving PartialEq/Eq simplifies assertions and avoids custom comparers.
-#[derive(Debug, Serialize, Deserialize, JsonSchema, Clone)] +#[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize, JsonSchema)] pub struct VestingFundLotusJson {
20-30: Same here: add PartialEq/Eq to VestingFundsLotusJsonKeeps JSON types uniform across the module.
-#[derive(Debug, Serialize, Deserialize, JsonSchema, Clone)] +#[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize, JsonSchema)] pub struct VestingFundsLotusJson {
41-82: VestingFund HasLotusJson impl: logic LGTM; unify BigInt import across snapshotsConversions look correct. Minor consistency nit: in this snapshot you import fvm_shared4::bigint::BigInt, while the later snapshot uses num_bigint::BigInt. Prefer one (num_bigint::BigInt is common in this repo) to avoid confusion.
Possible tidy-up:
- use fvm_shared4::bigint::BigInt; + use num_bigint::BigInt;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (32)
src/lotus_json/actors/states/account_state.rs(3 hunks)src/lotus_json/actors/states/cron_state.rs(2 hunks)src/lotus_json/actors/states/datacap_state.rs(2 hunks)src/lotus_json/actors/states/evm_state.rs(3 hunks)src/lotus_json/actors/states/init_state.rs(3 hunks)src/lotus_json/actors/states/market_state.rs(3 hunks)src/lotus_json/actors/states/miner_state.rs(5 hunks)src/lotus_json/actors/states/multisig_state.rs(2 hunks)src/lotus_json/actors/states/payment_channel_state.rs(2 hunks)src/lotus_json/actors/states/power_states.rs(4 hunks)src/lotus_json/actors/states/reward_state.rs(2 hunks)src/lotus_json/actors/states/system_state.rs(2 hunks)src/lotus_json/actors/states/verified_reg_state.rs(2 hunks)src/lotus_json/actors/states/vesting_funds.rs(1 hunks)src/lotus_json/bytecode_hash.rs(1 hunks)src/lotus_json/entry.rs(2 hunks)src/lotus_json/filter_estimate.rs(2 hunks)src/lotus_json/tombstone.rs(2 hunks)src/lotus_json/transient_data.rs(2 hunks)src/shim/actors/builtin/account/mod.rs(3 hunks)src/shim/actors/builtin/cron/mod.rs(3 hunks)src/shim/actors/builtin/datacap/mod.rs(2 hunks)src/shim/actors/builtin/evm/mod.rs(3 hunks)src/shim/actors/builtin/init/mod.rs(1 hunks)src/shim/actors/builtin/market/mod.rs(1 hunks)src/shim/actors/builtin/miner/mod.rs(2 hunks)src/shim/actors/builtin/multisig/mod.rs(1 hunks)src/shim/actors/builtin/paymentchannel/mod.rs(2 hunks)src/shim/actors/builtin/power/mod.rs(1 hunks)src/shim/actors/builtin/reward/mod.rs(1 hunks)src/shim/actors/builtin/system/mod.rs(1 hunks)src/shim/actors/builtin/verifreg/mod.rs(1 hunks)
🧰 Additional context used
🧠 Learnings (5)
📓 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.937Z
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.
📚 Learning: 2025-09-11T16:03:14.328Z
Learnt from: akaladarshi
PR: ChainSafe/forest#6000
File: src/tool/subcommands/api_cmd/state_decode_params_tests/miner.rs:4-9
Timestamp: 2025-09-11T16:03:14.328Z
Learning: In the Forest codebase's state_decode_params_tests module, common imports like `to_vec`, `TokenAmount`, `Cid`, etc. are centralized in the mod.rs file and made available to submodules through `use super::*;`, eliminating the need for duplicate imports in individual test files.
Applied to files:
src/shim/actors/builtin/datacap/mod.rs
📚 Learning: 2025-09-02T10:05:34.350Z
Learnt from: akaladarshi
PR: ChainSafe/forest#5923
File: src/rpc/registry/actors/miner.rs:221-223
Timestamp: 2025-09-02T10:05:34.350Z
Learning: For miner actor ChangeOwnerAddress and ChangeOwnerAddressExported methods: versions 8-10 use bare Address as parameter type, while versions 11+ use ChangeOwnerAddressParams. This reflects the evolution of the Filecoin miner actor parameter structures across versions.
Applied to files:
src/shim/actors/builtin/account/mod.rs
📚 Learning: 2025-08-18T12:25:29.183Z
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.
Applied to files:
src/lotus_json/actors/states/verified_reg_state.rs
📚 Learning: 2025-09-02T08:46:04.937Z
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.937Z
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.
Applied to files:
src/lotus_json/actors/states/payment_channel_state.rs
🧬 Code graph analysis (31)
src/shim/actors/builtin/verifreg/mod.rs (5)
src/shim/actors/builtin/account/mod.rs (1)
default_latest_version(43-45)src/shim/actors/builtin/cron/mod.rs (2)
default_latest_version(42-44)default_latest_version(63-71)src/shim/actors/builtin/datacap/mod.rs (1)
default_latest_version(36-41)src/shim/actors/builtin/init/mod.rs (1)
default_latest_version(31-41)src/shim/actors/builtin/verifreg/ext/state.rs (1)
root_key(259-272)
src/shim/actors/builtin/init/mod.rs (3)
src/shim/actors/builtin/account/mod.rs (1)
default_latest_version(43-45)src/shim/actors/builtin/cron/mod.rs (2)
default_latest_version(42-44)default_latest_version(63-71)src/shim/actors/builtin/system/mod.rs (1)
default_latest_version(30-32)
src/shim/actors/builtin/power/mod.rs (5)
src/shim/actors/builtin/account/mod.rs (1)
default_latest_version(43-45)src/shim/actors/builtin/market/mod.rs (1)
default_latest_version(82-112)src/shim/actors/builtin/miner/mod.rs (1)
default_latest_version(48-82)src/shim/actors/builtin/reward/mod.rs (1)
default_latest_version(59-85)src/shim/actors/builtin/power/ext.rs (4)
ramp_start_epoch(9-9)ramp_start_epoch(17-24)ramp_duration_epochs(13-13)ramp_duration_epochs(26-33)
src/shim/actors/builtin/cron/mod.rs (1)
src/shim/actors/builtin/account/mod.rs (1)
default_latest_version(43-45)
src/shim/actors/builtin/paymentchannel/mod.rs (5)
src/shim/actors/builtin/account/mod.rs (1)
default_latest_version(43-45)src/shim/actors/builtin/market/mod.rs (1)
default_latest_version(82-112)src/shim/actors/builtin/multisig/mod.rs (1)
default_latest_version(46-64)src/shim/actors/builtin/power/mod.rs (1)
default_latest_version(40-78)src/shim/actors/builtin/reward/mod.rs (1)
default_latest_version(59-85)
src/lotus_json/actors/states/account_state.rs (2)
src/shim/actors/builtin/account/mod.rs (1)
default_latest_version(43-45)src/lotus_json/mod.rs (4)
from_lotus_json(149-149)from_lotus_json(572-574)from_lotus_json(586-591)from_lotus_json(607-613)
src/shim/actors/builtin/market/mod.rs (6)
src/shim/actors/builtin/account/mod.rs (1)
default_latest_version(43-45)src/shim/actors/builtin/evm/mod.rs (2)
default_latest_version(28-44)default_latest_version(87-89)src/shim/actors/builtin/init/mod.rs (1)
default_latest_version(31-41)src/shim/actors/builtin/miner/mod.rs (1)
default_latest_version(48-82)src/shim/actors/builtin/multisig/mod.rs (1)
default_latest_version(46-64)src/shim/actors/builtin/verifreg/mod.rs (1)
default_latest_version(48-64)
src/shim/actors/builtin/evm/mod.rs (7)
src/shim/actors/builtin/account/mod.rs (1)
default_latest_version(43-45)src/shim/actors/builtin/init/mod.rs (1)
default_latest_version(31-41)src/shim/actors/builtin/market/mod.rs (11)
default_latest_version(82-112)from(357-359)from(363-365)from(369-371)from(375-377)from(381-383)from(387-389)from(393-395)from(399-401)from(405-407)from(410-412)src/shim/actors/builtin/miner/mod.rs (7)
default_latest_version(48-82)from(494-515)from(519-552)from(556-596)from(600-640)from(644-672)from(676-716)src/shim/actors/builtin/multisig/mod.rs (1)
default_latest_version(46-64)src/shim/actors/builtin/paymentchannel/mod.rs (1)
default_latest_version(22-38)src/shim/actors/builtin/system/mod.rs (1)
default_latest_version(30-32)
src/shim/actors/builtin/reward/mod.rs (2)
src/shim/actors/builtin/account/mod.rs (1)
default_latest_version(43-45)src/shim/actors/builtin/power/mod.rs (1)
default_latest_version(40-78)
src/shim/actors/builtin/datacap/mod.rs (1)
src/shim/actors/builtin/account/mod.rs (1)
default_latest_version(43-45)
src/shim/actors/builtin/multisig/mod.rs (3)
src/shim/actors/builtin/account/mod.rs (1)
default_latest_version(43-45)src/shim/actors/builtin/market/mod.rs (1)
default_latest_version(82-112)src/shim/actors/builtin/paymentchannel/mod.rs (1)
default_latest_version(22-38)
src/lotus_json/actors/states/datacap_state.rs (2)
src/shim/actors/builtin/datacap/mod.rs (1)
default_latest_version(36-41)src/lotus_json/actors/states/account_state.rs (1)
from_lotus_json(37-39)
src/lotus_json/entry.rs (1)
src/shim/actors/builtin/cron/mod.rs (2)
default_latest_version(42-44)default_latest_version(63-71)
src/lotus_json/tombstone.rs (4)
src/shim/actors/builtin/evm/mod.rs (3)
default_latest_version(28-44)default_latest_version(87-89)nonce(46-57)src/lotus_json/actors/states/account_state.rs (1)
from_lotus_json(37-39)src/lotus_json/entry.rs (1)
from_lotus_json(77-79)src/lotus_json/mod.rs (5)
from_lotus_json(149-149)from_lotus_json(572-574)from_lotus_json(586-591)from_lotus_json(607-613)from_lotus_json(632-639)
src/shim/actors/builtin/account/mod.rs (5)
src/shim/actors/builtin/cron/mod.rs (2)
default_latest_version(42-44)default_latest_version(63-71)src/shim/actors/builtin/datacap/mod.rs (1)
default_latest_version(36-41)src/shim/actors/builtin/multisig/mod.rs (1)
default_latest_version(46-64)src/shim/actors/builtin/paymentchannel/mod.rs (1)
default_latest_version(22-38)src/shim/actors/builtin/verifreg/mod.rs (1)
default_latest_version(48-64)
src/lotus_json/transient_data.rs (3)
src/lotus_json/actors/states/account_state.rs (3)
snapshots(20-28)into_lotus_json(30-34)from_lotus_json(37-39)src/lotus_json/entry.rs (3)
snapshots(21-30)into_lotus_json(32-75)from_lotus_json(77-79)src/lotus_json/mod.rs (17)
snapshots(147-147)snapshots(566-568)snapshots(580-582)snapshots(597-599)snapshots(621-623)serde_json(301-301)serde_json(325-325)into_lotus_json(148-148)into_lotus_json(569-571)into_lotus_json(583-585)into_lotus_json(600-606)into_lotus_json(624-631)from_lotus_json(149-149)from_lotus_json(572-574)from_lotus_json(586-591)from_lotus_json(607-613)from_lotus_json(632-639)
src/lotus_json/actors/states/verified_reg_state.rs (3)
src/shim/actors/builtin/verifreg/mod.rs (1)
default_latest_version(48-64)src/shim/address.rs (1)
new_id(142-144)src/lotus_json/actors/states/account_state.rs (1)
from_lotus_json(37-39)
src/shim/actors/builtin/miner/mod.rs (2)
src/shim/actors/builtin/account/mod.rs (1)
default_latest_version(43-45)src/shim/actors/builtin/market/mod.rs (1)
default_latest_version(82-112)
src/lotus_json/actors/states/system_state.rs (3)
src/shim/actors/builtin/system/mod.rs (1)
default_latest_version(30-32)src/lotus_json/actors/states/account_state.rs (1)
from_lotus_json(37-39)src/lotus_json/mod.rs (3)
from_lotus_json(149-149)from_lotus_json(572-574)from_lotus_json(586-591)
src/lotus_json/actors/states/init_state.rs (2)
src/shim/actors/builtin/init/mod.rs (1)
default_latest_version(31-41)src/lotus_json/actors/states/account_state.rs (1)
from_lotus_json(37-39)
src/lotus_json/actors/states/market_state.rs (2)
src/shim/actors/builtin/market/mod.rs (5)
default_latest_version(82-112)proposals(177-195)states(198-251)escrow_table(115-143)locked_table(146-174)src/lotus_json/actors/states/account_state.rs (1)
from_lotus_json(37-39)
src/lotus_json/bytecode_hash.rs (1)
src/lotus_json/mod.rs (17)
snapshots(147-147)snapshots(566-568)snapshots(580-582)snapshots(597-599)snapshots(621-623)serde_json(301-301)serde_json(325-325)into_lotus_json(148-148)into_lotus_json(569-571)into_lotus_json(583-585)into_lotus_json(600-606)into_lotus_json(624-631)from_lotus_json(149-149)from_lotus_json(572-574)from_lotus_json(586-591)from_lotus_json(607-613)from_lotus_json(632-639)
src/lotus_json/actors/states/multisig_state.rs (2)
src/shim/actors/builtin/multisig/mod.rs (1)
default_latest_version(46-64)src/lotus_json/actors/states/account_state.rs (2)
into_lotus_json(30-34)from_lotus_json(37-39)
src/lotus_json/actors/states/payment_channel_state.rs (4)
src/shim/actors/builtin/paymentchannel/mod.rs (1)
default_latest_version(22-38)src/shim/address.rs (1)
new_id(142-144)src/lotus_json/actors/states/account_state.rs (1)
from_lotus_json(37-39)src/lotus_json/entry.rs (1)
from_lotus_json(77-79)
src/lotus_json/actors/states/evm_state.rs (1)
src/shim/actors/builtin/evm/mod.rs (3)
nonce(46-57)default_latest_version(28-44)default_latest_version(87-89)
src/lotus_json/filter_estimate.rs (1)
src/lotus_json/mod.rs (17)
snapshots(147-147)snapshots(566-568)snapshots(580-582)snapshots(597-599)snapshots(621-623)serde_json(301-301)serde_json(325-325)into_lotus_json(148-148)into_lotus_json(569-571)into_lotus_json(583-585)into_lotus_json(600-606)into_lotus_json(624-631)from_lotus_json(149-149)from_lotus_json(572-574)from_lotus_json(586-591)from_lotus_json(607-613)from_lotus_json(632-639)
src/lotus_json/actors/states/cron_state.rs (3)
src/lotus_json/entry.rs (3)
snapshots(21-30)into_lotus_json(32-75)from_lotus_json(77-79)src/shim/actors/builtin/cron/mod.rs (3)
default_latest_version(42-44)default_latest_version(63-71)default_latest_version_from_entries(32-40)src/shim/address.rs (1)
new_id(142-144)
src/lotus_json/actors/states/reward_state.rs (3)
src/shim/actors/builtin/power/mod.rs (1)
default_latest_version(40-78)src/shim/actors/builtin/reward/mod.rs (2)
default_latest_version(59-85)this_epoch_baseline_power(104-117)src/shim/econ.rs (1)
from_atto(106-108)
src/lotus_json/actors/states/power_states.rs (2)
src/shim/actors/builtin/power/mod.rs (1)
default_latest_version(40-78)src/lotus_json/actors/states/account_state.rs (1)
from_lotus_json(37-39)
src/lotus_json/actors/states/vesting_funds.rs (1)
src/lotus_json/mod.rs (11)
snapshots(147-147)snapshots(566-568)snapshots(580-582)serde_json(301-301)serde_json(325-325)into_lotus_json(148-148)into_lotus_json(569-571)into_lotus_json(583-585)from_lotus_json(149-149)from_lotus_json(572-574)from_lotus_json(586-591)
src/lotus_json/actors/states/miner_state.rs (2)
src/shim/actors/builtin/miner/mod.rs (3)
default_latest_version(48-82)info(84-97)fee_debt(402-415)src/lotus_json/mod.rs (6)
into_lotus_json(148-148)into_lotus_json(569-571)into_lotus_json(583-585)from_lotus_json(149-149)from_lotus_json(572-574)from_lotus_json(586-591)
⏰ 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). (7)
- 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: tests
- GitHub Check: tests-release
🔇 Additional comments (49)
src/lotus_json/bytecode_hash.rs (1)
24-30: Confirm From/Into exist across v10–v17.Both conversions are required on every BytecodeHash type used here. Please confirm they exist in all versions.
src/lotus_json/actors/states/account_state.rs (2)
26-27: Snapshot uses default_latest_version — good.Keeps tests targeting the canonical latest form.
36-39: Deserialization normalizes to latest — verify downstream pattern-matchesFound call sites that reference historical account actor versions; confirm they don't rely on preserving older variants after from_lotus_json normalization.
- src/state_migration/nv23/mining_reserve.rs — uses fil_actor_account_state::v14::State (≈line 30)
- src/statediff/mod.rs — uses fil_actor_account_state::v10::State (alias AccountState, ≈line 236)
- src/shim/actors/builtin/account/mod.rs — enum lists V8..V16 variants.
- src/rpc/registry/actors_reg.rs — constructs account::State::V16(..., ≈line 218)
If any of these pattern-match or expect historical variants, either stop normalizing in from_lotus_json or update those callers to accept the normalized latest state.
src/shim/actors/builtin/datacap/mod.rs (1)
7-7: Import looks right.TokenState path is correct for V17 usage.
src/lotus_json/actors/states/payment_channel_state.rs (1)
83-93: Deserialize to latest version — LGTM.Consistent with the PR’s “latest-by-default” pattern and the shim ctor.
Please confirm that fields are identical across V9–V17 so no information is lost during normalization.
src/lotus_json/entry.rs (1)
77-79: from_lotus_json uses latest-version ctor — LGTM.src/shim/actors/builtin/verifreg/mod.rs (1)
48-64: Latest-version ctor added — LGTM.Constructor mirrors other actors and keeps params explicit.
src/shim/actors/builtin/paymentchannel/mod.rs (1)
21-39: Ctor for latest version — LGTM.Matches V17 fields and types.
src/lotus_json/actors/states/verified_reg_state.rs (2)
80-97: Snapshot and latest-version expectation — LGTM.
110-120: Normalize to latest with sensible defaults — confirm defaults match Lotus.
allocations/claimsdefault to empty CIDs andnext_allocation_idto0. Validate these are the intended v8-to-latest fallbacks in Lotus semantics to avoid subtle mismatches.src/lotus_json/actors/states/init_state.rs (2)
65-65: Version list order change is fine.Match arms are exhaustive; ordering is non‑semantic here. LGTM.
33-37: Verify Cid::default() serializes to "baeaaaaa".Ensure Cid::default() (used at src/lotus_json/actors/states/init_state.rs lines 33–37) serializes via crate::lotus_json to the snapshot JSON {"AddressMap":{"/":"baeaaaaa"}}; otherwise the round‑trip test may flake. Verify by serializing Cid::default() with the crate's lotus_json helper (e.g. in a small test/binary) and confirm the output equals the snapshot value.
src/shim/actors/builtin/account/mod.rs (2)
43-45: New latest-version ctor is consistent with the PR pattern.Straightforward and correct for V17.
28-41: Return type changed to fvm_shared2::address::Address — confirm downstream conversionsCallsites convert the result with .into(): src/shim/state_tree.rs:373, src/interpreter/mod.rs:49, src/lotus_json/actors/states/account_state.rs:32. Confirm those Into impls still yield the expected v3/v4 Address and that no caller depends on v3/v4-specific APIs or layout.
src/lotus_json/actors/states/datacap_state.rs (2)
61-64: Deserializing to latest version: consistent with strategy.Looks good and mirrors other actors.
39-44: Confirm hamt_bit_width = 0 in TokenState snapshotlotus_json uses hamt_bit_width: 0 in token snapshots (src/lotus_json/token_state.rs and src/lotus_json/actors/states/datacap_state.rs), while migrations/runtime create TokenState with fil_actors_shared::::builtin::HAMT_BIT_WIDTH (non‑zero). Confirm whether 0 is an intentional lotus‑JSON placeholder/legacy value or should be the actor's HAMT_BIT_WIDTH; if the latter, update the snapshot/tests to use the correct bit width.
src/shim/actors/builtin/evm/mod.rs (1)
86-90: Tombstone ctor LGTM.Clear and mirrors the state ctor.
src/shim/actors/builtin/reward/mod.rs (1)
58-85: Constructor for latest reward state looks correct.Matches v17 schema and mirrors the pattern used elsewhere. No functional concerns.
src/lotus_json/tombstone.rs (2)
42-45: Always returning latest on deserialize.Directionally good. Just note we now lose the original on‑chain version info at this layer; ensure callers don’t rely on it.
50-51: Macro order change is fine.Order in these match arms is non‑semantic; no issues expected.
src/lotus_json/actors/states/system_state.rs (2)
25-31: Snapshot + constructor switch to latest looks good.JSON casing and CID default align with existing patterns.
44-47: Deserialize to latest.Consistent with the PR’s goal; no functional concerns.
src/shim/actors/builtin/power/mod.rs (1)
39-78: Latest power state factory LGTM.Fields map 1:1 to v17; matches usage elsewhere.
src/lotus_json/actors/states/market_state.rs (2)
143-157: Snapshot switched to latest: OK.Matches shim default_latest_version signature and field order.
171-187: Coercing missing optional CIDs to default Cid is acceptable. Downstream code treats these fields as concrete Cid roots (see src/shim/actors/builtin/market/mod.rs, src/shim/actors/builtin/market/ext/state.rs, src/state_migration/nv22/market.rs), so normalizing absent Lotus JSON to a default/empty CID via unwrap_or_default() matches existing expectations — keep as-is and add a short comment in lotus_json/actors/states/market_state.rs documenting this normalization.src/shim/actors/builtin/market/mod.rs (1)
81-112: LGTM: default_latest_version constructor follows the new patternConstructor correctly targets V17 and mirrors the field layout.
src/shim/actors/builtin/cron/mod.rs (2)
31-45: LGTM: latest-version constructors for cron stateBoth helpers are consistent with the actors’ pattern and simplify callers.
62-80: LGTM: Entry helpersConstructors and into_latest_inner are straightforward and correct.
src/shim/actors/builtin/miner/mod.rs (1)
47-82: LGTM: default_latest_version for minerSignature and field mapping to V17 look correct.
src/lotus_json/actors/states/cron_state.rs (1)
16-48: LGTM: macro-based multi-version HasLotusJsonSnapshot and into_lotus_json coverage across V8–V17 look good.
src/lotus_json/actors/states/multisig_state.rs (3)
50-67: LGTM: updated snapshot to latest and PascalCase keysMatches the JSON schema and new constructor.
70-81: LGTM: into_lotus_json cleanupsIterating signers avoids move; cloning initial_balance is appropriate here.
86-96: LGTM: latest-only deserialization pathfrom_lotus_json uses the default_latest_version constructor consistently.
src/lotus_json/filter_estimate.rs (1)
19-58: Well-structured macro implementation for multi-version supportThe macro approach effectively reduces code duplication across versions v14-v17, implementing the consistent HasLotusJson trait for all FilterEstimate versions. This aligns perfectly with the PR's objective to eliminate code duplication.
src/lotus_json/actors/states/power_states.rs (2)
103-119: Improved macro naming enhances readabilityThe rename from
v15_to_v17_power_state_fieldstopower_state_fields_version_15_onwardsandv8_to_v14_power_state_fieldstopower_state_fields_version_8_to_14makes the versioning semantics clearer and more maintainable.
187-211: Consistent use of default_latest_version aligns with PR objectivesThe refactor to always construct the latest version (V17) via
State::default_latest_versionstandardizes deserialization across the codebase. The explicit comment "Always return the latest version when deserializing" improves code clarity.src/lotus_json/actors/states/reward_state.rs (2)
69-83: JSON key naming now consistent with PascalCase conventionThe migration to PascalCase for all JSON keys (CumsumBaseline, EffectiveNetworkTime, etc.) improves consistency with the broader codebase conventions.
126-144: Consistent latest-version construction patternThe implementation correctly uses
State::default_latest_versionwith the v17 FilterEstimate type, maintaining consistency with the broader refactoring pattern across all actor states.src/lotus_json/actors/states/evm_state.rs (2)
22-26: Well-documented version-aware transient_data handlingThe clear documentation explaining the three states of transient_data (None for v10-v15, Some(None) for null in v16+, Some(Some(data)) for actual data in v16+) helps maintainers understand the versioning semantics.
33-172: Effective macro design eliminates version-specific code duplicationThe dual-macro approach (
no_transient_dataandwith_transient_data) elegantly handles the structural differences between versions while maintaining type safety. This significantly reduces maintenance burden as requested in issue #6050.src/lotus_json/actors/states/miner_state.rs (3)
71-79: Clean enum design for VestingFundsValueThe new
VestingFundsValueenum withEmbeddedandCidvariants cleanly represents the different storage mechanisms for vesting funds across versions, replacing the previous version-specific approach.
104-130: Smart macro composition for handling version differencesThe three-tier macro structure (
common_miner_state_fields,embedded_vesting_funds_handler,cid_vesting_funds_handler) provides excellent separation of concerns between common fields and version-specific vesting funds handling.
191-232: Robust deserialization handling for all vesting funds scenariosThe from_lotus_json implementation correctly handles all three cases of vesting funds (Embedded data, Cid reference, and None) while consistently constructing the latest version via
State::default_latest_version.src/lotus_json/transient_data.rs (3)
54-72: into_lotus_json/from_lotus_json mapping looks correctField-by-field mapping is straightforward and version-agnostic. Once the snapshot fix lands, these should round-trip cleanly.
Please run the existing snapshot tests for v16 and v17 after applying the snapshot corrections to confirm round-trip parity.
75-105: Lifespan snapshots are correct; types align with schemaNumeric Origin/Nonce and direct struct mapping look good.
111-111: Macro invocation covers v16 and v17; future-proofingLGTM. When V18 lands, it’s a single-line addition here.
src/lotus_json/actors/states/vesting_funds.rs (3)
88-113: Snapshot JSON and expected Self are coherentNull case and populated case both look correct; CID sample is valid-looking and should parse via lotus_json.
143-144: Macro invocation for v16, v17 is cleanThis removes duplication without changing behavior. Nice.
84-137: VestingFunds HasLotusJson impl: confirm tuple‑struct shape & BigInt importNull handling and mapping look correct — two small asks:
- Verify fil_actor_miner_state::v{16,17}::VestingFunds is a tuple struct wrapping Option (so self.0 is valid) in both versions.
- Align the BigInt import with adjacent impls for consistency (minor style).
If helpful, run locally:
rg -nP -C3 '\bstruct\s+VestingFunds\s*\(\s*Option<\s*VestingFundsInner\s*>\s*\);' rg -nP -C3 '\bstruct\s+VestingFundsInner\b'
f3da276 to
3acff56
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/lotus_json/actors/states/market_state.rs (1)
56-62: Fix Lotus JSON field name and schema for PendingDealAllocationIDs and ProviderSectors.
- Field name should be “PendingDealAllocationIDs” (uppercase “IDs”) to match Lotus.
- schemars should reflect Option for both optional CIDs.
Apply:
- #[schemars(with = "LotusJson<Cid>")] - #[serde(with = "crate::lotus_json", skip_serializing_if = "Option::is_none")] - pub pending_deal_allocation_ids: Option<Cid>, + #[schemars(with = "LotusJson<Option<Cid>>")] + #[serde( + with = "crate::lotus_json", + rename = "PendingDealAllocationIDs", + skip_serializing_if = "Option::is_none" + )] + pub pending_deal_allocation_ids: Option<Cid>, - #[schemars(with = "LotusJson<Cid>")] - #[serde(with = "crate::lotus_json", skip_serializing_if = "Option::is_none")] - pub provider_sectors: Option<Cid>, + #[schemars(with = "LotusJson<Option<Cid>>")] + #[serde(with = "crate::lotus_json", skip_serializing_if = "Option::is_none")] + pub provider_sectors: Option<Cid>,Note: Without the rename, the serializer will emit “PendingDealAllocationIds”, which won’t match Lotus or your snapshot JSON.
src/lotus_json/actors/states/multisig_state.rs (1)
36-38: Schema/JSON field name mismatch for PendingTxnsSerde renames the field to "PendingTxns", but schemars will still emit "PendingTxs" due to PascalCase. Align the schema with runtime JSON.
- #[schemars(with = "LotusJson<Cid>")] - #[serde(with = "crate::lotus_json", rename = "PendingTxns")] + #[schemars(rename = "PendingTxns", with = "LotusJson<Cid>")] + #[serde(with = "crate::lotus_json", rename = "PendingTxns")] pub pending_txs: Cid,
♻️ Duplicate comments (1)
src/lotus_json/transient_data.rs (1)
37-43: Snapshot uses string numbers; should be numeric typesOrigin/Nonce must be numbers, not strings. This was flagged earlier; it appears to have regressed here.
- json! {{ - "TransientDataState": {"/":"baeaaaaa"}, - "TransientDataLifespan": { - "Origin": "2", - "Nonce": "3" - } - }}, + json!({ + "TransientDataState": {"/":"baeaaaaa"}, + "TransientDataLifespan": { + "Origin": 2, + "Nonce": 3 + } + }),Also applies to: 45-50
🧹 Nitpick comments (14)
src/shim/actors/builtin/reward/mod.rs (1)
58-85: Solid latest-version ctor; minimal polish.Consider a brief doc comment noting it constructs V17 to guide callers; otherwise LGTM.
src/shim/actors/builtin/multisig/mod.rs (1)
45-63: Prefer fvm_shared4::clock::ChainEpoch for consistencyConstructor mixes fvm_shared4 Address/TokenAmount with a v2 ChainEpoch — switch the two epoch params to fvm_shared4::clock::ChainEpoch to avoid a mixed-version API surface.
pub fn default_latest_version( signers: Vec<fvm_shared4::address::Address>, num_approvals_threshold: u64, next_tx_id: i64, initial_balance: fvm_shared4::econ::TokenAmount, - start_epoch: ChainEpoch, - unlock_duration: ChainEpoch, + start_epoch: fvm_shared4::clock::ChainEpoch, + unlock_duration: fvm_shared4::clock::ChainEpoch, pending_txs: cid::Cid, ) -> Self {src/lotus_json/actors/states/power_states.rs (1)
8-9: Tiny consistency nit: prefer a single BigInt import style across modules.This file uses
use num::BigInt;while reward_state.rs usesuse num_bigint::BigInt;. Consider standardizing (e.g.,num_bigint::BigInt) for consistency in lotus_json modules.src/lotus_json/actors/states/payment_channel_state.rs (1)
45-47: Remove redundant import inside snapshots().
use crate::shim::address::Address;here duplicates the top‑level import.Apply:
- use crate::shim::address::Address;src/lotus_json/actors/states/reward_state.rs (1)
9-9: BigInt import style consistency (optional).Align BigInt import style with power_states.rs to reduce churn across files (pick one:
num_bigint::BigIntornum::BigInt).src/lotus_json/actors/states/multisig_state.rs (2)
76-78: Avoid unnecessary clone of initial_balanceCloning here is avoidable; prefer moving the field via pattern destructuring in the match arm.
Example approach:
State::$version(fil_actor_multisig_state::$version::State { signers, num_approvals_threshold, next_tx_id, initial_balance, start_epoch, unlock_duration, pending_txs, }) => MultisigStateLotusJson { signers: signers.into_iter().map(Into::into).collect(), num_approvals_threshold, next_tx_id: next_tx_id.0, initial_balance: initial_balance.into(), start_epoch, unlock_duration, pending_txs, },
93-99: Prefer into_iter() to avoid copies when rebuilding signersYou own lotus_json; using into_iter avoids copies/extra derefs.
- lotus_json - .signers - .iter() - .map(|addr| (*addr).into()) - .collect(), + lotus_json + .signers + .into_iter() + .map(Into::into) + .collect(),src/lotus_json/actors/states/evm_state.rs (1)
153-157: Simplify Option<Option> handling with flatten()Minor readability win.
- let transient_data = lotus_json.transient_data - .and_then(|outer_option| outer_option) + let transient_data = lotus_json.transient_data + .flatten() .map(|transient_data_lotus| { fil_actor_evm_state::[<v $version>]::TransientData::from_lotus_json(transient_data_lotus) });src/lotus_json/transient_data.rs (1)
79-85: Consistent JSON macro style and delimitersPrefer
json!({ ... })consistently across the codebase (as used elsewhere in this PR).- json! {{ + json!({ "Origin": 1, "Nonce": 2 - }}, + }),src/lotus_json/actors/states/datacap_state.rs (1)
28-45: Snapshot JSON keys should be PascalCase to match #[serde(rename_all = "PascalCase")]The snapshot currently uses "governor"/"token". It should be "Governor"/"Token" (and Token’s fields likely PascalCase too) to reflect the on‑wire Lotus JSON shape.
Apply this diff:
- json!({ - "governor": "t00", - "token": { - "supply": "0", - "balances": {"/":"baeaaaaa"}, - "allowances": {"/":"baeaaaaa"}, - "hamt_bit_width": 0 - } - }), + json!({ + "Governor": "t00", + "Token": { + "Supply": "0", + "Balances": {"/":"baeaaaaa"}, + "Allowances": {"/":"baeaaaaa"}, + "HamtBitWidth": 0 + } + }),src/shim/actors/builtin/cron/mod.rs (2)
4-5: Remove unused import
use crate::shim;appears unused in this module.Apply this diff:
-use crate::lotus_json::HasLotusJson; -use crate::shim; +use crate::lotus_json::HasLotusJson;
62-80: Minor: avoid JSON round‑trip inside into_latest_inner
into_latest_innerconverts to Lotus JSON and back to V17. Pattern‑matching directly would avoid the extra conversion, but it’s non‑blocking.src/lotus_json/actors/states/cron_state.rs (1)
64-76: Simplify from_lotus_json: entries are already converted to latest by serdeSerde uses
HasLotusJsonforEntry, solotus_json.entriesare already V17 variants. You can skip the re‑mapping.Apply this diff:
- let entries = lotus_json - .entries - .into_iter() - .map(|entry| { - let lotus_entry = entry.into_lotus_json(); - Entry::default_latest_version(lotus_entry.receiver.into(), lotus_entry.method_num) - }) - .collect(); - - State::default_latest_version_from_entries(entries) + State::default_latest_version_from_entries(lotus_json.entries)src/shim/actors/builtin/miner/mod.rs (1)
47-82: Latest‑version constructor added — LGTM; consider a builder to reduce call‑site errorsThe long param list mirrors v17, but it’s easy to misorder at call sites. Consider a small builder or a typed struct for construction in a follow‑up.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (32)
src/lotus_json/actors/states/account_state.rs(3 hunks)src/lotus_json/actors/states/cron_state.rs(2 hunks)src/lotus_json/actors/states/datacap_state.rs(2 hunks)src/lotus_json/actors/states/evm_state.rs(2 hunks)src/lotus_json/actors/states/init_state.rs(2 hunks)src/lotus_json/actors/states/market_state.rs(3 hunks)src/lotus_json/actors/states/miner_state.rs(3 hunks)src/lotus_json/actors/states/multisig_state.rs(1 hunks)src/lotus_json/actors/states/payment_channel_state.rs(2 hunks)src/lotus_json/actors/states/power_states.rs(2 hunks)src/lotus_json/actors/states/reward_state.rs(2 hunks)src/lotus_json/actors/states/system_state.rs(1 hunks)src/lotus_json/actors/states/verified_reg_state.rs(1 hunks)src/lotus_json/actors/states/vesting_funds.rs(1 hunks)src/lotus_json/bytecode_hash.rs(1 hunks)src/lotus_json/entry.rs(1 hunks)src/lotus_json/filter_estimate.rs(2 hunks)src/lotus_json/tombstone.rs(1 hunks)src/lotus_json/transient_data.rs(2 hunks)src/shim/actors/builtin/account/mod.rs(3 hunks)src/shim/actors/builtin/cron/mod.rs(3 hunks)src/shim/actors/builtin/datacap/mod.rs(2 hunks)src/shim/actors/builtin/evm/mod.rs(3 hunks)src/shim/actors/builtin/init/mod.rs(1 hunks)src/shim/actors/builtin/market/mod.rs(1 hunks)src/shim/actors/builtin/miner/mod.rs(2 hunks)src/shim/actors/builtin/multisig/mod.rs(1 hunks)src/shim/actors/builtin/paymentchannel/mod.rs(2 hunks)src/shim/actors/builtin/power/mod.rs(1 hunks)src/shim/actors/builtin/reward/mod.rs(1 hunks)src/shim/actors/builtin/system/mod.rs(1 hunks)src/shim/actors/builtin/verifreg/mod.rs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (12)
- src/shim/actors/builtin/datacap/mod.rs
- src/shim/actors/builtin/system/mod.rs
- src/shim/actors/builtin/init/mod.rs
- src/shim/actors/builtin/market/mod.rs
- src/lotus_json/actors/states/account_state.rs
- src/shim/actors/builtin/verifreg/mod.rs
- src/lotus_json/bytecode_hash.rs
- src/shim/actors/builtin/paymentchannel/mod.rs
- src/shim/actors/builtin/evm/mod.rs
- src/lotus_json/filter_estimate.rs
- src/lotus_json/actors/states/vesting_funds.rs
- src/shim/actors/builtin/power/mod.rs
🧰 Additional context used
🧠 Learnings (4)
📓 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.937Z
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.
Learnt from: akaladarshi
PR: ChainSafe/forest#6000
File: src/lotus_json/actors/params/init_params.rs:34-47
Timestamp: 2025-09-10T12:07:10.578Z
Learning: In Lotus JSON for init actor Exec4Params, the InitExec4ParamsLotusJson struct uses "sub_address" as the field name which serializes to "SubAddress" in JSON via PascalCase conversion. This correctly maps to the internal fil_actor_init_state subaddress field through the conversion methods, maintaining Lotus API compatibility.
📚 Learning: 2025-09-02T10:05:34.350Z
Learnt from: akaladarshi
PR: ChainSafe/forest#5923
File: src/rpc/registry/actors/miner.rs:221-223
Timestamp: 2025-09-02T10:05:34.350Z
Learning: For miner actor ChangeOwnerAddress and ChangeOwnerAddressExported methods: versions 8-10 use bare Address as parameter type, while versions 11+ use ChangeOwnerAddressParams. This reflects the evolution of the Filecoin miner actor parameter structures across versions.
Applied to files:
src/shim/actors/builtin/account/mod.rs
📚 Learning: 2025-08-18T12:25:29.183Z
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.
Applied to files:
src/lotus_json/tombstone.rssrc/lotus_json/actors/states/verified_reg_state.rs
📚 Learning: 2025-08-11T14:00:47.060Z
Learnt from: hanabi1224
PR: ChainSafe/forest#5886
File: src/daemon/db_util.rs:236-259
Timestamp: 2025-08-11T14:00:47.060Z
Learning: In Forest's snapshot import (`src/daemon/db_util.rs`), when F3 CID is present in snapshot metadata but the actual F3 data is missing, this should cause a hard error as it indicates snapshot corruption. Only errors during the F3 import process itself (after data is successfully retrieved) should be non-fatal and logged.
Applied to files:
src/lotus_json/transient_data.rs
🧬 Code graph analysis (19)
src/shim/actors/builtin/multisig/mod.rs (3)
src/shim/actors/builtin/account/mod.rs (1)
default_latest_version(43-45)src/shim/actors/builtin/cron/mod.rs (2)
default_latest_version(42-44)default_latest_version(63-71)src/shim/actors/builtin/paymentchannel/mod.rs (1)
default_latest_version(22-38)
src/lotus_json/actors/states/cron_state.rs (2)
src/shim/actors/builtin/cron/mod.rs (3)
default_latest_version(42-44)default_latest_version(63-71)default_latest_version_from_entries(32-40)src/lotus_json/entry.rs (2)
into_lotus_json(31-46)from_lotus_json(48-50)
src/shim/actors/builtin/reward/mod.rs (4)
src/shim/actors/builtin/account/mod.rs (1)
default_latest_version(43-45)src/shim/actors/builtin/market/mod.rs (1)
default_latest_version(82-112)src/shim/actors/builtin/miner/mod.rs (1)
default_latest_version(48-82)src/shim/actors/builtin/power/mod.rs (1)
default_latest_version(40-78)
src/lotus_json/actors/states/init_state.rs (2)
src/lotus_json/tombstone.rs (3)
snapshots(20-28)into_lotus_json(30-45)from_lotus_json(48-50)src/shim/actors/builtin/init/mod.rs (1)
default_latest_version(31-41)
src/lotus_json/actors/states/reward_state.rs (2)
src/lotus_json/tombstone.rs (3)
snapshots(20-28)into_lotus_json(30-45)from_lotus_json(48-50)src/shim/actors/builtin/reward/mod.rs (1)
default_latest_version(59-85)
src/shim/actors/builtin/account/mod.rs (4)
src/shim/actors/builtin/cron/mod.rs (2)
default_latest_version(42-44)default_latest_version(63-71)src/shim/actors/builtin/datacap/mod.rs (1)
default_latest_version(36-41)src/shim/actors/builtin/multisig/mod.rs (1)
default_latest_version(45-63)src/shim/actors/builtin/paymentchannel/mod.rs (1)
default_latest_version(22-38)
src/lotus_json/actors/states/system_state.rs (2)
src/lotus_json/tombstone.rs (3)
snapshots(20-28)into_lotus_json(30-45)from_lotus_json(48-50)src/shim/actors/builtin/system/mod.rs (1)
default_latest_version(30-32)
src/lotus_json/tombstone.rs (1)
src/shim/actors/builtin/evm/mod.rs (3)
default_latest_version(28-44)default_latest_version(87-89)nonce(46-57)
src/lotus_json/entry.rs (4)
src/shim/actors/builtin/cron/mod.rs (2)
default_latest_version(42-44)default_latest_version(63-71)src/shim/address.rs (1)
new_id(142-144)src/lotus_json/tombstone.rs (2)
into_lotus_json(30-45)from_lotus_json(48-50)src/lotus_json/actors/states/cron_state.rs (2)
into_lotus_json(48-62)from_lotus_json(65-76)
src/lotus_json/actors/states/power_states.rs (4)
src/lotus_json/tombstone.rs (3)
snapshots(20-28)into_lotus_json(30-45)from_lotus_json(48-50)src/lotus_json/actors/states/market_state.rs (3)
snapshots(120-153)into_lotus_json(155-177)from_lotus_json(180-196)src/lotus_json/actors/states/reward_state.rs (3)
snapshots(64-99)into_lotus_json(101-128)from_lotus_json(131-148)src/shim/actors/builtin/power/mod.rs (1)
default_latest_version(40-78)
src/lotus_json/actors/states/datacap_state.rs (2)
src/lotus_json/tombstone.rs (3)
snapshots(20-28)into_lotus_json(30-45)from_lotus_json(48-50)src/shim/actors/builtin/datacap/mod.rs (1)
default_latest_version(36-41)
src/lotus_json/actors/states/multisig_state.rs (1)
src/shim/actors/builtin/multisig/mod.rs (1)
default_latest_version(45-63)
src/lotus_json/actors/states/verified_reg_state.rs (3)
src/lotus_json/tombstone.rs (3)
snapshots(20-28)into_lotus_json(30-45)from_lotus_json(48-50)src/lotus_json/actors/states/market_state.rs (3)
snapshots(120-153)into_lotus_json(155-177)from_lotus_json(180-196)src/shim/actors/builtin/verifreg/mod.rs (1)
default_latest_version(48-64)
src/lotus_json/actors/states/miner_state.rs (1)
src/shim/actors/builtin/miner/mod.rs (1)
default_latest_version(48-82)
src/lotus_json/actors/states/evm_state.rs (2)
src/lotus_json/tombstone.rs (3)
snapshots(20-28)into_lotus_json(30-45)from_lotus_json(48-50)src/shim/actors/builtin/evm/mod.rs (3)
nonce(46-57)default_latest_version(28-44)default_latest_version(87-89)
src/lotus_json/actors/states/payment_channel_state.rs (2)
src/shim/actors/builtin/paymentchannel/mod.rs (1)
default_latest_version(22-38)src/shim/address.rs (1)
new_id(142-144)
src/shim/actors/builtin/miner/mod.rs (4)
src/shim/actors/builtin/account/mod.rs (1)
default_latest_version(43-45)src/shim/actors/builtin/cron/mod.rs (2)
default_latest_version(42-44)default_latest_version(63-71)src/shim/actors/builtin/market/mod.rs (1)
default_latest_version(82-112)src/shim/actors/builtin/multisig/mod.rs (1)
default_latest_version(45-63)
src/lotus_json/actors/states/market_state.rs (2)
src/lotus_json/actors/states/miner_state.rs (1)
into_lotus_json(174-195)src/shim/actors/builtin/market/mod.rs (1)
default_latest_version(82-112)
src/lotus_json/transient_data.rs (2)
src/lotus_json/actors/states/evm_state.rs (3)
snapshots(178-190)into_lotus_json(192-204)from_lotus_json(207-217)src/lotus_json/mod.rs (17)
snapshots(147-147)snapshots(566-568)snapshots(580-582)snapshots(597-599)snapshots(621-623)serde_json(301-301)serde_json(325-325)into_lotus_json(148-148)into_lotus_json(569-571)into_lotus_json(583-585)into_lotus_json(600-606)into_lotus_json(624-631)from_lotus_json(149-149)from_lotus_json(572-574)from_lotus_json(586-591)from_lotus_json(607-613)from_lotus_json(632-639)
⏰ 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). (7)
- 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: cargo-publish-dry-run
- GitHub Check: Build MacOS
🔇 Additional comments (32)
src/lotus_json/actors/states/init_state.rs (1)
20-61: Good unification; version coverage looks complete.
- Macro covers V0 and V8–V17; nice.
- Deserialize-to-latest aligns with project direction.
LGTM.src/lotus_json/actors/states/system_state.rs (1)
17-52: Confirm no consumers depend on preserving State variant on deserialize→serialize (always‑latest → V17)from_lotus_json now always materializes V17 (src/lotus_json/actors/states/system_state.rs:17–52). My ripgrep check returned "No files were searched" so verification is inconclusive — run
rg -n --hidden -S 'State::V1[0-6]|match\s+State::V1[0-6]|"BuiltinActors"' || git grep -n 'State::V1'to locate callers; if any rely on preserving the original variant, restore per‑variant deserialization or add an explicit migration.src/lotus_json/tombstone.rs (1)
12-50: Rename fix + latest-version deserialization LGTM — no stale "Orign" found.
ripgrep returned no matches for "Orign" (case-insensitive); only correct "Origin" usages remain.src/lotus_json/actors/states/verified_reg_state.rs (3)
41-67: V8 vs V9+ field mapping looks correct.
- V8 exposes VerifiedClients only; V9+ exposes Allocations/NextAllocationId/Claims only. The split here mirrors Lotus.
94-115: Macro dispatch for into_lotus_json is clean and future‑proof.Matches the pattern used in other state modules; easy to extend with V18+.
117-127: Verify defaults used when upgrading older Lotus JSON to latest verifreg state.You default missing V9+ CIDs and NextAllocationId to Cid::default()/0. Repo search shows canonical empty CIDs are used elsewhere (e.g. fil_actors_shared::v10::runtime::EMPTY_ARR_CID). Confirm downstream verified-reg consumers accept all-zero CIDs and 0 as “empty” sentinels; if they require canonical empty AMT/HAMT CIDs, swap the defaults. Location: src/lotus_json/actors/states/verified_reg_state.rs — from_lotus_json -> State::default_latest_version(...).
src/lotus_json/actors/states/market_state.rs (2)
65-114: Macro factoring is solid.The v8/v9–v12/v13+ split lines up with actor field evolution; good reuse via common_market_state_fields!.
179-196: Latest‑version deserialization mapping LGTM.Field ordering/types align with shim::market::State::default_latest_version. Optional CIDs defaulted sensibly.
src/lotus_json/actors/states/power_states.rs (2)
171-192: Version dispatch for into_lotus_json looks correct.
- v8–v14 vs v15+ split matches ramp fields addition.
- v16 FilterEstimate used only for JSON surface is appropriate.
194-218: from_lotus_json upgrade path LGTM.
- Correctly converts FilterEstimate (v16 JSON) to v17 type.
- Reasonable defaults for ramp fields with unwrap_or(0).
src/lotus_json/actors/states/payment_channel_state.rs (2)
66-85: into_lotus_json mapping is correct.All fields are passed through with the expected conversions.
87-98: Latest‑version deserialization path LGTM.Arguments align with shim::paymentchannel::State::default_latest_version.
src/lotus_json/actors/states/reward_state.rs (2)
101-128: Version dispatch and FilterEstimate conversion look good.Using v16 FilterEstimate for JSON while holding v17 internally is the right boundary.
130-148: Latest‑version deserialization mapping LGTM.Field ordering/types match shim::reward::State::default_latest_version; TokenAmount conversions are correct.
src/lotus_json/actors/states/multisig_state.rs (1)
90-105: Confirm latest-version upcast is safe for all historic shapesAlways deserializing to the latest version is fine if all older shapes are fully representable in V17 semantics. Please confirm across real chain snapshots.
Would you like a quick scan script to load a sample of historic multisig states from your fixture corpus and round-trip them through from_lotus_json → into_lotus_json to ensure no lossy conversions?
src/lotus_json/actors/states/evm_state.rs (1)
206-217: Double-check cross-version tombstone/transient_data mappingLatest-version rehydration via v17’s
from_lotus_jsonis good. Please confirm v10–v15 inputs withouttransient_datayieldNone(notSome(None)) on the v17 side after the two-step conversion.I can provide a small property test to assert: for all v10..v15 states with/without tombstone,
State::from_lotus_json→into_lotus_jsonround-trips toTransientData: null.src/shim/actors/builtin/account/mod.rs (2)
28-41: Return type change: audit downstream call sites
pubkey_addressnow returnsfvm_shared2::address::Address. This is a public API change; ensure all call sites expect v2 and no implicit conversions are relied on.If helpful, I can generate a repo scan to list all usages and flag any that feed v3/v4 call paths.
43-45: LGTM: default_latest_version constructorConstructor aligns with the pattern used across other actors and keeps latest-version creation simple.
src/lotus_json/entry.rs (3)
27-28: LGTM: snapshot aligns with PascalCase and latest-version ctorSnapshot JSON and expected value match the new default_latest_version path and casing rules.
31-46: Nice macro cleanupThe convert_entry macro removes boilerplate across versions without altering behavior.
48-50: Deserialization to latest is consistent with PR directionfrom_lotus_json constructing latest-version Entry is consistent and clear.
src/lotus_json/actors/states/datacap_state.rs (2)
49-66: Macro-based into_lotus_json looks goodCovers V9–V17 and cleanly maps fields. Nothing blocking.
68-71: Behavior change: deserialization always yields latest versionReturning State::default_latest_version(...) is desirable for upgrades but can surprise callers expecting historical variants. Please confirm downstream code/tests don’t rely on a specific enum variant.
src/shim/actors/builtin/cron/mod.rs (1)
31-40: Constructor to latest version from mixed entries — LGTMThe conversion through
into_latest_inner()is clear and contained.src/lotus_json/actors/states/cron_state.rs (2)
35-45: Snapshot updated to latest entries — LGTMUses V17 entries via the new constructor. Matches the new API.
49-61: Macro for into_lotus_json — LGTMRemoves duplication across V8–V17. Clean.
src/lotus_json/actors/states/miner_state.rs (6)
71-79: Unified VestingFundsValue (Embedded|Cid) — LGTMThe untagged enum cleanly represents both eras; schema hints look fine.
104-120: V16+ embedded handler — confirm tail shape
tail: inner.tailis passed through. Please confirmVestingFundsLotusJson.tailmatches the inner type (e.g., Cid vs vector) for v16/v17.
123-129: CID handler — LGTMOlder versions map to a CID as expected.
134-172: Snapshot — LGTMMatches the latest‑version constructor and the new VestingFunds representation.
197-237: CID case drops vesting funds data (set to None) — confirm acceptabilityWhen Lotus JSON provides a CID (V8–V15), deserialization to V17 discards the schedule. If acceptable by design, add a short comment; else consider fetching/expanding via store in a higher layer.
81-101: Cannot verify TokenAmount .into() conversions — manual check requiredrg returned no matches for From/Into bridges; I cannot confirm .into() is implemented for every TokenAmount variant used by common_miner_state_fields! (src/lotus_json/actors/states/miner_state.rs:81-101). Ensure From/Into impls exist for all miner-state TokenAmount types, or replace .into() with explicit converters (see shim/miner for an example).
| fn snapshots() -> Vec<(serde_json::Value, Self)> { | ||
| vec
LesnyRumcajs
left a comment
There was a problem hiding this comment.
In general, anything blocking usage of the Default trait instead of the default_latest_version function?
Actually No, we can't use the Idea here was instead of hardcoding V16 or V17 in |
Summary of changes
Changes introduced in this pull request:
V18to the macro.default_latest_versionmethod for all actors, so the actor states can return the current latest actor version.V16use latest default version for actor states while deserialising the lotus json from the data.Reference issue to close (if applicable)
Closes: #6050
Other information and links
Change checklist
Summary by CodeRabbit
New Features
Refactor