chore: extract improvements from #21497 #21506
Conversation
julienrbrt
added
the
backport/v0.52.x
julienrbrt
requested review from
a team,
alpe,
JulianToledano,
kocubinski,
facundomedica,
sontrinh16 and
hieuvubk
as code owners
WalkthroughWalkthroughThe changes introduce significant updates to the Cosmos SDK documentation and codebase, including the removal of the GRPC-WEB embedded client, the introduction of the Changes
Possibly related issues
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
|
github-actions
bot
added
C:x/distribution
julienrbrt
requested review from
tac0turtle and
testinginprod
and removed request for
alpe,
kocubinski,
JulianToledano,
facundomedica,
sontrinh16 and
hieuvubk
julienrbrt
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Outside diff range, codebase verification and nitpick comments (2)
simapp/v2/app_config.go (1)
267-267: LGTM!The code change is approved. The comment serves as a useful TODO to remove the hardcoded
Authorityvalue in the future.Consider creating a GitHub issue to track the removal of the hardcoded
Authorityvalue, ensuring that it is not forgotten in future development.UPGRADING.md (1)
404-404: Consider using "before" instead of "prior to" for conciseness.The static analysis tool LanguageTool correctly points out that "prior to" can be wordy. Using "before" is a more concise alternative that maintains the same meaning:
- It is required to migrate to v0.50 prior to upgrading to v0.52 for not missing any module migrations. + It is required to migrate to v0.50 before upgrading to v0.52 to avoid missing any module migrations.Tools
LanguageTool
[style] ~404-~404: ‘prior to’ might be wordy. Consider a shorter alternative.
Context: ...ved. It is required to migrate to v0.50 prior to upgrading to v0.52 for not missing any ...(EN_WORDINESS_PREMIUM_PRIOR_TO)
Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Files selected for processing (6)
- UPGRADING.md (5 hunks)
- docs/learn/advanced/00-baseapp.md (1 hunks)
- server/v2/cometbft/abci.go (1 hunks)
- server/v2/types.go (1 hunks)
- simapp/v2/app_config.go (2 hunks)
- x/distribution/keeper/abci.go (1 hunks)
Files skipped from review due to trivial changes (2)
- docs/learn/advanced/00-baseapp.md
- x/distribution/keeper/abci.go
Additional context used
Path-based instructions (4)
server/v2/types.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.simapp/v2/app_config.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.server/v2/cometbft/abci.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.UPGRADING.md (1)
Pattern
**/*.md: "Assess the documentation for misspellings, grammatical errors, missing documentation and correctness"
LanguageTool
UPGRADING.md
[style] ~404-~404: ‘prior to’ might be wordy. Consider a shorter alternative.
Context: ...ved. It is required to migrate to v0.50 prior to upgrading to v0.52 for not missing any ...(EN_WORDINESS_PREMIUM_PRIOR_TO)
Additional comments not posted (2)
server/v2/types.go (1)
19-19: Verify the reason and timeline for removing theGetConsensusAuthoritymethod.The comment indicates a plan to remove this method in the future. Please ensure that:
- The reason for the removal is clearly documented and communicated to the relevant stakeholders.
- A timeline for the removal is established, considering the impact on the implementations of the
AppIinterface.- A tracking issue is created to plan and execute the removal.
To verify the reason for the planned removal, run the following script:
simapp/v2/app_config.go (1)
141-144: LGTM!The code changes are approved. The new entry for the
accountsmodule is correctly added to theOverrideStoreKeysconfiguration, specifying theModuleNameandKvStoreKeyusing the constants from theaccountspackage.
| @@ -67,7 +67,7 @@ type Consensus[T transaction.Tx] struct { | |||
| func NewConsensus[T transaction.Tx]( | |||
| logger log.Logger, | |||
| appName string, | |||
| consensusAuthority string, | |||
| consensusAuthority string, // TODO remove | |||
There was a problem hiding this comment.
Ensure all references to consensusAuthority are removed when removing the parameter.
The TODO comment indicates the intention to remove the consensusAuthority parameter in the future. When doing so, please make sure to:
- Update the
NewConsensusfunction body to remove any references toconsensusAuthority. - Update any code that invokes
NewConsensusto remove theconsensusAuthorityargument.
Consider creating a GitHub issue to track the removal of the consensusAuthority parameter and link it to the TODO comment.
There was a problem hiding this comment.
Actionable comments posted: 1
Outside diff range, codebase verification and nitpick comments (4)
server/v2/cometbft/abci.go (1)
70-70: TODO comment indicates future removal ofconsensusAuthorityparameter.The comment serves as a proactive flag for potential refactoring or removal of the
consensusAuthorityparameter in subsequent updates. The current implementation remains unaffected.Consider creating a GitHub issue to track the removal of the
consensusAuthorityparameter, ensuring it is addressed in a future update. This will help maintain a cleaner function signature and improve code clarity.UPGRADING.md (3)
404-404: LGTM with a minor suggestionThe migration instructions are clear and accurate. Just a minor suggestion - consider replacing "prior to" with "before" for more concise phrasing. For example:
"It is required to migrate to v0.50 before upgrading to v0.52 to avoid missing any module migrations."
But this is just a minor refinement suggestion, feel free to ignore it if you prefer the current phrasing.
Tools
LanguageTool
[style] ~404-~404: ‘prior to’ might be wordy. Consider a shorter alternative.
Context: ...ved. It is required to migrate to v0.50 prior to upgrading to v0.52 for not missing any ...(EN_WORDINESS_PREMIUM_PRIOR_TO)
447-447: More details neededThe migration instruction to add the new
x/protocolpoolmodule for chains usingx/distributionis important. However, it would be helpful to include more details or a code example showing how to add this new module to the existing app.Could you please expand this section with the necessary steps or code changes required to properly add the
x/protocolpoolmodule?
458-458: Code example neededThe introduction of the new
x/protocolpoolmodule to handle community pool funds is clearly explained. However, a code example demonstrating how to add thex/protocolpoolstore during the upgrade to v0.52.x would be very helpful.Could you please include a code snippet showing the necessary store upgrades to add the
x/protocolpoolstore?
Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Files selected for processing (6)
- UPGRADING.md (5 hunks)
- docs/learn/advanced/00-baseapp.md (1 hunks)
- server/v2/cometbft/abci.go (1 hunks)
- server/v2/types.go (1 hunks)
- simapp/v2/app_config.go (2 hunks)
- x/distribution/keeper/abci.go (1 hunks)
Files skipped from review due to trivial changes (2)
- docs/learn/advanced/00-baseapp.md
- x/distribution/keeper/abci.go
Additional context used
Path-based instructions (4)
server/v2/types.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.simapp/v2/app_config.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.server/v2/cometbft/abci.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.UPGRADING.md (1)
Pattern
**/*.md: "Assess the documentation for misspellings, grammatical errors, missing documentation and correctness"
LanguageTool
UPGRADING.md
[style] ~404-~404: ‘prior to’ might be wordy. Consider a shorter alternative.
Context: ...ved. It is required to migrate to v0.50 prior to upgrading to v0.52 for not missing any ...(EN_WORDINESS_PREMIUM_PRIOR_TO)
Additional comments not posted (6)
simapp/v2/app_config.go (2)
141-144: LGTM!The code changes are approved. The
accountsmodule is correctly added to the application's configuration with the appropriate module name and store key.
267-267: Acknowledged the TODO comment.The comment indicates a plan to remove the hardcoded
Authoritystring for theconsensusmodule configuration in the future. This doesn't introduce any immediate issues but serves as a reminder for a future cleanup.UPGRADING.md (4)
109-109: LGTM!The migration instructions for updating Tendermint imports to CometBFT look good.
350-351: LGTM!The migration instructions for updating module import paths look good.
443-443: LGTM!The removal of the unsupported
x/crisismodule is clearly documented.
Line range hint
475-481: Excellent!Thank you for adding the code example demonstrating how to include the
x/protocolpoolstore during the upgrade to v0.52.x. This is exactly what was needed to provide clarity on the upgrade process.
| @@ -16,7 +16,7 @@ type AppI[T transaction.Tx] interface { | |||
| Name() string | |||
| InterfaceRegistry() server.InterfaceRegistry | |||
| GetAppManager() *appmanager.AppManager[T] | |||
| GetConsensusAuthority() string | |||
| GetConsensusAuthority() string // TODO remove | |||
There was a problem hiding this comment.
Acknowledge the TODO comment for removing the GetConsensusAuthority method.
The comment // TODO remove indicates a future intention to remove the GetConsensusAuthority method from the AppI interface. This change suggests a potential shift in the design or functionality of the interface, indicating that the method may no longer be necessary or relevant in future iterations of the codebase.
Consider creating a GitHub issue to track the removal of the GetConsensusAuthority method. This will help ensure that the removal is properly planned and executed in a future update. Do you want me to open a GitHub issue for this?
There was a problem hiding this comment.
Actionable comments posted: 3
Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Files selected for processing (1)
- UPGRADING.md (5 hunks)
Additional context used
Path-based instructions (1)
UPGRADING.md (1)
Pattern
**/*.md: "Assess the documentation for misspellings, grammatical errors, missing documentation and correctness"
LanguageTool
UPGRADING.md
[style] ~404-~404: ‘prior to’ might be wordy. Consider a shorter alternative.
Context: ...ved. It is required to migrate to v0.50 prior to upgrading to v0.52 for not missing any ...(EN_WORDINESS_PREMIUM_PRIOR_TO)
Additional comments not posted (3)
UPGRADING.md (3)
109-109: LGTM!The change from
github.com/tendermint/tenderminttogithub.com/cometbft/cometbftis approved as part of the CometBFT migration.
350-351: Important: Update module imports!All modules (except
auth) now have their owngo.modfile. Make sure to replace their imports withcosmossdk.io/x/{moduleName}across your codebase.
Line range hint
475-480: Looks good!The provided code example for adding the
x/protocolpoolstore during an upgrade to v0.52.x is correct and helpful.
| ### `x/crsis` | ||
|
|
||
| The Crisis module was removed due to it not being supported or functional any longer. | ||
| The `x/crisis` module was removed due to it not being supported or functional any longer. |
There was a problem hiding this comment.
Action Required: Remove x/crisis module usage
The x/crisis module has been removed as it is no longer supported. If your application is using this module, make sure to remove it before upgrading to this version.
There was a problem hiding this comment.
| The `x/crisis` module was removed due to it not being supported or functional any longer. | |
| The `x/crisis` module was removed due to it's not being supported or functional any longer. |
There was a problem hiding this comment.
@akhilkumarpilli, the original sentence is grammatically correct. The phrase "it's" is a contraction of "it is" or "it has," which doesn't fit the context here. The correct possessive form is "its," so the original sentence should remain as is.
| #### `x/group` | ||
|
|
||
| Group was spun out into its own `go.mod`. To import it use `cosmossdk.io/x/group` | ||
| Existing chains using `x/distribution` module must add the new `x/protocolpool` module. |
There was a problem hiding this comment.
Required: Add x/protocolpool module
If your chain is using the x/distribution module, you must add the new x/protocolpool module when upgrading to this version.
| #### `x/protocolpool` | ||
|
|
||
| Introducing a new `x/protocolpool` module to handle community pool funds. Its store must be added while upgrading to v0.51.x. | ||
| Introducing a new `x/protocolpool` module to handle community pool funds. Its store must be added while upgrading to v0.52.x. |
There was a problem hiding this comment.
Required: Add x/protocolpool store during upgrade
When upgrading to v0.52.x, make sure to add the x/protocolpool store as part of the upgrade process.
| ### `x/crsis` | ||
|
|
||
| The Crisis module was removed due to it not being supported or functional any longer. | ||
| The `x/crisis` module was removed due to it not being supported or functional any longer. |
There was a problem hiding this comment.
| The `x/crisis` module was removed due to it not being supported or functional any longer. | |
| The `x/crisis` module was removed due to it's not being supported or functional any longer. |
Co-authored-by: Akhil Kumar P <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Files selected for processing (1)
- UPGRADING.md (5 hunks)
Additional context used
Path-based instructions (1)
UPGRADING.md (1)
Pattern
**/*.md: "Assess the documentation for misspellings, grammatical errors, missing documentation and correctness"
LanguageTool
UPGRADING.md
[style] ~404-~404: ‘prior to’ might be wordy. Consider a shorter alternative.
Context: ...ved. It is required to migrate to v0.50 prior to upgrading to v0.52 for not missing any ...(EN_WORDINESS_PREMIUM_PRIOR_TO)
Additional comments not posted (4)
UPGRADING.md (4)
109-109: LGTM!The code changes are approved.
350-351: LGTM!The code changes are approved.
441-443: LGTM!The code changes are approved.
Line range hint
475-483: LGTM!The example for adding the
x/protocolpoolstore while upgrading to v0.52.x looks good.
Co-authored-by: Akhil Kumar P <[email protected]> (cherry picked from commit 6ffa71a) # Conflicts: # server/v2/types.go
Co-authored-by: Julien Robert <[email protected]>
Description
ref #21497 (comment)
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 Updates
New Features
x/protocolpoolmodule for handling community pool funds.Chores