Skip to content

Commit

Permalink
fix to correctly parse denoms with slashes in the base denom (cosmos#…
Browse files Browse the repository at this point in the history
…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 (cosmos#1518)

* add migration guide

* Update docs/migrations/support-slashed-denoms.md

Co-authored-by: Federico Kunze Küllmer <[email protected]>

* clarify upgrade name

* remove unnecessary store loader

* review comment, update migration code

Co-authored-by: Federico Kunze Küllmer <[email protected]>
Co-authored-by: Carlos Rodriguez <[email protected]>

* rename migration file

Co-authored-by: Carlos Rodriguez <[email protected]>
Co-authored-by: Aditya <[email protected]>
Co-authored-by: Federico Kunze Küllmer <[email protected]>
  • Loading branch information
4 people authored Jun 14, 2022
1 parent dab07f9 commit 3a235af
Show file tree
Hide file tree
Showing 4 changed files with 162 additions and 12 deletions.
2 changes: 1 addition & 1 deletion CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down
101 changes: 101 additions & 0 deletions docs/migrations/support-denoms-with-slashes.md
Original file line number Diff line number Diff line change
@@ -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).
42 changes: 36 additions & 6 deletions modules/apps/transfer/types/trace.go
Original file line number Diff line number Diff line change
Expand Up @@ -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, "/")

Expand All @@ -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,
}
}

Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -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) != "" {
Expand All @@ -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)
}

Expand Down
29 changes: 24 additions & 5 deletions modules/apps/transfer/types/trace_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down Expand Up @@ -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},
Expand Down Expand Up @@ -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},
}

Expand All @@ -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},
Expand Down

0 comments on commit 3a235af

Please sign in to comment.