fix: x/bank/044 migrateDenomMetadata (#10239)

* fix: x/bank/044 migrateDenomMetadata

* adding changelog entry

* comment update

* fix tests

CHANGELOG.md

@@ -64,7 +64,7 @@ Ref: https://keepachangelog.com/en/1.0.0/
     * `SaveOfflineKey`
   * Take `keyring.Record` instead of `Info` as first argument in:
     * `MkConsKeyOutput`
-    * `MkValKeyOutput` 
+    * `MkValKeyOutput`
     * `MkAccKeyOutput`
 * [\#10077](https://github.com/cosmos/cosmos-sdk/pull/10077) Remove telemetry on `GasKV` and `CacheKV` store Get/Set operations, significantly improving their performance.
 * [\#10022](https://github.com/cosmos/cosmos-sdk/pull/10022) `AuthKeeper` interface in `x/auth` now includes a function `HasAccount`.
@@ -139,6 +139,7 @@ Ref: https://keepachangelog.com/en/1.0.0/
 * (server) [#10016](https://github.com/cosmos/cosmos-sdk/issues/10016) Fix marshaling of index-events into server config file.
 * (x/feegrant) [\#10049](https://github.com/cosmos/cosmos-sdk/issues/10049) Fixed the error message when `period` or `period-limit` flag is not set on a feegrant grant transaction.
 * [\#10184](https://github.com/cosmos/cosmos-sdk/pull/10184) Fixed CLI tx commands to no longer explicitly require the chain-id flag as this value can come from a user config.
+* [\#10239](https://github.com/cosmos/cosmos-sdk/pull/10239) Fixed x/bank/044 migrateDenomMetadata.
 * (x/upgrade) [\#10189](https://github.com/cosmos/cosmos-sdk/issues/10189) Removed potential sources of non-determinism in upgrades
 
 ### State Machine Breaking

x/bank/migrations/v044/keys.go

@@ -4,7 +4,13 @@ var (
 	DenomAddressPrefix = []byte{0x03}
 )
 
-func CreateAddressDenomPrefix(denom string) []byte {
-	key := append(DenomAddressPrefix, []byte(denom)...)
-	return append(key, 0)
+// CreateDenomAddressPrefix creates a prefix for a reverse index of denomination
+// to account balance for that denomination.
+func CreateDenomAddressPrefix(denom string) []byte {
+	// we add a "zero" byte at the end - null byte terminator, to allow prefix denom prefix
+	// scan. Setting it is not needed (key[last] = 0) - because this is the default.
+	key := make([]byte, len(DenomAddressPrefix)+len(denom)+1)
+	copy(key, DenomAddressPrefix)
+	copy(key[len(DenomAddressPrefix):], denom)
+	return key
 }

x/bank/migrations/v044/store.go

@@ -59,7 +59,7 @@ func addDenomReverseIndex(store sdk.KVStore, cdc codec.BinaryCodec) error {
 
 		denomPrefixStore, ok := denomPrefixStores[balance.Denom]
 		if !ok {
-			denomPrefixStore = prefix.NewStore(store, CreateAddressDenomPrefix(balance.Denom))
+			denomPrefixStore = prefix.NewStore(store, CreateDenomAddressPrefix(balance.Denom))
 			denomPrefixStores[balance.Denom] = denomPrefixStore
 		}
 
@@ -79,11 +79,14 @@ func migrateDenomMetadata(store sdk.KVStore) error {
 
 	for ; oldDenomMetaDataIter.Valid(); oldDenomMetaDataIter.Next() {
 		oldKey := oldDenomMetaDataIter.Key()
-		// old key: prefix_bytes | denom_bytes | denom_bytes
-		newKey := append(types.DenomMetadataPrefix, oldKey[:len(oldKey)/2+1]...)
+		l := len(oldKey)/2 + 1
 
+		var newKey = make([]byte, len(types.DenomMetadataPrefix)+l)
+		// old key: prefix_bytes | denom_bytes | denom_bytes
+		copy(newKey, types.DenomMetadataPrefix)
+		copy(newKey[len(types.DenomMetadataPrefix):], oldKey[:l])
 		store.Set(newKey, oldDenomMetaDataIter.Value())
-		oldDenomMetaDataStore.Delete(oldDenomMetaDataIter.Key())
+		oldDenomMetaDataStore.Delete(oldKey)
 	}
 
 	return nil

x/bank/migrations/v044/store_test.go

@@ -47,7 +47,7 @@ func TestMigrateStore(t *testing.T) {
 	}
 
 	for _, b := range balances {
-		denomPrefixStore := prefix.NewStore(store, v044.CreateAddressDenomPrefix(b.Denom))
+		denomPrefixStore := prefix.NewStore(store, v044.CreateDenomAddressPrefix(b.Denom))
 		bz := denomPrefixStore.Get(address.MustLengthPrefix(addr))
 		require.NotNil(t, bz)
 	}
@@ -88,6 +88,7 @@ func TestMigrateDenomMetaData(t *testing.T) {
 
 	for i := range []int{0, 1} {
 		key := append(v043.DenomMetadataPrefix, []byte(metaData[i].Base)...)
+		// keys before 0.44 had denom two times in the key
 		key = append(key, []byte(metaData[i].Base)...)
 		bz, err := encCfg.Codec.Marshal(&metaData[i])
 		require.NoError(t, err)
@@ -108,7 +109,7 @@ func TestMigrateDenomMetaData(t *testing.T) {
 		bz := denomMetadataStore.Get(oldKey)
 		require.Nil(t, bz)
 
-		require.Equal(t, string(newKey)[1:], metaData[i].Base)
+		require.Equal(t, string(newKey)[1:], metaData[i].Base, "idx: %d", i)
 		bz = denomMetadataStore.Get(denomMetadataIter.Key())
 		require.NotNil(t, bz)
 		err := encCfg.Codec.Unmarshal(bz, &result)

x/bank/types/key.go

@@ -60,6 +60,10 @@ func CreateAccountBalancesPrefix(addr []byte) []byte {
 // CreateDenomAddressPrefix creates a prefix for a reverse index of denomination
 // to account balance for that denomination.
 func CreateDenomAddressPrefix(denom string) []byte {
-	key := append(DenomAddressPrefix, []byte(denom)...)
-	return append(key, 0)
+	// we add a "zero" byte at the end - null byte terminator, to allow prefix denom prefix
+	// scan. Setting it is not needed (key[last] = 0) - because this is the default.
+	key := make([]byte, len(DenomAddressPrefix)+len(denom)+1)
+	copy(key, DenomAddressPrefix)
+	copy(key[len(DenomAddressPrefix):], denom)
+	return key
 }

x/bank/types/key_test.go

@@ -56,3 +56,15 @@ func TestAddressFromBalancesStore(t *testing.T) {
 		})
 	}
 }
+
+func TestCreateDenomAddressPrefix(t *testing.T) {
+	require := require.New(t)
+
+	key := types.CreateDenomAddressPrefix("")
+	require.Len(key, len(types.DenomAddressPrefix)+1)
+	require.Equal(append(types.DenomAddressPrefix, 0), key)
+
+	key = types.CreateDenomAddressPrefix("abc")
+	require.Len(key, len(types.DenomAddressPrefix)+4)
+	require.Equal(append(types.DenomAddressPrefix, 'a', 'b', 'c', 0), key)
+}