chore: fix stale doc comments#1
Open
simone-stacks wants to merge 102 commits into
Open
Conversation
…e_info Commit 480c59a (2024-05-30) removed the `if !burnchain.is_in_prepare_phase(burn_height) { return Err(Error::NotInPreparePhase); }` branch from get_nakamoto_reward_cycle_info, but left the doc comment claiming `Returns Err(Error::NotInPreparePhase) if `burn_height` is not in the prepare phase`. The function never constructs that variant; the caller passes a reward cycle (not a burn height) and the burn height is derived internally as the first block of the cycle.
Commit 7c7f9b3 (2024-07-26, 'feat: support multi-miner integration test') replaced the fallible `u32::try_from(i).map_err(...)` slot-id conversion in get_miner_slot with `signer_ranges.swap_remove(signer_ix)` on a precomputed `Vec<Range<u32>>`. After that refactor the function returns Ok(None) for every internal failure mode; the only Err path is upstream propagation from `make_miners_stackerdb_config(...)?`, which has nothing to do with the slot number being invalid. The doc-comment's fourth bullet — 'Returns an error if the miner is in the StackerDB config but the slot number is invalid' — no longer corresponds to any branch in the body.
Commit 16f8973 (2023-09-13, 'use tiny http instead of hand rolled') replaced the fallible `TcpListener::bind(listener)?` setup with `HttpServer::http(listener).expect("failed to start HttpServer")`. After that change the bind() implementation never constructs an Err: it always returns Ok(listener), and a failure inside HttpServer::http panics. The doc-comment still claims 'Errors out if bind(2) fails', which is false — bind(2) failure panics the thread instead of being returned to the caller. This may also warrant a code-level fix: callers (e.g. libsigner's runloop.rs:224) propagate the result with `?`, suggesting they expect to handle a bind failure rather than crash. Documenting the current behavior accurately is the conservative comment-only change; restoring fallibility is a separate design decision.
The comment 'Returns Ok() and a vector of PoxAddresses which were punished by this op' was added in commit e721bd5 (2024-06-03) when the punishment-via-bitvec feature was added. It misdescribes the function: the signature is Result<(), op_error> (returns the unit type, not a vector); the vector that the comment refers to is stored as a side effect in self.treatment (renamed from self.punished in commit 3fcd23f, 2024-06-10), and its element type is Vec<Treatment>, not Vec<PoxAddress> — Treatment is an enum {Reward, Punish}, so the vector contains both reward AND punishment addresses, not only 'punished' ones. Replace with an accurate one-liner that describes the side-effect-on-self path.
Three doc-comments on STXBalanceSnapshot::increase_lock_v* misdescribe the function behavior: * increase_lock_v2 (line 547): comment claims 'Panics if self was not locked by V2 PoX'. Commit 139ed66 (2024-01-17, mislabeled as 'chore: cargo fmt') replaced the panic! with return Err(InterpreterError::Expect(...)). The function returns an error; it no longer panics. * increase_lock_v3 (line 766): same pattern. Body returns Err(VmInternalError::Expect(...)) on the !is_v3_locked() branch; the doc still claims a panic. * increase_lock_v4 (line 892): the function body does panic on !is_v4_locked(), but the doc-comment says 'V3 PoX'. Off-by-one PoX version in the doc — fix the version, keep the 'Panics' claim (which is accurate for this function). Three minimum-touch doc edits, no behavior change.
SignerRunLoop trait's run_one_pass method returns Option<R>, but main_loop's doc-comment claims the loop continues until `run_one_pass()` returns 'false'. The loop body actually checks 'if let Some(final_state) = self.run_one_pass(...)' and exits on the Some(R) variant. The comment has been wrong since the libsigner crate was introduced in commit 4e306ce (2023-08-28); the function was declared as returning Option<R> from the start, so the 'false' in the doc was always inconsistent with the type signature.
make_block_commit's doc-comment says 'Returns None if we fail somehow' but the function signature is Result<LastCommit, NakamotoNodeError>: failure paths in the body all construct Err(NakamotoNodeError::...), never Ok(None). The 'on success' line also described the return as a tuple of three values when the actual return type is a 7-field struct (LastCommit) — replace with an accurate description of LastCommit's relevant fields.
The doc-comment claimed the canonicalized return is a struct with amount_unlocked = 0 and unlock_height = 0. STXBalance was a struct with those fields until commit 44cbd41 (2022-12-22, 'import clarity again'), which converted it into an enum with variants {Unlocked, LockedPoxOne, LockedPoxTwo, ...}. The body of canonical_repr_at_block now returns STXBalance::Unlocked { amount }, which has no amount_unlocked / unlock_height fields. Replace the comment's description with the actual return shape.
The doc-comment claimed the function returns the non-orphaned block, but the SQL query 'SELECT index_block_hash,processed,orphaned,signing_weight FROM nakamoto_staging_blocks WHERE consensus_hash = ?1 AND block_hash = ?2 ORDER BY signing_weight DESC, index_block_hash LIMIT 1' has no orphaned filter. The function returns the orphan flag in its tuple, and the caller in store_block_if_better (line 629) destructures and checks the flag — proving the function does return orphaned matches. Reword the doc to describe the actual behavior.
Doc claimed Ok(true)/Ok(false) but the function returns Result<BlockAcceptResponse, chainstate_error>, with variants Accepted, AlreadyStored, and Rejected(String). The bool->enum refactor in commit 3a0bc7a (2024-09-11, 'chore: have block acceptance return a BlockAcceptResult instead of a bool') changed the return type but left the doc-comment unchanged. Rewrite the doc to describe the actual variants returned.
Doc claimed Ok(true)/Ok(false) but the function returns Result<BlockAcceptResponse, chainstate_error>. When force_broadcast is true and the block is already known, the function returns BlockAcceptResponse::Accepted; when force_broadcast is false and the block is already known, it returns BlockAcceptResponse::AlreadyStored. The bool->enum refactor in commit 3a0bc7a (2024-09-11) changed both return paths but the doc-comment still describes the old bool contract. Rewrite the success/no-broadcast lines to describe the actual variants returned. Preserve the Err: list verbatim.
…pects_stacks_block Doc-comment said 'Caller must have called SortitionDB::expects_stacks_block()'. SortitionDB::expects_stacks_block exists nowhere in the repo. The function with the equivalent semantic (does the chain expect a block with this hash on this fork?) is SortitionHandleTx::expects_stacks_block_in_fork (sortdb.rs:1361). At commit c3212fb (2018-vintage) both 'expects_stacks_block' and 'expects_stacks_block_in_fork' existed on BurnDB; the non-fork variant was removed in subsequent refactors but the comment never followed. Rename in the doc to point readers at the real function.
The doc-comment on as_block_rejection() said 'Get the block accept data from the block response' (identical to the doc on the preceding as_block_accepted()). The function actually extracts the rejection case: matches BlockResponse::Rejected(rejection) and returns Option<&BlockRejection>. The comment was wrong from introduction in commit 7519cdb (2024-12-18) - clear copy-paste from the sibling. Rename 'accept' to 'rejection' in the doc.
…rns () The doc-comment said 'Returns true if we succeed in doing so; false if not.' The function signature has no return type — it returns (). Both the early no-op path (line 1029, 'return;') and the success path (line 1058) return implicitly with no value. Callers cannot observe success or failure from this method. The doc has been wrong since the function was authored in cfd1a12 (2023-12-04). Rewrite to describe the actual behavior: silent no-op on either already-in-flight or submission failure.
…new_nakamoto_block Doc said 'Wrapper around inner_process_new_nakamoto_block' but inner_process_new_nakamoto_block has never existed in the repo (grep returns only the doc reference). The wrapper actually calls process_new_nakamoto_block_ext with force_broadcast=false. The doc has been wrong since the wrapper was introduced in commit 21fc76d (2024-08-14, 'feat: allow forced block broadcast'). Replace with the actual callee name.
…rror
The doc-comment said 'a ChainstateError is returned' but the function
signature is Result<u32, String>. The Err arm at line 324 returns
Err('Unable to calculate total weight - No signers in reward set'.to_string()).
ChainstateError is not in the return type and is not constructed in
the body. Author may have intended ChainstateError originally but the
function shipped with String in commit f963b35 (2024-05-30). Update
the doc to describe the actual error type.
The doc-comment said 'Runtime-panics if the MARF was already sealed.' The function signature is Result<TrieHash, Error> and the body explicitly returns Err(Error::ReadOnlyError) when self.storage.readonly() is true — i.e. exactly the already-sealed precondition the doc names. Doc has been wrong since the function was introduced in 4e1a88a (2022-02-10). Replace the panic claim with the actual Err variant.
Doc said 'Returns true/false, based on whether or not the trie will be created (this can return false if we're resuming work on an unconfirmed trie)'. The signature is Result<(), Error> — no bool is returned. Every Ok arm yields Ok(()): the unconfirmed-resume case at line 888 returns Ok(()), the brand-new case at line 882 returns Ok(()), the extend case at line 899 returns Ok(()). There is no path that returns a bool. Doc was wrong since the function returned bool in commit 561499a (2020-07-05) and was changed to () in commit e1195b5 (2020-09-01) without updating the doc. Also rename 's' to 'storage' to match the parameter name.
The doc-comment said 'Return an error on overflow' but the function signature is Option<StacksMicroblockHeader> and the overflow path at line 823 returns None. There is no Result, no Error enum variant — the failure mode is plain None. Doc has been wrong since the function was introduced in 71c6205 (2019-09-16). Replace 'error' with the actual None return shape.
The doc-comment said 'Returns the BlockSnapshot created from this block.' The function signature returns Result<(BlockSnapshot, BurnchainStateTransition), BurnchainError> — a 2-tuple. The doc omits the BurnchainStateTransition. Update the description to name both members of the tuple.
Five net/api request constructors carried the doc-comment 'Make a new getinfo request to this endpoint' even though each builds a different endpoint: - getpoxinfo.rs new_getpoxinfo (/v2/pox) - getsigner.rs new_getsigner - getstackers.rs new_getstackers - gettenureinfo.rs new_get_nakamoto_tenure_info (/v3/tenures/info) - gettenuretip.rs new_get_tenure_tip (/v3/tenures/tip/...) Doc was copy-pasted from getinfo.rs (where it is correct) and never adjusted. Rename the noun in each per-file comment to match the endpoint the function actually builds.
The Environment struct was split into ExecutionState/InvocationContext/ GlobalContext in commit f9f5e5c; the inline reference in the doc- comment on inner_execute_contract still pointed at the old type name.
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
102 small doc-only fixes for stale/incorrect comments across stacks-core (wrong return types, renamed fns, removed branches, etc). One commit per fix for easy review/revert.
Result of a first smoke-test of the
quality-agent.