From 657921102ddd16d2beb881ffbab366f658949354 Mon Sep 17 00:00:00 2001 From: crodriguezvega Date: Fri, 27 May 2022 22:30:24 +0200 Subject: [PATCH 1/7] fix to correctly parse denoms with slashes in the base denom --- modules/apps/transfer/types/trace.go | 24 +++++++++++++++++++++-- modules/apps/transfer/types/trace_test.go | 18 +++++++++++++---- 2 files changed, 36 insertions(+), 6 deletions(-) diff --git a/modules/apps/transfer/types/trace.go b/modules/apps/transfer/types/trace.go index 303ddd3769e..bdb733d01b2 100644 --- a/modules/apps/transfer/types/trace.go +++ b/modules/apps/transfer/types/trace.go @@ -4,6 +4,7 @@ import ( "crypto/sha256" "encoding/hex" "fmt" + "regexp" "sort" "strings" @@ -12,6 +13,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" ) @@ -21,6 +23,9 @@ import ( // Examples: // // - "portidone/channelidone/uatom" => DenomTrace{Path: "portidone/channelidone", BaseDenom: "uatom"} +// - "portidone/channelidone/portidtwo/channelidtwo/uatom" => DenomTrace{Path: "portidone/channelidone/portidtwo/channelidtwo", BaseDenom: "uatom"} +// - "portidone/channelidone/gamm/pool/1" => DenomTrace{Path: "portidone/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 +37,24 @@ func ParseDenomTrace(rawDenom string) DenomTrace { } } + j := 1 + path := []string{} + baseDenom := []string{} + length := len(denomSplit) + r, _ := regexp.Compile(fmt.Sprintf("%s[0-9]+", channeltypes.ChannelPrefix)) + for i := 0; i < length; i = i + j { + if denomSplit[i] == PortID && r.MatchString(denomSplit[i+1]) { + path = append(path, denomSplit[i], denomSplit[i+1]) + j = 2 + } else { + baseDenom = append(baseDenom, denomSplit[i]) + j = 1 + } + } + return DenomTrace{ - Path: strings.Join(denomSplit[:len(denomSplit)-1], "/"), - BaseDenom: denomSplit[len(denomSplit)-1], + Path: strings.Join(path, "/"), + BaseDenom: strings.Join(baseDenom, "/"), } } diff --git a/modules/apps/transfer/types/trace_test.go b/modules/apps/transfer/types/trace_test.go index e35fd33317b..a99c97b0520 100644 --- a/modules/apps/transfer/types/trace_test.go +++ b/modules/apps/transfer/types/trace_test.go @@ -14,10 +14,19 @@ func TestParseDenomTrace(t *testing.T) { }{ {"empty denom", "", DenomTrace{}}, {"base denom", "uatom", DenomTrace{BaseDenom: "uatom"}}, - {"trace info", "transfer/channelToA/uatom", DenomTrace{BaseDenom: "uatom", Path: "transfer/channelToA"}}, - {"incomplete path", "transfer/uatom", DenomTrace{BaseDenom: "uatom", Path: "transfer"}}, - {"invalid path (1)", "transfer//uatom", DenomTrace{BaseDenom: "uatom", Path: "transfer/"}}, - {"invalid path (2)", "transfer/channelToA/uatom/", DenomTrace{BaseDenom: "", Path: "transfer/channelToA/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/channel-0/uatom", DenomTrace{BaseDenom: "uatom", Path: "transfer/channel-0"}}, + {"trace info with base denom ending in '/'", "transfer/channel-0/uatom/", DenomTrace{BaseDenom: "uatom/", Path: "transfer/channel-0"}}, + {"trace info with single '/' in base denom", "transfer/channel-0/erc20/0x85bcBCd7e79Ec36f4fBBDc54F90C643d921151AA", DenomTrace{BaseDenom: "erc20/0x85bcBCd7e79Ec36f4fBBDc54F90C643d921151AA", Path: "transfer/channel-0"}}, + {"trace info with multiple '/'s in base denom", "transfer/channel-0/gamm/pool/1", DenomTrace{BaseDenom: "gamm/pool/1", Path: "transfer/channel-0"}}, + {"trace info with multiple double '/'s in base denom", "transfer/channel-0/gamm//pool//1", DenomTrace{BaseDenom: "gamm//pool//1", Path: "transfer/channel-0"}}, + {"trace info with multiple port/channel pairs", "transfer/channel-0/transfer/channel-1/uatom", DenomTrace{BaseDenom: "uatom", Path: "transfer/channel-0/transfer/channel-1"}}, + {"incomplete path", "transfer/uatom", DenomTrace{BaseDenom: "transfer/uatom"}}, + {"invalid path (1)", "transfer//uatom", DenomTrace{BaseDenom: "transfer//uatom"}}, + {"invalid path (2)", "transfer/channelToA/uatom", DenomTrace{BaseDenom: "transfer/channelToA/uatom"}}, + {"invalid path (3)", "channel-0/transfer/uatom", DenomTrace{BaseDenom: "channel-0/transfer/uatom"}}, } for _, tc := range testCases { @@ -131,6 +140,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}, From 10c25ccd6eb2682e98905e8a5e33d4bd4a34236c Mon Sep 17 00:00:00 2001 From: crodriguezvega Date: Sat, 28 May 2022 10:20:43 +0200 Subject: [PATCH 2/7] some logic refinement --- modules/apps/transfer/types/trace.go | 15 ++++++++------- modules/apps/transfer/types/trace_test.go | 7 +++++++ 2 files changed, 15 insertions(+), 7 deletions(-) diff --git a/modules/apps/transfer/types/trace.go b/modules/apps/transfer/types/trace.go index bdb733d01b2..56e986641d5 100644 --- a/modules/apps/transfer/types/trace.go +++ b/modules/apps/transfer/types/trace.go @@ -37,18 +37,19 @@ func ParseDenomTrace(rawDenom string) DenomTrace { } } - j := 1 path := []string{} baseDenom := []string{} length := len(denomSplit) - r, _ := regexp.Compile(fmt.Sprintf("%s[0-9]+", channeltypes.ChannelPrefix)) - for i := 0; i < length; i = i + j { - if denomSplit[i] == PortID && r.MatchString(denomSplit[i+1]) { + r, err := regexp.Compile(fmt.Sprintf("%s[0-9]+", channeltypes.ChannelPrefix)) + if err != nil { + panic("could not compile regexp 'channel-[0-9]+'") + } + for i := 0; i < length; i = i + 2 { + if i < length-1 && denomSplit[i] == PortID && r.MatchString(denomSplit[i+1]) { path = append(path, denomSplit[i], denomSplit[i+1]) - j = 2 } else { - baseDenom = append(baseDenom, denomSplit[i]) - j = 1 + baseDenom = denomSplit[i:] + break } } diff --git a/modules/apps/transfer/types/trace_test.go b/modules/apps/transfer/types/trace_test.go index a99c97b0520..4aca4b9ee3b 100644 --- a/modules/apps/transfer/types/trace_test.go +++ b/modules/apps/transfer/types/trace_test.go @@ -27,6 +27,13 @@ func TestParseDenomTrace(t *testing.T) { {"invalid path (1)", "transfer//uatom", DenomTrace{BaseDenom: "transfer//uatom"}}, {"invalid path (2)", "transfer/channelToA/uatom", DenomTrace{BaseDenom: "transfer/channelToA/uatom"}}, {"invalid path (3)", "channel-0/transfer/uatom", DenomTrace{BaseDenom: "channel-0/transfer/uatom"}}, + {"invalid path (4)", "uatom/transfer", DenomTrace{BaseDenom: "uatom/transfer"}}, + {"invalid path (5)", "transfer/channel-0", DenomTrace{Path: "transfer/channel-0"}}, + {"invalid path (6)", "transfer/channel-0/", DenomTrace{Path: "transfer/channel-0"}}, + {"invalid path (7)", "transfer/channelToA", DenomTrace{BaseDenom: "transfer/channelToA"}}, + {"invalid path (8)", "transfer/channel-0/transfer", DenomTrace{BaseDenom: "transfer", Path: "transfer/channel-0"}}, + {"invalid path (9)", "transfer/channel-0/transfer/channel-1", DenomTrace{Path: "transfer/channel-0/transfer/channel-1"}}, + {"invalid path (10)", "transfer/channel-0/transfer/channelToB", DenomTrace{BaseDenom: "transfer/channelToB", Path: "transfer/channel-0"}}, } for _, tc := range testCases { From b713dd897c449f99cc018807c05a3bf969ad1ee6 Mon Sep 17 00:00:00 2001 From: crodriguezvega Date: Thu, 2 Jun 2022 14:18:45 +0200 Subject: [PATCH 3/7] review comments --- modules/apps/transfer/types/trace.go | 57 +++++++++++++---------- modules/apps/transfer/types/trace_test.go | 40 ++++++++-------- 2 files changed, 53 insertions(+), 44 deletions(-) diff --git a/modules/apps/transfer/types/trace.go b/modules/apps/transfer/types/trace.go index 56e986641d5..b8af88ae616 100644 --- a/modules/apps/transfer/types/trace.go +++ b/modules/apps/transfer/types/trace.go @@ -4,7 +4,6 @@ import ( "crypto/sha256" "encoding/hex" "fmt" - "regexp" "sort" "strings" @@ -13,7 +12,6 @@ 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" ) @@ -22,9 +20,9 @@ import ( // // Examples: // -// - "portidone/channelidone/uatom" => DenomTrace{Path: "portidone/channelidone", BaseDenom: "uatom"} -// - "portidone/channelidone/portidtwo/channelidtwo/uatom" => DenomTrace{Path: "portidone/channelidone/portidtwo/channelidtwo", BaseDenom: "uatom"} -// - "portidone/channelidone/gamm/pool/1" => DenomTrace{Path: "portidone/channelidone", BaseDenom: "gamm/pool/1"} +// - "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 { @@ -37,25 +35,10 @@ func ParseDenomTrace(rawDenom string) DenomTrace { } } - path := []string{} - baseDenom := []string{} - length := len(denomSplit) - r, err := regexp.Compile(fmt.Sprintf("%s[0-9]+", channeltypes.ChannelPrefix)) - if err != nil { - panic("could not compile regexp 'channel-[0-9]+'") - } - for i := 0; i < length; i = i + 2 { - if i < length-1 && denomSplit[i] == PortID && r.MatchString(denomSplit[i+1]) { - path = append(path, denomSplit[i], denomSplit[i+1]) - } else { - baseDenom = denomSplit[i:] - break - } - } - + path, baseDenom := extractPathAndBaseDenomFromDenomTrace(denomSplit) return DenomTrace{ - Path: strings.Join(path, "/"), - BaseDenom: strings.Join(baseDenom, "/"), + Path: path, + BaseDenom: baseDenom, } } @@ -91,6 +74,22 @@ func (dt DenomTrace) GetFullDenomPath() string { return dt.GetPrefix() + dt.BaseDenom } +func extractPathAndBaseDenomFromDenomTrace(denomTraceItems []string) (string, string) { + path := []string{} + baseDenom := []string{} + length := len(denomTraceItems) + for i := 0; i < length; i = i + 2 { + if i < length-1 && length > 2 && denomTraceItems[i] == PortID { + path = append(path, denomTraceItems[i], denomTraceItems[i+1]) + } else { + baseDenom = denomTraceItems[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) @@ -164,8 +163,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) != "" { @@ -177,7 +178,13 @@ func ValidatePrefixedDenom(denom string) error { return sdkerrors.Wrap(ErrInvalidDenomForTransfer, "base denomination cannot be blank") } - identifiers := denomSplit[:len(denomSplit)-1] + path, _ := extractPathAndBaseDenomFromDenomTrace(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 index 4aca4b9ee3b..6526cbd952d 100644 --- a/modules/apps/transfer/types/trace_test.go +++ b/modules/apps/transfer/types/trace_test.go @@ -17,23 +17,20 @@ func TestParseDenomTrace(t *testing.T) { {"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/channel-0/uatom", DenomTrace{BaseDenom: "uatom", Path: "transfer/channel-0"}}, - {"trace info with base denom ending in '/'", "transfer/channel-0/uatom/", DenomTrace{BaseDenom: "uatom/", Path: "transfer/channel-0"}}, - {"trace info with single '/' in base denom", "transfer/channel-0/erc20/0x85bcBCd7e79Ec36f4fBBDc54F90C643d921151AA", DenomTrace{BaseDenom: "erc20/0x85bcBCd7e79Ec36f4fBBDc54F90C643d921151AA", Path: "transfer/channel-0"}}, - {"trace info with multiple '/'s in base denom", "transfer/channel-0/gamm/pool/1", DenomTrace{BaseDenom: "gamm/pool/1", Path: "transfer/channel-0"}}, - {"trace info with multiple double '/'s in base denom", "transfer/channel-0/gamm//pool//1", DenomTrace{BaseDenom: "gamm//pool//1", Path: "transfer/channel-0"}}, - {"trace info with multiple port/channel pairs", "transfer/channel-0/transfer/channel-1/uatom", DenomTrace{BaseDenom: "uatom", Path: "transfer/channel-0/transfer/channel-1"}}, + {"trace info", "transfer/channelToA/uatom", DenomTrace{BaseDenom: "uatom", Path: "transfer/channelToA"}}, + {"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: "transfer//uatom"}}, - {"invalid path (2)", "transfer/channelToA/uatom", DenomTrace{BaseDenom: "transfer/channelToA/uatom"}}, - {"invalid path (3)", "channel-0/transfer/uatom", DenomTrace{BaseDenom: "channel-0/transfer/uatom"}}, - {"invalid path (4)", "uatom/transfer", DenomTrace{BaseDenom: "uatom/transfer"}}, - {"invalid path (5)", "transfer/channel-0", DenomTrace{Path: "transfer/channel-0"}}, - {"invalid path (6)", "transfer/channel-0/", DenomTrace{Path: "transfer/channel-0"}}, - {"invalid path (7)", "transfer/channelToA", DenomTrace{BaseDenom: "transfer/channelToA"}}, - {"invalid path (8)", "transfer/channel-0/transfer", DenomTrace{BaseDenom: "transfer", Path: "transfer/channel-0"}}, - {"invalid path (9)", "transfer/channel-0/transfer/channel-1", DenomTrace{Path: "transfer/channel-0/transfer/channel-1"}}, - {"invalid path (10)", "transfer/channel-0/transfer/channelToB", DenomTrace{BaseDenom: "transfer/channelToB", Path: "transfer/channel-0"}}, + {"invalid path (1)", "transfer//uatom", DenomTrace{BaseDenom: "uatom", Path: "transfer/"}}, + {"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 { @@ -65,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}, @@ -120,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}, } From 5c8dedc1bddb2a7dc6786159ba9b0bc3f08f7416 Mon Sep 17 00:00:00 2001 From: Carlos Rodriguez Date: Tue, 7 Jun 2022 13:19:26 +0200 Subject: [PATCH 4/7] add changelog entry an other review comments --- CHANGELOG.md | 1 + modules/apps/transfer/types/trace.go | 16 +++++++++------- 2 files changed, 10 insertions(+), 7 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index c0ac41e180c..a18ec98f5bb 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -75,6 +75,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 specifc channel did not follow the same format as the rest of queries. +* (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/modules/apps/transfer/types/trace.go b/modules/apps/transfer/types/trace.go index b8af88ae616..f6801ad5ab3 100644 --- a/modules/apps/transfer/types/trace.go +++ b/modules/apps/transfer/types/trace.go @@ -20,11 +20,11 @@ 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"} -// - "gamm/pool/1" => DenomTrace{Path: "", BaseDenom: "gamm/pool/1"} -// - "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, "/") @@ -74,9 +74,11 @@ func (dt DenomTrace) GetFullDenomPath() string { return dt.GetPrefix() + dt.BaseDenom } +// extractPathAndBaseDenomFromDenomTrace returns the trace path and the base denom from +// the elements that constitute the complete denom trace. func extractPathAndBaseDenomFromDenomTrace(denomTraceItems []string) (string, string) { - path := []string{} - baseDenom := []string{} + var path []string + var baseDenom []string length := len(denomTraceItems) for i := 0; i < length; i = i + 2 { if i < length-1 && length > 2 && denomTraceItems[i] == PortID { From 9bff5892255115956550539b155e6f04d1ebcf0e Mon Sep 17 00:00:00 2001 From: crodriguezvega Date: Wed, 8 Jun 2022 22:21:01 +0200 Subject: [PATCH 5/7] review comment --- modules/apps/transfer/types/trace.go | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/modules/apps/transfer/types/trace.go b/modules/apps/transfer/types/trace.go index f6801ad5ab3..e0a68ef8d9b 100644 --- a/modules/apps/transfer/types/trace.go +++ b/modules/apps/transfer/types/trace.go @@ -35,7 +35,7 @@ func ParseDenomTrace(rawDenom string) DenomTrace { } } - path, baseDenom := extractPathAndBaseDenomFromDenomTrace(denomSplit) + path, baseDenom := extractPathAndBaseFromFullDenom(denomSplit) return DenomTrace{ Path: path, BaseDenom: baseDenom, @@ -74,17 +74,17 @@ func (dt DenomTrace) GetFullDenomPath() string { return dt.GetPrefix() + dt.BaseDenom } -// extractPathAndBaseDenomFromDenomTrace returns the trace path and the base denom from -// the elements that constitute the complete denom trace. -func extractPathAndBaseDenomFromDenomTrace(denomTraceItems []string) (string, 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 - length := len(denomTraceItems) + length := len(fullDenomItems) for i := 0; i < length; i = i + 2 { - if i < length-1 && length > 2 && denomTraceItems[i] == PortID { - path = append(path, denomTraceItems[i], denomTraceItems[i+1]) + if i < length-1 && length > 2 && fullDenomItems[i] == PortID { + path = append(path, fullDenomItems[i], fullDenomItems[i+1]) } else { - baseDenom = denomTraceItems[i:] + baseDenom = fullDenomItems[i:] break } } @@ -180,7 +180,7 @@ func ValidatePrefixedDenom(denom string) error { return sdkerrors.Wrap(ErrInvalidDenomForTransfer, "base denomination cannot be blank") } - path, _ := extractPathAndBaseDenomFromDenomTrace(denomSplit) + path, _ := extractPathAndBaseFromFullDenom(denomSplit) if path == "" { // NOTE: base denom contains slashes, so no base denomination validation return nil From 64657c52a68a43de5af6e01c9749e8ca62a955c8 Mon Sep 17 00:00:00 2001 From: Aditya Date: Tue, 14 Jun 2022 10:28:22 +0200 Subject: [PATCH 6/7] Add slash migration guide (#1518) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * 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 --- docs/migrations/support-slashed-denoms.md | 101 ++++++++++++++++++++++ 1 file changed, 101 insertions(+) create mode 100644 docs/migrations/support-slashed-denoms.md diff --git a/docs/migrations/support-slashed-denoms.md b/docs/migrations/support-slashed-denoms.md new file mode 100644 index 00000000000..829849bc25a --- /dev/null +++ b/docs/migrations/support-slashed-denoms.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). \ No newline at end of file From 4a2a8cdcf694c9acb200bf446c0e62f9ddc440fc Mon Sep 17 00:00:00 2001 From: crodriguezvega Date: Tue, 14 Jun 2022 13:00:37 +0200 Subject: [PATCH 7/7] rename migration file --- .../{support-slashed-denoms.md => support-denoms-with-slashes.md} | 0 1 file changed, 0 insertions(+), 0 deletions(-) rename docs/migrations/{support-slashed-denoms.md => support-denoms-with-slashes.md} (100%) diff --git a/docs/migrations/support-slashed-denoms.md b/docs/migrations/support-denoms-with-slashes.md similarity index 100% rename from docs/migrations/support-slashed-denoms.md rename to docs/migrations/support-denoms-with-slashes.md