store/v2: remove auto migration and fix restore cmd #23568
Conversation
* main: (61 commits) build(deps): Bump cosmossdk.io/x/tx from 1.0.1 to 1.1.0 (#23547) feat(client/v2): add map support (#23544) fix(db): Fix pebbleDB integration (#23552) build(deps): Bump buf.build/gen/go/cometbft/cometbft/protocolbuffers/go from 1.36.3-20241120201313-68e42a58b301.1 to 1.36.4-20241120201313-68e42a58b301.1 in /api (#23520) build(deps): Bump github.com/golang/glog from 1.2.3 to 1.2.4 in /tools/hubl (#23541) fix(x/tx): add an option to encode maps using amino json (ref #23513) (#23539) build(deps): Bump cosmossdk.io/collections from 1.0.0 to 1.1.0 (#23534) build(deps): Bump github.com/bytedance/sonic from 1.12.7 to 1.12.8 in /log (#23516) chore: fix changelog for `v0.52.0-rc.2` (#23502) build(deps): Bump google.golang.org/protobuf from 1.36.3 to 1.36.4 in /x/upgrade (#23512) chore(collections): bring in `protocodec` in collections directly (#23515) feat(x/tx): add an option to encode maps using amino json (#23513) fix(codec): provide legacy amino og type (#23504) chore: remove unecessary SDKGasMeter impl (#23503) refactor: replace `golang.org/x/exp` with stdlib (#23501) build(deps): Bump google.golang.org/grpc from 1.69.4 to 1.70.0 in /tools/cosmovisor (#23496) build(deps): Bump google.golang.org/grpc from 1.69.4 to 1.70.0 in /store (#23494) build(deps): Bump github.com/hashicorp/go-plugin from 1.6.2 to 1.6.3 in /store (#23493) ci: Fix Ci job target (#23500) feat(server/v2): add swagger server component (#23486) ...
alpe
requested review from
a team,
julienrbrt,
tac0turtle,
lucaslopezf,
JulianToledano,
ziscky and
raynaudoe
as code owners
|
Warning There were issues while running some tools. Please review the errors and either fix the tool’s configuration or disable the tool if it’s a critical failure. 🔧 golangci-lint (1.62.2)level=warning msg="[runner] Can't run linter goanalysis_metalinter: buildir: failed to load package sqlite3: could not load export data: no export data for "github.com/bvinc/go-sqlite-lite/sqlite3"" 📝 WalkthroughWalkthroughThis pull request introduces significant changes to the Cosmos SDK's store and server modules, primarily focusing on removing migration-related functionality and simplifying the store management process. The modifications span multiple files across different packages, including Changes
Sequence DiagramsequenceDiagram
participant Server
participant RootStore
participant CommitStore
Server->>RootStore: Create with New()
RootStore->>CommitStore: Initialize
RootStore-->>Server: Return Store Instance
Note over Server,RootStore: Migration logic removed
Note over Server,RootStore: Simplified store initialization
Possibly related PRs
Suggested Labels
Suggested Reviewers
✨ Finishing Touches
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/staking
C:CLI
C:x/gov
C:x/slashing
C:x/distribution
github-actions
bot
removed
Type: CI
C:x/consensus
C:collections
C:Confix
This comment has been minimized.
This comment has been minimized.
![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)
store/v2/CHANGELOG.md (1)
29-29: Add blank lines around list items.According to markdown best practices, list items should be surrounded by blank lines for better readability.
Apply this diff to fix the formatting:
### Improvements + * [#23568](https://github.com/cosmos/cosmos-sdk/pull/23568) Remove auto migration and fix restore cmd * [#23013](https://github.com/cosmos/cosmos-sdk/pull/23013) Support memDB for sims + ### Bug Fixes🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
29-29: Lists should be surrounded by blank lines
null(MD032, blanks-around-lists)
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
runtime/v2/go.sumis excluded by!**/*.sum
📒 Files selected for processing (17)
runtime/v2/go.mod(3 hunks)server/v2/commands.go(1 hunks)server/v2/store/commands.go(2 hunks)server/v2/store/server.go(1 hunks)server/v2/store/snapshot.go(2 hunks)simapp/v2/go.mod(0 hunks)store/v2/CHANGELOG.md(1 hunks)store/v2/commitment/store.go(1 hunks)store/v2/root/builder.go(2 hunks)store/v2/root/factory.go(2 hunks)store/v2/root/factory_test.go(0 hunks)store/v2/root/migrate_test.go(1 hunks)store/v2/root/store.go(1 hunks)store/v2/root/store_mock_test.go(0 hunks)store/v2/root/store_test.go(3 hunks)store/v2/root/upgrade_test.go(2 hunks)tests/systemtests/store_test.go(2 hunks)
💤 Files with no reviewable changes (3)
- store/v2/root/factory_test.go
- simapp/v2/go.mod
- store/v2/root/store_mock_test.go
✅ Files skipped from review due to trivial changes (1)
- store/v2/root/store.go
🧰 Additional context used
📓 Path-based instructions (12)
server/v2/commands.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
store/v2/root/factory.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
store/v2/CHANGELOG.md (1)
Pattern **/*.md: "Assess the documentation for misspellings, grammatical errors, missing documentation and correctness"
store/v2/root/builder.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
store/v2/commitment/store.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
server/v2/store/snapshot.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
store/v2/root/upgrade_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"
tests/systemtests/store_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"
store/v2/root/migrate_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"
server/v2/store/server.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
server/v2/store/commands.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
store/v2/root/store_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"
🪛 markdownlint-cli2 (0.17.2)
store/v2/CHANGELOG.md
29-29: Lists should be surrounded by blank lines
null
(MD032, blanks-around-lists)
🪛 GitHub Actions: v2 core Tests
tests/systemtests/store_test.go
[error] TestAccountCreation failed: timeout waiting for node start: 45s
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: tests (00)
- GitHub Check: Analyze
- GitHub Check: Summary
🔇 Additional comments (19)
store/v2/CHANGELOG.md (1)
29-29: LGTM! The changelog entry is well-formatted and clear.The new entry follows the changelog format guidelines and provides a clear description of the changes.
🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
29-29: Lists should be surrounded by blank lines
null(MD032, blanks-around-lists)
server/v2/store/server.go (1)
27-27: LGTM! Type change improves type safetyThe change from
BackendtoRootStoreprovides better type safety and clearer intent about the required store implementation.Also applies to: 30-30
tests/systemtests/store_test.go (2)
29-31: LGTM! Simplified snapshot restore directoriesThe change to only include application.db in restorable directories aligns with the store implementation changes.
🧰 Tools
🪛 GitHub Actions: v2 core Tests
[error] TestAccountCreation failed: timeout waiting for node start: 45s
65-66: Verify chain restart timingThe added chain restart and block wait improves test reliability, but might be related to the TestAccountCreation timeout in CI.
✅ Verification successful
Chain restart timing is consistent with codebase patterns
The added 2-block wait is consistent with similar patterns across the test suite, where tests commonly wait for 2-10 blocks after chain start. The system tests have a 15-minute CI timeout, which is more than sufficient for this operation.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for similar timeout patterns in other test files rg -A 5 "StartChain\(t\)" --type go | grep -B 5 "AwaitNBlocks"Length of output: 2651
Script:
#!/bin/bash # Search for TestAccountCreation implementation and timing patterns rg -l "TestAccountCreation" --type go rg "func TestAccountCreation" -A 10 --type go # Look for test timeouts in CI configuration fd "\.github|\.gitlab-ci" --type f | xargs rg "timeout|TIMEOUT"Length of output: 66001
🧰 Tools
🪛 GitHub Actions: v2 core Tests
[error] TestAccountCreation failed: timeout waiting for node start: 45s
store/v2/root/builder.go (1)
28-29: LGTM! Well-implemented store keys accessorThe GetStoreKeys method is efficiently implemented with pre-allocated capacity and provides useful access to registered store keys.
Also applies to: 104-110
store/v2/root/factory.go (1)
117-117: LGTM! Successfully removed migration logicThe removal of migration logic simplifies the store creation process and aligns with the PR objectives. The code is now more focused and maintainable.
Also applies to: 142-142
store/v2/root/upgrade_test.go (2)
47-47: LGTM: Constructor call simplifiedThe removal of the last nil argument aligns with the simplified store initialization.
89-89: LGTM: Consistent constructor usageThe change maintains consistency with the earlier constructor call.
store/v2/root/migrate_test.go (1)
70-70: LGTM: Constructor call simplifiedThe change is consistent with the simplified store initialization pattern.
server/v2/store/commands.go (1)
Line range hint
48-61: LGTM: Simplified store access patternThe changes improve the code by:
- Directly using the server's store instance instead of creating a new one
- Simplifying the error handling flow
- Reducing unnecessary object creation
server/v2/commands.go (1)
187-189: LGTM: Added store commands to app building requirementsThe addition of store commands to
appBuildingCommandsensures proper application initialization for store operations.server/v2/store/snapshot.go (2)
45-48: LGTM! Simplified snapshot manager creation.The code now directly uses
s.storeinstead of creating a new root store, which simplifies the snapshot management process.
86-89: LGTM! Consistent with the snapshot manager creation changes.The restore command follows the same pattern of using
s.storedirectly.store/v2/commitment/store.go (1)
566-568: LGTM! Fixed version handling during snapshot restoration.Setting the version for nodes with version 0 ensures consistent versioning during snapshot restoration.
Run the following script to verify version handling in other snapshot-related code:
✅ Verification successful
✓ Version handling during snapshot restoration is correct
The version check and assignment for nodes with version 0 follows established patterns in the codebase for handling unset versions during snapshot restoration. This is consistent with similar version handling throughout the codebase, particularly in snapshot-related operations.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for other places where version 0 might be handled ast-grep --pattern $'if $x.Version == 0 { $$$ }'Length of output: 51
Script:
#!/bin/bash # Search for Version field assignments and comparisons rg "Version\s*(==|=)\s*\d+" -A 2 # Search for snapshot restoration related code with version handling rg "snapshot|restore.*[Vv]ersion" -A 3 # Try alternative ast-grep pattern for version checks ast-grep --pattern 'node.Version = $x'Length of output: 78656
store/v2/root/store_test.go (3)
55-56: LGTM! Updated constructor call.Removed unnecessary nil arguments from the New function call, reflecting the removal of migration-related parameters.
76-77: LGTM! Consistent constructor update.The changes maintain consistency with the updated New function signature.
85-86: LGTM! Consistent constructor update.The changes maintain consistency with the updated New function signature.
runtime/v2/go.mod (2)
28-29: LGTM! Added replace directive for local development.The replace directive ensures the store/v2 module is resolved from the local path.
36-36: Verify new indirect dependencies.Several new indirect dependencies have been added. Please ensure they are necessary and their versions are compatible.
Run the following script to check dependency compatibility:
Also applies to: 38-38, 50-50, 53-53, 65-65
✅ Verification successful
All dependencies verified and compatible ✅
The
go mod verifycheck passed successfully and all dependencies, including the new indirect ones, are properly versioned and compatible. No conflicts or issues were detected in the dependency graph.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for potential dependency conflicts go mod verify go mod graph | awk '{if ($1 !~ /@/ || $2 !~ /@/) print $0}'Length of output: 265
Script:
#!/bin/bash # First verify the file exists and check its content if [ -f runtime/v2/go.mod ]; then echo "=== go.mod content ===" cat runtime/v2/go.mod echo -e "\n=== Checking dependencies ===" cd runtime/v2 go mod verify echo -e "\n=== Dependency graph ===" go mod graph | awk '{if ($1 !~ /@/ || $2 !~ /@/) print $0}' else echo "go.mod file not found at runtime/v2/go.mod" fiLength of output: 9775
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Description
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
Release Notes
Improvements
Bug Fixes
Chores
Testing
These changes focus on streamlining the store module's functionality and improving overall system reliability.