chore: denom traces migration handler (backport #1680) (#1754)

* chore: denom traces migration handler (#1680)

* update code & test

* register migrator service

(cherry picked from commit be5ccf3)

# Conflicts:
#	CHANGELOG.md
#	docs/migrations/support-denoms-with-slashes.md
#	docs/migrations/v3-to-v4.md
#	modules/apps/transfer/types/trace.go

* fix conflicts

* fix go version package

* put back entry that got removed by mistake

Co-authored-by: Charly <charly@interchain.berlin>
Co-authored-by: crodriguezvega <carlos@interchain.io>

Author: 3 people

CHANGELOG.md

@@ -47,8 +47,10 @@ Ref: https://keepachangelog.com/en/1.0.0/
 
 ### Improvements
 
-* (core/02-client) [\#1570](https://github.com/cosmos/ibc-go/pull/1570) Emitting an event when handling an upgrade client proposal. 
-
+* (core/02-client) [\#1570](https://github.com/cosmos/ibc-go/pull/1570) Emitting an event when handling an upgrade client proposal.
+* (app/20-transfer) [\#1680](https://github.com/cosmos/ibc-go/pull/1680) Adds migration to correct any malformed trace path information of tokens with denoms that contains slashes. The transfer module consensus version has been bumped to 2.
+* (app/20-transfer) [\#1730](https://github.com/cosmos/ibc-go/pull/1730) parse the ics20 denomination provided via a packet using the channel identifier format specified by ibc-go.
+ 
 ### Features
 
 ### Bug Fixes

docs/migrations/support-denoms-with-slashes.md

@@ -9,7 +9,7 @@ There are four sections based on the four potential user groups of this document
 - Relayers
 - IBC Light Clients
 
-This document is necessary when chains are upgrading from a version that does not support base denoms with slashes (e.g. v3.0.0) to a version that does (e.g. v3.1.0). All versions of ibc-go smaller than v1.5.0 for the v1.x release line, v2.3.0 for the v2.x release line, and v3.1.0 for the v3.x release line do *NOT** support IBC token transfers of coins whose base denoms contain slashes. Therefore the in-place of genesis migration described in this document are required when upgrading.
+This document is necessary when chains are upgrading from a version that does not support base denoms with slashes (e.g. v3.0.0) to a version that does (e.g. v3.2.0). All versions of ibc-go smaller than v1.5.0 for the v1.x release line, v2.3.0 for the v2.x release line, and v3.1.0 for the v3.x release line do **NOT** support IBC token transfers of coins whose base denoms contain slashes. Therefore the in-place of genesis migration described in this document are required when upgrading.
 
 If a chain receives coins of a base denom with slashes before it upgrades to supporting it, the receive may pass however the trace information will be incorrect.
 
@@ -28,37 +28,16 @@ The transfer module will now support slashes in base denoms, so we must iterate
 ### Upgrade Proposal
 
 ```go
-// Here the upgrade name is the upgrade name set by the chain
-app.UpgradeKeeper.SetUpgradeHandler("supportSlashedDenomsUpgrade",
+app.UpgradeKeeper.SetUpgradeHandler("MigrateTraces",
     func(ctx sdk.Context, _ upgradetypes.Plan, fromVM module.VersionMap) (module.VersionMap, error) {
-        // list of traces that must replace the old traces in store
-        var newTraces []ibctransfertypes.DenomTrace
-        app.TransferKeeper.IterateDenomTraces(ctx,
-            func(dt ibctransfertypes.DenomTrace) bool {
-                // check if the new way of splitting FullDenom
-                // into Trace and BaseDenom passes validation and
-                // is the same as the current DenomTrace.
-                // If it isn't then store the new DenomTrace in the list of new traces.
-                newTrace := ibctransfertypes.ParseDenomTrace(dt.GetFullDenomPath())
-                if err := newTrace.Validate(); err == nil && !reflect.DeepEqual(newTrace, dt) {
-                    newTraces = append(newTraces, newTrace)
-                }
-
-                return false
-            })
-
-        // replace the outdated traces with the new trace information
-        for _, nt := range newTraces {
-            app.TransferKeeper.SetDenomTrace(ctx, nt)
-        }
-
+        // transfer module consensus version has been bumped to 2
         return app.mm.RunMigrations(ctx, app.configurator, fromVM)
     })
 ```
 
 This is only necessary if there are denom traces in the store with incorrect trace information from previously received coins that had a slash in the base denom. However, it is recommended that any chain upgrading to support base denominations with slashes runs this code for safety.
 
-For a more detailed sample, please check out the code changes in [this pull request](https://github.com/cosmos/ibc-go/pull/1527).
+For a more detailed sample, please check out the code changes in [this pull request](https://github.com/cosmos/ibc-go/pull/1680).
 
 ### Genesis Migration
 

modules/apps/transfer/keeper/migrations.go

@@ -0,0 +1,59 @@
+package keeper
+
+import (
+	"fmt"
+
+	sdk "github.com/cosmos/cosmos-sdk/types"
+
+	"github.com/cosmos/ibc-go/v3/modules/apps/transfer/types"
+)
+
+// Migrator is a struct for handling in-place store migrations.
+type Migrator struct {
+	keeper Keeper
+}
+
+// NewMigrator returns a new Migrator.
+func NewMigrator(keeper Keeper) Migrator {
+	return Migrator{keeper: keeper}
+}
+
+// MigrateTraces migrates the DenomTraces to the correct format, accounting for slashes in the BaseDenom.
+func (m Migrator) MigrateTraces(ctx sdk.Context) error {
+
+	// list of traces that must replace the old traces in store
+	var newTraces []types.DenomTrace
+	m.keeper.IterateDenomTraces(ctx,
+		func(dt types.DenomTrace) (stop bool) {
+			// check if the new way of splitting FullDenom
+			// is the same as the current DenomTrace.
+			// If it isn't then store the new DenomTrace in the list of new traces.
+			newTrace := types.ParseDenomTrace(dt.GetFullDenomPath())
+			err := newTrace.Validate()
+			if err != nil {
+				panic(err)
+			}
+
+			if dt.IBCDenom() != newTrace.IBCDenom() {
+				// The new form of parsing will result in a token denomination change.
+				// A bank migration is required. A panic should occur to prevent the
+				// chain from using corrupted state.
+				panic(fmt.Sprintf("migration will result in corrupted state. Previous IBC token (%s) requires a bank migration. Expected denom trace (%s)", dt, newTrace))
+			}
+
+			if !equalTraces(newTrace, dt) {
+				newTraces = append(newTraces, newTrace)
+			}
+			return false
+		})
+
+	// replace the outdated traces with the new trace information
+	for _, nt := range newTraces {
+		m.keeper.SetDenomTrace(ctx, nt)
+	}
+	return nil
+}
+
+func equalTraces(dtA, dtB types.DenomTrace) bool {
+	return dtA.BaseDenom == dtB.BaseDenom && dtA.Path == dtB.Path
+}

modules/apps/transfer/keeper/migrations_test.go

@@ -0,0 +1,123 @@
+package keeper_test
+
+import (
+	"fmt"
+
+	transferkeeper "github.com/cosmos/ibc-go/v3/modules/apps/transfer/keeper"
+	transfertypes "github.com/cosmos/ibc-go/v3/modules/apps/transfer/types"
+)
+
+func (suite *KeeperTestSuite) TestMigratorMigrateTraces() {
+
+	testCases := []struct {
+		msg            string
+		malleate       func()
+		expectedTraces transfertypes.Traces
+	}{
+
+		{
+			"success: two slashes in base denom",
+			func() {
+				suite.chainA.GetSimApp().TransferKeeper.SetDenomTrace(
+					suite.chainA.GetContext(),
+					transfertypes.DenomTrace{
+						BaseDenom: "pool/1", Path: "transfer/channel-0/gamm",
+					})
+			},
+			transfertypes.Traces{
+				{
+					BaseDenom: "gamm/pool/1", Path: "transfer/channel-0",
+				},
+			},
+		},
+		{
+			"success: one slash in base denom",
+			func() {
+				suite.chainA.GetSimApp().TransferKeeper.SetDenomTrace(
+					suite.chainA.GetContext(),
+					transfertypes.DenomTrace{
+						BaseDenom: "0x85bcBCd7e79Ec36f4fBBDc54F90C643d921151AA", Path: "transfer/channel-149/erc",
+					})
+			},
+			transfertypes.Traces{
+				{
+					BaseDenom: "erc/0x85bcBCd7e79Ec36f4fBBDc54F90C643d921151AA", Path: "transfer/channel-149",
+				},
+			},
+		},
+		{
+			"success: multiple slashes in a row in base denom",
+			func() {
+				suite.chainA.GetSimApp().TransferKeeper.SetDenomTrace(
+					suite.chainA.GetContext(),
+					transfertypes.DenomTrace{
+						BaseDenom: "1", Path: "transfer/channel-5/gamm//pool",
+					})
+			},
+			transfertypes.Traces{
+				{
+					BaseDenom: "gamm//pool/1", Path: "transfer/channel-5",
+				},
+			},
+		},
+		{
+			"success: multihop base denom",
+			func() {
+				suite.chainA.GetSimApp().TransferKeeper.SetDenomTrace(
+					suite.chainA.GetContext(),
+					transfertypes.DenomTrace{
+						BaseDenom: "transfer/channel-1/uatom", Path: "transfer/channel-0",
+					})
+			},
+			transfertypes.Traces{
+				{
+					BaseDenom: "uatom", Path: "transfer/channel-0/transfer/channel-1",
+				},
+			},
+		},
+		{
+			"success: non-standard port",
+			func() {
+				suite.chainA.GetSimApp().TransferKeeper.SetDenomTrace(
+					suite.chainA.GetContext(),
+					transfertypes.DenomTrace{
+						BaseDenom: "customport/channel-7/uatom", Path: "transfer/channel-0/transfer/channel-1",
+					})
+			},
+			transfertypes.Traces{
+				{
+					BaseDenom: "uatom", Path: "transfer/channel-0/transfer/channel-1/customport/channel-7",
+				},
+			},
+		},
+	}
+
+	for _, tc := range testCases {
+		suite.Run(fmt.Sprintf("case %s", tc.msg), func() {
+			suite.SetupTest() // reset
+
+			tc.malleate() // explicitly set up denom traces
+
+			migrator := transferkeeper.NewMigrator(suite.chainA.GetSimApp().TransferKeeper)
+			err := migrator.MigrateTraces(suite.chainA.GetContext())
+			suite.Require().NoError(err)
+
+			traces := suite.chainA.GetSimApp().TransferKeeper.GetAllDenomTraces(suite.chainA.GetContext())
+			suite.Require().Equal(tc.expectedTraces, traces)
+		})
+	}
+}
+
+func (suite *KeeperTestSuite) TestMigratorMigrateTracesCorruptionDetection() {
+	// IBCDenom() previously would return "customport/channel-0/uatom", but now should return ibc/{hash}
+	corruptedDenomTrace := transfertypes.DenomTrace{
+		BaseDenom: "customport/channel-0/uatom",
+		Path:      "",
+	}
+	suite.chainA.GetSimApp().TransferKeeper.SetDenomTrace(suite.chainA.GetContext(), corruptedDenomTrace)
+
+	migrator := transferkeeper.NewMigrator(suite.chainA.GetSimApp().TransferKeeper)
+	suite.Panics(func() {
+		migrator.MigrateTraces(suite.chainA.GetContext())
+	})
+}

modules/apps/transfer/module.go

@@ -120,6 +120,11 @@ func (am AppModule) LegacyQuerierHandler(*codec.LegacyAmino) sdk.Querier {
 func (am AppModule) RegisterServices(cfg module.Configurator) {
 	types.RegisterMsgServer(cfg.MsgServer(), am.keeper)
 	types.RegisterQueryServer(cfg.QueryServer(), am.keeper)
+
+	m := keeper.NewMigrator(am.keeper)
+	if err := cfg.RegisterMigration(types.ModuleName, 1, m.MigrateTraces); err != nil {
+		panic(fmt.Sprintf("failed to migrate transfer app from version 1 to 2: %v", err))
+	}
 }
 
 // InitGenesis performs genesis initialization for the ibc-transfer module. It returns
@@ -139,7 +144,7 @@ func (am AppModule) ExportGenesis(ctx sdk.Context, cdc codec.JSONCodec) json.Raw
 }
 
 // ConsensusVersion implements AppModule/ConsensusVersion.
-func (AppModule) ConsensusVersion() uint64 { return 1 }
+func (AppModule) ConsensusVersion() uint64 { return 2 }
 
 // BeginBlock implements the AppModule interface
 func (am AppModule) BeginBlock(ctx sdk.Context, req abci.RequestBeginBlock) {

modules/apps/transfer/types/trace.go

@@ -12,6 +12,7 @@ import (
 	tmbytes "github.com/tendermint/tendermint/libs/bytes"
 	tmtypes "github.com/tendermint/tendermint/types"
 
+	channeltypes "github.com/cosmos/ibc-go/v3/modules/core/04-channel/types"
 	host "github.com/cosmos/ibc-go/v3/modules/core/24-host"
 )
 
@@ -20,9 +21,9 @@ import (
 //
 // Examples:
 //
-// - "transfer/channelidone/uatom" => DenomTrace{Path: "transfer/channelidone", BaseDenom: "uatom"}
-// - "transfer/channelidone/transfer/channelidtwo/uatom" => DenomTrace{Path: "transfer/channelidone/transfer/channelidtwo", BaseDenom: "uatom"}
-// - "transfer/channelidone/gamm/pool/1" => DenomTrace{Path: "transfer/channelidone", BaseDenom: "gamm/pool/1"}
+// - "portidone/channel-0/uatom" => DenomTrace{Path: "portidone/channel-0", BaseDenom: "uatom"}
+// - "portidone/channel-0/portidtwo/channel-1/uatom" => DenomTrace{Path: "portidone/channel-0/portidtwo/channel-1", BaseDenom: "uatom"}
+// - "portidone/channel-0/gamm/pool/1" => DenomTrace{Path: "portidone/channel-0", BaseDenom: "gamm/pool/1"}
 // - "gamm/pool/1" => DenomTrace{Path: "", BaseDenom: "gamm/pool/1"}
 // - "uatom" => DenomTrace{Path: "", BaseDenom: "uatom"}
 func ParseDenomTrace(rawDenom string) DenomTrace {
@@ -77,11 +78,23 @@ func (dt DenomTrace) GetFullDenomPath() string {
 // extractPathAndBaseFromFullDenom returns the trace path and the base denom from
 // the elements that constitute the complete denom.
 func extractPathAndBaseFromFullDenom(fullDenomItems []string) (string, string) {
-	var path []string
-	var baseDenom []string
+	var (
+		path      []string
+		baseDenom []string
+	)
+
 	length := len(fullDenomItems)
 	for i := 0; i < length; i = i + 2 {
-		if i < length-1 && length > 2 && fullDenomItems[i] == PortID {
+		// The IBC specification does not guarentee the expected format of the
+		// destination port or destination channel identifier. A short term solution
+		// to determine base denomination is to expect the channel identifier to be the
+		// one ibc-go specifies. A longer term solution is to separate the path and base
+		// denomination in the ICS20 packet. If an intermediate hop prefixes the full denom
+		// with a channel identifier format different from our own, the base denomination
+		// will be incorrectly parsed, but the token will continue to be treated correctly
+		// as an IBC denomination. The hash used to store the token internally on our chain
+		// will be the same value as the base denomination being correctly parsed.
+		if i < length-1 && length > 2 && channeltypes.IsValidChannelID(fullDenomItems[i+1]) {
 			path = append(path, fullDenomItems[i], fullDenomItems[i+1])
 		} else {
 			baseDenom = fullDenomItems[i:]
@@ -165,7 +178,7 @@ func (t Traces) Sort() Traces {
 // ValidatePrefixedDenom checks that the denomination for an IBC fungible token packet denom is correctly prefixed.
 // The function will return no error if the given string follows one of the two formats:
 //
-//  - Prefixed denomination: 'transfer/{channelIDN}/.../transfer/{channelID0}/baseDenom'
+//  - Prefixed denomination: '{portIDN}/{channelIDN}/.../{portID0}/{channelID0}/baseDenom'
 //  - Unprefixed denomination: 'baseDenom'
 //
 // 'baseDenom' may or may not contain '/'s