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.

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

fix(x/bank): miss keypair of SendEnabled to restore legacy param set before migration #18107

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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 CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,7 @@ Ref: https://keepachangelog.com/en/1.0.0/
* (config) [#17649](https://github.com/cosmos/cosmos-sdk/pull/17649) Fix `mempool.max-txs` configuration is invalid in `app.config`.
* (mempool) [#17668](https://github.com/cosmos/cosmos-sdk/pull/17668) Fix `PriorityNonceIterator.Next()` nil pointer ref for min priority at the end of iteration.
* (x/auth) [#17902](https://github.com/cosmos/cosmos-sdk/pull/17902) Remove tip posthandler.
* (x/bank) [#18107](https://github.com/cosmos/cosmos-sdk/pull/18107) Add missing keypair of SendEnabled to restore legacy param set before migration.

### Client Breaking Changes

Expand Down
2 changes: 2 additions & 0 deletions tests/integration/bank/keeper/keeper_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -1642,6 +1642,8 @@ func (ms mockSubspace) GetParamSet(ctx sdk.Context, ps exported.ParamSet) {
*ps.(*types.Params) = ms.ps
}

func (ms mockSubspace) Get(ctx sdk.Context, key []byte, ptr interface{}) {}

func (suite *IntegrationTestSuite) TestMigrator_Migrate3to4() {
ctx, bankKeeper := suite.ctx, suite.bankKeeper

Expand Down
1 change: 1 addition & 0 deletions x/bank/exported/exported.go
Original file line number Diff line number Diff line change
Expand Up @@ -21,5 +21,6 @@ type (
// NOTE: This is used solely for migration of x/params managed parameters.
Subspace interface {
GetParamSet(ctx sdk.Context, ps ParamSet)
Get(ctx sdk.Context, key []byte, ptr interface{})
}
)
2 changes: 2 additions & 0 deletions x/bank/keeper/keeper_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -1697,6 +1697,8 @@ func (ms mockSubspace) GetParamSet(ctx sdk.Context, ps exported.ParamSet) {
*ps.(*banktypes.Params) = ms.ps
}

func (ms mockSubspace) Get(ctx sdk.Context, key []byte, ptr interface{}) {}

func (suite *KeeperTestSuite) TestMigrator_Migrate3to4() {
ctx, bankKeeper := suite.ctx, suite.bankKeeper
require := suite.Require()
Expand Down
9 changes: 9 additions & 0 deletions x/bank/keeper/migrations.go
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import (
v2 "github.com/cosmos/cosmos-sdk/x/bank/migrations/v2"
v3 "github.com/cosmos/cosmos-sdk/x/bank/migrations/v3"
v4 "github.com/cosmos/cosmos-sdk/x/bank/migrations/v4"
"github.com/cosmos/cosmos-sdk/x/bank/types"
)

// Migrator is a struct for handling in-place store migrations.
Expand All @@ -31,5 +32,13 @@ func (m Migrator) Migrate2to3(ctx sdk.Context) error {

// Migrate3to4 migrates x/bank storage from version 3 to 4.
func (m Migrator) Migrate3to4(ctx sdk.Context) error {
Copy link
Member

Choose a reason for hiding this comment

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

Can we add an helper, that gets already migrated bank params (so current v047 users), get the send enabled params from x/params and update the bank params?
This way affected users (I've checked, found none, but just in case), can use it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure if you mean add a public helper in app module, or do you have any example I should put it in.

Copy link
Member

@julienrbrt julienrbrt Oct 13, 2023

Choose a reason for hiding this comment

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

This is almost what I meant, yes, but no need to have anything in AppModule.

You can keep Migrate3to4 as you had it, with the fix, and move MigrateSubspaceParams here (package keeper or v4), named MigrateSendEnabledParams with a comment explaining this is only needed for chains having migrated from <= v0.47 to v0.47.0-5

m.MigrateSendEnabledParams(ctx)
return v4.MigrateStore(ctx, m.keeper.storeKey, m.legacySubspace, m.keeper.cdc)
}

// MigrateSendEnabledParams get params from x/params and update the bank params.
// This function is only needed for chains having migrated from <= v0.47 to v0.47.0-5
func (m Migrator) MigrateSendEnabledParams(ctx sdk.Context) {
sendEnabled := types.GetSendEnabledParams(ctx, m.legacySubspace)
m.keeper.SetAllSendEnabled(ctx, sendEnabled)
}
3 changes: 3 additions & 0 deletions x/bank/migrations/v4/store.go
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,9 @@ func MigrateStore(ctx sdk.Context, storeKey storetypes.StoreKey, legacySubspace
var currParams types.Params
legacySubspace.GetParamSet(ctx, &currParams)

// SendEnabled is migrated to the x/bank module store, so delete from the params
currParams = types.NewParams(currParams.DefaultSendEnabled)

if err := currParams.Validate(); err != nil {
return err
}
Expand Down
2 changes: 2 additions & 0 deletions x/bank/migrations/v4/store_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,8 @@ func (ms mockSubspace) GetParamSet(ctx sdk.Context, ps exported.ParamSet) {
*ps.(*types.Params) = ms.ps
}

func (ms mockSubspace) Get(ctx sdk.Context, key []byte, ptr interface{}) {}

func TestMigrate(t *testing.T) {
encCfg := moduletestutil.MakeTestEncodingConfig(bank.AppModuleBasic{})
cdc := encCfg.Codec
Expand Down
46 changes: 45 additions & 1 deletion x/bank/types/params_legacy.go
Original file line number Diff line number Diff line change
@@ -1,6 +1,12 @@
package types

import paramtypes "github.com/cosmos/cosmos-sdk/x/params/types"
import (
fmt "fmt"

sdk "github.com/cosmos/cosmos-sdk/types"
"github.com/cosmos/cosmos-sdk/x/bank/exported"
paramtypes "github.com/cosmos/cosmos-sdk/x/params/types"
)

var (
// KeySendEnabled is store's key for SendEnabled Params
Expand All @@ -18,6 +24,44 @@ func ParamKeyTable() paramtypes.KeyTable {
// Deprecated: ParamSetPairs implements params.ParamSet
func (p *Params) ParamSetPairs() paramtypes.ParamSetPairs {
return paramtypes.ParamSetPairs{
paramtypes.NewParamSetPair(KeySendEnabled, &p.SendEnabled, validateSendEnabledParams),
paramtypes.NewParamSetPair(KeyDefaultSendEnabled, &p.DefaultSendEnabled, validateIsBool),
}
}

// SendEnabledParams is a collection of parameters indicating if a coin denom is enabled for sending
type SendEnabledParams []*SendEnabled

// GetSendEnabledParams retrieves the send enabled parameters from the provided context and legacy subspace.
func GetSendEnabledParams(ctx sdk.Context, legacySubspace exported.Subspace) []*SendEnabled {
var sendEnabled []*SendEnabled
legacySubspace.Get(ctx, KeySendEnabled, &sendEnabled)
return sendEnabled
}

func validateSendEnabledParams(i interface{}) error {
params, ok := i.([]*SendEnabled)
if !ok {
return fmt.Errorf("invalid parameter type: %T", i)
}
// ensure each denom is only registered one time.
registered := make(map[string]bool)
for _, p := range params {
if _, exists := registered[p.Denom]; exists {
return fmt.Errorf("duplicate send enabled parameter found: '%s'", p.Denom)
}
if err := validateSendEnabled(*p); err != nil {
return err
}
registered[p.Denom] = true
}
return nil
}

func validateSendEnabled(i interface{}) error {
param, ok := i.(SendEnabled)
if !ok {
return fmt.Errorf("invalid parameter type: %T", i)
}
return sdk.ValidateDenom(param.Denom)
}
Loading