feat: merkle_root bundles migration
#236
Conversation
# Conflicts: # app/app.go
christopherbrumm
requested review from
mbreithecker and
troykessler
as code owners
WalkthroughThis pull request introduces modifications to enhance the upgrade mechanism for the Bundles module in the KYVE Network chain. Key changes include the addition of the Changes
Possibly related PRs
Suggested reviewers
Poem
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
CodeRabbit Configuration File (
|
![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 (2)
x/bundles/migration/migration.go (2)
22-25: Use consistent type specifications for all constants in this block.Static analysis reports a warning that only the first constant in this group has an explicit type. To resolve “SA9004,” specify types for all or none consistently.
const ( - BundlesMigrationStepSizePerPool uint64 = 100 - WaitingBlockPeriod = 1 + BundlesMigrationStepSizePerPool uint64 = 100 + WaitingBlockPeriod uint64 = 1 )🧰 Tools
🪛 GitHub Check: lint / golangci
[failure] 23-23:
SA9004: only the first constant in this group has an explicit type (staticcheck)
94-95: Merge variable declaration and assignment into a single statement.The pipeline flagged an S1021 warning (
gosimple) recommending you adopt a short variable declaration.- var iterator storeTypes.Iterator - iterator = store.Iterator(util.GetByteKey(offset), util.GetByteKey(offset+BundlesMigrationStepSizePerPool)) + iterator := store.Iterator(util.GetByteKey(offset), util.GetByteKey(offset+BundlesMigrationStepSizePerPool))🧰 Tools
🪛 GitHub Check: lint / golangci
[failure] 94-94:
S1021: should merge variable declaration with assignment on next line (gosimple)🪛 GitHub Actions: run all jobs
[error] 94-94: Variable declaration should be merged with assignment on next line (gosimple S1021)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
app/app.go(1 hunks)app/upgrades/v2_0/upgrade.go(3 hunks)x/bundles/keeper/getters_migration.go(1 hunks)x/bundles/keeper/keeper.go(2 hunks)x/bundles/migration/migration.go(1 hunks)x/bundles/module.go(2 hunks)x/bundles/types/keys.go(1 hunks)
🧰 Additional context used
🪛 GitHub Check: lint / golangci
x/bundles/migration/migration.go
[failure] 94-94:
S1021: should merge variable declaration with assignment on next line (gosimple)
[failure] 23-23:
SA9004: only the first constant in this group has an explicit type (staticcheck)
🪛 GitHub Actions: run all jobs
x/bundles/migration/migration.go
[error] 94-94: Variable declaration should be merged with assignment on next line (gosimple S1021)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Summary
🔇 Additional comments (11)
x/bundles/migration/migration.go (1)
109-111: Verify safe indexing when slicing merkleRoots.This line slices
merkleRootswithrawFinalizedBundle.Id * 32as a start index. Although there’s an upper bound check at line 105, please verify that themaxBundleIdconsistently matches the length ofmerkleRootsto avoid potential out-of-bounds errors if data is corrupt or inconsistent.x/bundles/keeper/getters_migration.go (1)
13-13: Confirm nil handling for the returned byte slice.
binary.BigEndian.Uint64(storeAdapter.Get(...))will return0if the store value isnil. Ensure this default of0is an intended behavior. If not, consider adding a check.x/bundles/types/keys.go (1)
30-31: Looks good!This key aligns with the rest of the module’s structure, providing clear semantics for tracking migration heights.
x/bundles/keeper/keeper.go (2)
8-8: LGTM!The import statement is correctly positioned.
94-102: Ensure temporary migration methods are removed post-migration.These methods expose internal fields for migration purposes. While this is acceptable for a migration, it's crucial to:
- Document when these methods will be removed
- Create a follow-up task to remove them post-migration
- Ensure they are only used within the migration context
app/upgrades/v2_0/upgrade.go (3)
6-6: LGTM!The import is correctly added for the bundles keeper.
37-37: LGTM!The bundlesKeeper parameter is properly added to the CreateUpgradeHandler function.
50-51: LGTM!The migration height is correctly set using the current block height. This ensures the migration process starts at the right block during the upgrade.
x/bundles/module.go (2)
7-7: LGTM!The migration package is correctly imported.
180-181: Verify migration trigger conditions.The migration is integrated into BeginBlock and triggered based on the upgrade height. Please ensure:
- The migration is idempotent
- There are proper error handling mechanisms
- The migration can handle chain halts and restarts
✅ Verification successful
Migration implementation verified successfully
The migration implementation includes proper safeguards:
- Idempotent execution through block height checks and offset-based processing
- Error handling at both critical (panic) and data processing (returns) levels
- Chain halt/restart safety via block height-based progress tracking and state-based continuation
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify migration implementation details # Test: Check migration implementation for error handling and idempotency ast-grep --pattern 'func MigrateBundlesModule($ctx, $keeper, $height) { $$$ }'Length of output: 82
Script:
#!/bin/bash # First locate the migration implementation rg -l "MigrateBundlesModule" # Then show the content of files containing this function rg -A 20 "func MigrateBundlesModule"Length of output: 1599
Script:
#!/bin/bash # Get the constants and bundlesMigration variable rg -B 10 "var bundlesMigration" x/bundles/migration/migration.go # Get the migrateFinalizedBundles implementation rg -A 20 "func migrateFinalizedBundles" x/bundles/migration/migration.go # Get any other migration-related constants rg "const.*Bundle.*" x/bundles/migration/migration.goLength of output: 1479
app/app.go (1)
412-412: LGTM!The BundlesKeeper is correctly added to the upgrade handler setup, enabling bundle migrations during the v2 upgrade.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
x/bundles/keeper/getters_migration.go (3)
11-18: Improve error handling and add documentation.The function would benefit from the following improvements:
- The error message could be more descriptive, e.g., "bundles migration height not found" instead of "upgrade height can't be zero".
- Consider adding validation for the retrieved value.
- Add godoc-style documentation explaining the function's purpose and return values.
Apply this diff to implement the suggested improvements:
+// GetBundlesMigrationUpgradeHeight retrieves the upgrade height for the v2.0 bundles migration +// from the KV store. +// Returns the upgrade height and nil if found, or 0 and an error if not found. func (k Keeper) GetBundlesMigrationUpgradeHeight(ctx sdk.Context) (uint64, error) { storeAdapter := runtime.KVStoreAdapter(k.storeService.OpenKVStore(ctx)) if storeAdapter.Has(types.BundlesMigrationHeightKey) { - return binary.BigEndian.Uint64(storeAdapter.Get(types.BundlesMigrationHeightKey)), nil + height := binary.BigEndian.Uint64(storeAdapter.Get(types.BundlesMigrationHeightKey)) + if height == 0 { + return 0, errors.New("invalid bundles migration height: height cannot be zero") + } + return height, nil } - return 0, errors.New("upgrade height can't be zero") + return 0, errors.New("bundles migration height not found") }
20-29: Add input validation and improve error handling.The function would benefit from the following improvements:
- Validate that upgradeHeight is not zero.
- Add error handling for potential store failures.
- Enhance the documentation with parameter details.
Apply this diff to implement the suggested improvements:
// SetBundlesMigrationUpgradeHeight stores the upgrade height of the v2.0 bundles migration -// upgrade in the KV-Store. -func (k Keeper) SetBundlesMigrationUpgradeHeight(ctx sdk.Context, upgradeHeight uint64) { +// upgrade in the KV-Store. The upgrade height must be greater than zero. +// Returns an error if the height is invalid or if the store operation fails. +func (k Keeper) SetBundlesMigrationUpgradeHeight(ctx sdk.Context, upgradeHeight uint64) error { + if upgradeHeight == 0 { + return errors.New("invalid bundles migration height: height cannot be zero") + } + storeAdapter := runtime.KVStoreAdapter(k.storeService.OpenKVStore(ctx)) bz := make([]byte, 8) binary.BigEndian.PutUint64(bz, upgradeHeight) storeAdapter.Set(types.BundlesMigrationHeightKey, bz) + return nil }
1-29: Consider architectural improvements for future migrations.The current implementation assumes a single migration height, which might limit future migrations. Consider the following architectural improvements:
- Use a more generic approach that supports multiple migrations.
- Define constants for error messages to ensure consistency.
Here's a suggested approach for supporting multiple migrations:
// Define in types/keys.go const ( BundlesMigrationHeightKeyPrefix = "bundles_migration_height_" ErrMigrationHeightNotFound = "bundles migration height not found for version %s" ErrInvalidMigrationHeight = "invalid bundles migration height: height cannot be zero" ) // Function to generate key for specific version func GetBundlesMigrationHeightKey(version string) []byte { return []byte(BundlesMigrationHeightKeyPrefix + version) } // Updated getter function func (k Keeper) GetBundlesMigrationUpgradeHeight(ctx sdk.Context, version string) (uint64, error) { storeAdapter := runtime.KVStoreAdapter(k.storeService.OpenKVStore(ctx)) key := GetBundlesMigrationHeightKey(version) if storeAdapter.Has(key) { height := binary.BigEndian.Uint64(storeAdapter.Get(key)) if height == 0 { return 0, errors.New(ErrInvalidMigrationHeight) } return height, nil } return 0, fmt.Errorf(ErrMigrationHeightNotFound, version) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
x/bundles/keeper/getters_migration.go(1 hunks)x/bundles/module.go(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- x/bundles/module.go
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Summary
🔇 Additional comments (1)
x/bundles/keeper/getters_migration.go (1)
1-10: LGTM!The package declaration and imports are appropriate for the functionality being implemented.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
x/bundles/migration/README.md (4)
9-11: Add details about binary files format and location.The documentation mentions binary files but could benefit from additional information.
Consider adding:
- Format/structure of the binary files
- Expected file size ranges
- Location of these files in the repository structure
- Instructions for regenerating these files if needed
12-33: Add language specification to code block and clarify pool numbering.The hash listings could be improved for better clarity and documentation standards.
Apply this diff to specify the language and improve formatting:
-``` +```text merkle_roots_pool_0 da4bb9bf0a60c5c79e399d8bb54ae4cf916f6c1dbdd5cdae45cb991f4e56158fAlso consider:
- Explaining why certain pool numbers (4, 6, 8) are missing
- Adding a note about the hash format (SHA-256, etc.)
- Including a timestamp or version identifier for when these hashes were generated
🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
12-12: Fenced code blocks should have a language specified
null(MD040, fenced-code-language)
1-33: Consider adding practical sections to enhance documentation.The documentation provides good technical information but could benefit from additional sections.
Consider adding:
- Prerequisites section
- Step-by-step migration procedure
- Troubleshooting guide
- Verification steps to confirm successful migration
- Rollback procedure if needed
🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
12-12: Fenced code blocks should have a language specified
null(MD040, fenced-code-language)
1-8: Verify external links and consider adding version information.The documentation clearly explains the purpose and implementation. Let's verify the referenced links:
Consider adding:
- Version information for the Go and Python implementations
- Link to the TypeScript runtime implementation mentioned
✅ Verification successful
🏁 Script executed:
#!/bin/bash # Description: Verify the existence and accessibility of referenced links # Check Trustless API documentation curl -I "https://docs.kyve.network/access-data-sets/trustless-api/overview" # Check merkle-script repository curl -I "https://github.com/KYVENetwork/merkle-script"Length of output: 5889
External links verified – consider version and reference improvements.
Both the Trustless API documentation and the merkle-script repository URLs return a successful (HTTP 200) response, confirming they are accessible. As noted, the current markdown snippet is accurate but might benefit from these enhancements:
- Add explicit language identifiers to code blocks if applicable.
- Include version information for the Go and Python implementations.
- Provide a direct link to the TypeScript runtime implementation for completeness.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
x/bundles/migration/README.md(1 hunks)
🧰 Additional context used
🪛 markdownlint-cli2 (0.17.2)
x/bundles/migration/README.md
12-12: Fenced code blocks should have a language specified
null
(MD040, fenced-code-language)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: lint / golangci
- GitHub Check: test / test
- GitHub Check: Summary
This PR implements the bundle migration for the upcoming
v2release. It adds the Merkle roots computed using the following script: https://github.com/KYVENetwork/merkle-scriptSummary by CodeRabbit
New Features
Chores
Bug Fixes