Skip to content

Commit

Permalink
Parse ICS20 denomination using channel identifier format (#1730)
Browse files Browse the repository at this point in the history 
* refactor: use channel identifier for parsing IBC tokens

Change IBC token denomination parsing to use the channel identifier rather than the port
Remove unfixable parsing validation test
Fix migration tests
Add check to migration of traces to panic upon denomination state corruption

* add migration test

* Update modules/apps/transfer/keeper/migrations.go

* Update modules/apps/transfer/types/trace_test.go

* add changelog entry

* Update modules/apps/transfer/types/trace.go

Co-authored-by: Damian Nolan <[email protected]>

* apply review suggestions

* update go doc

Co-authored-by: Damian Nolan <[email protected]>
Co-authored-by: Carlos Rodriguez <[email protected]>
  • Loading branch information
@colin-axner @damiannolan @crodriguezvega
3 people authored Jul 20, 2022
1 parent b608e89 commit b86cdd8
Show file tree
Hide file tree
Showing 5 changed files with 77 additions and 38 deletions.
1 change: 1 addition & 0 deletions CHANGELOG.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,7 @@ Ref: https://keepachangelog.com/en/1.0.0/

### Improvements

* (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.
* (cleanup) [\#1335](https://github.com/cosmos/ibc-go/pull/1335/) `gofumpt -w -l .` to standardize the code layout more strictly than `go fmt ./...`
* (middleware) [\#1022](https://github.com/cosmos/ibc-go/pull/1022) Add `GetAppVersion` to the ICS4Wrapper interface. This function should be used by IBC applications to obtain their own version since the version set in the channel structure may be wrapped many times by middleware.
* (modules/core/04-channel) [\#1232](https://github.com/cosmos/ibc-go/pull/1232) Updating params on `NewPacketId` and moving to bottom of file.
Expand Down
10 changes: 10 additions & 0 deletions modules/apps/transfer/keeper/migrations.go
View file
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
package keeper

import (
"fmt"

sdk "github.com/cosmos/cosmos-sdk/types"

"github.com/cosmos/ibc-go/v4/modules/apps/transfer/types"
Expand Down Expand Up @@ -33,6 +35,14 @@ func (m Migrator) MigrateTraces(ctx sdk.Context) error {
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)
}
Expand Down
19 changes: 16 additions & 3 deletions modules/apps/transfer/keeper/migrations_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -76,8 +76,7 @@ func (suite *KeeperTestSuite) TestMigratorMigrateTraces() {
},
},
{
// the expected value will change with the implementation of the fix for #1698
"no change: non-standard port",
"success: non-standard port",
func() {
suite.chainA.GetSimApp().TransferKeeper.SetDenomTrace(
suite.chainA.GetContext(),
Expand All @@ -87,7 +86,7 @@ func (suite *KeeperTestSuite) TestMigratorMigrateTraces() {
},
transfertypes.Traces{
{
BaseDenom: "customport/channel-7/uatom", Path: "transfer/channel-0/transfer/channel-1",
BaseDenom: "uatom", Path: "transfer/channel-0/transfer/channel-1/customport/channel-7",
},
},
},
Expand All @@ -108,3 +107,17 @@ func (suite *KeeperTestSuite) TestMigratorMigrateTraces() {
})
}
}

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())
})
}
27 changes: 20 additions & 7 deletions modules/apps/transfer/types/trace.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ import (
tmbytes "github.com/tendermint/tendermint/libs/bytes"
tmtypes "github.com/tendermint/tendermint/types"

channeltypes "github.com/cosmos/ibc-go/v4/modules/core/04-channel/types"
host "github.com/cosmos/ibc-go/v4/modules/core/24-host"
)

Expand All @@ -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 {
Expand Down Expand Up @@ -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:]
Expand Down Expand Up @@ -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
Expand Down
58 changes: 30 additions & 28 deletions modules/apps/transfer/types/trace_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -17,20 +17,23 @@ 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/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"}},
{"trace info", "transfer/channel-1/uatom", DenomTrace{BaseDenom: "uatom", Path: "transfer/channel-1"}},
{"trace info with custom port", "customtransfer/channel-1/uatom", DenomTrace{BaseDenom: "uatom", Path: "customtransfer/channel-1"}},
{"trace info with base denom ending in '/'", "transfer/channel-1/uatom/", DenomTrace{BaseDenom: "uatom/", Path: "transfer/channel-1"}},
{"trace info with single '/' in base denom", "transfer/channel-1/erc20/0x85bcBCd7e79Ec36f4fBBDc54F90C643d921151AA", DenomTrace{BaseDenom: "erc20/0x85bcBCd7e79Ec36f4fBBDc54F90C643d921151AA", Path: "transfer/channel-1"}},
{"trace info with multiple '/'s in base denom", "transfer/channel-1/gamm/pool/1", DenomTrace{BaseDenom: "gamm/pool/1", Path: "transfer/channel-1"}},
{"trace info with multiple double '/'s in base denom", "transfer/channel-1/gamm//pool//1", DenomTrace{BaseDenom: "gamm//pool//1", Path: "transfer/channel-1"}},
{"trace info with multiple port/channel pairs", "transfer/channel-1/transfer/channel-2/uatom", DenomTrace{BaseDenom: "uatom", Path: "transfer/channel-1/transfer/channel-2"}},
{"trace info with multiple custom ports", "customtransfer/channel-1/alternativetransfer/channel-2/uatom", DenomTrace{BaseDenom: "uatom", Path: "customtransfer/channel-1/alternativetransfer/channel-2"}},
{"incomplete path", "transfer/uatom", DenomTrace{BaseDenom: "transfer/uatom"}},
{"invalid path (1)", "transfer//uatom", DenomTrace{BaseDenom: "uatom", Path: "transfer/"}},
{"invalid path (2)", "channelToA/transfer/uatom", DenomTrace{BaseDenom: "channelToA/transfer/uatom"}},
{"invalid path (1)", "transfer//uatom", DenomTrace{BaseDenom: "transfer//uatom", Path: ""}},
{"invalid path (2)", "channel-1/transfer/uatom", DenomTrace{BaseDenom: "channel-1/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"}},
{"invalid path (4)", "transfer/channel-1", DenomTrace{BaseDenom: "transfer/channel-1"}},
{"invalid path (5)", "transfer/channel-1/", DenomTrace{Path: "transfer/channel-1"}},
{"invalid path (6)", "transfer/channel-1/transfer", DenomTrace{BaseDenom: "transfer", Path: "transfer/channel-1"}},
{"invalid path (7)", "transfer/channel-1/transfer/channel-2", DenomTrace{Path: "transfer/channel-1/transfer/channel-2"}},
{"invalid path (8)", "transfer/channelToA/uatom", DenomTrace{BaseDenom: "transfer/channelToA/uatom", Path: ""}},
}

for _, tc := range testCases {
Expand All @@ -46,7 +49,7 @@ func TestDenomTrace_IBCDenom(t *testing.T) {
expDenom string
}{
{"base denom", DenomTrace{BaseDenom: "uatom"}, "uatom"},
{"trace info", DenomTrace{BaseDenom: "uatom", Path: "transfer/channelToA"}, "ibc/7F1D3FCF4AE79E1554D670D1AD949A9BA4E4A3C76C63093E17E446A46061A7A2"},
{"trace info", DenomTrace{BaseDenom: "uatom", Path: "transfer/channel-1"}, "ibc/C4CFF46FD6DE35CA4CF4CE031E643C8FDC9BA4B99AE598E9B0ED98FE3A2319F9"},
}

for _, tc := range testCases {
Expand All @@ -65,12 +68,12 @@ func TestDenomTrace_Validate(t *testing.T) {
{"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},
{"valid single trace info", DenomTrace{BaseDenom: "uatom", Path: "transfer/channel-1"}, false},
{"valid multiple trace info", DenomTrace{BaseDenom: "uatom", Path: "transfer/channel-1/transfer/channel-2"}, false},
{"single trace identifier", DenomTrace{BaseDenom: "uatom", Path: "transfer"}, true},
{"invalid port ID", DenomTrace{BaseDenom: "uatom", Path: "(transfer)/channelToA"}, true},
{"invalid channel ID", DenomTrace{BaseDenom: "uatom", Path: "transfer/(channelToA)"}, true},
{"empty base denom with trace", DenomTrace{BaseDenom: "", Path: "transfer/channelToA"}, true},
{"invalid port ID", DenomTrace{BaseDenom: "uatom", Path: "(transfer)/channel-1"}, true},
{"invalid channel ID", DenomTrace{BaseDenom: "uatom", Path: "transfer/(channel-1)"}, true},
{"empty base denom with trace", DenomTrace{BaseDenom: "", Path: "transfer/channel-1"}, true},
}

for _, tc := range testCases {
Expand All @@ -90,16 +93,16 @@ func TestTraces_Validate(t *testing.T) {
expError bool
}{
{"empty Traces", Traces{}, false},
{"valid multiple trace info", Traces{{BaseDenom: "uatom", Path: "transfer/channelToA/transfer/channelToB"}}, false},
{"valid multiple trace info", Traces{{BaseDenom: "uatom", Path: "transfer/channel-1/transfer/channel-2"}}, false},
{
"valid multiple trace info",
Traces{
{BaseDenom: "uatom", Path: "transfer/channelToA/transfer/channelToB"},
{BaseDenom: "uatom", Path: "transfer/channelToA/transfer/channelToB"},
{BaseDenom: "uatom", Path: "transfer/channel-1/transfer/channel-2"},
{BaseDenom: "uatom", Path: "transfer/channel-1/transfer/channel-2"},
},
true,
},
{"empty base denom with trace", Traces{{BaseDenom: "", Path: "transfer/channelToA"}}, true},
{"empty base denom with trace", Traces{{BaseDenom: "", Path: "transfer/channel-1"}}, true},
}

for _, tc := range testCases {
Expand All @@ -118,26 +121,25 @@ func TestValidatePrefixedDenom(t *testing.T) {
denom string
expError bool
}{
{"prefixed denom", "transfer/channelToA/uatom", false},
{"prefixed denom with '/'", "transfer/channelToA/gamm/pool/1", false},
{"prefixed denom", "transfer/channel-1/uatom", false},
{"prefixed denom with '/'", "transfer/channel-1/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},
{"invalid port ID", "(transfer)/channel-1/uatom", true},
{"empty denom", "", true},
{"single trace identifier", "transfer/", true},
{"invalid channel ID", "transfer/(channelToA)/uatom", true},
}

for _, tc := range testCases {
err := ValidatePrefixedDenom(tc.denom)
if tc.expError {
require.Error(t, err, tc.name)
continue
} else {
require.NoError(t, err, tc.name)
}
require.NoError(t, err, tc.name)
}
}

Expand Down

0 comments on commit b86cdd8

Please sign in to comment.