diff --git a/docs/migrations/support-denoms-with-slashes.md b/docs/migrations/support-denoms-with-slashes.md index 0d68b3d55fa..0447cf57d99 100644 --- a/docs/migrations/support-denoms-with-slashes.md +++ b/docs/migrations/support-denoms-with-slashes.md @@ -9,7 +9,7 @@ There are four sections based on the four potential user groups of this document - 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. +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.2.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. @@ -28,37 +28,16 @@ The transfer module will now support slashes in base denoms, so we must iterate ### Upgrade Proposal ```go -// Here the upgrade name is the upgrade name set by the chain -app.UpgradeKeeper.SetUpgradeHandler("supportSlashedDenomsUpgrade", +app.UpgradeKeeper.SetUpgradeHandler("MigrateTraces", 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) - } - + // transfer module consensus version has been bumped to 2 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). +For a more detailed sample, please check out the code changes in [this pull request](https://github.com/cosmos/ibc-go/pull/1680). ### Genesis Migration diff --git a/modules/apps/transfer/keeper/migrations.go b/modules/apps/transfer/keeper/migrations.go new file mode 100644 index 00000000000..2ce3d13fcb6 --- /dev/null +++ b/modules/apps/transfer/keeper/migrations.go @@ -0,0 +1,59 @@ +package keeper + +import ( + "fmt" + + sdk "github.com/line/lbm-sdk/types" + + "github.com/line/ibc-go/v3/modules/apps/transfer/types" +) + +// Migrator is a struct for handling in-place store migrations. +type Migrator struct { + keeper Keeper +} + +// NewMigrator returns a new Migrator. +func NewMigrator(keeper Keeper) Migrator { + return Migrator{keeper: keeper} +} + +// MigrateTraces migrates the DenomTraces to the correct format, accounting for slashes in the BaseDenom. +func (m Migrator) MigrateTraces(ctx sdk.Context) error { + + // list of traces that must replace the old traces in store + var newTraces []types.DenomTrace + m.keeper.IterateDenomTraces(ctx, + func(dt types.DenomTrace) (stop bool) { + // check if the new way of splitting FullDenom + // is the same as the current DenomTrace. + // If it isn't then store the new DenomTrace in the list of new traces. + newTrace := types.ParseDenomTrace(dt.GetFullDenomPath()) + err := newTrace.Validate() + 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) + } + return false + }) + + // replace the outdated traces with the new trace information + for _, nt := range newTraces { + m.keeper.SetDenomTrace(ctx, nt) + } + return nil +} + +func equalTraces(dtA, dtB types.DenomTrace) bool { + return dtA.BaseDenom == dtB.BaseDenom && dtA.Path == dtB.Path +} diff --git a/modules/apps/transfer/keeper/migrations_test.go b/modules/apps/transfer/keeper/migrations_test.go new file mode 100644 index 00000000000..3834625446c --- /dev/null +++ b/modules/apps/transfer/keeper/migrations_test.go @@ -0,0 +1,123 @@ +package keeper_test + +import ( + "fmt" + + transferkeeper "github.com/line/ibc-go/v3/modules/apps/transfer/keeper" + transfertypes "github.com/line/ibc-go/v3/modules/apps/transfer/types" +) + +func (suite *KeeperTestSuite) TestMigratorMigrateTraces() { + + testCases := []struct { + msg string + malleate func() + expectedTraces transfertypes.Traces + }{ + + { + "success: two slashes in base denom", + func() { + suite.chainA.GetSimApp().TransferKeeper.SetDenomTrace( + suite.chainA.GetContext(), + transfertypes.DenomTrace{ + BaseDenom: "pool/1", Path: "transfer/channel-0/gamm", + }) + }, + transfertypes.Traces{ + { + BaseDenom: "gamm/pool/1", Path: "transfer/channel-0", + }, + }, + }, + { + "success: one slash in base denom", + func() { + suite.chainA.GetSimApp().TransferKeeper.SetDenomTrace( + suite.chainA.GetContext(), + transfertypes.DenomTrace{ + BaseDenom: "0x85bcBCd7e79Ec36f4fBBDc54F90C643d921151AA", Path: "transfer/channel-149/erc", + }) + }, + transfertypes.Traces{ + { + BaseDenom: "erc/0x85bcBCd7e79Ec36f4fBBDc54F90C643d921151AA", Path: "transfer/channel-149", + }, + }, + }, + { + "success: multiple slashes in a row in base denom", + func() { + suite.chainA.GetSimApp().TransferKeeper.SetDenomTrace( + suite.chainA.GetContext(), + transfertypes.DenomTrace{ + BaseDenom: "1", Path: "transfer/channel-5/gamm//pool", + }) + }, + transfertypes.Traces{ + { + BaseDenom: "gamm//pool/1", Path: "transfer/channel-5", + }, + }, + }, + { + "success: multihop base denom", + func() { + suite.chainA.GetSimApp().TransferKeeper.SetDenomTrace( + suite.chainA.GetContext(), + transfertypes.DenomTrace{ + BaseDenom: "transfer/channel-1/uatom", Path: "transfer/channel-0", + }) + }, + transfertypes.Traces{ + { + BaseDenom: "uatom", Path: "transfer/channel-0/transfer/channel-1", + }, + }, + }, + { + "success: non-standard port", + func() { + suite.chainA.GetSimApp().TransferKeeper.SetDenomTrace( + suite.chainA.GetContext(), + transfertypes.DenomTrace{ + BaseDenom: "customport/channel-7/uatom", Path: "transfer/channel-0/transfer/channel-1", + }) + }, + transfertypes.Traces{ + { + BaseDenom: "uatom", Path: "transfer/channel-0/transfer/channel-1/customport/channel-7", + }, + }, + }, + } + + for _, tc := range testCases { + suite.Run(fmt.Sprintf("case %s", tc.msg), func() { + suite.SetupTest() // reset + + tc.malleate() // explicitly set up denom traces + + migrator := transferkeeper.NewMigrator(suite.chainA.GetSimApp().TransferKeeper) + err := migrator.MigrateTraces(suite.chainA.GetContext()) + suite.Require().NoError(err) + + traces := suite.chainA.GetSimApp().TransferKeeper.GetAllDenomTraces(suite.chainA.GetContext()) + suite.Require().Equal(tc.expectedTraces, traces) + }) + } +} + +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()) + }) +} diff --git a/modules/apps/transfer/module.go b/modules/apps/transfer/module.go index 541130db9d0..bc9021e1ccf 100644 --- a/modules/apps/transfer/module.go +++ b/modules/apps/transfer/module.go @@ -118,6 +118,11 @@ func (am AppModule) LegacyQuerierHandler(*codec.LegacyAmino) sdk.Querier { func (am AppModule) RegisterServices(cfg module.Configurator) { types.RegisterMsgServer(cfg.MsgServer(), am.keeper) types.RegisterQueryServer(cfg.QueryServer(), am.keeper) + + m := keeper.NewMigrator(am.keeper) + if err := cfg.RegisterMigration(types.ModuleName, 1, m.MigrateTraces); err != nil { + panic(fmt.Sprintf("failed to migrate transfer app from version 1 to 2: %v", err)) + } } // InitGenesis performs genesis initialization for the ibc-transfer module. It returns @@ -137,7 +142,7 @@ func (am AppModule) ExportGenesis(ctx sdk.Context, cdc codec.JSONCodec) json.Raw } // ConsensusVersion implements AppModule/ConsensusVersion. -func (AppModule) ConsensusVersion() uint64 { return 1 } +func (AppModule) ConsensusVersion() uint64 { return 2 } // AppModuleSimulation functions diff --git a/modules/apps/transfer/types/trace.go b/modules/apps/transfer/types/trace.go index 76b3fce0e33..559f61a7877 100644 --- a/modules/apps/transfer/types/trace.go +++ b/modules/apps/transfer/types/trace.go @@ -12,6 +12,7 @@ import ( ocbytes "github.com/line/ostracon/libs/bytes" octypes "github.com/line/ostracon/types" + channeltypes "github.com/line/ibc-go/v3/modules/core/04-channel/types" host "github.com/line/ibc-go/v3/modules/core/24-host" ) @@ -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 { @@ -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:] @@ -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 diff --git a/modules/apps/transfer/types/trace_test.go b/modules/apps/transfer/types/trace_test.go index 6526cbd952d..ba0690423bd 100644 --- a/modules/apps/transfer/types/trace_test.go +++ b/modules/apps/transfer/types/trace_test.go @@ -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 { @@ -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 { @@ -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 { @@ -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 { @@ -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) } }