Skip to content

Conversation

@LesnyRumcajs
Copy link
Member

@LesnyRumcajs LesnyRumcajs commented Jan 16, 2026

Summary of changes

Changes introduced in this pull request:

  • remove some boilerplate implementations in favor of derives.

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

  • Refactor
    • Streamlined internal code structure across multiple modules for improved maintainability and long-term stability. No visible changes to end-user functionality.

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

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Jan 16, 2026

Walkthrough

This PR systematically replaces manual trait implementations with derive_more-based macros across thirteen files, reducing boilerplate code while maintaining functional equivalence through automatic trait derivation.

Changes

Cohort / File(s) Summary
Deref trait derivation
src/blocks/header.rs
CachingBlockHeader now derives Deref via derive_more instead of manual implementation; #[deref] attribute added to uncached field for deref coercion.
From trait derivation
src/db/car/any.rs, src/message/chain_message.rs, src/rpc/methods/eth/types.rs
AnyCar, ChainMessage, and EthAddressList now derive From to replace manual From implementations; AnyCar uses #[from(skip)] on Memory variant.
Network type trait derivation
src/networks/network_name.rs
GenesisNetworkName and StateNetworkName gain Display, From, Into, and AsRef derives with #[from(String, &str)] attribute; ~60 lines of manual impls removed.
Shim wrapper type trait derivation
src/shim/address.rs, src/shim/bigint.rs, src/shim/error.rs, src/shim/piece.rs, src/shim/randomness.rs
Address, StrictAddress, BigInt, ExitCode, PieceInfo, and PaddedPieceSize gain From and/or Into derives; v4 conversion paths removed for some types.
Shim wrapper types with multi-trait derivation
src/shim/econ.rs, src/shim/gas.rs, src/shim/version.rs
TokenAmount, Gas-related types, and NetworkVersion gain Debug, Display, From, and Into derives with special attributes (#[from] and #[into] specifications); TokenAmount, GasDuration, GasOutputs, GasCharge, and NetworkVersion undergo significant impl removals.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~12 minutes

Possibly related PRs

Suggested reviewers

  • akaladarshi
  • sudo-shashank
🚥 Pre-merge checks | ✅ 2 | ❌ 1
❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title 'chore: more derives, less manual boilerplate' accurately summarizes the main change: systematically replacing manual trait implementations with derive_more macros across multiple files.

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

✨ Finishing touches
  • 📝 Generate docstrings


📜 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 ab5f82f and f22c2aa.

📒 Files selected for processing (13)
  • src/blocks/header.rs
  • src/db/car/any.rs
  • src/message/chain_message.rs
  • src/networks/network_name.rs
  • src/rpc/methods/eth/types.rs
  • src/shim/address.rs
  • src/shim/bigint.rs
  • src/shim/econ.rs
  • src/shim/error.rs
  • src/shim/gas.rs
  • src/shim/piece.rs
  • src/shim/randomness.rs
  • src/shim/version.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/shim/bigint.rs
  • src/message/chain_message.rs
  • src/shim/error.rs
  • src/shim/piece.rs
  • src/networks/network_name.rs
  • src/db/car/any.rs
  • src/rpc/methods/eth/types.rs
  • src/shim/gas.rs
  • src/shim/address.rs
  • src/shim/randomness.rs
  • src/shim/econ.rs
  • src/shim/version.rs
  • src/blocks/header.rs
🧠 Learnings (9)
📓 Common learnings
Learnt from: CR
Repo: ChainSafe/forest PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-14T10:25:49.996Z
Learning: Applies to **/*.rs : Use `derive_more` crate for common trait implementations
Learnt from: hanabi1224
Repo: ChainSafe/forest PR: 5886
File: interop-tests/src/tests/go_app/gen.go:29-29
Timestamp: 2025-08-07T13:39:15.107Z
Learning: Auto-generated files like those created by rust2go (indicated by "Generated by rust2go. Please DO NOT edit this C part manually." comment) in the Forest project should be skipped during code review as they are not intended for manual editing.
Learnt from: LesnyRumcajs
Repo: ChainSafe/forest PR: 5907
File: src/rpc/methods/state.rs:523-570
Timestamp: 2025-08-06T15:44:33.467Z
Learning: LesnyRumcajs prefers to rely on BufWriter's Drop implementation for automatic flushing rather than explicit flush() calls in Forest codebase.
📚 Learning: 2026-01-14T10:25:49.996Z
Learnt from: CR
Repo: ChainSafe/forest PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-14T10:25:49.996Z
Learning: Applies to **/*.rs : Use `derive_more` crate for common trait implementations

Applied to files:

  • src/shim/bigint.rs
  • src/message/chain_message.rs
  • src/shim/error.rs
  • src/shim/piece.rs
  • src/networks/network_name.rs
  • src/db/car/any.rs
  • src/rpc/methods/eth/types.rs
  • src/shim/gas.rs
  • src/shim/address.rs
  • src/shim/randomness.rs
  • src/shim/econ.rs
  • src/shim/version.rs
  • src/blocks/header.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/shim/bigint.rs
  • src/message/chain_message.rs
  • src/shim/error.rs
  • src/shim/piece.rs
  • src/networks/network_name.rs
  • src/db/car/any.rs
  • src/rpc/methods/eth/types.rs
  • src/shim/gas.rs
  • src/shim/address.rs
  • src/shim/randomness.rs
  • src/shim/econ.rs
  • src/shim/version.rs
  • src/blocks/header.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/shim/bigint.rs
  • src/message/chain_message.rs
  • src/shim/error.rs
  • src/shim/piece.rs
  • src/networks/network_name.rs
  • src/db/car/any.rs
  • src/rpc/methods/eth/types.rs
  • src/shim/gas.rs
  • src/shim/address.rs
  • src/shim/randomness.rs
  • src/shim/econ.rs
  • src/shim/version.rs
  • src/blocks/header.rs
📚 Learning: 2026-01-05T13:02:14.604Z
Learnt from: hanabi1224
Repo: ChainSafe/forest PR: 6381
File: src/shim/actors/builtin/cron/mod.rs:47-47
Timestamp: 2026-01-05T13:02:14.604Z
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.

Applied to files:

  • src/shim/error.rs
  • src/shim/econ.rs
📚 Learning: 2025-12-18T08:12:03.919Z
Learnt from: hanabi1224
Repo: ChainSafe/forest PR: 6362
File: f3-sidecar/manifest.go:14-17
Timestamp: 2025-12-18T08:12:03.919Z
Learning: In the Forest codebase, F3.GetRawNetworkName queries the network name from genesis and returns "testnetnet" on mainnet, not "mainnet". This is different from StateNetworkName which returns "mainnet". The raw network name "testnetnet" is the correct key for manifest lookups in f3-sidecar when using RawNetwork2PredefinedManifestMappings.

Applied to files:

  • src/networks/network_name.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/db/car/any.rs
📚 Learning: 2025-09-02T10:05:34.350Z
Learnt from: akaladarshi
Repo: ChainSafe/forest PR: 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/rpc/methods/eth/types.rs
📚 Learning: 2025-10-16T11:05:13.586Z
Learnt from: elmattic
Repo: ChainSafe/forest PR: 6166
File: src/auth/mod.rs:127-150
Timestamp: 2025-10-16T11:05:13.586Z
Learning: In Rust 2024, `std::env::set_var` and `std::env::remove_var` are unsafe functions. They require `unsafe` blocks because they are only safe in single-threaded programs (Windows is an exception). When these functions are used in tests, ensure proper serialization with `#[serial]` or similar mechanisms.

Applied to files:

  • src/shim/econ.rs
🧬 Code graph analysis (1)
src/shim/version.rs (1)
src/networks/mod.rs (1)
  • from (186-224)
⏰ 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 forest binaries on Linux AMD64
  • GitHub Check: tests-release
  • GitHub Check: Coverage
  • GitHub Check: Build MacOS
  • GitHub Check: cargo-publish-dry-run
  • GitHub Check: Build Ubuntu
  • GitHub Check: All lint checks
  • GitHub Check: rubocop
🔇 Additional comments (14)
src/message/chain_message.rs (1)

14-19: LGTM!

Using derive_more::From to automatically generate From<Message> and From<SignedMessage> implementations is the correct approach here. This reduces manual boilerplate while maintaining the same behavior. Based on coding guidelines and learnings, this aligns with the project's preference for using derive_more for common trait implementations.

src/blocks/header.rs (1)

229-232: LGTM — derive-based Deref is clean and consistent.

The derive + #[deref] on uncached keeps the API ergonomic while removing manual boilerplate.

src/db/car/any.rs (1)

24-30: LGTM! Clean replacement of manual From implementations with derive.

The #[from(skip)] on Memory is correct since it wraps PlainCar<Vec<u8>> rather than PlainCar<ReaderT>, which would conflict with the derived From<PlainCar<ReaderT>>. This aligns with the coding guidelines to use derive_more for common trait implementations.

src/shim/error.rs (1)

27-39: LGTM! Proper use of derive for direct wrapper conversion.

The derive_more::From and derive_more::Into correctly handle the direct ExitCodeV4 <-> ExitCode conversion. The manual From implementations for ExitCodeV2, ExitCodeV3, and u32 (lines 123-151) are correctly retained since they require value-based conversion through .value() rather than simple wrapping. This aligns with the coding guidelines to use derive_more for common trait implementations.

src/rpc/methods/eth/types.rs (1)

467-472: LGTM! Derive correctly replaces manual From implementations.

The derive_more::From will generate From<EthAddress> (mapping to Single) and From<Vec<EthAddress>> (mapping to List), which is functionally equivalent to the removed manual implementations. This aligns with the coding guidelines to use derive_more for common trait implementations.

src/shim/address.rs (2)

112-130: LGTM! Correct derive usage for the Address wrapper.

The derive_more::From and derive_more::Into correctly replace the manual From<Address_v4> for Address and From<Address> for Address_v4 implementations. The manual implementations for Address_v2 and Address_v3 (lines 340-398) are correctly retained since they require byte-based conversion with error handling. This aligns with the coding guidelines to use derive_more for common trait implementations.


277-294: LGTM! Clean derive usage for StrictAddress.

The derive_more::From and derive_more::Into correctly handle the Address <-> StrictAddress conversions. The remaining manual implementations for converting StrictAddress to Address_v3 and Address_v4 (lines 310-320) are correctly retained.

src/networks/network_name.rs (1)

14-22: LGTM! Excellent use of derive_more to eliminate boilerplate.

The #[from(String, &str)] attribute correctly generates both From<String> and From<&str> implementations. Combined with Display, Into, and AsRef derives, this cleanly replaces the manual implementations while maintaining identical behavior. This is a great example of leveraging derive_more as recommended in the coding guidelines.

src/shim/version.rs (1)

44-49: Verify derived Display/conversion parity with the removed manual impls.

derive_more::Display will delegate to NetworkVersion_latest, and the #[from]/#[into] u32-bridged conversions replace manual logic. Please confirm formatting and conversion behavior (including any validation) matches the previous implementations and update tests if needed.

src/shim/bigint.rs (1)

7-18: LGTM for the derived conversion.

Please confirm derive_more v2.0’s From derive produces the same From<num_bigint::BigInt> behavior as the removed manual impl in your MSRV.

src/shim/piece.rs (1)

11-13: Confirm derive-based conversions still cover required v4/latest paths.

Manual v4 conversions were removed; please verify piece_latest is the intended target (e.g., v4) and downstream callers don’t still require explicit v4 conversions. If they do, add explicit #[from(...)]/#[into(...)] for v4 types.

Also applies to: 63-65

src/shim/randomness.rs (1)

41-42: Ensure docs/examples remain valid with derive-based conversions.

The doctest references fvm_shared4; with v4-specific From removed, please confirm it still compiles and conversions remain correct (or add explicit derive_more attributes for v4 if needed).

src/shim/gas.rs (1)

20-33: Verify v4 conversions remain available after derive-based swap.

Manual From<GasV4> / From<GasChargeV4> were removed. Please confirm fvm_latest still aliases v4 (so the derives cover v4) or add explicit #[from]/#[into] for v4 to preserve API expectations and the GasOutputsV4::compute(...).into() usage.

Also applies to: 111-112

src/shim/econ.rs (1)

39-42: LGTM! Good use of derive_more for newtype wrapper.

The derived traits correctly handle the transparent delegation pattern for this newtype wrapper. The From and Into derives replace the removed manual implementations, while Debug and Display provide the expected delegation to the inner type.

This aligns well with the coding guidelines requiring the use of derive_more for common trait implementations. Based on learnings, this is the preferred approach.

✏️ 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.

@LesnyRumcajs LesnyRumcajs force-pushed the use-derive-more-from-shims branch from 63c842d to ad0f9ff Compare January 16, 2026 17:17
@LesnyRumcajs LesnyRumcajs force-pushed the use-derive-more-from-shims branch from ad0f9ff to f22c2aa Compare January 16, 2026 17:22
@LesnyRumcajs LesnyRumcajs marked this pull request as ready for review January 16, 2026 17:25
@LesnyRumcajs LesnyRumcajs requested a review from a team as a code owner January 16, 2026 17:25
@LesnyRumcajs LesnyRumcajs requested review from hanabi1224 and sudo-shashank and removed request for a team January 16, 2026 17:25
@hanabi1224 hanabi1224 added this pull request to the merge queue Jan 16, 2026
Merged via the queue into main with commit ffe19b9 Jan 16, 2026
47 checks passed
@hanabi1224 hanabi1224 deleted the use-derive-more-from-shims branch January 16, 2026 18:52
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