feat(x): add module_query_safe on queries #22095
Conversation
📝 Walkthrough📝 Walkthrough📝 Walkthrough📝 WalkthroughWalkthroughThe pull request updates the changelogs and proto files for the 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
|
Teyz
requested review from
julienrbrt,
sontrinh16,
a team,
alpe and
JulianToledano
as code owners
|
For the reviewers, here is the definition of |
![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 (8)
x/distribution/CHANGELOG.md (5)
33-33: Remove or populate the empty bullet point in the Features section.An empty bullet point doesn't provide any information to the readers. Either remove this line or add details about a new feature introduced in this version.
35-36: Improvements section looks good, minor formatting suggestion.The new entries in the Improvements section are clear and informative. The second entry directly addresses the PR objective of marking more queries with
module_query_safe.For consistency, consider using the same formatting for both entries:
* [#20790](https://github.com/cosmos/cosmos-sdk/pull/20790) `x/distribution` does not depend on `x/protocolpool` anymore, now `x/distribution` only does token transfers and `x/protocolpool` does the rest. * [#20219](https://github.com/cosmos/cosmos-sdk/issues/20219) Mark more queries with `module_query_safe`.
Line range hint
38-63: API Breaking Changes section is comprehensive, minor suggestion for clarity.The API Breaking Changes section provides detailed information about the changes, which is crucial for developers. The entries align well with the PR objectives and the AI-generated summary.
To improve clarity, consider grouping related changes together. For example, you could group all the changes related to the use of collections for state management under a single bullet point with sub-points. This would make it easier for readers to understand the overall direction of the changes.
Example:
* Use of collections for state management: * [#17115](https://github.com/cosmos/cosmos-sdk/pull/17115) `PreviousProposer` and `ValidatorSlashEvents` * [#16483](https://github.com/cosmos/cosmos-sdk/pull/16483) `DelegatorStartingInfo` * [#16571](https://github.com/cosmos/cosmos-sdk/pull/16571) `ValidatorAccumulatedCommission` * [#16590](https://github.com/cosmos/cosmos-sdk/pull/16590) `ValidatorOutstandingRewards` * [#16607](https://github.com/cosmos/cosmos-sdk/pull/16607) `ValidatorHistoricalRewards` * [#16440](https://github.com/cosmos/cosmos-sdk/pull/16440) `DelegatorWithdrawAddresState` * [#16459](https://github.com/cosmos/cosmos-sdk/pull/16459) `ValidatorCurrentRewards`This grouping would make the changelog more readable while still preserving all the necessary information.
Line range hint
71-75: State Machine Breaking section is informative, minor suggestion for consistency.The State Machine Breaking section provides clear information about significant changes to the state machine. These changes align well with the PR objectives and the AI-generated summary.
For consistency with other sections, consider adding links to the corresponding pull requests for each entry. For example:
* [#20735](https://github.com/cosmos/cosmos-sdk/pull/20735) Remove PreviousProposer from the state machine. * [#17657](https://github.com/cosmos/cosmos-sdk/pull/17657) Migrate community pool funds from `x/distribution` to `x/protocolpool`. * [#17115](https://github.com/cosmos/cosmos-sdk/pull/17115) Migrate `PreviousProposer` to collections. * [#18539](https://github.com/cosmos/cosmos-sdk/pull/18539) Introduce `FeePool.DecimalPool` to replace `FeePool.CommunityPool`, which temporarily holds fractional rewards until they are distributed to the community pool every 1000 blocks.This would make it easier for readers to find more detailed information about each change if needed.
Line range hint
80-81: Consider adding more details to the Bug Fixes entry.While it's good to mention the fix for the vulnerability in
incrementReferenceCount, providing more context would be helpful for users. Consider adding brief details about:
- The nature of the vulnerability (without exposing sensitive information).
- Potential impact on users or the system.
- Versions affected (if applicable).
This additional information would help users understand the importance of the fix and whether they need to take any action.
x/gov/CHANGELOG.md (1)
49-49: Enhance the changelog entry with more details.The new entry accurately reflects the PR objectives, but it could benefit from more specificity. Consider expanding it to include:
- The specific modules affected (
x/distributionandx/gov).- The purpose of marking these queries as
module_query_safe(to enable execution by interchain accounts).Suggested revision:
* [#20219](https://github.com/cosmos/cosmos-sdk/issues/20219#event-14494455201) Mark more queries in `x/distribution` and `x/gov` modules with `module_query_safe` to enable execution by interchain accounts.This provides more context and aligns closely with the PR objectives.
x/gov/proto/cosmos/gov/v1beta1/query.proto (1)
Line range hint
1-63: Summary: All gov queries successfully marked as module_query_safeThis PR successfully adds the
module_query_safeoption to all query RPCs in the gov module. These changes align perfectly with the PR objectives, enabling these queries to be executed by interchain accounts. The implementation is consistent across all methods and should significantly enhance the functionality of interchain accounts within the Cosmos ecosystem.Consider the following suggestions for future improvements:
- Update documentation to reflect these changes and their implications for interchain account usage.
- Ensure that any client-side code interacting with these RPCs is updated to handle potential changes in behavior.
- Consider adding similar changes to other relevant modules to further expand interchain account capabilities.
x/distribution/proto/cosmos/distribution/v1beta1/query.proto (1)
278-278: Consider updating documentation for CommunityPool deprecationThe
CommunityPoolRPC method and its associated messages have been marked as deprecated. While this is not directly related to the PR objective of addingmodule_query_safeoptions, it's an important change that affects the query service.Consider updating any relevant documentation or guides to reflect this deprecation and to direct users to the
x/protocolpoolmodule'sCommunityPoolRPC method instead.
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
📒 Files selected for processing (4)
- x/distribution/CHANGELOG.md (1 hunks)
- x/distribution/proto/cosmos/distribution/v1beta1/query.proto (3 hunks)
- x/gov/CHANGELOG.md (1 hunks)
- x/gov/proto/cosmos/gov/v1beta1/query.proto (2 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
x/distribution/CHANGELOG.md (1)
Pattern
**/*.md: "Assess the documentation for misspellings, grammatical errors, missing documentation and correctness"x/gov/CHANGELOG.md (1)
Pattern
**/*.md: "Assess the documentation for misspellings, grammatical errors, missing documentation and correctness"
🔇 Additional comments (23)
x/distribution/CHANGELOG.md (2)
Line range hint
68-69: CLI Breaking Changes section is clear and informative.The entry in the CLI Breaking Changes section clearly explains the change in the
withdraw-rewardscommand's behavior and provides information about the new command for withdrawing validator commission. This is an important breaking change that users need to be aware of, and it's documented well.
Line range hint
77-78: Client Breaking Changes section is clear and helpful.The entry in the Client Breaking Changes section clearly explains the deprecation of the
CommunityPoolandFundCommunityPoolRPC methods and directs users to the new methods in thex/protocolpoolmodule. This information is crucial for clients using these methods and is documented well.x/gov/CHANGELOG.md (1)
Line range hint
1-49: Overall changelog structure and content review.The changelog follows a clear and organized structure, which is commendable. However, there are a few points to consider:
- Consistency: Ensure all entries start with a capital letter and end with a period for uniformity.
- Versioning: The changes are under the "Unreleased" section. Consider adding a version number if these changes are for a specific upcoming release.
- Breaking Changes: The presence of multiple breaking changes sections (State Machine, Client, and API) is good for clarity but ensure all breaking changes are appropriately categorized.
- Deprecated Section: Good practice to include this section, but make sure to provide migration paths or alternatives for deprecated features.
Overall, the changelog is well-structured and informative, providing a comprehensive overview of the changes in the
x/govmodule.x/gov/proto/cosmos/gov/v1beta1/query.proto (9)
9-9: LGTM: Import added for module_query_safe optionThe addition of this import is necessary and aligns with the PR objective of marking queries as
module_query_safe.
18-18: LGTM: Proposal query marked as module_query_safeThis change enables the Proposal query to be executed by interchain accounts, aligning with the PR objective.
24-24: LGTM: Proposals query marked as module_query_safeThis change enables the Proposals query to be executed by interchain accounts, aligning with the PR objective.
31-31: LGTM: Vote query marked as module_query_safeThis change enables the Vote query to be executed by interchain accounts, aligning with the PR objective.
37-37: LGTM: Votes query marked as module_query_safeThis change enables the Votes query to be executed by interchain accounts, aligning with the PR objective.
43-43: LGTM: Params query marked as module_query_safeThis change enables the Params query to be executed by interchain accounts, aligning with the PR objective.
49-49: LGTM: Deposit query marked as module_query_safeThis change enables the Deposit query to be executed by interchain accounts, aligning with the PR objective.
55-55: LGTM: Deposits query marked as module_query_safeThis change enables the Deposits query to be executed by interchain accounts, aligning with the PR objective.
61-61: LGTM: TallyResult query marked as module_query_safeThis change enables the TallyResult query to be executed by interchain accounts, aligning with the PR objective.
x/distribution/proto/cosmos/distribution/v1beta1/query.proto (11)
10-10: LGTM: Import added for module_query_safe optionThe addition of
import "cosmos/query/v1/query.proto";is correct and necessary for using themodule_query_safeoption in the RPC methods.
19-19: LGTM: module_query_safe option added to Params RPCThe addition of
option (cosmos.query.v1.module_query_safe) = true;to theParamsRPC method is correct and aligns with the PR objective of enabling these queries to be executed by interchain accounts.
26-26: LGTM: module_query_safe option added to ValidatorDistributionInfo RPCThe addition of
option (cosmos.query.v1.module_query_safe) = true;to theValidatorDistributionInfoRPC method is correct and consistent with the PR objective.
33-33: LGTM: module_query_safe option added to ValidatorOutstandingRewards RPCThe addition of
option (cosmos.query.v1.module_query_safe) = true;to theValidatorOutstandingRewardsRPC method is correct and consistent with the PR objective.
40-40: LGTM: module_query_safe option added to ValidatorCommission RPCThe addition of
option (cosmos.query.v1.module_query_safe) = true;to theValidatorCommissionRPC method is correct and consistent with the PR objective.
47-47: LGTM: module_query_safe option added to ValidatorSlashes RPCThe addition of
option (cosmos.query.v1.module_query_safe) = true;to theValidatorSlashesRPC method is correct and consistent with the PR objective.
53-53: LGTM: module_query_safe option added to DelegationRewards RPCThe addition of
option (cosmos.query.v1.module_query_safe) = true;to theDelegationRewardsRPC method is correct and consistent with the PR objective.
61-61: LGTM: module_query_safe option added to DelegationTotalRewards RPCThe addition of
option (cosmos.query.v1.module_query_safe) = true;to theDelegationTotalRewardsRPC method is correct and consistent with the PR objective.
67-67: LGTM: module_query_safe option added to DelegatorValidators RPCThe addition of
option (cosmos.query.v1.module_query_safe) = true;to theDelegatorValidatorsRPC method is correct and consistent with the PR objective.
74-74: LGTM: module_query_safe option added to DelegatorWithdrawAddress RPCThe addition of
option (cosmos.query.v1.module_query_safe) = true;to theDelegatorWithdrawAddressRPC method is correct and consistent with the PR objective.
Line range hint
1-278: Summary: module_query_safe options added and CommunityPool deprecatedThis PR successfully adds the
module_query_safeoption to all relevant RPC methods in the distribution module's query service. These changes align with the objective of enabling these queries to be executed by interchain accounts.Key points:
- The necessary import for the
module_query_safeoption has been added.- All relevant RPC methods now include the
module_query_safeoption.- The
CommunityPoolRPC method and its associated messages have been deprecated, with a suggestion to use thex/protocolpoolmodule'sCommunityPoolRPC method instead.These changes enhance the functionality of the distribution module within the Cosmos SDK ecosystem, particularly for interchain operations. The deprecation of the
CommunityPoolelements should be noted for future updates to documentation and client implementations.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
api/cosmos/gov/v1beta1/query.pulsar.go (1)
Line range hint
1-1000: Add license header.The file is missing a license header. Please add the appropriate license header at the top of the file.
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
⛔ Files ignored due to path filters (2)
x/distribution/types/query.pb.gois excluded by!**/*.pb.gox/gov/types/v1beta1/query.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (4)
- api/cosmos/distribution/v1beta1/query.pulsar.go (3 hunks)
- api/cosmos/gov/v1beta1/query.pulsar.go (2 hunks)
- x/distribution/proto/cosmos/distribution/v1beta1/query.proto (3 hunks)
- x/gov/proto/cosmos/gov/v1beta1/query.proto (2 hunks)
🔥 Files not summarized due to errors (1)
- api/cosmos/distribution/v1beta1/query.pulsar.go: Error: Server error: no LLM provider could handle the message
🚧 Files skipped from review as they are similar to previous changes (2)
- x/distribution/proto/cosmos/distribution/v1beta1/query.proto
- x/gov/proto/cosmos/gov/v1beta1/query.proto
🧰 Additional context used
📓 Path-based instructions (2)
api/cosmos/distribution/v1beta1/query.pulsar.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.api/cosmos/gov/v1beta1/query.pulsar.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
🔇 Additional comments (6)
api/cosmos/distribution/v1beta1/query.pulsar.go (4)
Line range hint
1-10623: Summary of changes in query.pulsar.goThe modifications in this file align with the PR objectives of marking additional queries with the
module_query_safetag. The changes include:
- Adding a new import for the cosmos query package.
- Modifications to the generated protobuf code.
- Updates to RPC methods with the
module_query_safetag.These changes appear to be correct and consistent with the PR goals. However, as this is a generated file, please ensure that:
- The corresponding
.protofiles have been updated appropriately.- The new import is being utilized in the codebase.
- All intended queries have been marked with the
module_query_safetag.As a final check, run the following command to ensure all intended queries are tagged:
#!/bin/bash # List all RPC methods and check for module_query_safe tag sed -n '/rpc/,/}/p' api/cosmos/distribution/v1beta1/query.pulsar.go | grep -E 'rpc|module_query_safe'
10572-10623: RPC methods updated withmodule_query_safetag.The changes add the
module_query_safetag to several RPC methods in the distribution module. This aligns with the PR objectives to enable these queries to be executed by interchain accounts.To verify the consistency of these changes:
#!/bin/bash # Check for the addition of module_query_safe tags in the file grep -n "module_query_safe" api/cosmos/distribution/v1beta1/query.pulsar.go
8-8: New import added for cosmos query package.The addition of
_ "cosmossdk.io/api/cosmos/query/v1"as a blank import suggests new query functionality might have been added. This looks good, but ensure it's being used in the code.To verify the usage of this import, run the following command:
✅ Verification successful
Blank import
_ "cosmossdk.io/api/cosmos/query/v1"is used consistently for side effects.The addition of
_ "cosmossdk.io/api/cosmos/query/v1"aligns with existing patterns in the codebase, indicating it's necessary for initializing query-related functionalities. No issues found.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for usage of the newly imported package grep -R "cosmos/query/v1" .Length of output: 1829
Line range hint
10275-10623: Changes in generated protobuf code.This section contains modifications to the generated protobuf code. As this is auto-generated, direct edits are not recommended. Please ensure that these changes correspond to modifications in the source
.protofiles.To verify the changes, compare the
.protofiles:api/cosmos/gov/v1beta1/query.pulsar.go (2)
Line range hint
32-38: LGTM!The
QueryProposalRequestmessage definition looks good.
Line range hint
32-38: Verify the error handling.The error handling looks correct. However, please verify that the error message "Vectors must have the same length" is appropriate and provides enough context for the user to understand and fix the issue.
| @@ -4,6 +4,7 @@ package govv1beta1 | |||
| import ( | |||
| _ "cosmossdk.io/api/amino" | |||
| v1beta1 "cosmossdk.io/api/cosmos/base/query/v1beta1" | |||
| _ "cosmossdk.io/api/cosmos/query/v1" | |||
There was a problem hiding this comment.
Remove unused import.
The _ "cosmossdk.io/api/cosmos/query/v1" import is unused and can be removed.
|
Going though all queries in @Teyz can you confirm that you did that before opening the PR? |
|
Yep |
|
Can we merge this PR? |
Hey! It needs to be throughly reviewed first. |
|
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. |
Description
Closes: #20219
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
x/distributionandx/govmodules, including custom tally functions and support for multiple choice proposals.Improvements
module_query_safefor various RPC methods.x/protocolpoolfor token transfers in thex/distributionmodule.Bug Fixes
incrementReferenceCountmethod within thex/distributionmodule.Breaking Changes
x/distributionandx/govmodules, including changes to method signatures and removal of deprecated features.