docs(x/staking): update readme #22216
Conversation
JulianToledano
added
the
backport/v0.52.x
JulianToledano
requested review from
facundomedica,
testinginprod,
ziscky and
a team
as code owners
📝 WalkthroughWalkthroughThe pull request introduces significant updates to the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (20)
x/staking/types/params.go (2)
25-26: LGTM: Enhanced constant documentation with a minor suggestionThe updated comment for
DefaultMaxEntriesprovides a more detailed explanation of the constant's purpose, including the full meaning of the acronyms UBD and RED. This improvement aligns with best practices for code documentation.Consider adding a brief explanation of what a "pair" means in this context for even greater clarity.
Line range hint
185-196: LGTM: Well-implemented new validation functionThe new
ValidatePowerReductionfunction is a good addition to the existing validation functions. It properly checks the type and value of the input, returning appropriate error messages for invalid cases.Consider using a constant for the minimum allowed value (1) to improve maintainability. For example:
const MinPowerReduction = 1 // In the function if v.LT(math.NewInt(MinPowerReduction)) { return fmt.Errorf("power reduction cannot be lower than %d", MinPowerReduction) }x/staking/README.md (18)
72-72: Update LastTotalPower description for clarityThe description of
LastTotalPowerhas been updated to use a new storage prefix. Consider adding a brief explanation of why this change was made and its implications.Consider updating the line to:
* LastTotalPower: `collections.NewPrefix(18)` // Stores the last total power value
78-78: Clarify UnbondingID storage and usageThe
UnbondingIDdescription has been updated with a new storage value. It would be helpful to explain the significance of this change and how it affects the unbonding process.Consider expanding the description:
* UnbondingID: `55` // Stores the latest unbonding operation ID, used to create unique IDs for unbonding operations
85-85: Update Params storage descriptionThe
Paramsstorage has been simplified. It would be beneficial to explain why this change was made and its impact on the module's functionality.Consider updating the line to:
* Params: `81` // Stores the staking module parameters
120-124: Review and update Validators storage descriptionsThe storage prefixes for various validator-related data have been updated. Ensure that these changes are consistent with the actual implementation and explain their purpose.
Consider adding brief explanations for each storage prefix:
* Validators: `33 | OperatorAddrLen (1 byte) | OperatorAddr -> ProtocolBuffer(validator)` // Stores validator data * ValidatorsByConsAddr: `34 | ConsAddrLen (1 byte) | ConsAddr -> OperatorAddr` // Maps consensus address to operator address * ValidatorsByPower: `23 | BigEndian(ConsensusPower) | OperatorAddrLen (1 byte) | OperatorAddr -> OperatorAddr` // Indexes validators by power * LastValidatorsPower: `17 | OperatorAddrLen (1 byte) | OperatorAddr -> ProtocolBuffer(ConsensusPower)` // Stores the last known power for each validator * UnbondingIndex: `56 | UnbondingID -> 21 | OperatorAddrLen (1 byte) | OperatorAddr` // Maps unbonding IDs to validator addresses
131-132: Clarify UnbondingIndex usageThe description of
UnbondingIndexis new and might benefit from more context about its purpose and how it's used in the unbonding process.Consider expanding the description:
`UnbondingIndex` is an additional index that enables lookups for validators by the unbonding IDs corresponding to their current unbonding. This helps in tracking and managing the unbonding process more efficiently.
166-166: Update Delegation storage prefixThe storage prefix for
Delegationhas been updated. Ensure this change is consistent with the implementation and explain its significance.Consider updating the line to:
* Delegation: `49 | DelegatorAddrLen (1 byte) | DelegatorAddr | ValidatorAddrLen (1 byte) | ValidatorAddr -> ProtocolBuffer(delegation)` // Stores delegation information
204-206: Review and update UnbondingDelegation storage descriptionsThe storage prefixes for unbonding delegations have been updated, and a new
UnbondingIndexhas been added. Ensure these changes are consistent with the implementation and explain their purpose.Consider adding brief explanations for each storage prefix:
* UnbondingDelegation: `50 | DelegatorAddrLen (1 byte) | DelegatorAddr | ValidatorAddrLen (1 byte) | ValidatorAddr -> ProtocolBuffer(unbondingDelegation)` // Stores unbonding delegation data * UnbondingDelegationByValIndex: `51 | ValidatorAddrLen (1 byte) | ValidatorAddr | DelegatorAddrLen (1 byte) | DelegatorAddr -> nil` // Index for querying unbonding delegations by validator * UnbondingIndex: `56 | UnbondingId -> 0x32 | DelegatorAddrLen (1 byte) | DelegatorAddr | ValidatorAddrLen (1 byte) | ValidatorAddr` // Maps unbonding IDs to unbonding delegations
210-214: Clarify usage of UnbondingDelegationByValIndex and UnbondingIndexThe descriptions of
UnbondingDelegationByValIndexandUnbondingIndexare new and might benefit from more context about their purpose and how they're used in the unbonding process.Consider expanding the descriptions:
`UnbondingDelegationByValIndex` is used in slashing, to look up all unbonding delegations associated with a given validator that need to be slashed. This index improves the efficiency of the slashing process. `UnbondingIndex` is an additional index that enables lookups for unbonding delegations by the unbonding IDs of the containing unbonding delegation entries. This helps in tracking and managing the unbonding process more efficiently.🧰 Tools
🪛 LanguageTool
[grammar] ~210-~210: The word “lookup” is a noun. The verb is spelled with a white space.
Context: ...tionByValIndex` is used in slashing, to lookup all unbonding delegations associated w...(NOUN_VERB_CONFUSION)
235-238: Review and update Redelegation storage descriptionsThe storage prefixes for redelegations have been updated, and a new
RedelegationByUnbondingIdhas been added. Ensure these changes are consistent with the implementation and explain their purpose.Consider adding brief explanations for each storage prefix:
* Redelegations: `52 | DelegatorAddrLen (1 byte) | DelegatorAddr | ValidatorAddrLen (1 byte) | ValidatorSrcAddr | ValidatorDstAddrLen (1 byte) | ValidatorDstAddr -> ProtocolBuffer(redelegation)` // Stores redelegation data * RedelegationsByValSrc: `53 | ValidatorSrcAddrLen (1 byte) | ValidatorSrcAddr | ValidatorDstAddrLen (1 byte) | ValidatorDstAddr | DelegatorAddrLen (1 byte) | DelegatorAddr -> nil` // Index for querying redelegations by source validator * RedelegationsByValDst: `54 | ValidatorDstAddrLen (1 byte) | ValidatorDstAddr | ValidatorSrcAddrLen (1 byte) | ValidatorSrcAddr | DelegatorAddrLen (1 byte) | DelegatorAddr -> nil` // Index for querying redelegations by destination validator * RedelegationByUnbondingId: `56 | UnbondingId -> 0x34 | DelegatorAddrLen (1 byte) | DelegatorAddr | ValidatorAddrLen (1 byte) | ValidatorSrcAddr | ValidatorDstAddr` // Maps unbonding IDs to redelegations
243-251: Clarify usage of RedelegationsByValSrc, RedelegationsByValDst, and RedelegationByUnbondingIdThe descriptions of these indices have been updated or added. Provide more context about their purpose and how they're used in the redelegation process.
Consider expanding the descriptions:
`RedelegationsByValSrc` is used for slashing based on the `ValidatorSrcAddr`. This index allows efficient lookup of redelegations that need to be slashed when the source validator is penalized. `RedelegationsByValDst` is used for slashing based on the `ValidatorDstAddr`. This index allows efficient lookup of redelegations that need to be considered when the destination validator is penalized. `RedelegationByUnbondingId` is an additional index that enables lookups for redelegations by the unbonding IDs of the containing redelegation entries. This helps in tracking and managing the redelegation process more efficiently, especially when coordinating with the unbonding process.🧰 Tools
🪛 LanguageTool
[style] ~245-~245: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...atorSrcAddr.RedelegationsByValDstis used for slashing based on theValidat...(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[grammar] ~247-~247: The word “lookup” is a noun. The verb is spelled with a white space.
Context: ... first map here is used for queries, to lookup all redelegations for a given delegator...(NOUN_VERB_CONFUSION)
320-320: Update UnbondingQueue storage prefixThe storage prefix for
UnbondingQueuehas been updated. Ensure this change is consistent with the implementation and explain its significance.Consider updating the line to:
* UnbondingQueue: `65 | format(time) -> []DVPair` // Stores the queue of unbonding delegations
331-331: Update RedelegationQueue storage prefixThe storage prefix for
RedelegationQueuehas been updated. Ensure this change is consistent with the implementation and explain its significance.Consider updating the line to:
* RedelegationQueue: `66 | format(time) -> []DVVTriplet` // Stores the queue of redelegations
342-342: Update ValidatorQueue storage prefixThe storage prefix for
ValidatorQueuehas been updated. Ensure this change is consistent with the implementation and explain its significance.Consider updating the line to:
* ValidatorQueue: `67 | format(time) -> []sdk.ValAddress` // Stores the queue of unbonding validators
Line range hint
351-365: Add description for ValidatorConsensusKeyRotationRecordQueueKeyA new section has been added for
ValidatorConsensusKeyRotationRecordQueueKey. This is an important addition that should be explained clearly.Consider restructuring this section for clarity:
#### ValidatorConsensusKeyRotationRecordQueueKey For the purpose of tracking progress of consensus pubkey rotations, the `ValidatorConsensusKeyRotationRecordQueueKey` is kept. * ValidatorConsensusKeyRotationRecordQueueKey: `103 | format(time) -> types.ValAddrsOfRotatedConsKeys` This queue serves the following purposes: 1. It acts as a unique identifier in the queue, representing a future time (calculated by adding the current block time to the unbonding period). 2. When a new item with the same waiting time enters the queue, the system retrieves the current store information, appends the `ValAddress` to the array, and updates the store. The stored object is defined as follows: ```protobuf reference https://github.com/cosmos/cosmos-sdk/blob/release/v0.52.x/x/staking/proto/cosmos/staking/v1beta1/staking.proto#L420-L424This mechanism ensures efficient tracking and management of consensus pubkey rotations over time.
--- `401-401`: **Clarify the transition from unbonding to unbonded** The description of the transition from unbonding to unbonded state could be clearer. Consider rewording this line for clarity: ```markdown A validator transitions from unbonding to unbonded state when the `ValidatorQueue` entry for this validator matures (i.e., the unbonding period has passed).
423-423: Clarify the delegation processThe description of the delegation process could be more detailed to provide a clearer understanding of what happens during delegation.
Consider expanding this section:
When a delegation occurs, both the validator and the delegation objects are affected. The process involves the following steps: 1. Calculate the delegator's shares based on the tokens delegated and the validator's current exchange rate. 2. Remove the delegated tokens from the delegator's account. 3. Add the calculated shares to the delegation object, or create a new delegation object if it doesn't exist. 4. Update the validator object with the new delegator shares. 5. Transfer the `delegation.Amount` from the delegator's account to the appropriate pool (`BondedPool` or `NotBondedPool`) based on the validator's status. 6. Update the `ValidatorByPowerIndex` to reflect the changes in the validator's total delegated tokens.
Line range hint
555-582: Add description for ConsPubkeyRotationA new section has been added for
ConsPubkeyRotation. This is an important addition that should be explained clearly.Consider restructuring this section for clarity:
## ConsPubkeyRotation The `ConsPubkey` of a validator can be instantly rotated to a new `ConsPubkey`. This rotation is tracked to allow only a limited number of rotations within an unbonding period. `ConsPubkeyRotation` data is indexed in the store as follows: * ValidatorConsPubKeyRotationHistoryKey: `101 | valAddr | rotatedHeight -> ProtocolBuffer(ConsPubKeyRotationHistory)` * BlockConsPubKeyRotationHistoryKey (index): `102 | rotatedHeight | valAddr | -> ProtocolBuffer(ConsPubKeyRotationHistory)` * ValidatorConsensusKeyRotationRecordQueueKey: `103 | format(time) -> ProtocolBuffer(ValAddrsOfRotatedConsKeys)` * ValidatorConsensusKeyRotationRecordIndexKey: `104 | valAddr | format(time) -> ProtocolBuffer([]Byte{})` * OldToNewConsAddrMap: `105 | byte(oldConsAddr) -> byte(newConsAddr)` * ConsAddrToValidatorIdentifierMap: `106 | byte(newConsAddr) -> byte(initialConsAddr)` These indices serve the following purposes: - `ConsPubKeyRotationHistory` is used for querying the rotation history of a validator. - `ValidatorConsensusKeyRotationRecordQueueKey` tracks rotations across the unbonding period and is pruned after the waiting period. - `ValidatorConsensusKeyRotationRecordIndexKey` keeps track of how many rotations a validator has made within the unbonding period and is pruned after the waiting period. - `OldToNewConsAddrMap` is updated for every rotation to handle evidences submitted with old consensus keys. - `ConsAddrToValidatorIdentifierMap` is updated for every rotation to prevent validators from rotating to a consensus key that has been used in the past. To prevent spam: - There is a limit on the number of rotations allowed within the unbonding period. - A non-negligible fee is deducted for rotating a consensus key. This mechanism ensures the integrity and security of the consensus process while allowing validators to update their keys when necessary.
207-207: Address grammar issues with "lookup" usageThe word "lookup" is used as a verb in several places, but it's grammatically incorrect. It should be "look up" when used as a verb.
Consider updating the following lines:
- Line 207: "...is used in queries, to look up all unbonding delegations for..."
- Line 210: "...is used in slashing, to look up all unbonding delegations associated with..."
- Line 240: "...is used for queries, to look up all redelegations for a given delegator..."
- Line 247: "The first map here is used for queries, to look up all redelegations for a given delegator..."
Also applies to: 210-210, 240-240, 247-247
🧰 Tools
🪛 LanguageTool
[grammar] ~207-~207: The word “lookup” is a noun. The verb is spelled with a white space.
Context: ...ndingDelegation` is used in queries, to lookup all unbonding delegations for a given ...(NOUN_VERB_CONFUSION)
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
📒 Files selected for processing (2)
- x/staking/README.md (16 hunks)
- x/staking/types/params.go (5 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
x/staking/README.md (1)
Pattern
**/*.md: "Assess the documentation for misspellings, grammatical errors, missing documentation and correctness"x/staking/types/params.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
🪛 LanguageTool
x/staking/README.md
[grammar] ~207-~207: The word “lookup” is a noun. The verb is spelled with a white space.
Context: ...ndingDelegation` is used in queries, to lookup all unbonding delegations for a given ...(NOUN_VERB_CONFUSION)
[grammar] ~210-~210: The word “lookup” is a noun. The verb is spelled with a white space.
Context: ...tionByValIndex` is used in slashing, to lookup all unbonding delegations associated w...(NOUN_VERB_CONFUSION)
[grammar] ~240-~240: The word “lookup” is a noun. The verb is spelled with a white space.
Context: ...Redelegationsis used for queries, to lookup all redelegations for a given delegato...(NOUN_VERB_CONFUSION)
[style] ~245-~245: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...atorSrcAddr.RedelegationsByValDstis used for slashing based on theValidat...(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[grammar] ~247-~247: The word “lookup” is a noun. The verb is spelled with a white space.
Context: ... first map here is used for queries, to lookup all redelegations for a given delegator...(NOUN_VERB_CONFUSION)
🔇 Additional comments (2)
x/staking/types/params.go (2)
22-22: LGTM: Improved constant documentationThe updated comment for
DefaultMaxValidatorsprovides a clearer description of the constant's purpose, which aligns with best practices for code documentation.
67-67: LGTM: Improved function documentationThe updated comments for
MustUnmarshalParamsandUnmarshalParamsprovide more accurate descriptions of the functions' behavior. The comment forMustUnmarshalParamsnow correctly indicates that it may panic, which is important information for users of this function.Also applies to: 77-77
(cherry picked from commit db6a835)
Co-authored-by: Julián Toledano <[email protected]>
Description
Ref:
#21429
Author Checklist
All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.
I have...
!in the type prefix if API or client breaking changeCHANGELOG.mdReviewers Checklist
All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.
Please see Pull Request Reviewer section in the contributing guide for more information on how to review a pull request.
I have...
Summary by CodeRabbit
Documentation
PowerReductionparameter.Bug Fixes