fix to correctly parse denoms with slashes in the base denom (#1451)

* fix to correctly parse denoms with slashes in the base denom * some logic refinement * review comments * add changelog entry an other review comments * review comment * Add slash migration guide (#1518) * add migration guide * Update docs/migrations/support-slashed-denoms.md Co-authored-by: Federico Kunze Küllmer <31522760+fedekunze@users.noreply.github.com> * clarify upgrade name * remove unnecessary store loader * review comment, update migration code Co-authored-by: Federico Kunze Küllmer <31522760+fedekunze@users.noreply.github.com> Co-authored-by: Carlos Rodriguez <crodveg@gmail.com> * rename migration file Co-authored-by: Carlos Rodriguez <crodveg@gmail.com> Co-authored-by: Aditya <adityasripal@gmail.com> Co-authored-by: Federico Kunze Küllmer <31522760+fedekunze@users.noreply.github.com>
cosmos · Jun 14, 2022 · 3a235af · 3a235af
1 parent dab07f9
commit 3a235af
Show file tree

Hide file tree

Showing 4 changed files with 162 additions and 12 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -80,7 +80,7 @@ Ref: https://keepachangelog.com/en/1.0.0/
 
 * (modules/core/04-channel) [\#1130](https://github.com/cosmos/ibc-go/pull/1130) Call `packet.GetSequence()` rather than passing func in `WriteAcknowledgement` log output
 * (apps/29-fee) [\#1278](https://github.com/cosmos/ibc-go/pull/1278) The URI path for the query to get all incentivized packets for a specific channel did not follow the same format as the rest of queries.
-* (apps/29-fee)[\#1343](https://github.com/cosmos/ibc-go/pull/1523) Fixed an issue where a bad refund address would prevent channel closure.
+* (apps/transfer) [\#1451](https://github.com/cosmos/ibc-go/pull/1451) Fixing the support for base denoms that contain slashes.
 
 ## [v3.0.0](https://github.com/cosmos/ibc-go/releases/tag/v3.0.0) - 2022-03-15
 

diff --git a/docs/migrations/support-denoms-with-slashes.md b/docs/migrations/support-denoms-with-slashes.md
@@ -0,0 +1,101 @@
+# Migrating from not supporing base denoms with slashes to supporting base denoms with slashes
+
+This document is intended to highlight significant changes which may require more information than presented in the CHANGELOG.
+Any changes that must be done by a user of ibc-go should be documented here.
+
+There are four sections based on the four potential user groups of this document:
+- Chains
+- IBC Apps
+- 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.
+
+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.
+
+E.g. If a base denom of `testcoin/testcoin/testcoin` is sent to a chain that does not support slashes in the base denom, the receive will be successful. However, the trace information stored on the receiving chain will be: `Trace: "transfer/{channel-id}/testcoin/testcoin", BaseDenom: "testcoin"`.
+
+This incorrect trace information must be corrected when the chain does upgrade to fully supporting denominations with slashes.
+
+To do so, chain binaries should include a migration script that will run when the chain upgrades from not supporting base denominations with slashes to supporting base denominations with slashes.
+
+## Chains
+
+### ICS20 - Transfer
+
+The transfer module will now support slashes in base denoms, so we must iterate over current traces to check if any of them are incorrectly formed and correct the trace information.
+
+### Upgrade Proposal
+
+```go
+// Here the upgrade name is the upgrade name set by the chain
+app.UpgradeKeeper.SetUpgradeHandler("supportSlashedDenomsUpgrade",
+    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)
+        }
+
+        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).
+
+### Genesis Migration
+
+If the chain chooses to add support for slashes in base denoms via genesis export, then the trace information must be corrected during genesis migration.
+
+The migration code required may look like:
+
+```go
+func migrateGenesisSlashedDenomsUpgrade(appState genutiltypes.AppMap, clientCtx client.Context, genDoc *tmtypes.GenesisDoc) (genutiltypes.AppMap, error) {
+	if appState[ibctransfertypes.ModuleName] != nil {
+		transferGenState := &ibctransfertypes.GenesisState{}
+		clientCtx.Codec.MustUnmarshalJSON(appState[ibctransfertypes.ModuleName], transferGenState)
+
+		substituteTraces := make([]ibctransfertypes.DenomTrace, len(transferGenState.DenomTraces))
+		for i, dt := range transferGenState.DenomTraces {
+			// replace all previous traces with the latest trace if validation passes
+			// note most traces will have same value
+			newTrace := ibctransfertypes.ParseDenomTrace(dt.GetFullDenomPath())
+
+			if err := newTrace.Validate(); err != nil {
+				substituteTraces[i] = dt
+			} else {
+				substituteTraces[i] = newTrace
+			}
+		}
+
+		transferGenState.DenomTraces = substituteTraces
+
+		// delete old genesis state
+		delete(appState, ibctransfertypes.ModuleName)
+
+		// set new ibc transfer genesis state
+		appState[ibctransfertypes.ModuleName] = clientCtx.Codec.MustMarshalJSON(transferGenState)
+	}
+
+	return appState, nil
+}
+```
+
+For a more detailed sample, please check out the code changes in [this pull request](https://github.com/cosmos/ibc-go/pull/1528).
diff --git a/modules/apps/transfer/types/trace.go b/modules/apps/transfer/types/trace.go
@@ -20,8 +20,11 @@ import (
 //
 // Examples:
 //
-// 	- "portidone/channelidone/uatom" => DenomTrace{Path: "portidone/channelidone", BaseDenom: "uatom"}
-// 	- "uatom" => DenomTrace{Path: "", BaseDenom: "uatom"}
+// - "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"}
+// - "gamm/pool/1" => DenomTrace{Path: "", BaseDenom: "gamm/pool/1"}
+// - "uatom" => DenomTrace{Path: "", BaseDenom: "uatom"}
 func ParseDenomTrace(rawDenom string) DenomTrace {
 	denomSplit := strings.Split(rawDenom, "/")
 
@@ -32,9 +35,10 @@ func ParseDenomTrace(rawDenom string) DenomTrace {
 		}
 	}
 
+	path, baseDenom := extractPathAndBaseFromFullDenom(denomSplit)
 	return DenomTrace{
-		Path:      strings.Join(denomSplit[:len(denomSplit)-1], "/"),
-		BaseDenom: denomSplit[len(denomSplit)-1],
+		Path:      path,
+		BaseDenom: baseDenom,
 	}
 }
 
@@ -70,6 +74,24 @@ func (dt DenomTrace) GetFullDenomPath() string {
 	return dt.GetPrefix() + dt.BaseDenom
 }
 
+// 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
+	length := len(fullDenomItems)
+	for i := 0; i < length; i = i + 2 {
+		if i < length-1 && length > 2 && fullDenomItems[i] == PortID {
+			path = append(path, fullDenomItems[i], fullDenomItems[i+1])
+		} else {
+			baseDenom = fullDenomItems[i:]
+			break
+		}
+	}
+
+	return strings.Join(path, "/"), strings.Join(baseDenom, "/")
+}
+
 func validateTraceIdentifiers(identifiers []string) error {
 	if len(identifiers) == 0 || len(identifiers)%2 != 0 {
 		return fmt.Errorf("trace info must come in pairs of port and channel identifiers '{portID}/{channelID}', got the identifiers: %s", identifiers)
@@ -143,8 +165,10 @@ 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: '{portIDN}/{channelIDN}/.../{portID0}/{channelID0}/baseDenom'
+//  - Prefixed denomination: 'transfer/{channelIDN}/.../transfer/{channelID0}/baseDenom'
 //  - Unprefixed denomination: 'baseDenom'
+//
+// 'baseDenom' may or may not contain '/'s
 func ValidatePrefixedDenom(denom string) error {
 	denomSplit := strings.Split(denom, "/")
 	if denomSplit[0] == denom && strings.TrimSpace(denom) != "" {
@@ -156,7 +180,13 @@ func ValidatePrefixedDenom(denom string) error {
 		return sdkerrors.Wrap(ErrInvalidDenomForTransfer, "base denomination cannot be blank")
 	}
 
-	identifiers := denomSplit[:len(denomSplit)-1]
+	path, _ := extractPathAndBaseFromFullDenom(denomSplit)
+	if path == "" {
+		// NOTE: base denom contains slashes, so no base denomination validation
+		return nil
+	}
+
+	identifiers := strings.Split(path, "/")
 	return validateTraceIdentifiers(identifiers)
 }
 

diff --git a/modules/apps/transfer/types/trace_test.go b/modules/apps/transfer/types/trace_test.go
@@ -14,10 +14,23 @@ func TestParseDenomTrace(t *testing.T) {
 	}{
 		{"empty denom", "", DenomTrace{}},
 		{"base denom", "uatom", DenomTrace{BaseDenom: "uatom"}},
+		{"base denom ending with '/'", "uatom/", DenomTrace{BaseDenom: "uatom/"}},
+		{"base denom with single '/'s", "gamm/pool/1", DenomTrace{BaseDenom: "gamm/pool/1"}},
+		{"base denom with double '/'s", "gamm//pool//1", DenomTrace{BaseDenom: "gamm//pool//1"}},
 		{"trace info", "transfer/channelToA/uatom", DenomTrace{BaseDenom: "uatom", Path: "transfer/channelToA"}},
-		{"incomplete path", "transfer/uatom", DenomTrace{BaseDenom: "uatom", Path: "transfer"}},
+		{"trace info with base denom ending in '/'", "transfer/channelToA/uatom/", DenomTrace{BaseDenom: "uatom/", Path: "transfer/channelToA"}},
+		{"trace info with single '/' in base denom", "transfer/channelToA/erc20/0x85bcBCd7e79Ec36f4fBBDc54F90C643d921151AA", DenomTrace{BaseDenom: "erc20/0x85bcBCd7e79Ec36f4fBBDc54F90C643d921151AA", Path: "transfer/channelToA"}},
+		{"trace info with multiple '/'s in base denom", "transfer/channelToA/gamm/pool/1", DenomTrace{BaseDenom: "gamm/pool/1", Path: "transfer/channelToA"}},
+		{"trace info with multiple double '/'s in base denom", "transfer/channelToA/gamm//pool//1", DenomTrace{BaseDenom: "gamm//pool//1", Path: "transfer/channelToA"}},
+		{"trace info with multiple port/channel pairs", "transfer/channelToA/transfer/channelToB/uatom", DenomTrace{BaseDenom: "uatom", Path: "transfer/channelToA/transfer/channelToB"}},
+		{"incomplete path", "transfer/uatom", DenomTrace{BaseDenom: "transfer/uatom"}},
 		{"invalid path (1)", "transfer//uatom", DenomTrace{BaseDenom: "uatom", Path: "transfer/"}},
-		{"invalid path (2)", "transfer/channelToA/uatom/", DenomTrace{BaseDenom: "", Path: "transfer/channelToA/uatom"}},
+		{"invalid path (2)", "channelToA/transfer/uatom", DenomTrace{BaseDenom: "channelToA/transfer/uatom"}},
+		{"invalid path (3)", "uatom/transfer", DenomTrace{BaseDenom: "uatom/transfer"}},
+		{"invalid path (4)", "transfer/channelToA", DenomTrace{BaseDenom: "transfer/channelToA"}},
+		{"invalid path (5)", "transfer/channelToA/", DenomTrace{Path: "transfer/channelToA"}},
+		{"invalid path (6)", "transfer/channelToA/transfer", DenomTrace{BaseDenom: "transfer", Path: "transfer/channelToA"}},
+		{"invalid path (7)", "transfer/channelToA/transfer/channelToB", DenomTrace{Path: "transfer/channelToA/transfer/channelToB"}},
 	}
 
 	for _, tc := range testCases {
@@ -49,6 +62,8 @@ func TestDenomTrace_Validate(t *testing.T) {
 		expError bool
 	}{
 		{"base denom only", DenomTrace{BaseDenom: "uatom"}, false},
+		{"base denom only with single '/'", DenomTrace{BaseDenom: "erc20/0x85bcBCd7e79Ec36f4fBBDc54F90C643d921151AA"}, false},
+		{"base denom only with multiple '/'s", DenomTrace{BaseDenom: "gamm/pool/1"}, false},
 		{"empty DenomTrace", DenomTrace{}, true},
 		{"valid single trace info", DenomTrace{BaseDenom: "uatom", Path: "transfer/channelToA"}, false},
 		{"valid multiple trace info", DenomTrace{BaseDenom: "uatom", Path: "transfer/channelToA/transfer/channelToB"}, false},
@@ -104,12 +119,15 @@ func TestValidatePrefixedDenom(t *testing.T) {
 		expError bool
 	}{
 		{"prefixed denom", "transfer/channelToA/uatom", false},
+		{"prefixed denom with '/'", "transfer/channelToA/gamm/pool/1", false},
+		{"empty prefix", "/uatom", false},
+		{"empty identifiers", "//uatom", false},
 		{"base denom", "uatom", false},
+		{"base denom with single '/'", "erc20/0x85bcBCd7e79Ec36f4fBBDc54F90C643d921151AA", false},
+		{"base denom with multiple '/'s", "gamm/pool/1", false},
+		{"invalid port ID", "(transfer)/channelToA/uatom", false},
 		{"empty denom", "", true},
-		{"empty prefix", "/uatom", true},
-		{"empty identifiers", "//uatom", true},
 		{"single trace identifier", "transfer/", true},
-		{"invalid port ID", "(transfer)/channelToA/uatom", true},
 		{"invalid channel ID", "transfer/(channelToA)/uatom", true},
 	}
 
@@ -131,6 +149,7 @@ func TestValidateIBCDenom(t *testing.T) {
 	}{
 		{"denom with trace hash", "ibc/7F1D3FCF4AE79E1554D670D1AD949A9BA4E4A3C76C63093E17E446A46061A7A2", false},
 		{"base denom", "uatom", false},
+		{"base denom ending with '/'", "uatom/", false},
 		{"base denom with single '/'s", "gamm/pool/1", false},
 		{"base denom with double '/'s", "gamm//pool//1", false},
 		{"non-ibc prefix with hash", "notibc/7F1D3FCF4AE79E1554D670D1AD949A9BA4E4A3C76C63093E17E446A46061A7A2", false},