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.

Sign up for GitHub

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

Jump to bottom

Parse ICS20 denomination using channel identifier format #1730

Merged
merged 10 commits into from
Jul 20, 2022
Merged

Parse ICS20 denomination using channel identifier format #1730

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
900e20c
refactor: use channel identifier for parsing IBC tokens
colin-axner Jul 19, 2022
80117b7
add migration test
colin-axner Jul 19, 2022
f46506e
Update modules/apps/transfer/keeper/migrations.go
colin-axner Jul 19, 2022
8e43aa4
Update modules/apps/transfer/types/trace_test.go
colin-axner Jul 19, 2022
e0a79fe
add changelog entry
colin-axner Jul 19, 2022
88eb4d2
Merge branch 'colin/1698-parse-ics20-via-channelidentifier' of github…
colin-axner Jul 19, 2022
2be1b63
Update modules/apps/transfer/types/trace.go
colin-axner Jul 19, 2022
8b57dc5
apply review suggestions
colin-axner Jul 19, 2022
2e74b59
Merge branch 'colin/1698-parse-ics20-via-channelidentifier' of github…
colin-axner Jul 19, 2022
1348d37
update go doc
crodriguezvega Jul 20, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 10 additions & 0 deletions modules/apps/transfer/keeper/migrations.go
View file
Open in desktop
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
View file
Open in desktop
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
View file
Open in desktop
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
View file
Open in desktop
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