Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Parse ICS20 denomination using channel identifier format #1730

1 change: 1 addition & 0 deletions CHANGELOG.md
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
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 @@ -34,6 +36,14 @@ func (m Migrator) MigrateTraces(ctx sdk.Context) error {
iterErr = err
return true
}

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))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We expect this to not occur correct?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

correct. From my understanding, this should only be possible if any chains using the latest minor versions (1.5, 2.3, 3.1), use custom ports for transfer and allow receives of foreign assets. I'm believe there is no single case of this, but to be safe, the check is added to ensure that if this assumption is wrong we can fix the issue before it worsens

}

if !equalTraces(newTrace, dt) {
newTraces = append(newTraces, newTrace)
}
Expand Down
18 changes: 15 additions & 3 deletions modules/apps/transfer/keeper/migrations_test.go
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,16 @@ func (suite *KeeperTestSuite) TestMigratorMigrateTraces() {
})
}
}

func (suite *KeeperTestSuite) TestMigratorMigrateTracesCorruptionDetection() {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Made as a separate test since it is the only test case which should panic

// IBCDenom() previously would return "customport/channel-0/uatom", but now should return ibc/{hash}
corruptedDenomTrace := transfertypes.DenomTrace{
BaseDenom: "customport/channel-0/uatom", Path: "",
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

mega nit but could we separate fields onto different lines for readability, e.g.

corruptedDenomTrace := transfertypes.DenomTrace{
	BaseDenom: "customport/channel-0/uatom", 
        Path:      "",
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh definitely! Didn't even notice it was two fields

suite.chainA.GetSimApp().TransferKeeper.SetDenomTrace(suite.chainA.GetContext(), corruptedDenomTrace)

migrator := transferkeeper.NewMigrator(suite.chainA.GetSimApp().TransferKeeper)
suite.Panics(func() {
migrator.MigrateTraces(suite.chainA.GetContext())
})
}
19 changes: 16 additions & 3 deletions modules/apps/transfer/types/trace.go
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 Down Expand Up @@ -80,11 +81,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 idenitifer to be the
colin-axner marked this conversation as resolved.
Show resolved Hide resolved
// 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
58 changes: 30 additions & 28 deletions modules/apps/transfer/types/trace_test.go
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},
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

had to remove this test, the ValidatePrefixedDenom relies on the parsing function. As the documentation indicates, an invalid channel identifier will result in the channel identifier being treated as a base denomination, thus making the test case "invalid channel id" always pass (we don't do validation on the base denomination, since it isn't ours to validate)

}

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