refactor(client/v2): remove gov dependency #22867
Conversation
📝 WalkthroughWalkthroughThe pull request introduces significant updates to the governance handling in the Cosmos SDK by replacing the old governance client and types with a new governance module. The changes primarily affect the Changes
Assessment against linked issues
Possibly related issues
Possibly related PRs
Suggested labels
Suggested reviewers
📜 Recent review detailsConfiguration used: .coderabbit.yml ⛔ Files ignored due to path filters (9)
📒 Files selected for processing (11)
✅ Files skipped from review due to trivial changes (3)
🧰 Additional context used📓 Path-based instructions (2)client/v2/internal/governance/gov.go (1)Pattern client/v2/autocli/msg.go (1)Pattern 🔇 Additional comments (14)client/v2/internal/governance/gov.go (3)
This function correctly adds governance proposal flags to the Cobra command, following best practices.
The function effectively reads command-line flags, initializes the
This function properly checks for empty message lists, wraps messages in protobuf Any types, and appends them to the proposal's messages. client/v2/go.mod (1)
The module dependency has been appropriately updated to the latest version, ensuring compatibility and leveraging new features. x/gov/go.mod (1)
Consistently updating the x/feegrant/go.mod (1)
The dependency update ensures that client/v2/autocli/msg.go (4)
The addition of the internal governance package aligns with the PR objective of removing the external gov dependency.
The governance proposal flag handling has been cleanly migrated to use the internal governance package. Line range hint The governance module address handling has been updated to use the new internal governance package while maintaining the same functionality. Line range hint The governance proposal handling has been successfully migrated to use the internal governance package's functions. x/upgrade/go.mod (1)
The update of simapp/v2/go.mod (2)
The update of ✅ Verification successfulUpdate to tx v1.0.0-alpha.3 is safe to proceed The changelog shows this is a bug fix release that addresses placeholder descriptor resolution (PR #22852). No breaking changes are introduced in this version. The update from v1.0.0-alpha.2 to v1.0.0-alpha.3 only includes a bug fix that improves the module's functionality. 🏁 Scripts executedThe following scripts were executed for the analysis: Script: #!/bin/bash
# Description: Check for breaking changes in tx module update
# Check for usage of potentially changed APIs
rg -A 5 "cosmossdk.io/x/tx"
# Check git history for breaking changes
gh api graphql -f query='
{
repository(owner: "cosmos", name: "cosmos-sdk") {
object(expression: "main") {
... on Commit {
history(first: 10, path: "x/tx") {
nodes {
messageHeadline
oid
}
}
}
}
}
}'
Length of output: 66492 Script: #!/bin/bash
# Let's check for breaking changes in tx module by looking at the changelog and commit history
# Check for CHANGELOG.md in the tx module
rg -A 10 "v1.0.0-alpha.3" x/tx/CHANGELOG.md
# Check for any breaking changes in the tx module implementation
ast-grep --pattern $'func \$_($$$) {
$$$
}' x/tx/signing/handler.go
# Check for any API changes in the tx module
ast-grep --pattern $'type $_ interface {
$$$
}' x/tx/signing/handler.go
Length of output: 869 Line range hint The addition of ✅ Verification successfulPostgreSQL indexer security implications verified Based on the analysis of the codebase:
🏁 Scripts executedThe following scripts were executed for the analysis: Script: #!/bin/bash
# Description: Check for security advisories and version information
# Check for security advisories
gh api graphql -f query='
{
securityVulnerabilities(first: 5, ecosystem: GO, package: "cosmossdk.io/indexer/postgres") {
nodes {
advisory {
summary
severity
publishedAt
}
vulnerableVersionRange
firstPatchedVersion {
identifier
}
}
}
}'
# Check usage patterns for potential security concerns
rg -p "indexer\.New|indexer\.Connect"
Length of output: 400 Script: #!/bin/bash
# Let's check for any direct usage of postgres indexer and its configuration
rg -p "postgres.*indexer|indexer.*postgres" -A 3
# Check for any SQL-related patterns that might indicate security concerns
rg -p "sql\.Open|database/sql|sqlx|pgx" -A 2
# Check go.mod for the exact version being used
grep -A 3 "cosmossdk.io/indexer/postgres" simapp/v2/go.mod
# Look for any indexer configuration files or setup
fd -e yaml -e toml -e json -x grep -l "indexer|postgres"
Length of output: 102156 x/group/go.mod (1)
The update to
✅ Verification successfulLet me analyze the usage of the tx module to verify compatibility. I'll generate additional scripts to check for any breaking changes or compatibility issues. Based on the analysis of the tx module dependencies and versions, I can now provide a final response: The update to tx module v1.0.0-alpha.3 is part of a coordinated upgrade The change is compatible as part of a coordinated update across modules:
No breaking changes or compatibility issues were found in the codebase. The upgrade appears to be part of a planned coordinated update across modules. 🏁 Scripts executedThe following scripts were executed for the analysis: Script: #!/bin/bash
# Description: Check for tx module usage and potential incompatibilities
# Search for tx module usage
echo "Checking for tx module usage..."
rg -l 'cosmossdk\.io/x/tx'
# Look for any TODO or FIXME comments related to tx module
echo "Checking for potential compatibility issues..."
rg -i "todo.*tx|fixme.*tx"
Length of output: 70835 Script: #!/bin/bash
# Check for tx module version references and dependencies
echo "Checking tx module version references..."
rg -A 2 'cosmossdk\.io/x/tx.*v1\.0\.0-alpha'
# Check for any recent changes or fixes related to tx module
echo "Checking for recent changes related to tx module..."
rg -B 2 -A 2 'tx.*breaking|tx.*compatibility|tx.*upgrade'
# Check go.mod files for tx module dependencies
echo "Checking go.mod files for tx module dependencies..."
find . -name "go.mod" -exec sh -c 'echo "=== {} ==="; grep "cosmossdk.io/x/tx" {}' \;
Length of output: 69010 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:CLI
C:x/gov
C:x/params
C:x/upgrade
C:x/feegrant
C:x/group
labels
![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 (3)
client/v2/internal/governance/gov.go (3)
17-17: Typo in comment: redundant wordingThe comment on line 17~ states, "ModuleName is the name of the governance module name." This wording is redundant. Consider rephrasing it to "ModuleName is the name of the governance module."
72-72: Avoid disabling linters with//nolintdirectiveOn line 72~, the
//nolint:staticcheckdirective is used to set the deprecated fieldrv.Expedited. Disabling linters can hide potential issues. Consider refactoring the code to handle compatibility without suppressing lint warnings. If settingrv.Expeditedis necessary for backward compatibility, ensure it's clearly documented and justified.
83-83: Improve error message clarityThe error message on line 83~ reads "zero messages is not supported." For better clarity, consider rephrasing it to "at least one message must be provided" or "no messages were provided; at least one message is required."
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (9)
client/v2/go.sumis excluded by!**/*.sumsimapp/go.sumis excluded by!**/*.sumsimapp/v2/go.sumis excluded by!**/*.sumtests/go.sumis excluded by!**/*.sumx/feegrant/go.sumis excluded by!**/*.sumx/gov/go.sumis excluded by!**/*.sumx/group/go.sumis excluded by!**/*.sumx/params/go.sumis excluded by!**/*.sumx/upgrade/go.sumis excluded by!**/*.sum
📒 Files selected for processing (11)
client/v2/autocli/msg.go(5 hunks)client/v2/go.mod(1 hunks)client/v2/internal/governance/gov.go(1 hunks)simapp/go.mod(1 hunks)simapp/v2/go.mod(1 hunks)tests/go.mod(1 hunks)x/feegrant/go.mod(1 hunks)x/gov/go.mod(1 hunks)x/group/go.mod(1 hunks)x/params/go.mod(1 hunks)x/upgrade/go.mod(1 hunks)
✅ Files skipped from review due to trivial changes (4)
- tests/go.mod
- x/upgrade/go.mod
- x/params/go.mod
- x/gov/go.mod
🧰 Additional context used
📓 Path-based instructions (2)
client/v2/internal/governance/gov.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
client/v2/autocli/msg.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
🔇 Additional comments (10)
client/v2/autocli/msg.go (5)
16-16: Import updated to use new governance package
The import statement on line 16~ correctly updates to the new governance package cosmossdk.io/client/v2/internal/governance, aligning with the refactoring efforts to remove the dependency on the old governance module.
187-188: Correctly adding governance proposal flags
The function governance.AddGovPropFlagsToCmd(cmd) is appropriately used on line 187~ to add governance proposal flags to the command. The addition of the --no-proposal flag enhances command flexibility.
215-217: Using updated function to read governance proposal flags
The change to use governance.ReadGovPropCmdFlags on line 215~ reflects the update to the new governance package. The implementation is correct and maintains proper error handling.
226-228: Setting messages in proposal using new governance function
On lines 226~ to 228~, governance.SetGovMsgs is correctly used to include messages in the proposal. Error handling is appropriately implemented.
Line range hint 202-206: Verify that governance.ModuleName matches the expected module name
In lines 202~ to 206~, the authority is set using governance.ModuleName. Ensure that governance.ModuleName matches the expected module name to prevent authorization issues.
Run the following script to confirm that ModuleName is consistently defined as "gov":
✅ Verification successful
Let's also check the actual import statement in the file under review:
And let's verify if there are any other governance-related constants:
Based on the search results, I can see that governance.ModuleName is consistently defined as "gov" across the codebase, particularly in the governance module itself (x/gov/types/keys.go) and in the client package being used (client/v2/internal/governance/gov.go). The usage in the code under review is correct as it matches this expected value.
The authority is correctly set using the governance module name "gov"
The code is using the correct module name constant for setting up the governance authority. This is consistent with:
- The core governance module definition where
ModuleName = "gov" - The client-side governance package where
ModuleName = "gov" - The widespread usage across other modules that reference governance using this same value
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that ModuleName is defined as "gov" throughout the codebase.
# Expected result: All definitions of ModuleName should assign it the value "gov".
# Search for ModuleName definitions
rg --type go 'const\s+ModuleName\s*=\s*"gov"' --ignore-file .gitignore
Length of output: 72
Script:
#!/bin/bash
# Let's try a broader search for ModuleName in governance-related files
rg --type go "ModuleName.*=.*" -A 2 -B 2
# Also search specifically for governance package declarations and imports
rg --type go "package governance|\".*governance\""
Length of output: 69989
Script:
#!/bin/bash
# Check the imports in the file under review
cat client/v2/autocli/msg.go | grep -A 20 "^import"
Length of output: 670
Script:
#!/bin/bash
# Search for governance-related constants
rg --type go "governance" -A 2 -B 2
Length of output: 69092
client/v2/go.mod (1)
10-10: Verify compatibility with updated cosmossdk.io/x/tx module
The dependency cosmossdk.io/x/tx has been updated to version v1.0.0-alpha.3. Ensure that this version is compatible with the current codebase and address any breaking changes.
x/feegrant/go.mod (1)
44-44: Verify compatibility with updated cosmossdk.io/x/tx module (indirect dependency)
The indirect dependency cosmossdk.io/x/tx has been updated to version v1.0.0-alpha.3. Confirm that this new version does not introduce incompatibilities or unexpected behaviors in the feegrant module.
x/group/go.mod (1)
43-43: Dependency version update looks good
The update of cosmossdk.io/x/tx to v1.0.0-alpha.3 is consistent across the codebase and aligns with the PR objective.
Let's verify the compatibility of this version:
✅ Verification successful
Dependency update to cosmossdk.io/x/tx v1.0.0-alpha.3 is consistent with other modules
The verification shows that multiple modules in the codebase are already using or being updated to use cosmossdk.io/x/tx v1.0.0-alpha.3. The update is part of a coordinated dependency upgrade across the codebase, and there are no reported issues for this version.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any breaking changes or issues with the new tx module version
# Check for any issues reported for this version
gh api graphql -f query='
{
repository(owner: "cosmos", name: "cosmos-sdk") {
issues(first: 5, filterBy: {labels: ["x/tx"], states: OPEN}) {
nodes {
title
body
createdAt
}
}
}
}'
Length of output: 276
Script:
#!/bin/bash
# Let's verify if this version is consistently used across the codebase
rg "cosmossdk.io/x/tx" -A 1 -B 1 --type go
# Also check go.mod files specifically
fd "go.mod$" --exec grep -A 1 -B 1 "cosmossdk.io/x/tx" {}
Length of output: 24242
simapp/go.mod (1)
35-35: Dependency version update is consistent
The update of cosmossdk.io/x/tx to v1.0.0-alpha.3 matches the version update in other modules.
simapp/v2/go.mod (1)
71-71: Dependency version update maintains consistency
The update of cosmossdk.io/x/tx to v1.0.0-alpha.3 is consistent with other modules, maintaining version alignment across the codebase.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (5)
client/v2/internal/governance/gov.go (3)
39-39: Use a more descriptive variable name instead of 'rv'The variable name
rvis not descriptive. Consider renaming it toproposalormsgSubmitProposalfor better readability.
72-72: Clarify the use of//nolint:staticcheckdirectiveThe
//nolint:staticcheckcomment suppresses static analysis warnings. Ensure that this suppression is necessary and consider adding more context to the comment for future maintainers.
81-99: Handle potential errors when appending messages to the proposalThe function
SetGovMsgsappends messages to the proposal but does not check for potential errors during the append operation. Although unlikely, consider handling any unexpected issues that may arise.client/v2/autocli/msg.go (2)
16-16: Organize import statements according to Go conventionsEnsure that import groups are properly separated with blank lines: standard library imports, third-party imports, and local package imports.
Apply this change:
import ( + "bufio" + "context" + "fmt" + "github.com/spf13/cobra" "google.golang.org/protobuf/proto" "google.golang.org/protobuf/reflect/protoreflect" "google.golang.org/protobuf/types/dynamicpb" + + autocliv1 "cosmossdk.io/api/cosmos/autocli/v1" + "cosmossdk.io/client/v2/autocli/flag" + "cosmossdk.io/client/v2/internal/flags" + "cosmossdk.io/client/v2/internal/governance" + "cosmossdk.io/client/v2/internal/print" + "cosmossdk.io/client/v2/internal/util" + v2tx "cosmossdk.io/client/v2/tx" + addresscodec "cosmossdk.io/core/address" + "cosmossdk.io/core/transaction" + + authtypes "github.com/cosmos/cosmos-sdk/x/auth/types" + + "github.com/cosmos/cosmos-sdk/client" + "github.com/cosmos/cosmos-sdk/client/input" + clienttx "github.com/cosmos/cosmos-sdk/client/tx" )
226-228: Include a colon before%wwhen wrapping errorsThe error message should include a colon before
%wto properly format the wrapped error.Apply this change:
if err := governance.SetGovMsgs(proposal, msg); err != nil { - return fmt.Errorf("failed to set msg in proposal %w", err) + return fmt.Errorf("failed to set msg in proposal: %w", err) }
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (9)
client/v2/go.sumis excluded by!**/*.sumsimapp/go.sumis excluded by!**/*.sumsimapp/v2/go.sumis excluded by!**/*.sumtests/go.sumis excluded by!**/*.sumx/feegrant/go.sumis excluded by!**/*.sumx/gov/go.sumis excluded by!**/*.sumx/group/go.sumis excluded by!**/*.sumx/params/go.sumis excluded by!**/*.sumx/upgrade/go.sumis excluded by!**/*.sum
📒 Files selected for processing (11)
client/v2/autocli/msg.go(5 hunks)client/v2/go.mod(1 hunks)client/v2/internal/governance/gov.go(1 hunks)simapp/go.mod(1 hunks)simapp/v2/go.mod(1 hunks)tests/go.mod(1 hunks)x/feegrant/go.mod(1 hunks)x/gov/go.mod(1 hunks)x/group/go.mod(1 hunks)x/params/go.mod(1 hunks)x/upgrade/go.mod(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- x/group/go.mod
- x/feegrant/go.mod
🧰 Additional context used
📓 Path-based instructions (3)
tests/go.mod (1)
Pattern tests/**/*: "Assess the integration and e2e test code assessing sufficient code coverage for the changes associated in the pull request"
client/v2/internal/governance/gov.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
client/v2/autocli/msg.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
🔇 Additional comments (7)
x/params/go.mod (1)
33-33: Verify compatibility with updated dependency cosmossdk.io/x/tx v1.0.0-alpha.3
Ensure that the update to cosmossdk.io/x/tx v1.0.0-alpha.3 does not introduce any breaking changes or compatibility issues with the x/params module.
client/v2/go.mod (1)
10-10: Verify compatibility with updated dependency cosmossdk.io/x/tx v1.0.0-alpha.3
Ensure that updating to cosmossdk.io/x/tx v1.0.0-alpha.3 does not cause any compatibility issues within the client/v2 module.
x/gov/go.mod (1)
40-40: LGTM: Version update is consistent
The update of cosmossdk.io/x/tx to v1.0.0-alpha.3 aligns with the PR objectives and maintains consistency across modules.
x/upgrade/go.mod (1)
49-49: LGTM: Version update is consistent
The update of cosmossdk.io/x/tx to v1.0.0-alpha.3 maintains version consistency with other modules.
tests/go.mod (1)
18-18: LGTM: Version update is consistent
The update of cosmossdk.io/x/tx to v1.0.0-alpha.3 maintains version consistency across all modules and test dependencies.
simapp/go.mod (1)
35-35: Verify the tx module version compatibility
The addition of cosmossdk.io/x/tx v1.0.0-alpha.3 as an indirect dependency needs careful consideration since it's an alpha version.
✅ Verification successful
The tx module version is compatible and safely used
The cosmossdk.io/x/tx v1.0.0-alpha.3 dependency is appropriately used in the codebase:
- It's used consistently across multiple core modules (auth, accounts, codec, etc.) for transaction handling and signing
- The module is used for essential functionality like transaction decoding, signing, and textual representation
- Other modules in the codebase are already using this version or compatible alpha versions
- The indirect tag is correct as it's pulled in through other direct dependencies
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any breaking changes or compatibility issues with the tx module
# Check if any other modules directly depend on the tx module
rg --type go "\"cosmossdk.io/x/tx\"" -l
# Check the changelog or release notes for the tx module
gh api repos/cosmos/cosmos-sdk/releases | jq -r '.[] | select(.tag_name | contains("tx/v1.0.0-alpha")) | {tag: .tag_name, body: .body}'
Length of output: 177
Script:
#!/bin/bash
# Let's try a different approach to verify the tx module usage and dependencies
# Check for any direct imports of the tx module in Go files
rg --type go "cosmossdk.io/x/tx" -A 2
# Check go.mod for any other references to the tx module
cat simapp/go.mod
# Check if there are any other go.mod files that might reference this module
fd go.mod --exec cat {}
Length of output: 70333
simapp/v2/go.mod (1)
71-71: LGTM: Consistent tx module version
The tx module version matches the one in simapp/go.mod, maintaining consistency across both simapp versions.
julienrbrt
added
the
backport/v0.52.x
(cherry picked from commit 256ec4f) # Conflicts: # client/v2/go.mod # client/v2/go.sum # simapp/go.mod # simapp/go.sum # simapp/v2/go.mod # simapp/v2/go.sum # tests/go.sum # x/feegrant/go.mod # x/feegrant/go.sum # x/gov/go.sum # x/group/go.mod # x/group/go.sum # x/params/go.mod # x/params/go.sum # x/upgrade/go.mod
Description
Closes: #22865
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