docs(x/distribution): updates #22148
Conversation
📝 WalkthroughWalkthroughThe changes in this pull request involve updates to documentation and code within 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 (9)
x/distribution/README.md (7)
Line range hint
1-41: LGTM! Overview section improvementsThe changes in the overview section enhance clarity and readability. The refined explanation of the distribution mechanism and the enhanced description of the withdrawal process provide a better understanding for readers.
Consider adding a brief mention of the global reward pool in the first paragraph to provide context for the later discussion of reward pooling.
Line range hint
64-80: LGTM! Clearer explanation of staking effectsThe clarification of the relationship between commission charges and auto-bonding of Atom provisions improves the understanding of the staking mechanism's complexities.
Consider adding a brief explanation of why stakeholders might want to set up a script to periodically withdraw and rebond rewards, as mentioned in the last sentence.
Line range hint
97-102: LGTM! Enhanced clarity in Concepts sectionThe rewording of the reward calculation explanation and the clarification of conditions for updating rewards improve the overall understanding of the distribution mechanism.
Consider adding a brief example or diagram to illustrate the reward calculation process, which could further enhance reader comprehension.
Line range hint
104-166: LGTM! Improved state descriptionsThe updates to the FeePool, ValidatorDistInfo, and DelegationDistInfo descriptions provide more accurate and detailed information about state management in the distribution module.
In the FeePool description, consider clarifying how often the decimal rewards are converted to
sdk.Coinsand sent to the community pool.
Line range hint
168-185: LGTM! Clearer parameter descriptionsThe streamlined description of parameters and the clear notation of deprecated parameters enhance the understanding of the module's configuration options.
Consider adding a brief explanation of why
baseproposerrewardandbonusproposerrewardwere deprecated in v0.47, to provide context for the change.
Line range hint
272-442: LGTM! Enhanced message explanationsThe updates to the explanations of various messages and the addition of code examples significantly improve the understanding of message handling in the distribution module.
Consider adding a brief explanation of the potential impact of the
MsgUpdateParamsmessage on the module's behavior, to help users understand the implications of parameter changes.
Line range hint
444-1037: LGTM! Improved client interaction documentationThe updates to the CLI commands and gRPC endpoints, along with clearer examples and refined explanations, significantly enhance the user's ability to interact with the distribution module.
Consider adding a brief troubleshooting section or common pitfalls to watch out for when using these commands and endpoints, to further assist users in their interactions with the module.
x/distribution/keeper/delegation.go (2)
Line range hint
88-199: Simplify error handling in the iterator callback function.In
CalculateDelegationRewards, the use of theiterErrvariable to capture errors from the iterator callback can be simplified. Modifying the callback to return an error directly enhances readability and aligns with Go error-handling best practices.Apply this diff to streamline error handling:
if endingHeight > startingHeight { - err = k.IterateValidatorSlashEventsBetween(ctx, valAddr, startingHeight, endingHeight, - func(height uint64, event types.ValidatorSlashEvent) (stop bool) { + err = k.IterateValidatorSlashEventsBetween(ctx, valAddr, startingHeight, endingHeight, + func(height uint64, event types.ValidatorSlashEvent) (stop bool, err error) { endingPeriod := event.ValidatorPeriod if endingPeriod > startingPeriod { delRewards, err := k.calculateDelegationRewardsBetween(ctx, val, startingPeriod, endingPeriod, stake) if err != nil { - iterErr = err - return true + return true, err } rewards = rewards.Add(delRewards...) // Note: It is necessary to truncate so we don't allow withdrawing // more rewards than owed. stake = stake.MulTruncate(math.LegacyOneDec().Sub(event.Fraction)) startingPeriod = endingPeriod } - return false + return false, nil }, ) - if iterErr != nil { - return sdk.DecCoins{}, iterErr - } if err != nil { return sdk.DecCoins{}, err } }This change allows errors to be returned directly from the iterator function, eliminating the need for the
iterErrvariable and simplifying the control flow.
Line range hint
203-287: Add unit tests forwithdrawDelegationRewards.The new method
withdrawDelegationRewardsis a significant addition to theKeeper. To ensure its reliability and correctness, consider adding comprehensive unit tests that cover various scenarios, including:
- Successful reward withdrawal.
- Cases where the delegator has no rewards.
- Error conditions, such as missing starting info or failure in sending coins.
Would you like assistance in creating these unit tests or opening a GitHub issue to track this task?
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
📒 Files selected for processing (4)
- x/distribution/README.md (2 hunks)
- x/distribution/keeper/delegation.go (3 hunks)
- x/distribution/keeper/hooks.go (4 hunks)
- x/distribution/keeper/keeper.go (2 hunks)
✅ Files skipped from review due to trivial changes (1)
- x/distribution/keeper/keeper.go
🧰 Additional context used
📓 Path-based instructions (3)
x/distribution/README.md (1)
Pattern
**/*.md: "Assess the documentation for misspellings, grammatical errors, missing documentation and correctness"x/distribution/keeper/delegation.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.x/distribution/keeper/hooks.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
🔇 Additional comments (9)
x/distribution/keeper/hooks.go (5)
23-26: LGTM: Improved method documentationThe updated comment for the
Hooksmethod is more descriptive and follows the Golang convention of starting with the function name. This change enhances code readability without altering functionality.
Line range hint
28-35: LGTM: Enhanced method documentationThe updated comment for the
AfterValidatorCreatedmethod provides a clearer description of the method's purpose. This change improves code documentation and maintainability without affecting the underlying functionality.
Line range hint
143-160: LGTM: Improved method documentationThe updated comment for the
BeforeDelegationSharesModifiedmethod provides a clearer description of the method's dual functionality (withdrawing delegation rewards and incrementing the period). This change enhances code readability and maintainability without affecting the underlying functionality.
162-170: LGTM: Enhanced method documentationThe updated comments for both
AfterDelegationModifiedandBeforeValidatorSlashedmethods provide clearer descriptions of their respective purposes. These changes improve code documentation and maintainability without affecting the underlying functionality.
Line range hint
132-140: Verify the removal ofdelAddrparameter usageThe updated comment for the
BeforeDelegationCreatedmethod is more concise and descriptive, which is good. However, thedelAddrparameter has been replaced with an underscore, indicating it's no longer used within the method. Please verify if this is intentional and that thedelAddris not needed for any operations within this method or for maintaining consistency with other parts of the codebase.To check for any other usages of
delAddrin similar contexts, you can run:x/distribution/README.md (3)
Line range hint
43-62: LGTM! Improved clarity in Shortcomings sectionThe rephrasing of the context around constant reward flows enhances readability while maintaining the essential information about the distribution mechanism's limitations.
Line range hint
82-95: LGTM! Updated table of contentsThe table of contents has been appropriately updated to reflect the current structure of the document, improving navigation for readers.
Line range hint
187-270: LGTM! Improved BeginBlock and distribution scheme explanationsThe refinements to the BeginBlock explanation and the updates to the distribution scheme provide a clearer understanding of the fee distribution process.
x/distribution/keeper/delegation.go (1)
Line range hint
49-72: FunctioncalculateDelegationRewardsBetweenis correctly implemented.The function includes proper input validation and error handling, and it accurately calculates the rewards accrued between two periods for a delegation. The sanity checks help prevent invalid inputs, and the computations use appropriate methods to ensure correctness.
julienrbrt
added
the
backport/v0.52.x
(cherry picked from commit 6211be6)
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