chore: migrate OutgoingTxBatchBlockKey with batch nonce #773
Conversation
WalkthroughThe pull request introduces several modifications across multiple files to enhance cross-chain functionality and testing within the application. Key changes include the addition of a new function to verify outgoing transaction batches, updates to migration methods to accommodate a codec parameter, and adjustments to key generation for outgoing transaction batches. These enhancements aim to improve the robustness of the upgrade process and ensure that transaction batches are correctly managed and validated during application upgrades. Changes
Possibly related PRs
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🧰 Additional context used🔇 Additional comments (4)
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: 3
🧹 Outside diff range and nitpick comments (1)
app/upgrade_test.go (1)
151-160: LGTM with minor suggestions!The implementation correctly verifies the presence of outgoing transaction batches after upgrade.
Consider these improvements:
- Add function documentation to explain its purpose
- Add assertion for non-nil batch
+// checkOutgoingBatch verifies that all outgoing transaction batches are correctly +// migrated during the upgrade by checking their presence in the store using the +// new key format that includes both block height and batch nonce. func checkOutgoingBatch(t *testing.T, ctx sdk.Context, myApp *app.App) { t.Helper() for _, keeper := range myApp.CrosschainKeepers.ToSlice() { kvStore := ctx.KVStore(myApp.GetKey(keeper.ModuleName())) keeper.IterateOutgoingTxBatches(ctx, func(batch *types.OutgoingTxBatch) bool { + require.NotNil(t, batch) assert.True(t, kvStore.Has(types.GetOutgoingTxBatchBlockKey(batch.Block, batch.BatchNonce))) return false }) } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (5)
- app/upgrade_test.go (3 hunks)
- x/crosschain/keeper/migrations.go (1 hunks)
- x/crosschain/keeper/outgoing_tx.go (2 hunks)
- x/crosschain/migrations/v8/migrate.go (2 hunks)
- x/crosschain/types/key.go (1 hunks)
🧰 Additional context used
🔇 Additional comments (9)
x/crosschain/keeper/migrations.go (1)
20-20: LGTM! Verify v8.Migrate implementation.The addition of codec parameter aligns with the PR objective of migrating OutgoingTxBatchBlockKey with batch nonce, as it's needed for proper serialization of batch data during migration.
Let's verify the v8.Migrate implementation:
✅ Verification successful
Let me verify the implementation of
migrateOutgoingTxBatchBlockKeysince it's the core function handling the batch nonce migration.
Verified: Migration implementation is correct and properly uses codec
The verification confirms that:
v8.Migratefunction signature matches the call with correct parameters (ctx, storeKey, codec)migrateOutgoingTxBatchBlockKeyproperly uses the codec for:
- Unmarshaling existing batch data from store
- Marshaling batch data with the new key format that includes batch nonce
- Correctly iterates and migrates all existing batches
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify v8.Migrate function signature matches the new call ast-grep --pattern 'func Migrate(ctx sdk.Context, storeKey storetypes.StoreKey, cdc codec.BinaryCodec) error'Length of output: 534
Script:
#!/bin/bash # Check implementation of migrateOutgoingTxBatchBlockKey ast-grep --pattern 'func migrateOutgoingTxBatchBlockKey(ctx sdk.Context, storeKey storetypes.StoreKey, cdc codec.BinaryCodec)'Length of output: 1438
app/upgrade_test.go (2)
28-28: LGTM!The added import is necessary for accessing crosschain types in the new test function.
109-110: LGTM!The addition of
checkOutgoingBatchverification is appropriately placed alongside other upgrade verification steps.x/crosschain/keeper/outgoing_tx.go (3)
131-131: LGTM!The deletion of the block key is correctly updated to match the new key structure using both block height and batch nonce.
Line range hint
118-124: Update the comment to reflect current implementation.The implementation looks good, but the comment "Only one OutgoingTxBatch can be submitted in a block" appears to be outdated since the block key now includes the batch nonce, allowing multiple batches per block.
- // Note: Only one OutgoingTxBatch can be submitted in a block + // Note: Only one OutgoingTxBatch can be submitted per batch nonce in a blockLet's verify if there are any assumptions about single batch per block in tests:
#!/bin/bash # Search for test cases that might assume single batch per block rg -A 5 "OutgoingTxBatch.*block" "*_test.go"
Line range hint
214-234: Update IterateBatchByBlockHeight to handle new key format.The
IterateBatchByBlockHeightmethod needs to be updated to handle the new key format that includes batch nonce. The current implementation might miss batches or provide incorrect results since it assumes a simpler key structure.Consider updating the method to handle the new key format:
func (k Keeper) IterateBatchByBlockHeight(ctx sdk.Context, start, end uint64, cb func(*types.OutgoingTxBatch) bool) { store := ctx.KVStore(k.storeKey) - startKey := append(types.OutgoingTxBatchBlockKey, sdk.Uint64ToBigEndian(start)...) - endKey := append(types.OutgoingTxBatchBlockKey, sdk.Uint64ToBigEndian(end)...) + // Use only block height prefix to catch all batches in the block range + startKey := types.GetOutgoingTxBatchBlockPrefix(start) + endKey := types.GetOutgoingTxBatchBlockPrefix(end) iter := store.Iterator(startKey, endKey) defer iter.Close()Let's verify the usage of this method:
x/crosschain/migrations/v8/migrate.go (3)
5-5: Imports are updated appropriatelyThe necessary packages
codecandtypesare correctly imported to support serialization and type definitions required for the migration process.Also applies to: 9-9
36-36: UpdatedMigratefunction signature to includecdcparameterIncluding
cdc codec.BinaryCodecallows for proper serialization and deserialization within the migration, which is essential for processing store data.
38-38: Calling newmigrateOutgoingTxBatchBlockKeyfunctionThe
Migratefunction now invokesmigrateOutgoingTxBatchBlockKeyto handle the migration of outgoing transaction batch keys, ensuring that batch keys are updated correctly.
Co-authored-by: nulnut <151493716+nulnut@users.noreply.github.com>
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Documentation