feat(x/staking): StakeAuthorization: Allow delegating to all validators #22527
Conversation
📝 WalkthroughWalkthroughThe pull request introduces modifications to the authorization (authz) module, specifically focusing on stake authorization. The changes primarily address the ability to delegate to all validators by allowing empty allow and deny lists. This involves updates to test cases, README documentation, and validation logic in the staking module. The modifications aim to provide more flexibility in authorization grants for delegation permissions. Changes
Assessment against linked issues
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
|
|
This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
|
Closing this as stale. Feel free to re-open if willing to continue. |
Back from vacation. Looking into it again |
ziscky
requested review from
facundomedica,
testinginprod,
JulianToledano and
a team
as code owners
github-prbot
assigned akhilkumarpilli, facundomedica, JulianToledano, raynaudoe and testinginprod
![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
🧹 Nitpick comments (3)
x/staking/CHANGELOG.md (1)
56-56: Consider moving the change to the "Features" section.The change to allow delegating
StakeAuthorizationto all validators appears to be a new feature rather than a bug fix, as it extends the functionality of the authorization system.🧰 Tools
🪛 Markdownlint (0.37.0)
56-56: null
Lists should be surrounded by blank lines(MD032, blanks-around-lists)
x/staking/types/authz_test.go (1)
Line range hint
87-87: Minor language improvement suggestedConsider replacing "amount of tokens" with "number of tokens" as it's more grammatically correct when referring to countable items.
-`MaxTokens` that keeps track of a limit to the amount of tokens that can be delegated/undelegated/redelegated. +`MaxTokens` that keeps track of a limit to the number of tokens that can be delegated/undelegated/redelegated.x/authz/README.md (1)
87-87: Consider improving grammar.Replace "amount of tokens" with "number of tokens" as "tokens" is a countable noun.
-`StakeAuthorization` implements the `Authorization` interface for messages in the [staking module](https://docs.cosmos.network/main/build/modules/staking). It takes an `AuthorizationType` to specify whether you want to authorize delegation, undelegation, redelegation or cancel unbonding delegation, each of which must be authorized separately. It also takes an optional `MaxTokens` that keeps track of a limit to the amount of tokens that can be delegated/undelegated/redelegated. If left empty, the amount is unlimited. Additionally, this Msg takes an `AllowList` or a `DenyList`, enabling you to specify which validators the grantee can or cannot stake with. If both `AllowList` and `DenyList` are empty delegation to all validators is allowed. +`StakeAuthorization` implements the `Authorization` interface for messages in the [staking module](https://docs.cosmos.network/main/build/modules/staking). It takes an `AuthorizationType` to specify whether you want to authorize delegation, undelegation, redelegation or cancel unbonding delegation, each of which must be authorized separately. It also takes an optional `MaxTokens` that keeps track of a limit to the number of tokens that can be delegated/undelegated/redelegated. If left empty, the amount is unlimited. Additionally, this Msg takes an `AllowList` or a `DenyList`, enabling you to specify which validators the grantee can or cannot stake with. If both `AllowList` and `DenyList` are empty delegation to all validators is allowed.🧰 Tools
🪛 LanguageTool
[uncategorized] ~87-~87: ‘Amount of’ should usually only be used with uncountable or mass nouns. Consider using “number” if this is not the case.
Context: ...ens` that keeps track of a limit to the amount of tokens that can be delegated/undeleg...(AMOUNTOF_TO_NUMBEROF)
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
tests/systemtests/authz_test.go(2 hunks)x/authz/README.md(1 hunks)x/staking/CHANGELOG.md(1 hunks)x/staking/types/authz.go(0 hunks)x/staking/types/authz_test.go(1 hunks)
💤 Files with no reviewable changes (1)
- x/staking/types/authz.go
🧰 Additional context used
📓 Path-based instructions (4)
x/authz/README.md (1)
Pattern **/*.md: "Assess the documentation for misspellings, grammatical errors, missing documentation and correctness"
x/staking/types/authz_test.go (2)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern **/*_test.go: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"
x/staking/CHANGELOG.md (1)
Pattern **/*.md: "Assess the documentation for misspellings, grammatical errors, missing documentation and correctness"
tests/systemtests/authz_test.go (3)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern tests/**/*: "Assess the integration and e2e test code assessing sufficient code coverage for the changes associated in the pull request"
Pattern **/*_test.go: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"
🪛 LanguageTool
x/authz/README.md
[uncategorized] ~87-~87: ‘Amount of’ should usually only be used with uncountable or mass nouns. Consider using “number” if this is not the case.
Context: ...ens` that keeps track of a limit to the amount of tokens that can be delegated/undeleg...
(AMOUNTOF_TO_NUMBEROF)
🪛 Markdownlint (0.37.0)
x/staking/CHANGELOG.md
56-56: null
Lists should be surrounded by blank lines
(MD032, blanks-around-lists)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Summary
🔇 Additional comments (8)
x/staking/types/authz_test.go (3)
214-228: Well-structured test case for empty validator lists!The test case effectively validates that delegation is allowed when both allow and deny lists are empty, which is a key requirement for enabling delegation to all validators.
Line range hint
53-58: LGTM! Good test setup.The addition of new grantee addresses properly expands the test coverage for the new authorization scenarios.
Line range hint
169-174: Comprehensive test coverage for empty validator lists!The new test cases effectively verify that all staking operations (delegate/unbond/redelegate) work correctly without specifying allow or deny lists, while properly enforcing spend limits.
Also applies to: 182-188, 196-202
tests/systemtests/authz_test.go (4)
53-58: LGTM!The new grantee address declarations follow the existing pattern and include proper validation checks against the granter address.
169-174: LGTM!The new test case properly validates the delegate authorization without allow or deny list, which aligns with the PR objective to allow delegating to all validators.
182-188: LGTM!The new test case properly validates the unbond authorization without allow or deny list, maintaining consistency with the delegate authorization test case.
196-202: LGTM!The new test case properly validates the redelegate authorization without allow or deny list, maintaining consistency with the delegate and unbond authorization test cases.
x/authz/README.md (1)
87-87: LGTM!The added clarification about empty allow and deny lists properly documents the new behavior, aligning with the PR objective to allow delegating to all validators.
🧰 Tools
🪛 LanguageTool
[uncategorized] ~87-~87: ‘Amount of’ should usually only be used with uncountable or mass nouns. Consider using “number” if this is not the case.
Context: ...ens` that keeps track of a limit to the amount of tokens that can be delegated/undeleg...(AMOUNTOF_TO_NUMBEROF)
Description
Closes: #11126
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
New Features
Bug Fixes
Documentation
Tests