fix(bank): make DenomAddressIndex less strict with respect to index…

… values. (#16841)

Co-authored-by: unknown unknown <unknown@unknown>
(cherry picked from commit acce343)

# Conflicts:
#	CHANGELOG.md
#	x/bank/keeper/view.go

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).

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)
+}

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)),
 	}
 

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
-}
+})

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)
 	})