From 8ef342d775a4d9b3a510dbfea6647d620e9aea65 Mon Sep 17 00:00:00 2001 From: testinginprod <98415576+testinginprod@users.noreply.github.com> Date: Thu, 6 Jul 2023 09:51:58 +0200 Subject: [PATCH 1/3] fix(bank): make `DenomAddressIndex` less strict with respect to index values. (#16841) Co-authored-by: unknown unknown (cherry picked from commit acce343a1dfa1047ba7f7ea5eba9e25da12513b8) # Conflicts: # CHANGELOG.md # x/bank/keeper/view.go --- CHANGELOG.md | 14 +++++ x/bank/keeper/collections_test.go | 85 +++++++++++++++++++++++++++++++ x/bank/keeper/view.go | 7 ++- x/bank/types/keys.go | 24 ++------- x/bank/types/keys_test.go | 5 +- 5 files changed, 112 insertions(+), 23 deletions(-) create mode 100644 x/bank/keeper/collections_test.go diff --git a/CHANGELOG.md b/CHANGELOG.md index 2afdf3b6c6d6..aa7d17d32584 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -38,6 +38,20 @@ Ref: https://keepachangelog.com/en/1.0.0/ ## [Unreleased] +<<<<<<< HEAD +======= +### Features + +### Improvements + +* (all) [#16537](https://github.com/cosmos/cosmos-sdk/pull/16537) Properly propagated fmt.Errorf errors + using errors.New where appropriate. + +### Bug Fixes + +* (server) [#16827](https://github.com/cosmos/cosmos-sdk/pull/16827) Properly use `--trace` flag (before it was setting the trace level instead of displaying the stacktraces). +* (x/bank) [#16841](https://github.com/cosmos/cosmos-sdk/pull/16841) correctly process legacy `DenomAddressIndex` values. + +>>>>>>> acce343a1 (fix(bank): make `DenomAddressIndex` less strict with respect to index values. (#16841)) ### API Breaking Changes * (types/math) [#16040](https://github.com/cosmos/cosmos-sdk/pull/16798) Remove aliases in `types/math.go` (part 2). diff --git a/x/bank/keeper/collections_test.go b/x/bank/keeper/collections_test.go new file mode 100644 index 000000000000..15c22c489d06 --- /dev/null +++ b/x/bank/keeper/collections_test.go @@ -0,0 +1,85 @@ +package keeper_test + +import ( + "testing" + + cmtproto "github.com/cometbft/cometbft/proto/tendermint/types" + cmttime "github.com/cometbft/cometbft/types/time" + "github.com/golang/mock/gomock" + "github.com/stretchr/testify/require" + + "cosmossdk.io/collections" + "cosmossdk.io/log" + "cosmossdk.io/math" + storetypes "cosmossdk.io/store/types" + + "github.com/cosmos/cosmos-sdk/codec/address" + "github.com/cosmos/cosmos-sdk/runtime" + "github.com/cosmos/cosmos-sdk/testutil" + sdk "github.com/cosmos/cosmos-sdk/types" + moduletestutil "github.com/cosmos/cosmos-sdk/types/module/testutil" + authtypes "github.com/cosmos/cosmos-sdk/x/auth/types" + "github.com/cosmos/cosmos-sdk/x/bank/keeper" + banktestutil "github.com/cosmos/cosmos-sdk/x/bank/testutil" + banktypes "github.com/cosmos/cosmos-sdk/x/bank/types" +) + +func TestBankStateCompatibility(t *testing.T) { + key := storetypes.NewKVStoreKey(banktypes.StoreKey) + testCtx := testutil.DefaultContextWithDB(t, key, storetypes.NewTransientStoreKey("transient_test")) + ctx := testCtx.Ctx.WithBlockHeader(cmtproto.Header{Time: cmttime.Now()}) + encCfg := moduletestutil.MakeTestEncodingConfig() + + storeService := runtime.NewKVStoreService(key) + + // gomock initializations + ctrl := gomock.NewController(t) + authKeeper := banktestutil.NewMockAccountKeeper(ctrl) + authKeeper.EXPECT().AddressCodec().Return(address.NewBech32Codec("cosmos")).AnyTimes() + + k := keeper.NewBaseKeeper( + encCfg.Codec, + storeService, + authKeeper, + map[string]bool{accAddrs[4].String(): true}, + authtypes.NewModuleAddress(banktypes.GovModuleName).String(), + log.NewNopLogger(), + ) + + // test we can decode balances without problems + // using the old value format of the denom to address index + bankDenomAddressLegacyIndexValue := []byte{0} // taken from: https://github.com/cosmos/cosmos-sdk/blob/v0.47.3/x/bank/keeper/send.go#L361 + rawKey, err := collections.EncodeKeyWithPrefix( + banktypes.DenomAddressPrefix, + k.Balances.Indexes.Denom.KeyCodec(), + collections.Join("atom", sdk.AccAddress("test")), + ) + require.NoError(t, err) + // we set the index key to the old value. + require.NoError(t, storeService.OpenKVStore(ctx).Set(rawKey, bankDenomAddressLegacyIndexValue)) + + // test walking is ok + err = k.Balances.Indexes.Denom.Walk(ctx, nil, func(indexingKey string, indexedKey sdk.AccAddress) (stop bool, err error) { + require.Equal(t, indexedKey, sdk.AccAddress("test")) + require.Equal(t, indexingKey, "atom") + return true, nil + }) + require.NoError(t, err) + + // test matching is also ok + iter, err := k.Balances.Indexes.Denom.MatchExact(ctx, "atom") + require.NoError(t, err) + defer iter.Close() + pks, err := iter.PrimaryKeys() + require.NoError(t, err) + require.Len(t, pks, 1) + require.Equal(t, pks[0], collections.Join(sdk.AccAddress("test"), "atom")) + + // assert the index value will be updated to the new format + err = k.Balances.Indexes.Denom.Reference(ctx, collections.Join(sdk.AccAddress("test"), "atom"), math.ZeroInt(), nil) + require.NoError(t, err) + + newRawValue, err := storeService.OpenKVStore(ctx).Get(rawKey) + require.NoError(t, err) + require.Equal(t, []byte{}, newRawValue) +} diff --git a/x/bank/keeper/view.go b/x/bank/keeper/view.go index 8d7fae395954..df9f88624a36 100644 --- a/x/bank/keeper/view.go +++ b/x/bank/keeper/view.go @@ -42,7 +42,12 @@ func newBalancesIndexes(sb *collections.SchemaBuilder) BalancesIndexes { return BalancesIndexes{ Denom: indexes.NewReversePair[math.Int]( sb, types.DenomAddressPrefix, "address_by_denom_index", +<<<<<<< HEAD collections.PairKeyCodec(sdk.AddressKeyAsIndexKey(sdk.AccAddressKey), collections.StringKey), // nolint:staticcheck // Note: refer to the AddressKeyAsIndexKey docs to understand why we do this. +======= + collections.PairKeyCodec(sdk.LengthPrefixedAddressKey(sdk.AccAddressKey), collections.StringKey), // nolint:staticcheck // Note: refer to the LengthPrefixedAddressKey docs to understand why we do this. + indexes.WithReversePairUncheckedValue(), // denom to address indexes were stored as Key: Join(denom, address) Value: []byte{0}, this will migrate the value to []byte{} in a lazy way. +>>>>>>> acce343a1 (fix(bank): make `DenomAddressIndex` less strict with respect to index values. (#16841)) ), } } @@ -81,7 +86,7 @@ func NewBaseViewKeeper(cdc codec.BinaryCodec, storeService store.KVStoreService, Supply: collections.NewMap(sb, types.SupplyKey, "supply", collections.StringKey, sdk.IntValue), DenomMetadata: collections.NewMap(sb, types.DenomMetadataPrefix, "denom_metadata", collections.StringKey, codec.CollValue[types.Metadata](cdc)), SendEnabled: collections.NewMap(sb, types.SendEnabledPrefix, "send_enabled", collections.StringKey, codec.BoolValue), // NOTE: we use a bool value which uses protobuf to retain state backwards compat - Balances: collections.NewIndexedMap(sb, types.BalancesPrefix, "balances", collections.PairKeyCodec(sdk.AccAddressKey, collections.StringKey), types.NewBalanceCompatValueCodec(), newBalancesIndexes(sb)), + Balances: collections.NewIndexedMap(sb, types.BalancesPrefix, "balances", collections.PairKeyCodec(sdk.AccAddressKey, collections.StringKey), types.BalanceValueCodec, newBalancesIndexes(sb)), Params: collections.NewItem(sb, types.ParamsKey, "params", codec.CollValue[types.Params](cdc)), } diff --git a/x/bank/types/keys.go b/x/bank/types/keys.go index 7330459cd238..b4ea683d4b69 100644 --- a/x/bank/types/keys.go +++ b/x/bank/types/keys.go @@ -34,27 +34,13 @@ var ( ParamsKey = collections.NewPrefix(5) ) -// NewBalanceCompatValueCodec is a codec for encoding Balances in a backwards compatible way -// with respect to the old format. -func NewBalanceCompatValueCodec() collcodec.ValueCodec[math.Int] { - return balanceCompatValueCodec{ - sdk.IntValue, - } -} - -type balanceCompatValueCodec struct { - collcodec.ValueCodec[math.Int] -} - -func (v balanceCompatValueCodec) Decode(b []byte) (math.Int, error) { - i, err := v.ValueCodec.Decode(b) - if err == nil { - return i, nil - } +// BalanceValueCodec is a codec for encoding bank balances in a backwards compatible way. +// Historically, balances were represented as Coin, now they're represented as a simple math.Int +var BalanceValueCodec = collcodec.NewAltValueCodec(sdk.IntValue, func(bytes []byte) (math.Int, error) { c := new(sdk.Coin) - err = c.Unmarshal(b) + err := c.Unmarshal(bytes) if err != nil { return math.Int{}, err } return c.Amount, nil -} +}) diff --git a/x/bank/types/keys_test.go b/x/bank/types/keys_test.go index cf0c01eddd62..fa2c48669b61 100644 --- a/x/bank/types/keys_test.go +++ b/x/bank/types/keys_test.go @@ -12,16 +12,15 @@ import ( ) func TestBalanceValueCodec(t *testing.T) { - c := NewBalanceCompatValueCodec() t.Run("value codec implementation", func(t *testing.T) { - colltest.TestValueCodec(t, c, math.NewInt(100)) + colltest.TestValueCodec(t, BalanceValueCodec, math.NewInt(100)) }) t.Run("legacy coin", func(t *testing.T) { coin := sdk.NewInt64Coin("coin", 1000) b, err := coin.Marshal() require.NoError(t, err) - amt, err := c.Decode(b) + amt, err := BalanceValueCodec.Decode(b) require.NoError(t, err) require.Equal(t, coin.Amount, amt) }) From 3821ff94c191df1c59c59e7ad8d93cef65e1bf4d Mon Sep 17 00:00:00 2001 From: Julien Robert Date: Thu, 6 Jul 2023 12:36:01 +0200 Subject: [PATCH 2/3] updates --- CHANGELOG.md | 12 +----------- x/bank/keeper/view.go | 4 ---- 2 files changed, 1 insertion(+), 15 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index aa7d17d32584..be5c97c17b6d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -38,20 +38,10 @@ Ref: https://keepachangelog.com/en/1.0.0/ ## [Unreleased] -<<<<<<< HEAD -======= -### Features - -### Improvements - -* (all) [#16537](https://github.com/cosmos/cosmos-sdk/pull/16537) Properly propagated fmt.Errorf errors + using errors.New where appropriate. - ### Bug Fixes -* (server) [#16827](https://github.com/cosmos/cosmos-sdk/pull/16827) Properly use `--trace` flag (before it was setting the trace level instead of displaying the stacktraces). -* (x/bank) [#16841](https://github.com/cosmos/cosmos-sdk/pull/16841) correctly process legacy `DenomAddressIndex` values. +* (x/bank) [#16841](https://github.com/cosmos/cosmos-sdk/pull/16841) Correctly process legacy `DenomAddressIndex` values. ->>>>>>> acce343a1 (fix(bank): make `DenomAddressIndex` less strict with respect to index values. (#16841)) ### API Breaking Changes * (types/math) [#16040](https://github.com/cosmos/cosmos-sdk/pull/16798) Remove aliases in `types/math.go` (part 2). diff --git a/x/bank/keeper/view.go b/x/bank/keeper/view.go index df9f88624a36..9132032f7744 100644 --- a/x/bank/keeper/view.go +++ b/x/bank/keeper/view.go @@ -42,12 +42,8 @@ func newBalancesIndexes(sb *collections.SchemaBuilder) BalancesIndexes { return BalancesIndexes{ Denom: indexes.NewReversePair[math.Int]( sb, types.DenomAddressPrefix, "address_by_denom_index", -<<<<<<< HEAD - collections.PairKeyCodec(sdk.AddressKeyAsIndexKey(sdk.AccAddressKey), collections.StringKey), // nolint:staticcheck // Note: refer to the AddressKeyAsIndexKey docs to understand why we do this. -======= collections.PairKeyCodec(sdk.LengthPrefixedAddressKey(sdk.AccAddressKey), collections.StringKey), // nolint:staticcheck // Note: refer to the LengthPrefixedAddressKey docs to understand why we do this. indexes.WithReversePairUncheckedValue(), // denom to address indexes were stored as Key: Join(denom, address) Value: []byte{0}, this will migrate the value to []byte{} in a lazy way. ->>>>>>> acce343a1 (fix(bank): make `DenomAddressIndex` less strict with respect to index values. (#16841)) ), } } From 4aff5bfa4e95a11f158d7e9ac7399c0079b4e35f Mon Sep 17 00:00:00 2001 From: Julien Robert Date: Thu, 6 Jul 2023 12:36:50 +0200 Subject: [PATCH 3/3] updates --- x/bank/keeper/collections_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/x/bank/keeper/collections_test.go b/x/bank/keeper/collections_test.go index 15c22c489d06..e1af343cce56 100644 --- a/x/bank/keeper/collections_test.go +++ b/x/bank/keeper/collections_test.go @@ -42,7 +42,7 @@ func TestBankStateCompatibility(t *testing.T) { storeService, authKeeper, map[string]bool{accAddrs[4].String(): true}, - authtypes.NewModuleAddress(banktypes.GovModuleName).String(), + authtypes.NewModuleAddress("gov").String(), log.NewNopLogger(), )