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(gov): migrate gov module to v8 #735

Merged
merged 1 commit into from
Oct 12, 2024
Merged

refactor(gov): migrate gov module to v8 #735

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
26 changes: 21 additions & 5 deletions app/upgrade_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,8 @@ import (
dbm "github.com/cosmos/cosmos-db"
cryptotypes "github.com/cosmos/cosmos-sdk/crypto/types"
sdk "github.com/cosmos/cosmos-sdk/types"
distributiontypes "github.com/cosmos/cosmos-sdk/x/distribution/types"
govtypes "github.com/cosmos/cosmos-sdk/x/gov/types"
stakingtypes "github.com/cosmos/cosmos-sdk/x/staking/types"
"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"
Expand All @@ -22,6 +24,8 @@ import (
nextversion "github.com/functionx/fx-core/v8/app/upgrades/v8"
"github.com/functionx/fx-core/v8/testutil/helpers"
fxtypes "github.com/functionx/fx-core/v8/types"
fxgovv8 "github.com/functionx/fx-core/v8/x/gov/migrations/v8"
fxgovtypes "github.com/functionx/fx-core/v8/x/gov/types"
fxstakingv8 "github.com/functionx/fx-core/v8/x/staking/migrations/v8"
)

Expand Down Expand Up @@ -91,14 +95,26 @@ func newContext(t *testing.T, myApp *app.App, chainId string, deliveState bool)

func checkAppUpgrade(t *testing.T, ctx sdk.Context, myApp *app.App) {
checkStakingMigrationDelete(t, ctx, myApp)

checkGovCustomParams(t, ctx, myApp)
}

func checkGovCustomParams(t *testing.T, ctx sdk.Context, myApp *app.App) {
egfCustomParams, found := myApp.GovKeeper.GetCustomParams(ctx, sdk.MsgTypeURL(&distributiontypes.MsgCommunityPoolSpend{}))
require.True(t, found)
expectEGFParams := fxgovtypes.NewCustomParams(fxgovtypes.EGFCustomParamDepositRatio.String(), fxgovtypes.DefaultEGFCustomParamVotingPeriod, fxgovtypes.DefaultCustomParamQuorum40.String())
assert.Equal(t, expectEGFParams, egfCustomParams)

checkKeysIsDelete(t, ctx.KVStore(myApp.GetKey(govtypes.StoreKey)), fxgovv8.GetRemovedStoreKeys())
}

func checkStakingMigrationDelete(t *testing.T, ctx sdk.Context, myApp *app.App) {
storeKey := myApp.GetKey(stakingtypes.StoreKey)
kvStore := ctx.KVStore(storeKey)
removeKeys := fxstakingv8.GetRemovedValidatorStoreKeys()
require.Greater(t, len(removeKeys), 0)
for _, removeKey := range removeKeys {
checkKeysIsDelete(t, ctx.KVStore(myApp.GetKey(stakingtypes.StoreKey)), fxstakingv8.GetRemovedStoreKeys())
}

func checkKeysIsDelete(t *testing.T, kvStore storetypes.KVStore, keys [][]byte) {
require.Greater(t, len(keys), 0)
for _, removeKey := range keys {
iterator := storetypes.KVStorePrefixIterator(kvStore, removeKey)
defer iterator.Close()
for ; iterator.Valid(); iterator.Next() {
Expand Down
16 changes: 16 additions & 0 deletions app/upgrades/v8/upgrade.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,12 +3,16 @@ package v8
import (
"context"

storetypes "cosmossdk.io/store/types"
upgradetypes "cosmossdk.io/x/upgrade/types"
sdk "github.com/cosmos/cosmos-sdk/types"
"github.com/cosmos/cosmos-sdk/types/module"
govtypes "github.com/cosmos/cosmos-sdk/x/gov/types"
stakingtypes "github.com/cosmos/cosmos-sdk/x/staking/types"

"github.com/functionx/fx-core/v8/app/keepers"
"github.com/functionx/fx-core/v8/x/gov/keeper"
fxgovv8 "github.com/functionx/fx-core/v8/x/gov/migrations/v8"
fxstakingv8 "github.com/functionx/fx-core/v8/x/staking/migrations/v8"
)

Expand All @@ -24,8 +28,20 @@ func CreateUpgradeHandler(mm *module.Manager, configurator module.Configurator,

fxstakingv8.DeleteMigrationValidatorStore(cacheCtx, app.GetKey(stakingtypes.StoreKey))

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

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

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

// 2. init custom params
return keeper.InitCustomParams(ctx)
}
Comment on lines +41 to +47
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 Potential Errors from DeleteOldParamsStore Function

The fxgovv8.DeleteOldParamsStore(ctx, storeKey) function might return an error that is currently not being handled. To enhance robustness, consider checking and handling any potential errors from this function.

Apply this diff to handle the error:

 func migrationGovCustomParam(ctx sdk.Context, keeper *keeper.Keeper, storeKey *storetypes.KVStoreKey) error {
     // 1. delete fxParams key
-    fxgovv8.DeleteOldParamsStore(ctx, storeKey)
+    if err := fxgovv8.DeleteOldParamsStore(ctx, storeKey); err != nil {
+        return err
+    }

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

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
func migrationGovCustomParam(ctx sdk.Context, keeper *keeper.Keeper, storeKey *storetypes.KVStoreKey) error {
// 1. delete fxParams key
fxgovv8.DeleteOldParamsStore(ctx, storeKey)
// 2. init custom params
return keeper.InitCustomParams(ctx)
}
func migrationGovCustomParam(ctx sdk.Context, keeper *keeper.Keeper, storeKey *storetypes.KVStoreKey) error {
// 1. delete fxParams key
if err := fxgovv8.DeleteOldParamsStore(ctx, storeKey); err != nil {
return err
}
// 2. init custom params
return keeper.InitCustomParams(ctx)
}

31 changes: 31 additions & 0 deletions x/gov/migrations/v8/migrate.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,31 @@
package v8

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

var (
// Deprecated: do not use, remove in v8
FxBaseParamsKeyPrefix = []byte("0x90")
// Deprecated: do not use, remove in v8
FxEGFParamsKey = []byte("0x91")
)
Comment on lines +8 to +13
Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Refactor deprecated key declarations and clarify deprecation timeline.

  1. Consider using const instead of var for these values, as they appear to be constants.
  2. The deprecation comments state "remove in v8", but this file is already in the v8 package. Please clarify the deprecation timeline.
  3. Add comments explaining the significance of the byte values 0x90 and 0x91.

Here's a suggested refactoring:

const (
	// FxBaseParamsKeyPrefix is the deprecated key prefix for base parameters.
	// It will be removed in the next major version.
	// The value 0x90 represents [explain significance].
	FxBaseParamsKeyPrefix = []byte{0x90}

	// FxEGFParamsKey is the deprecated key for EGF parameters.
	// It will be removed in the next major version.
	// The value 0x91 represents [explain significance].
	FxEGFParamsKey = []byte{0x91}
)


func GetRemovedStoreKeys() [][]byte {
return [][]byte{FxBaseParamsKeyPrefix, FxEGFParamsKey}
}

func DeleteOldParamsStore(
ctx sdk.Context,
storeKey storetypes.StoreKey,
) {
store := ctx.KVStore(storeKey)
removeKeys := GetRemovedStoreKeys()
for _, key := range removeKeys {
iterator := storetypes.KVStorePrefixIterator(store, key)
for ; iterator.Valid(); iterator.Next() {
store.Delete(iterator.Key())
}
}
}
Comment on lines +19 to +31
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

Close iterator and consider minor optimization in DeleteOldParamsStore.

The function correctly deletes deprecated entries, but there are two points to address:

  1. The iterator is not being closed, which could lead to resource leaks.
  2. A minor optimization can be made by moving the store retrieval inside the loop.

Here's a suggested refactoring:

func DeleteOldParamsStore(
	ctx sdk.Context,
	storeKey storetypes.StoreKey,
) {
	removeKeys := GetRemovedStoreKeys()
	for _, key := range removeKeys {
		store := ctx.KVStore(storeKey)
		iterator := storetypes.KVStorePrefixIterator(store, key)
		defer iterator.Close()
		for ; iterator.Valid(); iterator.Next() {
			store.Delete(iterator.Key())
		}
	}
}

This change ensures that the iterator is properly closed and slightly optimizes the function by moving the store retrieval inside the loop, ensuring we always work with the most up-to-date store state.

7 changes: 0 additions & 7 deletions x/gov/types/params.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,13 +24,6 @@ var (
)

var (
// todo delete this store
// FxBaseParamsKeyPrefix is the key to query all base params
FxBaseParamsKeyPrefix = []byte("0x90")
// todo delete this store
// FxEGFParamsKey is the key to query all EGF params
FxEGFParamsKey = []byte("0x91")

FxSwitchParamsKey = []byte{0x92}
CustomParamsKey = []byte{0x93}
)
Expand Down
4 changes: 2 additions & 2 deletions x/staking/migrations/v8/migrate.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ var (
ConsensusProcessKey = []byte{0x93}
)

func GetRemovedValidatorStoreKeys() [][]byte {
func GetRemovedStoreKeys() [][]byte {
return [][]byte{ValidatorOperatorKey, ConsensusPubKey, ConsensusProcessKey}
}

Expand All @@ -23,7 +23,7 @@ func DeleteMigrationValidatorStore(
storeKey storetypes.StoreKey,
) {
store := ctx.KVStore(storeKey)
removeKeys := GetRemovedValidatorStoreKeys()
removeKeys := GetRemovedStoreKeys()
for _, key := range removeKeys {
iterator := storetypes.KVStorePrefixIterator(store, key)
for ; iterator.Valid(); iterator.Next() {
Expand Down