refactor: remove v1 commands and begin server removal #23018
Conversation
julienrbrt
requested review from
kocubinski,
hieuvubk,
a team and
JulianToledano
as code owners
|
Warning Rate limit exceeded@julienrbrt has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 18 minutes and 34 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ⛔ Files ignored due to path filters (27)
📒 Files selected for processing (24)
📝 Walkthrough📝 Walkthrough📝 WalkthroughWalkthroughThe pull request involves the removal of several files and functions related to command-line interface (CLI) management in the Cosmos SDK. Specifically, it removes 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
🧹 Nitpick comments (1)
x/genutil/client/cli/gentx.go (1)
29-32: Consider improving interface naming and documentationThe newly introduced interface name “genesisMM” may not be immediately obvious to future maintainers. Also, adding doc comments for the interface and its methods would clarify usage expectations. For example:
• Rename “genesisMM” to something more descriptive (e.g., “GenesisModuleManager”).
• Provide doc comments describing the purpose of “DefaultGenesis” and “ValidateGenesis”.Below is a potential diff:
-// genesisMM defines methods for default and validated genesis data. -type genesisMM interface { +// GenesisModuleManager defines methods for managing the genesis state of modules. +type GenesisModuleManager interface { DefaultGenesis() map[string]json.RawMessage ValidateGenesis(genesisData map[string]json.RawMessage) error }
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
client/pruning/main.go(0 hunks)x/genutil/client/cli/commands.go(0 hunks)x/genutil/client/cli/export.go(0 hunks)x/genutil/client/cli/gentx.go(1 hunks)
💤 Files with no reviewable changes (3)
- x/genutil/client/cli/commands.go
- client/pruning/main.go
- x/genutil/client/cli/export.go
🧰 Additional context used
📓 Path-based instructions (1)
x/genutil/client/cli/gentx.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
github-actions
bot
added
C:x/staking
C:x/gov
C:x/slashing
C:x/distribution
github-actions
bot
added
C:x/circuit
C:server/v2
runtime/app.go
Outdated
There was a problem hiding this comment.
I could have removed more, but that would touch the config and I think we need to think about server/config functionalities a bit more because of config.
julienrbrt
changed the title
julienrbrt
changed the title
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (5)
server/flags.go (3)
3-18: Add documentation for critical configuration flags.While the grouping is clear, consider adding documentation for critical flags like
FlagMinGasPrices,FlagHaltHeight, andFlagHaltTimeto explain their purpose and expected values.Consider adding documentation like this:
const ( // CometBFT full-node start flags flagWithComet = "with-comet" flagAddress = "address" flagTransport = "transport" flagTraceStore = "trace-store" flagCPUProfile = "cpu-profile" + // FlagMinGasPrices defines the minimum gas prices a validator is willing to accept for processing a + // transaction. A transaction's fees must meet the minimum of any denomination specified in this config. FlagMinGasPrices = "minimum-gas-prices" + // FlagHaltHeight defines a height at which a node will gracefully halt and shutdown FlagHaltHeight = "halt-height" + // FlagHaltTime defines a UNIX timestamp at which a node will gracefully halt and shutdown FlagHaltTime = "halt-time"
19-28: Improve flag grouping organization.Consider reorganizing the flags into more specific groups:
- Move
FlagShutdownGraceto the CometBFT group as it's related to node operation- Create a separate group for IAVL-specific flags
Consider this organization:
// Pruning flags FlagPruning = "pruning" FlagPruningKeepRecent = "pruning-keep-recent" FlagPruningInterval = "pruning-interval" FlagIndexEvents = "index-events" FlagMinRetainBlocks = "min-retain-blocks" - FlagIAVLCacheSize = "iavl-cache-size" - FlagDisableIAVLFastNode = "iavl-disable-fastnode" - FlagIAVLSyncPruning = "iavl-sync-pruning" - FlagShutdownGrace = "shutdown-grace" + + // IAVL configuration flags + FlagIAVLCacheSize = "iavl-cache-size" + FlagDisableIAVLFastNode = "iavl-disable-fastnode" + FlagIAVLSyncPruning = "iavl-sync-pruning"
49-57: Add documentation and use consistent naming for testnet keys.The testnet keys would benefit from documentation explaining their purpose. Also, consider making the naming more consistent.
Consider these improvements:
// testnet keys + // Keys used for testnet configuration and upgrade management KeyIsTestnet = "is-testnet" - KeyNewChainID = "new-chain-ID" - KeyNewOpAddr = "new-operator-addr" - KeyNewValAddr = "new-validator-addr" + KeyChainID = "chain-id" // The new chain ID to use + KeyOperatorAddr = "operator-addr" // The new operator address + KeyValidatorAddr = "validator-addr" // The new validator address KeyUserPubKey = "user-pub-key" KeyTriggerTestnetUpgrade = "trigger-testnet-upgrade"x/genutil/client/cli/export.go (2)
47-47: Consider closing the database.
The database handle (db) isn't closed after use. Although it may be acceptable to let it persist, best practice is to close DB connections, especially if this command is part of a longer-running process.
126-131: Validate openDB error handling.
The openDB function simply returns an error from dbm.NewDB. Depending on usage, confirm if additional context or logging is desirable for troubleshooting DB open failures.
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (27)
client/v2/go.sumis excluded by!**/*.sumgo.sumis excluded by!**/*.sumserver/v2/cometbft/go.sumis excluded by!**/*.sumsimapp/v2/go.sumis excluded by!**/*.sumtests/go.sumis excluded by!**/*.sumtools/benchmark/go.sumis excluded by!**/*.sumx/accounts/defaults/base/go.sumis excluded by!**/*.sumx/accounts/defaults/lockup/go.sumis excluded by!**/*.sumx/accounts/defaults/multisig/go.sumis excluded by!**/*.sumx/accounts/go.sumis excluded by!**/*.sumx/authz/go.sumis excluded by!**/*.sumx/bank/go.sumis excluded by!**/*.sumx/circuit/go.sumis excluded by!**/*.sumx/consensus/go.sumis excluded by!**/*.sumx/distribution/go.sumis excluded by!**/*.sumx/epochs/go.sumis excluded by!**/*.sumx/evidence/go.sumis excluded by!**/*.sumx/feegrant/go.sumis excluded by!**/*.sumx/gov/go.sumis excluded by!**/*.sumx/group/go.sumis excluded by!**/*.sumx/mint/go.sumis excluded by!**/*.sumx/nft/go.sumis excluded by!**/*.sumx/params/go.sumis excluded by!**/*.sumx/protocolpool/go.sumis excluded by!**/*.sumx/slashing/go.sumis excluded by!**/*.sumx/staking/go.sumis excluded by!**/*.sumx/upgrade/go.sumis excluded by!**/*.sum
📒 Files selected for processing (43)
client/v2/go.mod(0 hunks)go.mod(1 hunks)runtime/app.go(0 hunks)server/README.md(0 hunks)server/api/server.go(0 hunks)server/cmd/execute.go(0 hunks)server/cmt_cmds.go(0 hunks)server/constructors_test.go(0 hunks)server/doc.go(0 hunks)server/flags.go(1 hunks)server/module_hash.go(0 hunks)server/pruning.go(0 hunks)server/pruning_test.go(0 hunks)server/rollback.go(0 hunks)server/start.go(0 hunks)server/types/app.go(0 hunks)server/util.go(0 hunks)server/util_test.go(0 hunks)server/v2/cometbft/go.mod(0 hunks)simapp/v2/go.mod(0 hunks)tests/go.mod(0 hunks)tests/integration/server/grpc_test.go(0 hunks)x/accounts/defaults/base/go.mod(0 hunks)x/accounts/defaults/multisig/go.mod(0 hunks)x/accounts/go.mod(0 hunks)x/authz/go.mod(0 hunks)x/bank/go.mod(0 hunks)x/circuit/go.mod(0 hunks)x/consensus/go.mod(0 hunks)x/distribution/go.mod(0 hunks)x/epochs/go.mod(0 hunks)x/evidence/go.mod(0 hunks)x/feegrant/go.mod(1 hunks)x/genutil/client/cli/export.go(4 hunks)x/gov/go.mod(0 hunks)x/group/go.mod(0 hunks)x/mint/go.mod(0 hunks)x/nft/go.mod(0 hunks)x/params/go.mod(0 hunks)x/protocolpool/go.mod(0 hunks)x/slashing/go.mod(0 hunks)x/staking/go.mod(0 hunks)x/upgrade/go.mod(0 hunks)
💤 Files with no reviewable changes (39)
- server/types/app.go
- server/cmd/execute.go
- server/pruning.go
- server/constructors_test.go
- server/README.md
- server/doc.go
- server/start.go
- x/slashing/go.mod
- server/rollback.go
- server/module_hash.go
- server/pruning_test.go
- client/v2/go.mod
- runtime/app.go
- tests/integration/server/grpc_test.go
- x/consensus/go.mod
- x/upgrade/go.mod
- x/mint/go.mod
- server/util_test.go
- x/epochs/go.mod
- x/accounts/defaults/multisig/go.mod
- x/protocolpool/go.mod
- server/api/server.go
- x/accounts/defaults/base/go.mod
- x/circuit/go.mod
- server/util.go
- x/accounts/go.mod
- x/distribution/go.mod
- x/bank/go.mod
- tests/go.mod
- x/evidence/go.mod
- server/v2/cometbft/go.mod
- simapp/v2/go.mod
- x/authz/go.mod
- x/nft/go.mod
- x/staking/go.mod
- server/cmt_cmds.go
- x/group/go.mod
- x/params/go.mod
- x/gov/go.mod
🧰 Additional context used
📓 Path-based instructions (2)
x/genutil/client/cli/export.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
server/flags.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
🔇 Additional comments (9)
server/flags.go (2)
29-33: LGTM!
The state sync flags are well-organized and clearly named.
45-48: LGTM!
The mempool flag is well-defined and appropriately grouped.
x/genutil/client/cli/export.go (5)
9-9: No issues with this import.
Importing "path/filepath" appears correct and aligns well with usage in the newly introduced openDB function.
11-11: Dependency reference is valid.
Import alias "dbm" from "github.com/cosmos/cosmos-db" is consistent with the usage in openDB and getAppDBBackend.
15-15: Core store import looks good.
Import alias "corestore" from "cosmossdk.io/core/store" is utilized for the KVStore interface without any issues.
80-80: Passing in nil for trace writer.
Confirm you do not require a trace writer for debugging or profiling. If exporting large states, you may want to log/troubleshoot details.
125-125: Empty line.
There is nothing to address here.
x/feegrant/go.mod (1)
31-31: Check version validity.
You have introduced “github.com/cockroachdb/fifo” with a specific commit hash. Verify that this commit is stable and doesn't introduce unexpected dependencies or issues.
✅ Verification successful
Version is stable and widely used across the codebase
The commit 0bbfbd93a7ce from June 6, 2024, is a merge commit for a safety improvement PR ("queue-safer"). This version is consistently used across multiple modules in the codebase as an indirect dependency, indicating it's a stable and vetted version. The widespread adoption across different modules (x/feegrant, x/bank, x/gov, etc.) suggests it has been thoroughly tested within the ecosystem.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Validate the introduced CockroachDB FIFO version
gh api graphql -f query='
{
repository(name: "fifo", owner: "cockroachdb") {
object(expression: "0bbfbd93a7ce") {
... on Commit {
committedDate
messageHeadline
}
}
}
}'
Length of output: 357
Script:
#!/bin/bash
# Check the go.mod file for any potential conflicts or issues
cat x/feegrant/go.mod
# Check if this dependency is used in the codebase
rg "github.com/cockroachdb/fifo" -A 2
Length of output: 35857
go.mod (1)
160-160: Verify newly added sync version.
Confirm that “golang.org/x/sync v0.10.0” does not introduce any known vulnerabilities or backward-incompatible changes for the project.
✅ Verification successful
Let me gather more information about the changes between versions and usage.
Let me check the changelog and release notes for this version.
Let me check the actual usage of the sync package in the codebase with a different pattern.
golang.org/x/sync v0.10.0 update is safe to use
The package is used in a limited scope within the codebase, specifically for the errgroup package in:
- x/gov/types/v1/genesis.go
- store/v2/migration/manager.go
The v0.10.0 release of golang.org/x/sync is a stable version that maintains backward compatibility. The package provides additional concurrency primitives and is well-maintained as part of the Go ecosystem. No known vulnerabilities or breaking changes are reported for this version.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for potential vulnerabilities or advisories regarding golang.org/x/sync v0.10.0
gh api graphql -f query='
{
securityVulnerabilities(first: 5, ecosystem: GO, package: "golang.org/x/sync") {
nodes {
advisory {
summary
severity
publishedAt
}
vulnerableVersionRange
firstPatchedVersion {
identifier
}
}
}
}'
Length of output: 349
Script:
#!/bin/bash
# Check the previous version of golang.org/x/sync in go.mod
git log -p go.mod | grep "golang.org/x/sync" | head -n 2
# Find all usages of sync package in the codebase
rg "golang.org/x/sync" -A 2
# Check if there are any breaking changes by looking at imports
rg "sync/" --type go
Length of output: 62098
Script:
#!/bin/bash
# Check the changelog or release notes for golang.org/x/sync v0.10.0
curl -s https://raw.githubusercontent.com/golang/sync/v0.10.0/CHANGES.md || true
curl -s https://raw.githubusercontent.com/golang/sync/v0.10.0/README.md || true
# Check actual usage of sync package in the codebase
ast-grep --pattern 'import "$_"' | grep sync
Length of output: 849
Script:
#!/bin/bash
# Check where golang.org/x/sync is actually used
rg "golang.org/x/sync" --type go -B 2 -A 2
# Check what version was used before
git log -p go.mod | grep "golang.org/x/sync" | head -n 4
Length of output: 505
Description
ref: #22904
Server/config needs to stay I think as confix uses it, and you can go install it from main.
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
Summary by CodeRabbit
New Features
Bug Fixes
Chores