chore: more delegate_enum to reduce repetitive code #6481
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Contributor
WalkthroughCentralizes policy handling into a new Changes
Sequence Diagram(s)(omitted) Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Comment |
hanabi1224
requested review from
LesnyRumcajs and
akaladarshi
and removed request for
a team
hanabi1224
commented
Jan 26, 2026
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/shim/actors/builtin/multisig/ext/state.rs`:
- Around line 6-8: The empty impl block "impl MultisigExt for State {}" must
either be removed or completed: implement the trait's required method
get_vesting_schedule on State (or forward to the existing concrete
implementation in mod.rs) so the trait contract is satisfied; update the impl
block to include fn get_vesting_schedule(&self, ...) -> ... calling the existing
implementation in mod.rs (or delete the empty impl entirely if the trait is
already implemented elsewhere) and ensure the signature exactly matches
MultisigExt's declaration.
🧹 Nitpick comments (1)
src/shim/actors/builtin/multisig/mod.rs (1)
110-116: Consider propagating errors instead of using.expect().The
PendingTxnMap::loadcalls use.expect("Could not load pending transactions")which will panic on failure. Per coding guidelines, production code should avoidunwrap()andexpect()where possible, preferring?for error propagation.♻️ Suggested fix
State::V12(st) => { let txns = fil_actor_multisig_state::v12::PendingTxnMap::load( store, &st.pending_txs, fil_actor_multisig_state::v12::PENDING_TXN_CONFIG, "pending txns", - ) - .expect("Could not load pending transactions"); + )?; parse_pending_transactions_v4!(res, txns); Ok(res) }Apply similar changes to V13-V17 branches.
Also applies to: 121-127, 132-138, 143-149, 154-160, 165-171
hanabi1224
force-pushed
the
hm/more-delegate-enum-2
branch
from
9b8e5ff to
6822c55
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@src/shim/actors/builtin/multisig/mod.rs`:
- Around line 178-183: Add a rustdoc comment for the public function
get_vesting_schedule describing what the function returns and under what
conditions it may error: document that get_vesting_schedule returns the multisig
vesting schedule as an MsigVesting (fields: initial_balance, start_epoch,
unlock_duration), that it reads from the actor state via delegate_state! and
that it returns an anyhow::Result which may contain an error if state access
fails; include examples or panics only if relevant and tag return value with #
Returns and # Errors headings.
In `@src/shim/actors/builtin/verifreg/mod.rs`:
- Around line 275-277: The public method root_key() lacks rustdoc; add a /// doc
comment above pub fn root_key(&self) -> Address describing its purpose (returns
the verifier registry's root key address), what it returns (Address) and any
important behavior/ownership/format expectations, and include examples or notes
if needed for callers; place the doc immediately above the root_key function
declaration so the new public API is documented.
In `@src/shim/runtime.rs`:
- Around line 14-28: Add a rustdoc comment for the public Policy wrapper
explaining that Policy is a transparent newtype wrapper around PolicyV13,
describing its purpose and typical usage (e.g., that it delegates to PolicyV13
behavior and preserves serialization via #[serde(transparent)]), mention any
important invariants or versioning note (PolicyV13) and include a short example
or pointer to PolicyV13 docs; place the doc comment immediately above the pub
struct Policy declaration referencing Policy and PolicyV13.
🧹 Nitpick comments (7)
src/shim/actors/macros.rs (1)
10-29: Consider consolidating identical macros.
parse_pending_transactionsandparse_pending_transactions_v3have identical implementations. If they're meant to behave the same, consider removing one and aliasing. If divergence is expected, a brief comment clarifying the intent would help future maintainers.Also applies to: 36-55
src/shim/actors/builtin/miner/ext/state.rs (2)
14-185: Opportunity to applydelegated_enumpattern here as well.The
load_sectors_extmethod has 10 nearly identical match arms (V8-V17), each following the same pattern. Given this PR's goal of reducing repetitive code withdelegated_enum, this method could potentially benefit from the same treatment in a follow-up.
5-6: Remove unused imports from lines 5-6.
ChainEpoch,Policy, andanyhow::Contextare not used in this file. These can be safely removed from the imports.♻️ Cleanup
-use crate::shim::{actors::convert::*, clock::ChainEpoch, runtime::Policy}; -use anyhow::Context as _; +use crate::shim::actors::convert::*;src/shim/actors/builtin/evm/mod.rs (1)
57-75: Add rustdoc for the newTombstoneStateAPI.
Public enum and constructor lack rustdoc (Line 60, Line 72).As per coding guidelines, public items should have rustdoc comments.Suggested doc additions
+/// EVM tombstone state across actor versions. pub enum TombstoneState { @@ impl TombstoneState { + /// Constructs the latest-version tombstone state. pub fn default_latest_version(origin: fvm_shared4::ActorID, nonce: u64) -> Self { TombstoneState::V17(fil_actor_evm_state::v17::Tombstone { origin, nonce }) } }src/shim/actors/builtin/miner/mod.rs (1)
413-447: Add rustdoc forallocated_sectors.
Line 413 introduces a new public method without rustdoc.As per coding guidelines, public items should have rustdoc comments.Suggested doc addition
+ /// Returns the CID of the allocated sectors bitfield. pub fn allocated_sectors(&self) -> Cid { delegate_state!(self.allocated_sectors) }src/cli/subcommands/config_cmd.rs (1)
18-22: Add rustdoc and confirm clippy won’t flagunused_async.
New publicrunlacks rustdoc, and it currently doesn’t await. With--deny=warnings, please verifyclippy::unused_asyncisn’t triggered; if it is, add a targeted allow or adjust the structure. As per coding guidelines, public functions should be documented.✍️ Suggested rustdoc
- pub async fn run(self, _: rpc::Client) -> anyhow::Result<()> { + /// Execute the config subcommand. + pub async fn run(self, _: rpc::Client) -> anyhow::Result<()> { self.run_internal(&mut std::io::stdout()) }src/cli/subcommands/mod.rs (1)
106-109: Add rustdoc for the new publicrunmethod.
As per coding guidelines, public functions should be documented.✍️ Suggested rustdoc
impl Subcommand { + /// Execute the selected CLI subcommand. pub async fn run(self, client: crate::rpc::Client) -> anyhow::Result<()> { delegate_subcommand!(self.run(client).await) } }
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary of changes
Changes introduced in this pull request:
Reference issue to close (if applicable)
Closes
Other information and links
Change checklist
Summary by CodeRabbit
Refactor
New Features
Breaking Changes
✏️ Tip: You can customize this high-level summary in your review settings.