Skip to content

Conversation

@hanabi1224
Copy link
Contributor

@hanabi1224 hanabi1224 commented Jan 16, 2026

Summary of changes

Changes introduced in this pull request:

Reference issue to close (if applicable)

Closes

Other information and links

Change checklist

  • I have performed a self-review of my own code,
  • I have made corresponding changes to the documentation. All new code adheres to the team's documentation standards,
  • I have added tests that prove my fix is effective or that my feature works (if possible),
  • I have made sure the CHANGELOG is up-to-date. All user-facing changes should be reflected in this document.

Summary by CodeRabbit

  • Tests
    • Added and reorganized tests to cover additional epochs on Calibnet and Mainnet.
    • Renamed and expanded test suite entries for clearer coverage.
    • Tests now run full state computation and validation (not just preparation) across multiple epochs.
    • Included inline annotations indicating FVM version contexts for relevant test cases.

✏️ Tip: You can customize this high-level summary in your review settings.

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Jan 16, 2026

Walkthrough

Renames and expands state manager tests in src/state_manager/utils.rs: one test renamed to include a block height, three new tests added, and all renamed/added tests now call state_compute(sm, ts).await after preparing state to assert computed state across multiple epochs.

Changes

Cohort / File(s) Summary
State manager tests
src/state_manager/utils.rs
Renamed state_compute_calibnet()state_compute_calibnet_3111900(); added state_compute_calibnet_480000, state_compute_calibnet_300000, and state_compute_mainnet_5427431. Updated tests to call state_compute(sm, ts).await after state preparation; added FVM-version comments.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Possibly related PRs

Suggested reviewers

  • akaladarshi
  • LesnyRumcajs
  • sudo-shashank
🚥 Pre-merge checks | ✅ 3
✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title 'test: cover state compuation with fvm2 and fvm3' directly corresponds to the main changes: new tests added to cover state computation across multiple FVM versions (FVM2, FVM3, and FVM4).
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing touches
  • 📝 Generate docstrings

🧹 Recent nitpick comments
src/state_manager/utils.rs (1)

276-284: Consider extracting a shared test helper for state compute setup.

The setup is identical across the new tests; a small helper like run_state_compute(chain, epoch) would reduce duplication and make it easier to add more epochs later.


📜 Recent review details

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 746d2b7 and cb50e65.

📒 Files selected for processing (1)
  • src/state_manager/utils.rs
🧰 Additional context used
📓 Path-based instructions (1)
**/*.rs

📄 CodeRabbit inference engine (AGENTS.md)

**/*.rs: Use anyhow::Result<T> for most operations and add context with .context() when operations fail
Use tokio::task::spawn_blocking for CPU-intensive work such as VM execution and cryptography operations
Use .get() instead of indexing with [index] for safe element access
Do not use unwrap() in production code; use ? or expect() with descriptive messages instead
Do not use dbg! macro in non-test Rust code
Do not use todo! macro in non-test Rust code
Use strum crate for enum string conversions
Use derive_more crate for common trait implementations
Document all public functions and structs with doc comments
Use #[cfg(test)] or #[cfg(feature = "doctest-private")] for test-only code
Use channel-based communication (flume, tokio channels) between async tasks
Use consistent formatting following cargo fmt standards
Ensure code passes cargo clippy --all-targets --no-deps -- --deny=warnings linter checks

Files:

  • src/state_manager/utils.rs
🧠 Learnings (4)
📓 Common learnings
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.
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-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/state_manager/utils.rs
📚 Learning: 2026-01-05T12:54:40.850Z
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 Rust code reviews, do not derive Eq for a struct if any field does not implement Eq (e.g., types from external dependencies). If a type like CronStateLotusJson includes fields wrapping external dependencies that lack Eq, derive PartialEq (or implement PartialEq manually) but avoid deriving Eq. This ensures comparisons compile and reflect actual equivalence semantics. When needed, consider implementing custom PartialEq (and possibly Eq) only after ensuring all fields (or wrappers) implement Eq, or keep PartialEq-only if full equality semantics cannot be expressed.

Applied to files:

  • src/state_manager/utils.rs
📚 Learning: 2026-01-05T12:56:13.802Z
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 Rust codebases (e.g., Forest), do not add #[cfg(test)] to functions already annotated with #[test]. The #[test] attribute ensures the function is compiled only for tests, so a separate #[cfg(test)] is redundant and can be removed if present. Apply this check to all Rust files that contain #[test] functions.

Applied to files:

  • src/state_manager/utils.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). (8)
  • GitHub Check: Build Ubuntu
  • GitHub Check: Build MacOS
  • GitHub Check: cargo-publish-dry-run
  • GitHub Check: Build forest binaries on Linux AMD64
  • GitHub Check: rubocop
  • GitHub Check: Coverage
  • GitHub Check: All lint checks
  • GitHub Check: tests-release
🔇 Additional comments (3)
src/state_manager/utils.rs (3)

287-295: Looks good—coverage for Calibnet FVM@3 epoch added.


298-306: Looks good—coverage for Calibnet FVM@2 epoch added.


308-316: Looks good—coverage for Mainnet epoch added.

✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.


Comment @coderabbitai help to get the list of available commands and usage tips.

@hanabi1224 hanabi1224 marked this pull request as ready for review January 16, 2026 11:28
@hanabi1224 hanabi1224 requested a review from a team as a code owner January 16, 2026 11:28
@hanabi1224 hanabi1224 requested review from LesnyRumcajs and sudo-shashank and removed request for a team January 16, 2026 11:28
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It'd be nice to mention in the comment why are we testing those particular epochs.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed.

@LesnyRumcajs LesnyRumcajs added this pull request to the merge queue Jan 16, 2026
Merged via the queue into main with commit ab5f82f Jan 16, 2026
42 checks passed
@LesnyRumcajs LesnyRumcajs deleted the hm/test-state-compute-fvm-2-3 branch January 16, 2026 16:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants