Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

refactor(x/crosschain): migrate module data #764

Merged
merged 1 commit into from
Oct 22, 2024
Merged

refactor(x/crosschain): migrate module data #764

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions app/app.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -202,6 +202,7 @@ func New(
// can do so safely.
app.mm.SetOrderInitGenesis(orderInitBlockers()...)
app.mm.SetOrderExportGenesis(orderInitBlockers()...)
app.mm.SetOrderMigrations(orderMigrations()...)

app.mm.RegisterInvariants(app.CrisisKeeper)
app.configurator = module.NewConfigurator(app.appCodec, app.MsgServiceRouter(), app.GRPCQueryRouter())
Expand Down
42 changes: 42 additions & 0 deletions app/modules.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -280,6 +280,48 @@ func orderInitBlockers() []string {
}
}

func orderMigrations() []string {
return []string{
capabilitytypes.ModuleName,
authtypes.ModuleName,
banktypes.ModuleName,
distrtypes.ModuleName,
stakingtypes.ModuleName,
slashingtypes.ModuleName,
govtypes.ModuleName,
minttypes.ModuleName,
crisistypes.ModuleName,
genutiltypes.ModuleName,
evidencetypes.ModuleName,
authz.ModuleName,
feegrant.ModuleName,
paramstypes.ModuleName,
upgradetypes.ModuleName,
vestingtypes.ModuleName,

erc20types.ModuleName,

bsctypes.ModuleName,
trontypes.ModuleName,
polygontypes.ModuleName,
avalanchetypes.ModuleName,
ethtypes.ModuleName,
arbitrumtypes.ModuleName,
optimismtypes.ModuleName,
layer2types.ModuleName,

feemarkettypes.ModuleName,
evmtypes.ModuleName,
migratetypes.ModuleName,

ibctm.ModuleName,
ibctransfertypes.ModuleName,
ibcexported.ModuleName,

consensustypes.ModuleName,
}
}

func GetMaccPerms() map[string][]string {
dupMaccPerms := make(map[string][]string)
for k, v := range maccPerms {
Expand Down
28 changes: 28 additions & 0 deletions app/upgrades/store/store.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
package store

import (
storetypes "cosmossdk.io/store/types"
sdk "github.com/cosmos/cosmos-sdk/types"
)

func RemoveStoreKeys(ctx sdk.Context, storeKey storetypes.StoreKey, prefixKeys [][]byte) {
store := ctx.KVStore(storeKey)

deleteFn := func(prefixKey []byte) {
iterator := storetypes.KVStorePrefixIterator(store, prefixKey)
defer iterator.Close()
deleteCount := 0
for ; iterator.Valid(); iterator.Next() {
store.Delete(iterator.Key())
deleteCount++
}
if deleteCount > 0 {
ctx.Logger().Info("remove store key", "kvStore", storeKey.Name(),
"prefix", prefixKey[0], "deleteKeyCount", deleteCount)
}
}

for _, key := range prefixKeys {
deleteFn(key)
}
}
37 changes: 4 additions & 33 deletions app/upgrades/v8/upgrade.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,6 @@ package v8

import (
"context"
"encoding/hex"
"strings"

storetypes "cosmossdk.io/store/types"
Expand All @@ -19,8 +18,8 @@ import (
ibcchanneltypes "github.com/cosmos/ibc-go/v8/modules/core/04-channel/types"

"github.com/functionx/fx-core/v8/app/keepers"
"github.com/functionx/fx-core/v8/app/upgrades/store"
fxtypes "github.com/functionx/fx-core/v8/types"
crosschainkeeper "github.com/functionx/fx-core/v8/x/crosschain/keeper"
crosschaintypes "github.com/functionx/fx-core/v8/x/crosschain/types"
erc20v8 "github.com/functionx/fx-core/v8/x/erc20/migrations/v8"
erc20types "github.com/functionx/fx-core/v8/x/erc20/types"
Expand All @@ -43,62 +42,34 @@ func CreateUpgradeHandler(mm *module.Manager, configurator module.Configurator,
return fromVM, err
}

removeStoreKeys(cacheCtx, app.GetKey(stakingtypes.StoreKey), fxstakingv8.GetRemovedStoreKeys())
store.RemoveStoreKeys(cacheCtx, app.GetKey(stakingtypes.StoreKey), fxstakingv8.GetRemovedStoreKeys())

if err = migrationGovCustomParam(cacheCtx, app.GovKeeper, app.GetKey(govtypes.StoreKey)); err != nil {
return fromVM, err
}

if err = updateArbitrumParams(cacheCtx, app.ArbitrumKeeper); err != nil {
return fromVM, err
}

if err = migrateBridgeBalance(cacheCtx, app.BankKeeper, app.AccountKeeper); err != nil {
return fromVM, err
}

updateMetadata(cacheCtx, app.BankKeeper)

removeStoreKeys(cacheCtx, app.GetKey(erc20types.StoreKey), erc20v8.GetRemovedStoreKeys())
store.RemoveStoreKeys(cacheCtx, app.GetKey(erc20types.StoreKey), erc20v8.GetRemovedStoreKeys())

commit()
cacheCtx.Logger().Info("upgrade complete", "module", "upgrade")
return toVM, nil
}
}

func updateArbitrumParams(ctx sdk.Context, keeper crosschainkeeper.Keeper) error {
params := keeper.GetParams(ctx)
params.AverageExternalBlockTime = 250
return keeper.SetParams(ctx, &params)
}

func migrationGovCustomParam(ctx sdk.Context, keeper *keeper.Keeper, storeKey *storetypes.KVStoreKey) error {
// 1. delete fxParams key
removeStoreKeys(ctx, storeKey, fxgovv8.GetRemovedStoreKeys())
store.RemoveStoreKeys(ctx, storeKey, fxgovv8.GetRemovedStoreKeys())

// 2. init custom params
return keeper.InitCustomParams(ctx)
}

func removeStoreKeys(ctx sdk.Context, storeKey *storetypes.KVStoreKey, keys [][]byte) {
store := ctx.KVStore(storeKey)

deleteFn := func(key []byte) {
iterator := storetypes.KVStorePrefixIterator(store, key)
defer iterator.Close()
for ; iterator.Valid(); iterator.Next() {
store.Delete(iterator.Key())
ctx.Logger().Info("remove store key", "kvStore", storeKey.Name(),
"prefix", hex.EncodeToString(key), "key", string(iterator.Key()))
}
}

for _, key := range keys {
deleteFn(key)
}
}

func migrateCrosschainModuleAccount(ctx sdk.Context, ak authkeeper.AccountKeeper) error {
addr, perms := ak.GetModuleAddressAndPermissions(crosschaintypes.ModuleName)
if addr == nil {
Expand Down
4 changes: 2 additions & 2 deletions x/arbitrum/module.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -106,7 +106,7 @@ func (am AppModule) RegisterInvariants(_ sdk.InvariantRegistry) {}
// RegisterServices registers module services.
func (am AppModule) RegisterServices(cfg module.Configurator) {
migrator := crosschainkeeper.NewMigrator(am.keeper)
if err := cfg.RegisterMigration(am.Name(), 1, migrator.Migrate); err != nil {
if err := cfg.RegisterMigration(am.Name(), 2, migrator.Migrate7to8WithArbExternalBlockTime); err != nil {
panic(err)
}
}
Comment on lines +109 to 112
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Handle errors without panicking to improve stability

In the RegisterServices function, using panic on error can lead to unexpected crashes. It's better to handle the error gracefully to enhance the module's robustness.

Apply this diff to return the error instead of panicking:

 if err := cfg.RegisterMigration(am.Name(), 2, migrator.Migrate7to8WithArbExternalBlockTime); err != nil {
-    panic(err)
+    // Return the error to be handled by the caller
+    return err
 }

Committable suggestion was skipped due to low confidence.

Expand All @@ -128,7 +128,7 @@ func (am AppModule) ExportGenesis(ctx sdk.Context, cdc codec.JSONCodec) json.Raw

// ConsensusVersion implements AppModule/ConsensusVersion.
func (am AppModule) ConsensusVersion() uint64 {
return 2
return 3
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codebase verification

Migration Registration Mismatch Detected

The ConsensusVersion is set to 3, but the registered migration for version 2 uses a function (Migrate7to8WithArbExternalBlockTime) that suggests migrating from version 7 to 8. This indicates that migrations from version 2 to 3 might be missing or incorrectly implemented.

🔗 Analysis chain

Ensure consistency between migration versions

The ConsensusVersion method returns 3, indicating the module's current version. However, you're registering a migration from version 2. Verify that all necessary migrations from version 2 to 3 are properly handled.

Run the following script to list all registered migrations for the module:

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify registered migrations for the arbitrum module.

# Expected: Should list migrations including from version 2 to 3.
rg 'RegisterMigration\(' -A 2 --multiline --glob 'x/arbitrum/*.go'

Length of output: 254

Script:

#!/bin/bash
# Description: Verify if migration from version 2 to 3 is registered for the arbitrum module.

ast-grep --pattern $'cfg.RegisterMigration($_, 2, $_)' --glob 'x/arbitrum/*.go'

Length of output: 322

Script:

#!/bin/bash
# Description: Verify if migration from version 2 to 3 is registered for the arbitrum module.

ast-grep --pattern $'cfg.RegisterMigration($_, 2, $_)' --globs 'x/arbitrum/*.go'

Length of output: 213

}

// EndBlock implements app module
Expand Down
4 changes: 2 additions & 2 deletions x/avalanche/module.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -108,7 +108,7 @@ func (am AppModule) RegisterInvariants(_ sdk.InvariantRegistry) {}
// RegisterServices registers module services.
func (am AppModule) RegisterServices(cfg module.Configurator) {
migrator := crosschainkeeper.NewMigrator(am.keeper)
if err := cfg.RegisterMigration(am.Name(), 2, migrator.Migrate); err != nil {
if err := cfg.RegisterMigration(am.Name(), 3, migrator.Migrate7to8); err != nil {
panic(err)
}
Comment on lines +111 to 113
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codebase verification

Migrate7to8 Function Not Implemented

The Migrate7to8 function is missing in x/avalanche/module.go. Please implement this function to handle the migration from version 7 to 8.

🔗 Analysis chain

LGTM. Verify migration function implementation.

The update to use migrator.Migrate7to8 and increment the migration version is consistent with changes in other modules. This change suggests a significant update in the module's data structure or logic.

Please ensure that the Migrate7to8 function in the migrator is correctly implemented to handle the migration from version 7 to 8. You can verify this with the following script:

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify the implementation of Migrate7to8 function

# Test: Search for the Migrate7to8 function definition
rg -A 10 'func \(Migrator\) Migrate7to8'

Length of output: 23819

}
Expand All @@ -130,7 +130,7 @@ func (am AppModule) ExportGenesis(ctx sdk.Context, cdc codec.JSONCodec) json.Raw

// ConsensusVersion implements AppModule/ConsensusVersion.
func (am AppModule) ConsensusVersion() uint64 {
return 3
return 4
}

// EndBlock implements app module
Expand Down
4 changes: 2 additions & 2 deletions x/bsc/module.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -108,7 +108,7 @@ func (am AppModule) RegisterInvariants(_ sdk.InvariantRegistry) {}
// RegisterServices registers module services.
func (am AppModule) RegisterServices(cfg module.Configurator) {
migrator := crosschainkeeper.NewMigrator(am.keeper)
if err := cfg.RegisterMigration(am.Name(), 4, migrator.Migrate); err != nil {
if err := cfg.RegisterMigration(am.Name(), 5, migrator.Migrate7to8); err != nil {
panic(err)
}
}
Expand All @@ -130,7 +130,7 @@ func (am AppModule) ExportGenesis(ctx sdk.Context, cdc codec.JSONCodec) json.Raw

// ConsensusVersion implements AppModule/ConsensusVersion.
func (am AppModule) ConsensusVersion() uint64 {
return 5
return 6
}

// EndBlock implements app module
Expand Down
3 changes: 2 additions & 1 deletion x/crosschain/keeper/legacy.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,13 +3,14 @@ package keeper
import (
sdk "github.com/cosmos/cosmos-sdk/types"

"github.com/functionx/fx-core/v8/x/crosschain/migrations/v8"
"github.com/functionx/fx-core/v8/x/crosschain/types"
)

// Deprecated: do not use, remove in v8
func (k Keeper) LegacyGetDenomBridgeToken(ctx sdk.Context, denom string) (*types.BridgeToken, bool) {
store := ctx.KVStore(k.storeKey)
data := store.Get(types.GetTokenToDenomKey(denom))
data := store.Get(v8.GetTokenToDenomKey(denom))
if len(data) == 0 {
return nil, false
}
Expand Down
22 changes: 9 additions & 13 deletions x/crosschain/keeper/migrations.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,8 +3,7 @@ package keeper
import (
sdk "github.com/cosmos/cosmos-sdk/types"

fxtypes "github.com/functionx/fx-core/v8/types"
"github.com/functionx/fx-core/v8/x/crosschain/types"
"github.com/functionx/fx-core/v8/x/crosschain/migrations/v8"
)

type Migrator struct {
Expand All @@ -17,18 +16,15 @@ func NewMigrator(k Keeper) Migrator {
}
}

func (m Migrator) Migrate(ctx sdk.Context) error {
params := m.keeper.GetParams(ctx)

params.BridgeCallTimeout = types.DefBridgeCallTimeout
params.BridgeCallMaxGasLimit = types.MaxGasLimit
func (m Migrator) Migrate7to8(ctx sdk.Context) error {
return v8.Migrate(ctx, m.keeper.storeKey)
}

enablePending := false
if ctx.ChainID() == fxtypes.TestnetChainId {
enablePending = true
func (m Migrator) Migrate7to8WithArbExternalBlockTime(ctx sdk.Context) error {
if err := m.Migrate7to8(ctx); err != nil {
return err
}
params.EnableSendToExternalPending = enablePending
params.EnableBridgeCallPending = enablePending

params := m.keeper.GetParams(ctx)
params.AverageExternalBlockTime = 250
Comment on lines +23 to +28
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Consider making AverageExternalBlockTime configurable

Hardcoding AverageExternalBlockTime may limit flexibility in different environments or future updates. Making this value configurable through parameters or configuration files can enhance adaptability and ease of maintenance.

return m.keeper.SetParams(ctx, &params)
}
37 changes: 37 additions & 0 deletions x/crosschain/migrations/v8/migrate.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,37 @@
package v8

import (
storetypes "cosmossdk.io/store/types"
sdk "github.com/cosmos/cosmos-sdk/types"

"github.com/functionx/fx-core/v8/app/upgrades/store"
)

var (
// Deprecated: DenomToTokenKey prefixes the index of asset denom to external token
DenomToTokenKey = []byte{0x26}

// Deprecated: TokenToDenomKey prefixes the index of assets external token to denom
TokenToDenomKey = []byte{0x27}

// Deprecated: remove data in upgrade
BridgeCallFromMsgKey = []byte{0x51}
)

// Deprecated: GetTokenToDenomKey returns the following key format
func GetTokenToDenomKey(denom string) []byte {
return append(TokenToDenomKey, []byte(denom)...)
}

func GetRemovedStoreKeys() [][]byte {
return [][]byte{
DenomToTokenKey,
TokenToDenomKey,
BridgeCallFromMsgKey,
}
}

func Migrate(ctx sdk.Context, storeKey storetypes.StoreKey) error {
store.RemoveStoreKeys(ctx, storeKey, GetRemovedStoreKeys())
return nil
}
Comment on lines +34 to +37
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Enhance error handling and logging in Migrate function.

While the Migrate function correctly calls store.RemoveStoreKeys, there are a few areas for improvement:

  1. Error Handling: The function always returns nil, potentially hiding errors from RemoveStoreKeys. Consider returning the error from RemoveStoreKeys.

  2. Logging: Add logging to track the migration process. This can help with debugging and monitoring.

Here's a suggested improvement:

func Migrate(ctx sdk.Context, storeKey storetypes.StoreKey) error {
    removedKeys := GetRemovedStoreKeys()
    err := store.RemoveStoreKeys(ctx, storeKey, removedKeys)
    if err != nil {
        ctx.Logger().Error("Failed to remove deprecated store keys", "error", err)
        return err
    }
    ctx.Logger().Info("Successfully removed deprecated store keys", "count", len(removedKeys))
    return nil
}

This version provides better error handling and adds logging for both success and failure scenarios.

26 changes: 0 additions & 26 deletions x/crosschain/types/key.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,12 +66,6 @@ var (
// KeyLastOutgoingBatchID indexes the lastBatchID
KeyLastOutgoingBatchID = append(SequenceKeyPrefix, []byte("lastBatchId")...)

// Deprecated: DenomToTokenKey prefixes the index of asset denom to external token
DenomToTokenKey = []byte{0x26}

// Deprecated: TokenToDenomKey prefixes the index of assets external token to denom
TokenToDenomKey = []byte{0x27}

// LastSlashedOracleSetNonce indexes the latest slashed oracleSet nonce
LastSlashedOracleSetNonce = []byte{0x28}

Expand Down Expand Up @@ -121,13 +115,7 @@ var (
OutgoingBridgeCallNonceKey = []byte{0x48}
OutgoingBridgeCallAddressAndNonceKey = []byte{0x49}

// Deprecated: remove data in upgrade
BridgeCallFromMsgKey = []byte{0x51}

PendingExecuteClaimKey = []byte{0x54}

// BridgeDenomKey -> value BridgeDenom
BridgeDenomKey = []byte{0x60}
)

// GetOracleKey returns the following key format
Expand Down Expand Up @@ -202,16 +190,6 @@ func GetLastEventBlockHeightByOracleKey(oracleAddr sdk.AccAddress) []byte {
return append(LastEventBlockHeightByOracleKey, oracleAddr.Bytes()...)
}

// Deprecated: GetDenomToTokenKey returns the following key format
func GetDenomToTokenKey(tokenContract string) []byte {
return append(DenomToTokenKey, []byte(tokenContract)...)
}

// Deprecated: GetTokenToDenomKey returns the following key format
func GetTokenToDenomKey(denom string) []byte {
return append(TokenToDenomKey, []byte(denom)...)
}

func GetBridgeCallConfirmKey(nonce uint64, addr sdk.AccAddress) []byte {
return append(BridgeCallConfirmKey, append(sdk.Uint64ToBigEndian(nonce), addr.Bytes()...)...)
}
Expand Down Expand Up @@ -241,7 +219,3 @@ func ParseOutgoingBridgeCallNonce(key []byte, address string) (nonce uint64) {
func GetPendingExecuteClaimKey(nonce uint64) []byte {
return append(PendingExecuteClaimKey, sdk.Uint64ToBigEndian(nonce)...)
}

func GetBridgeDenomKey(denom string) []byte {
return append(BridgeDenomKey, []byte(denom)...)
}
4 changes: 2 additions & 2 deletions x/eth/module.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -108,7 +108,7 @@ func (am AppModule) RegisterInvariants(_ sdk.InvariantRegistry) {}
// RegisterServices registers module services.
func (am AppModule) RegisterServices(cfg module.Configurator) {
migrator := crosschainkeeper.NewMigrator(am.keeper)
if err := cfg.RegisterMigration(am.Name(), 2, migrator.Migrate); err != nil {
if err := cfg.RegisterMigration(am.Name(), 3, migrator.Migrate7to8); err != nil {
panic(err)
}
Comment on lines +111 to 113
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codebase verification

Migration Function Migrate7to8 Not Found

The migrator.Migrate7to8 function is registered but its definition does not exist in the codebase. Please implement this migration function or update the registration accordingly.

🔗 Analysis chain

LGTM. Verify impact and update documentation.

The migration version update from 2 to 3 and the change to migrator.Migrate7to8 aligns with similar updates in other modules. This change appears to be part of a coordinated update across the codebase.

Please ensure the following:

  1. All dependent modules are updated to use the new migration version.
  2. The Migrate7to8 function is properly implemented in the migrator.
  3. Documentation is updated to reflect this change in migration strategy.

Run the following script to verify the consistency of migration versions across modules:

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Check for consistency in migration versions across modules

# Test: Search for RegisterMigration calls in all module files
rg --type go 'RegisterMigration.*Migrate.*to.*'

Length of output: 62

Script:

#!/bin/bash
# Description: Search for all RegisterMigration calls and list their migration functions

rg --type go 'RegisterMigration\s*\(\s*[^,]+,\s*\d+,\s*([^)\s]+)\s*\)' --replace '$1' --count-matches

Length of output: 309

Script:

#!/bin/bash
# Description: Find the definition of migrator.Migrate7to8

rg --type go 'func\s+Migrate7to8\s*\('

Length of output: 40

}
Expand All @@ -130,7 +130,7 @@ func (am AppModule) ExportGenesis(ctx sdk.Context, cdc codec.JSONCodec) json.Raw

// ConsensusVersion implements AppModule/ConsensusVersion.
func (am AppModule) ConsensusVersion() uint64 {
return 3
return 4
}

// EndBlock implements app module
Expand Down
Loading