test: add migration integration test #150
Conversation
|
Note Gemini is unable to generate a summary for this pull request due to the file types involved not being currently supported. |
|
""" WalkthroughThis update introduces a GitHub Actions workflow for automated migration testing from a Cosmos SDK chain to Rollkit, refactors the migration command to generate a minimal Rollkit genesis and handle multiple stores, and updates node startup logic to support migration genesis files. Additional minor dependency and struct updates are included. Changes
Sequence Diagram(s)sequenceDiagram
participant GitHub Actions
participant CosmosSDK Chain
participant Rollkit Chain
participant DA Node
GitHub Actions->>CosmosSDK Chain: Scaffold, build, initialize, start
GitHub Actions->>CosmosSDK Chain: Send transactions, verify state
GitHub Actions->>CosmosSDK Chain: Stop chain
GitHub Actions->>Rollkit Chain: Add Rollkit, rebuild
GitHub Actions->>Rollkit Chain: Run migration command (generate minimal genesis, migrate stores)
GitHub Actions->>DA Node: Start DA node
GitHub Actions->>Rollkit Chain: Start migrated chain
GitHub Actions->>Rollkit Chain: Send transactions, verify state
GitHub Actions->>Rollkit Chain: Query old blocks/transactions
sequenceDiagram
participant MigrationCmd
participant CometBFT State
participant Rollkit Stores
participant Genesis File
MigrationCmd->>CometBFT State: Load blockstore and state
MigrationCmd->>Rollkit Stores: Initialize multiple stores (rollkit, abci exec, header sync, data sync)
MigrationCmd->>Rollkit Stores: Migrate blocks and state
MigrationCmd->>Genesis File: Generate minimal Rollkit migration genesis file
Assessment against linked issues
Assessment against linked issues: Out-of-scope changesNo out-of-scope changes found. Possibly related PRs
Suggested reviewers
Poem
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.64.8)Error: you are using a configuration file for golangci-lint v2 with golangci-lint v1: please use golangci-lint v2 ✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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
CodeRabbit Configuration File (
|
julienrbrt
force-pushed
the
julien/migration-2
branch
from
ce72557 to
b15457e
Compare
![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
♻️ Duplicate comments (1)
server/migration_cmd.go (1)
374-378: TODO is already tracked by maintainer.As noted in the existing comment, this will be logged as a separate issue.
🧹 Nitpick comments (2)
server/start.go (1)
650-672: Consider extracting the genesis filename as a shared constant.The hardcoded filename "rollkit_genesis.json" appears both here and in the migration command. Consider defining it as a shared constant to ensure consistency.
-const rollkitGenesisFilename = "rollkit_genesis.json" +const RollkitGenesisFilename = "rollkit_genesis.json"Then update both files to use this shared constant.
.github/workflows/migration_test.yml (1)
63-63: Remove trailing spaces.YAMLlint detected trailing spaces on these lines.
- ATTEMPT=$((ATTEMPT+1)) - + ATTEMPT=$((ATTEMPT+1)) +- fi - + fi +Also applies to: 70-70, 233-233, 240-240
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
.github/workflows/migration_test.yml(1 hunks)go.mod(1 hunks)pkg/rpc/core/status.go(1 hunks)server/migration_cmd.go(8 hunks)server/start.go(10 hunks)
🧰 Additional context used
🧠 Learnings (1)
server/migration_cmd.go (1)
Learnt from: julienrbrt
PR: rollkit/go-execution-abci#113
File: server/migration_cmd.go:231-231
Timestamp: 2025-06-20T13:20:04.030Z
Learning: In the rollkit migration command (server/migration_cmd.go), InitialHeight is correctly set to LastBlockHeight because the Rollkit chain is technically a new chain that starts where the CometBFT chain ended, ensuring continuity during migration.
🪛 YAMLlint (1.37.1)
.github/workflows/migration_test.yml
[error] 63-63: trailing spaces
(trailing-spaces)
[error] 70-70: trailing spaces
(trailing-spaces)
[error] 233-233: trailing spaces
(trailing-spaces)
[error] 240-240: trailing spaces
(trailing-spaces)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: test / Run Unit Tests
- GitHub Check: Test with Rollkit Chain
- GitHub Check: Test Migration from Cosmos SDK to Rollkit
🔇 Additional comments (10)
go.mod (1)
18-18: LGTM!The dependency change is correct -
github.com/celestiaorg/go-headeris now directly imported inserver/migration_cmd.gofor the header and data sync stores, so it should be a direct dependency.pkg/rpc/core/status.go (1)
56-59: LGTM!Removing
ProposerPriorityaligns with Rollkit's single-sequencer model and the simplified validator representation used in the minimal genesis format.server/start.go (2)
331-380: Well-structured genesis loading with dual-source support.The implementation elegantly handles both migration and normal startup scenarios. The manually constructed app genesis for RPC compatibility is a good solution.
674-686: LGTM!Clean conversion from CometBFT genesis to Rollkit genesis format.
.github/workflows/migration_test.yml (2)
175-175: Address the TODO before merging.The workflow currently uses a specific commit hash. Ensure this is updated to use the main branch once PR #209 is merged.
1-373: Excellent end-to-end migration test coverage!This workflow provides comprehensive testing of the migration process, including chain initialization, transaction verification, and post-migration state consistency checks.
server/migration_cmd.go (4)
41-59: Well-designed minimal genesis structure.The
rollkitMigrationGenesisstruct contains exactly the essential information needed for post-migration startup, keeping the genesis file minimal and focused.
64-73: Improved command naming and documentation.The rename to
rollkit-migrateis more concise, and the detailed description clearly explains the migration process.
309-343: Comprehensive store loading with proper isolation.The multi-store approach properly separates concerns and uses prefix transforms for namespace isolation. Good addition of metrics for the sync stores.
102-104: Excellent error handling improvements.The consistent use of error wrapping with descriptive messages and proper resource cleanup with
errors.Joinfollows Go best practices.Also applies to: 117-118, 122-123, 150-151, 208-208
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
.github/workflows/migration_test.yml (1)
63-63: Remove trailing whitespace to satisfy YAML-lintThe CI already flags these lines; deleting the stray spaces keeps the workflow lint-clean.
Also applies to: 70-70, 233-233, 240-240
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/migration_test.yml(1 hunks)
🧰 Additional context used
🪛 YAMLlint (1.37.1)
.github/workflows/migration_test.yml
[error] 63-63: trailing spaces
(trailing-spaces)
[error] 70-70: trailing spaces
(trailing-spaces)
[error] 233-233: trailing spaces
(trailing-spaces)
[error] 240-240: trailing spaces
(trailing-spaces)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: lint / golangci-lint
- GitHub Check: test / Run Unit Tests
- GitHub Check: Test Migration from Cosmos SDK to Rollkit
- GitHub Check: Test with Rollkit Chain
🔇 Additional comments (2)
.github/workflows/migration_test.yml (2)
297-299: Arithmetic relies on all-numeric strings
gmd query bank balances … | jq '.balances[0].amount'returns a JSON string.
If the amount ever contains non-numeric characters (e.g."0"vs"\"0\""), Bash arithmetic expansion will fail.Add explicit coercion:
- EXPECTED_BOB_BALANCE=$((BOB_BALANCE_BEFORE - 4000 + 1000)) + EXPECTED_BOB_BALANCE=$((10#${BOB_BALANCE_BEFORE} - 4000 + 1000))(or use
bcfor clarity).
335-337:--type=heightis no longer a valid flagLatest Cosmos-SDK CLIs accept
--height <n>or positional<height>.
Please confirm this actually works; otherwise the old-block queries will always return the latest block.
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (3)
.github/workflows/migration_test.yml (3)
29-31: Pipe-to-bash Ignite installer – risk already discussedSame unpinned
curl … | bashpattern that was addressed in previous reviews. No further action if you’re comfortable with the trade-off.
39-50:gmdstill assumed to be on PATH afterignite chain buildEarlier feedback noted that
ignite chain builddrops the binary in./buildwithout exporting it. Subsequentgmd …invocations in this step will fail on a clean runner.
335-339: Duplicated misuse of--type=heightSame issue as earlier: replace with positional height argument.
🧹 Nitpick comments (2)
.github/workflows/migration_test.yml (2)
60-63: Trailing whitespace flagged by YAML-lintLines 63 (blank line) contain stray spaces, breaking strict linters.
71-73: Height polling uses unnecessarytail -n +2
gmd query block --output jsonalready emits pure JSON; trimming the first line is unnecessary and will break if the CLI output format changes.- BLOCKS_FOUND=$(gmd query block --output json | tail -n +2 | jq -r '.header.height') + BLOCKS_FOUND=$(gmd query block --output json | jq -r '.header.height')
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.github/workflows/integration_test.yml(1 hunks).github/workflows/migration_test.yml(1 hunks)
🧰 Additional context used
🪛 YAMLlint (1.37.1)
.github/workflows/migration_test.yml
[error] 63-63: trailing spaces
(trailing-spaces)
[error] 70-70: trailing spaces
(trailing-spaces)
[error] 233-233: trailing spaces
(trailing-spaces)
[error] 240-240: trailing spaces
(trailing-spaces)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: Test with Rollkit Chain
- GitHub Check: Test Migration from Cosmos SDK to Rollkit
- GitHub Check: test / Run Unit Tests
- GitHub Check: lint / golangci-lint
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
server/migration_cmd.go (1)
374-381: Verify validator handling aligns with migration requirements.The code enforces exactly one validator during migration, which may be overly restrictive. The TODO comment references issue #164 for multi-validator support.
Verify if the single validator requirement is appropriate for all migration scenarios:
#!/bin/bash # Check if there are other references to validator handling in migration context rg -A 5 -B 5 "validator.*migration|migration.*validator" --type go
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
server/migration_cmd.go(8 hunks)
🧰 Additional context used
🧠 Learnings (1)
server/migration_cmd.go (1)
Learnt from: julienrbrt
PR: rollkit/go-execution-abci#113
File: server/migration_cmd.go:231-231
Timestamp: 2025-06-20T13:20:04.030Z
Learning: In the rollkit migration command (server/migration_cmd.go), InitialHeight is correctly set to LastBlockHeight because the Rollkit chain is technically a new chain that starts where the CometBFT chain ended, ensuring continuity during migration.
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: Test with Rollkit Chain
- GitHub Check: test / Run Unit Tests
- GitHub Check: lint / golangci-lint
- GitHub Check: Test Migration from Cosmos SDK to Rollkit
🔇 Additional comments (9)
server/migration_cmd.go (9)
40-47: LGTM! Well-structured migration genesis struct.The
rollkitMigrationGenesisstruct appropriately represents the minimal data needed for Rollkit migration, with proper JSON tags and essential fields.
49-59: LGTM! Clean conversion method.The
ToRollkitGenesismethod correctly converts the migration genesis to a full Rollkit genesis using the appropriate constructor.
64-74: LGTM! Improved command documentation.The command rename to "rollkit-migrate" and enhanced documentation clearly explain the migration process and expected outcomes.
96-99: LGTM! Comprehensive store management.The new
rollkitStoresstruct andloadRollkitStoresfunction provide a clean abstraction for managing multiple Rollkit stores with proper initialization and key transforms.Also applies to: 302-343
101-104: LGTM! Proper state preservation.Saving the CometBFT state to the ABCI exec store before migration ensures state continuity and proper error handling.
154-164: LGTM! Proper sync store initialization.The logic to initialize header and data sync stores with the first migrated block's data is correct and ensures proper state synchronization.
202-205: LGTM! Proper height tracking.Setting the last height in the Rollkit store after migration ensures continuity and proper block height tracking.
392-400: LGTM! Proper JSON marshalling and file creation.The use of
cmtjson.MarshalIndentensures compatibility with CometBFT genesis format, and the file creation includes proper error handling and permissions.
405-416: LGTM! Robust file existence checking.The
fileExistshelper function properly handles both non-existence and other errors, providing clear error messages.
Closes #77 (#77 (comment))
Summary by CodeRabbit
New Features
Refactor
Chores