fix(test): JSON snapshot tests for actor states#6381
Conversation
WalkthroughThis PR systematically adds equality trait derivations (Eq, PartialEq) to Lotus JSON state structs across multiple actor modules, refactors HasLotusJson implementations into per-version modules with snapshot testing, adjusts JSON field naming conventions and serde attributes, and introduces a new Cid variant in VestingFundsValue alongside builtin state equality trait additions. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~40 minutes Possibly related issues
Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
Comment |
hanabi1224
requested review from
akaladarshi and
sudo-shashank
and removed request for
a team
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Fix all issues with AI Agents 🤖
In @src/lotus_json/actors/states/cron_state.rs:
- Line 8: CronStateLotusJson currently derives PartialEq but not Eq
(inconsistency with other state structs) because its field type Entry lacks Eq;
after updating Entry in cron/mod.rs to derive Eq, update the CronStateLotusJson
derive list to include Eq (i.e., change #[derive(Debug, PartialEq, Serialize,
Deserialize, JsonSchema)] to include Eq) so the struct matches
InitStateLotusJson, TombstoneLotusJson, and PaymentChannelStateLotusJson.
In @src/lotus_json/actors/states/evm_state.rs:
- Around line 41-44: The test functions named snapshots that call
crate::lotus_json::assert_all_snapshots::<T>() need a #[cfg(test)] attribute so
they are compiled only in test builds; add #[cfg(test)] above each snapshots fn
in both macro arms (no_transient_data and with_transient_data) — apply the same
change to the other occurrence (the second snapshots function) to avoid
release-mode compilation errors when assert_all_snapshots is test-only.
In @src/lotus_json/actors/states/market_state.rs:
- Around line 134-135: The JSON field for pending deal allocation IDs must
preserve uppercase "IDs": add an explicit serde rename for the struct field
pending_deal_allocation_ids (the field that currently serializes to
"PendingDealAllocationIds") by annotating it with rename =
"PendingDealAllocationIDs" (same style as the NextID explicit rename used for
next_id) so the serialized/deserialized key matches the Lotus API exactly.
In @src/shim/actors/builtin/cron/mod.rs:
- Line 47: The Entry enum in src/shim/actors/builtin/cron/mod.rs derives
PartialEq but not Eq which prevents CronStateLotusJson (which contains
Vec<Entry>) from deriving Eq; update the derive on the Entry enum (the
#[derive(...)] line for Entry) to include Eq alongside Clone, PartialEq,
Serialize, Debug so Entry implements Eq and allow CronStateLotusJson in
cron_state.rs to derive Eq as intended.
🧹 Nitpick comments (1)
src/lotus_json/actors/states/verified_reg_state.rs (1)
41-42: Consider addingdefaultattribute for consistency.The
next_allocation_idfield is missing thedefaultserde attribute, unlike the otherOptionfields (verified_clients,allocations,claims) which now have it. This inconsistency could cause deserialization issues if the field is absent in JSON from older versions.🔎 Proposed fix
- #[serde(skip_serializing_if = "Option::is_none")] + #[serde(default, skip_serializing_if = "Option::is_none")] pub next_allocation_id: Option<u64>, // not available in verified reg state version 8
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (17)
src/lotus_json/actors/states/account_state.rssrc/lotus_json/actors/states/cron_state.rssrc/lotus_json/actors/states/datacap_state.rssrc/lotus_json/actors/states/evm_state.rssrc/lotus_json/actors/states/init_state.rssrc/lotus_json/actors/states/market_state.rssrc/lotus_json/actors/states/miner_state.rssrc/lotus_json/actors/states/multisig_state.rssrc/lotus_json/actors/states/payment_channel_state.rssrc/lotus_json/actors/states/power_states.rssrc/lotus_json/actors/states/reward_state.rssrc/lotus_json/actors/states/system_state.rssrc/lotus_json/actors/states/verified_reg_state.rssrc/lotus_json/actors/states/vesting_funds.rssrc/lotus_json/tombstone.rssrc/lotus_json/transient_data.rssrc/shim/actors/builtin/cron/mod.rs
🧰 Additional context used
🧠 Learnings (8)
📓 Common learnings
Learnt from: akaladarshi
Repo: ChainSafe/forest PR: 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.
Learnt from: akaladarshi
Repo: ChainSafe/forest PR: 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: hanabi1224
Repo: ChainSafe/forest PR: 5930
File: build.rs:64-77
Timestamp: 2025-08-13T09:43:20.301Z
Learning: hanabi1224 prefers hard compile-time errors in build scripts rather than runtime safeguards or collision detection, believing it's better to fail fast and fix root causes of issues like malformed snapshot names.
Learnt from: hanabi1224
Repo: ChainSafe/forest PR: 6057
File: src/cli/subcommands/f3_cmd.rs:0-0
Timestamp: 2025-09-09T10:37:17.947Z
Learning: hanabi1224 prefers having default timeouts (like 10m for --no-progress-timeout) to prevent commands from hanging indefinitely, even when the timeout flag isn't explicitly provided by users. This fail-fast approach is preferred over requiring explicit flag usage.
📚 Learning: 2025-08-18T12:25:29.183Z
Learnt from: akaladarshi
Repo: ChainSafe/forest PR: 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/transient_data.rssrc/lotus_json/tombstone.rssrc/lotus_json/actors/states/multisig_state.rssrc/lotus_json/actors/states/power_states.rssrc/lotus_json/actors/states/cron_state.rssrc/lotus_json/actors/states/datacap_state.rssrc/lotus_json/actors/states/market_state.rssrc/lotus_json/actors/states/miner_state.rssrc/lotus_json/actors/states/init_state.rssrc/lotus_json/actors/states/account_state.rssrc/lotus_json/actors/states/system_state.rssrc/lotus_json/actors/states/verified_reg_state.rssrc/lotus_json/actors/states/vesting_funds.rssrc/lotus_json/actors/states/evm_state.rssrc/lotus_json/actors/states/reward_state.rs
📚 Learning: 2025-09-10T12:07:10.578Z
Learnt from: akaladarshi
Repo: ChainSafe/forest PR: 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.
Applied to files:
src/lotus_json/transient_data.rssrc/lotus_json/tombstone.rssrc/lotus_json/actors/states/datacap_state.rssrc/lotus_json/actors/states/init_state.rssrc/lotus_json/actors/states/account_state.rs
📚 Learning: 2025-09-02T08:46:04.937Z
Learnt from: akaladarshi
Repo: ChainSafe/forest PR: 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/tombstone.rssrc/lotus_json/actors/states/multisig_state.rssrc/lotus_json/actors/states/payment_channel_state.rs
📚 Learning: 2025-08-25T13:35:24.230Z
Learnt from: hanabi1224
Repo: ChainSafe/forest PR: 5969
File: src/tool/subcommands/snapshot_cmd.rs:412-412
Timestamp: 2025-08-25T13:35:24.230Z
Learning: In src/tool/subcommands/snapshot_cmd.rs, the +1 in `last_epoch = ts.epoch() - epochs as i64 + 1` fixes an off-by-1 bug where specifying --check-stateroots=N would validate N+1 epochs instead of N epochs, causing out-of-bounds errors when the snapshot contains only N recent state roots.
Applied to files:
src/lotus_json/tombstone.rs
📚 Learning: 2025-10-17T14:24:47.046Z
Learnt from: hanabi1224
Repo: ChainSafe/forest PR: 6167
File: src/tool/subcommands/state_compute_cmd.rs:89-91
Timestamp: 2025-10-17T14:24:47.046Z
Learning: In `src/tool/subcommands/state_compute_cmd.rs`, when using `ReadOpsTrackingStore` to generate minimal snapshots, `HEAD_KEY` should be written to `db.tracker` (not `db` itself) before calling `export_forest_car()`, because the export reads from the tracker MemoryDB which accumulates only the accessed data during computation.
Applied to files:
src/lotus_json/tombstone.rs
📚 Learning: 2025-09-11T16:03:14.328Z
Learnt from: akaladarshi
Repo: ChainSafe/forest PR: 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/lotus_json/actors/states/vesting_funds.rs
📚 Learning: 2025-09-02T08:44:08.346Z
Learnt from: akaladarshi
Repo: ChainSafe/forest PR: 5996
File: src/lotus_json/actor_states/methods/paych_params.rs:56-58
Timestamp: 2025-09-02T08:44:08.346Z
Learning: In Lotus JSON for payment channel SignedVoucher, the secret_pre_image field should use serde rename to "SecretHash" (not "SecretPreImage") for Lotus API compatibility, even though the internal struct field is named SecretPreimage. This matches the actual JSON format used by Lotus.
Applied to files:
src/lotus_json/actors/states/vesting_funds.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). (7)
- GitHub Check: Coverage
- GitHub Check: tests-release
- GitHub Check: Build forest binaries on Linux AMD64
- GitHub Check: cargo-publish-dry-run
- GitHub Check: Build Ubuntu
- GitHub Check: Build MacOS
- GitHub Check: All lint checks
🔇 Additional comments (36)
src/lotus_json/actors/states/init_state.rs (1)
8-8: LGTM! Equality traits enable snapshot testing.The addition of
EqandPartialEqderives toInitStateLotusJsonis appropriate for enabling snapshot-based equality assertions in tests. All fields (Cid,u64,String) correctly implementEq. Thetest_snapshots!macro invocation properly wires up the snapshot testing infrastructure.Also applies to: 62-62
src/lotus_json/tombstone.rs (1)
8-8: LGTM! Equality traits and snapshot testing properly added.The equality derives and snapshot test macro are correctly implemented. All fields (
ActorID,u64) implementEq, making the derives valid.Also applies to: 52-52
src/lotus_json/actors/states/payment_channel_state.rs (1)
11-11: LGTM! Equality traits and snapshot testing correctly implemented.The addition of
EqandPartialEqderives along with the snapshot testing macro follows the consistent pattern across this PR. All fields implementEq, making the derives valid.Also applies to: 99-99
src/lotus_json/actors/states/cron_state.rs (1)
78-78: LGTM! Snapshot testing macro properly added.The snapshot testing infrastructure is correctly wired up.
src/lotus_json/actors/states/system_state.rs (2)
8-8: LGTM: Equality traits enable snapshot testing.The addition of
EqandPartialEqderives is appropriate for this data structure and enables the snapshot testing added at line 53.
53-53: LGTM: Snapshot testing improves test coverage.The snapshot test macro invocation provides automated testing to catch unintended serialization changes.
src/lotus_json/actors/states/reward_state.rs (3)
11-11: LGTM: Equality traits enable snapshot testing.The addition of
EqandPartialEqderives is appropriate for this data structure.
150-150: LGTM: Snapshot testing improves test coverage.
73-74: No action needed. The field names"PositionEstimate"and"VelocityEstimate"are correct for theFilterEstimateserialization. The struct has#[serde(rename_all = "PascalCase")], which automatically converts the snake_case field names (position_estimateandvelocity_estimate) to PascalCase during JSON serialization, matching the snapshot data.src/lotus_json/actors/states/multisig_state.rs (3)
11-11: LGTM: Equality traits enable snapshot testing.The addition of
EqandPartialEqderives is appropriate for this data structure.
48-48: LGTM: Aligns with Lotus JSON conventions.The change to serialize an empty signers list as
null(instead of an empty array) aligns with Lotus behavior for empty collections. Based on learnings, this is the expected format for compatibility.
107-107: LGTM: Snapshot testing improves test coverage.src/lotus_json/actors/states/power_states.rs (3)
11-11: LGTM: Equality traits enable snapshot testing.The addition of
EqandPartialEqderives is appropriate for this data structure.
137-138: Note: Consistent field naming update across multiple state files.The field name changes from
"Position"/"Velocity"to"PositionEstimate"/"VelocityEstimate"are consistent with similar updates inreward_state.rs. This appears to be a systematic correction across the codebase. The verification requested inreward_state.rsapplies here as well.
220-220: LGTM: Snapshot testing improves test coverage.src/lotus_json/transient_data.rs (2)
9-9: LGTM: Equality traits enable comparisons.The addition of
EqandPartialEqderives is appropriate for this data structure.
19-19: LGTM: Equality traits enable comparisons.The addition of
EqandPartialEqderives is appropriate for this data structure.src/lotus_json/actors/states/market_state.rs (2)
10-10: LGTM! AddingEqandPartialEqderives enables equality comparisons for snapshot testing.
198-198: LGTM! Snapshot test macro invocation properly added.src/lotus_json/actors/states/datacap_state.rs (3)
9-9: LGTM! AddingEqandPartialEqderives for equality comparisons in snapshot testing.
29-35: LGTM! Snapshot JSON keys properly updated to PascalCase format, consistent with the#[serde(rename_all = "PascalCase")]attribute.
73-73: LGTM! Snapshot test macro invocation properly added.src/lotus_json/actors/states/vesting_funds.rs (3)
10-10: LGTM! AddingEqandPartialEqderives enables equality comparisons for snapshot testing.Also applies to: 21-21
113-115: Verify Tail CID format matches Lotus API.The
Tailfield now serializes as an object with a "/" key (i.e.,{"Tail": {"/": "<cid>"}}). This is the standard IPLD link format. Ensure this matches the expected Lotus API format for VestingFunds.
40-43: Missing#[cfg(test)]on test functions may cause compilation issues.The
snapshotstest functions at lines 40-43 and 95-98 are missing#[cfg(test)]attributes. Sincecrate::lotus_json::assert_all_snapshotsis likely only available in test builds, this could cause compilation errors in release mode.🔎 Proposed fix
mod [<impl_vesting_fund_ $version_num>] { use super::*; type T = fil_actor_miner_state::[<v $version_num>]::VestingFund; + #[cfg(test)] #[test] fn snapshots() { crate::lotus_json::assert_all_snapshots::<T>(); }Apply similar fix to the
impl_vesting_funds_module at line 95.Also applies to: 95-98
⛔ Skipped due to learnings
Learnt from: akaladarshi Repo: ChainSafe/forest PR: 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.Learnt from: elmattic Repo: ChainSafe/forest PR: 6128 File: src/ipld/util.rs:23-30 Timestamp: 2025-10-17T09:36:15.757Z Learning: Always run `cargo check` or `cargo build` to verify actual compilation errors in the Forest codebase before flagging them as issues. Do not rely solely on documentation or assumptions about trait implementations.Learnt from: elmattic Repo: ChainSafe/forest PR: 6128 File: src/ipld/util.rs:23-30 Timestamp: 2025-10-17T09:36:15.757Z Learning: Always run `cargo check` or `cargo build` to verify actual compilation errors in the Forest codebase before flagging them as issues. Do not rely solely on documentation or assumptions about trait implementations.src/lotus_json/actors/states/account_state.rs (1)
5-5: LGTM! Clean refactoring to importStatedirectly and add equality traits for snapshot testing.Also applies to: 8-8, 16-16, 41-41
src/lotus_json/actors/states/verified_reg_state.rs (3)
9-9: LGTM! AddingEqandPartialEqderives for snapshot testing.
26-30: LGTM! Addingdefaultattribute ensures proper deserialization when fields are absent.Also applies to: 34-38, 45-49
141-141: LGTM! Snapshot test macro invocation properly added.src/lotus_json/actors/states/miner_state.rs (4)
13-13: LGTM! AddingEqandPartialEqderives enables equality comparisons for snapshot testing.Also applies to: 72-72
74-79: LGTM! The newCidvariant inVestingFundsValueproperly handles V8-V15 states where VestingFunds is stored as a CID reference. The#[serde(untagged)]attribute allows seamless deserialization by tryingEmbeddedfirst, thenCid.
139-145: LGTM! Snapshot JSON values updated to use string representation"0"forTokenAmountfields, consistent with Lotus JSON serialization format.
239-239: LGTM! Snapshot test macro invocation properly added.src/lotus_json/actors/states/evm_state.rs (3)
9-9: LGTM! AddingEqandPartialEqderives for snapshot testing.
167-171: LGTM! Thetransient_datadeserialization correctly handles the nestedOption<Option<T>>structure usingand_thento flatten andmapto convert.
234-234: LGTM! Snapshot test macro invocation properly added.
| use serde::{Deserialize, Serialize}; | ||
|
|
||
| #[derive(Debug, Serialize, Deserialize, JsonSchema)] | ||
| #[derive(Debug, PartialEq, Serialize, Deserialize, JsonSchema)] |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Missing Eq derive creates inconsistency.
CronStateLotusJson derives only PartialEq, not Eq, which is inconsistent with other state structs in this PR (InitStateLotusJson, TombstoneLotusJson, PaymentChannelStateLotusJson all derive both). This happens because Entry (from cron/mod.rs) doesn't derive Eq. Once Entry is updated to derive Eq, this struct should also be updated for consistency.
🔎 Proposed fix (apply after fixing Entry)
-#[derive(Debug, PartialEq, Serialize, Deserialize, JsonSchema)]
+#[derive(Debug, Eq, PartialEq, Serialize, Deserialize, JsonSchema)]
#[serde(rename_all = "PascalCase")]
pub struct CronStateLotusJson {Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In @src/lotus_json/actors/states/cron_state.rs around line 8, CronStateLotusJson
currently derives PartialEq but not Eq (inconsistency with other state structs)
because its field type Entry lacks Eq; after updating Entry in cron/mod.rs to
derive Eq, update the CronStateLotusJson derive list to include Eq (i.e., change
#[derive(Debug, PartialEq, Serialize, Deserialize, JsonSchema)] to include Eq)
so the struct matches InitStateLotusJson, TombstoneLotusJson, and
PaymentChannelStateLotusJson.
There was a problem hiding this comment.
Eq is not applicable for external dependencies fil_actor_cron_state::v8::Entry
There was a problem hiding this comment.
@hanabi1224, thank you for the clarification! You're absolutely right—since Entry wraps external types from fil_actor_cron_state that don't implement Eq, it's not possible for CronStateLotusJson to derive Eq either. Deriving only PartialEq is the correct approach here given this constraint.
The inconsistency with other state structs that derive both Eq and PartialEq is justified and unavoidable in this case.
✏️ Learnings added
Learnt from: hanabi1224
Repo: ChainSafe/forest PR: 6381
File: src/lotus_json/actors/states/cron_state.rs:8-8
Timestamp: 2026-01-05T12:54:40.850Z
Learning: In the Forest codebase, CronStateLotusJson in src/lotus_json/actors/states/cron_state.rs derives only PartialEq (not Eq) because it contains Entry types that wrap external dependencies from fil_actor_cron_state, which don't implement Eq. This constraint from external dependencies prevents deriving Eq.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
Learnt from: akaladarshi
Repo: ChainSafe/forest PR: 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.
Learnt from: hanabi1224
Repo: ChainSafe/forest PR: 5930
File: build.rs:64-77
Timestamp: 2025-08-13T09:43:20.301Z
Learning: hanabi1224 prefers hard compile-time errors in build scripts rather than runtime safeguards or collision detection, believing it's better to fail fast and fix root causes of issues like malformed snapshot names.
Learnt from: hanabi1224
Repo: ChainSafe/forest PR: 6057
File: src/cli/subcommands/f3_cmd.rs:0-0
Timestamp: 2025-09-09T10:37:17.947Z
Learning: hanabi1224 prefers having default timeouts (like 10m for --no-progress-timeout) to prevent commands from hanging indefinitely, even when the timeout flag isn't explicitly provided by users. This fail-fast approach is preferred over requiring explicit flag usage.
| #[test] | ||
| fn snapshots() { | ||
| crate::lotus_json::assert_all_snapshots::<T>(); | ||
| } |
There was a problem hiding this comment.
Missing #[cfg(test)] on test functions may cause compilation issues.
Similar to the issue in vesting_funds.rs, the snapshots test functions inside the macro-generated modules are missing #[cfg(test)] attributes. This could cause compilation errors in release mode if crate::lotus_json::assert_all_snapshots is only available in test builds.
🔎 Proposed fix
mod [<impl_evm_state_lotus_json_ $version>] {
use super::*;
type T = fil_actor_evm_state::[<v $version>]::State;
+ #[cfg(test)]
#[test]
fn snapshots() {
crate::lotus_json::assert_all_snapshots::<T>();
}Apply to both macro arms (no_transient_data and with_transient_data).
Also applies to: 112-115
🤖 Prompt for AI Agents
In @src/lotus_json/actors/states/evm_state.rs around lines 41-44, The test
functions named snapshots that call
crate::lotus_json::assert_all_snapshots::<T>() need a #[cfg(test)] attribute so
they are compiled only in test builds; add #[cfg(test)] above each snapshots fn
in both macro arms (no_transient_data and with_transient_data) — apply the same
change to the other occurrence (the second snapshots function) to avoid
release-mode compilation errors when assert_all_snapshots is test-only.
There was a problem hiding this comment.
CI proves the code compiles fine
There was a problem hiding this comment.
@hanabi1224, you're absolutely right! I apologize for the incorrect suggestion. Functions marked with #[test] are already only compiled during test builds by Rust's built-in test framework, so the additional #[cfg(test)] attribute is unnecessary. The code compiles correctly as-is, and CI confirms this.
Thank you for the clarification!
✏️ Learnings added
Learnt from: hanabi1224
Repo: ChainSafe/forest PR: 6381
File: src/lotus_json/actors/states/evm_state.rs:41-44
Timestamp: 2026-01-05T12:56:13.802Z
Learning: In the Forest codebase, functions marked with #[test] do not require an additional #[cfg(test)] attribute, as the #[test] attribute already ensures they are only compiled during test builds. Do not suggest adding #[cfg(test)] to functions that already have #[test].
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
Learnt from: hanabi1224
Repo: ChainSafe/forest PR: 6381
File: src/lotus_json/actors/states/cron_state.rs:8-8
Timestamp: 2026-01-05T12:54:40.850Z
Learning: In the Forest codebase, CronStateLotusJson in src/lotus_json/actors/states/cron_state.rs derives only PartialEq (not Eq) because it contains Entry types that wrap external dependencies from fil_actor_cron_state, which don't implement Eq. This constraint from external dependencies prevents deriving Eq.
Learnt from: akaladarshi
Repo: ChainSafe/forest PR: 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.
Learnt from: hanabi1224
Repo: ChainSafe/forest PR: 6074
File: src/rpc/methods/chain.rs:55-56
Timestamp: 2025-09-17T11:32:44.185Z
Learning: In the Forest codebase, hanabi1224 prefers that CodeRabbit should not warn about potential compilation issues (such as Send bounds, async/await compatibility, etc.) since they are already guarded by CI tests. Focus should be on higher-level design and logic issues instead.
Learnt from: elmattic
Repo: ChainSafe/forest PR: 6128
File: src/ipld/util.rs:23-30
Timestamp: 2025-10-17T09:36:15.757Z
Learning: Always run `cargo check` or `cargo build` to verify actual compilation errors in the Forest codebase before flagging them as issues. Do not rely solely on documentation or assumptions about trait implementations.
Learnt from: elmattic
Repo: ChainSafe/forest PR: 6128
File: src/ipld/util.rs:23-30
Timestamp: 2025-10-17T09:36:15.757Z
Learning: Always run `cargo check` or `cargo build` to verify actual compilation errors in the Forest codebase before flagging them as issues. Do not rely solely on documentation or assumptions about trait implementations.
Learnt from: hanabi1224
Repo: ChainSafe/forest PR: 5946
File: src/rpc/methods/state.rs:1459-1463
Timestamp: 2025-08-19T11:25:56.710Z
Learning: hanabi1224 prefers that CodeRabbit should not warn about compilation errors in the Forest codebase since they are already guarded by CI tests. Focus should be on higher-level design and logic issues instead.
Learnt from: hanabi1224
Repo: ChainSafe/forest PR: 5930
File: build.rs:64-77
Timestamp: 2025-08-13T09:43:20.301Z
Learning: hanabi1224 prefers hard compile-time errors in build scripts rather than runtime safeguards or collision detection, believing it's better to fail fast and fix root causes of issues like malformed snapshot names.
Learnt from: hanabi1224
Repo: ChainSafe/forest PR: 6057
File: src/cli/subcommands/f3_cmd.rs:0-0
Timestamp: 2025-09-09T10:37:17.947Z
Learning: hanabi1224 prefers having default timeouts (like 10m for --no-progress-timeout) to prevent commands from hanging indefinitely, even when the timeout flag isn't explicitly provided by users. This fail-fast approach is preferred over requiring explicit flag usage.
| "PendingDealAllocationIds": {"/":"baeaaaaa"}, | ||
| "ProviderSectors": {"/":"baeaaaaa"} |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's look at the market_state.rs file to understand the full context
fd -t f "market_state.rs" | head -5Repository: ChainSafe/forest
Length of output: 104
🏁 Script executed:
# Read the market_state.rs file to see the struct definition and serde attributes
cat -n src/lotus_json/actors/states/market_state.rs | head -100Repository: ChainSafe/forest
Length of output: 4143
🏁 Script executed:
# Search for PendingDealAllocation references in the codebase
rg -n "PendingDealAllocation" --type rust -C 2Repository: ChainSafe/forest
Length of output: 534
🏁 Script executed:
# Check for NextID patterns and other explicit renames in the same file
rg -n "rename = " src/lotus_json/actors/states/market_state.rsRepository: ChainSafe/forest
Length of output: 161
🏁 Script executed:
# Search for test files related to market_state
fd -t f "market_state" --type rust | grep -i testRepository: ChainSafe/forest
Length of output: 231
🏁 Script executed:
# Look for JSON examples or test data that might show the expected format
rg -l "PendingDealAllocationIds\|PendingDealAllocationIDs" --type rustRepository: ChainSafe/forest
Length of output: 42
🏁 Script executed:
# Check if there are any Lotus-related documentation or comments about the format
rg -n "Lotus\|lotus\|API" src/lotus_json/actors/states/market_state.rs -C 2Repository: ChainSafe/forest
Length of output: 42
🏁 Script executed:
# Search for references in tests to see how this field is used
rg -n "pending_deal_allocation" --type rust -C 3Repository: ChainSafe/forest
Length of output: 13223
🏁 Script executed:
# Look for test JSON data in the repo
find . -name "*.json" -type f | head -20Repository: ChainSafe/forest
Length of output: 811
🏁 Script executed:
# Search for references to Lotus documentation or API specs
rg -n "lotus\|Lotus" src/lotus_json/actors/states/market_state.rs -A 2 -B 2Repository: ChainSafe/forest
Length of output: 42
🏁 Script executed:
# Look for comparison with NextID pattern - check other ID fields
rg -n "rename = \".*ID\"" src/lotus_json/ --type rustRepository: ChainSafe/forest
Length of output: 1552
🏁 Script executed:
# Check git history or comments around this field
git log -p --follow -S "pending_deal_allocation" -- src/lotus_json/actors/states/market_state.rs 2>/dev/null | head -100Repository: ChainSafe/forest
Length of output: 42
🏁 Script executed:
# Look for test files that might contain market state JSON examples
find . -path ./node_modules -prune -o -name "*market*" -type f | grep -E "\.(json|txt|toml)" | head -20Repository: ChainSafe/forest
Length of output: 42
🏁 Script executed:
# Search for test functions that serialize/deserialize MarketStateLotusJson
rg -n "MarketStateLotusJson" --type rust -A 5 -B 2 | head -80Repository: ChainSafe/forest
Length of output: 3886
🏁 Script executed:
# Check if there's any documentation about the field name format
rg -n "PendingDealAllocationIDs\|AllocationIds" --type rustRepository: ChainSafe/forest
Length of output: 42
🏁 Script executed:
# Get the test snapshot JSON to see the actual expected format
sed -n '119,170p' src/lotus_json/actors/states/market_state.rsRepository: ChainSafe/forest
Length of output: 1828
Add explicit rename to preserve uppercase "IDs" in field name.
The field should use rename = "PendingDealAllocationIDs" to match the Lotus API format and be consistent with the next_id field (line 35) which explicitly renames to "NextID". The current reliance on rename_all = "PascalCase" produces "PendingDealAllocationIds", but all similar ID fields in the codebase use explicit renames to preserve uppercase "IDs" for API compatibility.
🤖 Prompt for AI Agents
In @src/lotus_json/actors/states/market_state.rs around lines 134-135, The JSON
field for pending deal allocation IDs must preserve uppercase "IDs": add an
explicit serde rename for the struct field pending_deal_allocation_ids (the
field that currently serializes to "PendingDealAllocationIds") by annotating it
with rename = "PendingDealAllocationIDs" (same style as the NextID explicit
rename used for next_id) so the serialized/deserialized key matches the Lotus
API exactly.
There was a problem hiding this comment.
@akaladarshi does this comment make sense?
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
There was a problem hiding this comment.
@hanabi1224 It would have been relevant if the lotus was consistent with their naming convention, but right now Lotus is using PendingDealAllocationIds so we have to manually write these.
If the naming was consistent we could just use rename as the comment suggest but sadly it's not.
There was a problem hiding this comment.
Thanks! So the current field key is correct and no changes are needed.
| } | ||
|
|
||
| #[derive(Clone, Serialize, Debug)] | ||
| #[derive(Clone, PartialEq, Serialize, Debug)] |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Add Eq derive to maintain consistency.
The Entry enum derives PartialEq but not Eq. Since all fields (Address, u64) implement Eq, the enum should also derive Eq for consistency with other state types in this PR. The missing Eq prevents CronStateLotusJson (in cron_state.rs) from deriving Eq because it contains Vec<Entry>.
🔎 Proposed fix
-#[derive(Clone, PartialEq, Serialize, Debug)]
+#[derive(Clone, Eq, PartialEq, Serialize, Debug)]
#[serde(untagged)]
pub enum Entry {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| #[derive(Clone, PartialEq, Serialize, Debug)] | |
| #[derive(Clone, Eq, PartialEq, Serialize, Debug)] |
🤖 Prompt for AI Agents
In @src/shim/actors/builtin/cron/mod.rs around line 47, The Entry enum in
src/shim/actors/builtin/cron/mod.rs derives PartialEq but not Eq which prevents
CronStateLotusJson (which contains Vec<Entry>) from deriving Eq; update the
derive on the Entry enum (the #[derive(...)] line for Entry) to include Eq
alongside Clone, PartialEq, Serialize, Debug so Entry implements Eq and allow
CronStateLotusJson in cron_state.rs to derive Eq as intended.
There was a problem hiding this comment.
Eq is not applicable for external dependencies fil_actor_cron_state::v8::Entry
There was a problem hiding this comment.
@hanabi1224, thank you for the clarification! You're absolutely right—since the Entry enum wraps external types from fil_actor_cron_state::vX::Entry that don't implement Eq, the wrapper enum cannot derive Eq either, regardless of whether the wrapper fields would otherwise support it.
This aligns with the existing learning about CronStateLotusJson being constrained by these same external dependencies. I'll note this specific constraint for the Entry enum in mod.rs as well.
✏️ Learnings added
Learnt from: hanabi1224
Repo: ChainSafe/forest PR: 6381
File: src/shim/actors/builtin/cron/mod.rs:47-47
Timestamp: 2026-01-05T13:02:04.848Z
Learning: The Entry enum in src/shim/actors/builtin/cron/mod.rs cannot derive Eq because it wraps external fil_actor_cron_state::vX::Entry types (v8-v17) that don't implement Eq. Only PartialEq can be derived for this enum due to this external dependency constraint.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
Learnt from: hanabi1224
Repo: ChainSafe/forest PR: 6381
File: src/lotus_json/actors/states/cron_state.rs:8-8
Timestamp: 2026-01-05T12:54:40.850Z
Learning: In the Forest codebase, CronStateLotusJson in src/lotus_json/actors/states/cron_state.rs derives only PartialEq (not Eq) because it contains Entry types that wrap external dependencies from fil_actor_cron_state, which don't implement Eq. This constraint from external dependencies prevents deriving Eq.
Learnt from: hanabi1224
Repo: ChainSafe/forest PR: 5930
File: build.rs:64-77
Timestamp: 2025-08-13T09:43:20.301Z
Learning: hanabi1224 prefers hard compile-time errors in build scripts rather than runtime safeguards or collision detection, believing it's better to fail fast and fix root causes of issues like malformed snapshot names.
Learnt from: hanabi1224
Repo: ChainSafe/forest PR: 6057
File: src/cli/subcommands/f3_cmd.rs:0-0
Timestamp: 2025-09-09T10:37:17.947Z
Learning: hanabi1224 prefers having default timeouts (like 10m for --no-progress-timeout) to prevent commands from hanging indefinitely, even when the timeout flag isn't explicitly provided by users. This fail-fast approach is preferred over requiring explicit flag usage.
Codecov Report❌ Patch coverage is
Additional details and impacted files
... and 20 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
3 participants
Summary of changes
Part of #6365
Changes introduced in this pull request:
Reference issue to close (if applicable)
Closes
Other information and links
Change checklist
Summary by CodeRabbit
Release Notes
Bug Fixes
New Features
Refactor
✏️ Tip: You can customize this high-level summary in your review settings.