From 72c1e8d6a04bc3d4d4b92d91f9d129c48c5a7ac8 Mon Sep 17 00:00:00 2001 From: Aditya Sripal Date: Thu, 9 Jun 2022 16:37:40 +0200 Subject: [PATCH 1/5] add migration guide --- docs/migrations/support-slashed-denoms.md | 113 ++++++++++++++++++++++ 1 file changed, 113 insertions(+) create mode 100644 docs/migrations/support-slashed-denoms.md diff --git a/docs/migrations/support-slashed-denoms.md b/docs/migrations/support-slashed-denoms.md new file mode 100644 index 00000000000..93ed2290fa6 --- /dev/null +++ b/docs/migrations/support-slashed-denoms.md @@ -0,0 +1,113 @@ +# Migrating from Unsupported Base Denom Slashes to Supporting Slashed BaseDenoms + +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 + +**Note:** ibc-go supports golang semantic versioning and therefore all imports must be updated to bump the version number on major releases. +```go +github.com/cosmos/ibc-go/v3 -> github.com/cosmos/ibc-go/v4 +``` + +This document is necessary when chains are upgrading from a version that does not support slashed base denoms (e.g. v3.0.0) to a version that does (e.g. v3.1.0). + +If a chain receives a slashed denom 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: "testcoin/testcoin", BaseDenom: "testcoin"` + +This incorrect trace information must be corrected when the chain does upgrade to fully supporting slashed denominations. + +To do so, chain binaries should include a migration script that will run when the chain upgrades from not supporting slashed base denominations to supporting slashed base denominations. + +## Chains + +### 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 Propsoal + +```go +app.UpgradeKeeper.SetUpgradeHandler("v3.1.0", + 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 []transfertypes.DenomTrace + + transferKeeper.IterateDenomTraces(ctx, + func(dt transfertypes.DenomTrace) bool { + // check if the new way of splitting FullDenom + // into Trace and BaseDenom is the same as the current + // DenomTrace. + // If it isn't then store the new DenomTrace in the list of new traces. + newTrace := transfertypes.ParseDenomTrace(dt.GetFullDenomPath()) + + if !reflect.DeepEqual(newTrace, dt) { + append(newTraces, newTrace) + } + }) + + // replace the outdated traces with the new trace information + for _, nt := range newTraces { + transferKeeper.SetDenomTrace(ctx, nt) + } + } +) +``` + +This is only necessary if there are DenomTraces 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 slashed denominations runs this code for safety. + +#### Add `StoreUpgrades` for Transfer module + +For Transfer it is also necessary to [manually add store upgrades](https://docs.cosmos.network/v0.44/core/upgrade.html#add-storeupgrades-for-new-modules) for the transfer module and then configure the store loader to apply those upgrades in `app.go` if you wish to use the upgrade handler method above. + +```go +// Here the upgrade name is just an example +if upgradeInfo.Name == "supportSlashingDenomUpgrade" && !app.UpgradeKeeper.IsSkipHeight(upgradeInfo.Height) { + storeUpgrades := store.StoreUpgrades{ + Added: []string{transfertypes.StoreKey} + } + + app.SetStoreLoader(upgradetypes.UpgradeStoreLoader(upgradeInfo.Height, &storeUpgrades)) +} +``` + +This ensures that the transfer module's stores are added to the multistore before the migrations begin. + +### 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 MigrateGenesis(appState genutiltypes.AppMap, clientCtx client.Context, genDoc tmtypes.GenesisDoc) (genutiltypes.AppMap, error) { + if appState[transfertypes.ModuleName] != nil { + transferGenState := &transfertypes.GenesisState + clientCtx.JSONCodec.MustUnmarshalJSON(appState[transfertypes.ModuleName], transferGenState) + + substituteTraces := make([]transfertypes.DenomTrace, len(transferGenState.Traces) + for i, dt := range transferGenState.Traces { + // replace all previous traces with the latest trace + // note most traces will have same value + newTrace := transfertypes.ParseDenomTrace(dt.GetFullDenomPath()) + + subsituteTraces[i] = newTrace + } + + transferGenState.Traces = substituteTraces + + // delete old genesis state + delete(appState, transfertypes.ModuleName) + + // set new ibc transfer genesis state + appState[transfertypes.ModuleName] = clientCtx.JSONCodec.MustMarshalJSON(transferGenState) + } + + return appState, nil +} +``` \ No newline at end of file From aef43eacb4d855920fd24a4097d8ae710d5af1dd Mon Sep 17 00:00:00 2001 From: Aditya Date: Thu, 9 Jun 2022 17:37:25 +0200 Subject: [PATCH 2/5] Update docs/migrations/support-slashed-denoms.md MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: Federico Kunze Küllmer <31522760+fedekunze@users.noreply.github.com> --- docs/migrations/support-slashed-denoms.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/migrations/support-slashed-denoms.md b/docs/migrations/support-slashed-denoms.md index 93ed2290fa6..d15b8a0b8c7 100644 --- a/docs/migrations/support-slashed-denoms.md +++ b/docs/migrations/support-slashed-denoms.md @@ -30,7 +30,7 @@ To do so, chain binaries should include a migration script that will run when th 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 Propsoal +### Upgrade Proposal ```go app.UpgradeKeeper.SetUpgradeHandler("v3.1.0", From 9f3842ee646470a5b7d33823a29036ccf5ff8c1c Mon Sep 17 00:00:00 2001 From: Aditya Sripal Date: Thu, 9 Jun 2022 17:44:00 +0200 Subject: [PATCH 3/5] clarify upgrade name --- docs/migrations/support-slashed-denoms.md | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/docs/migrations/support-slashed-denoms.md b/docs/migrations/support-slashed-denoms.md index 93ed2290fa6..ff0da1bfab1 100644 --- a/docs/migrations/support-slashed-denoms.md +++ b/docs/migrations/support-slashed-denoms.md @@ -33,7 +33,8 @@ The transfer module will now support slashes in base denoms, so we must iterate ### Upgrade Propsoal ```go -app.UpgradeKeeper.SetUpgradeHandler("v3.1.0", +// Here the upgrade name is the upgrade name set by the chain +app.UpgradeKeeper.SetUpgradeHandler("supportSlashingDenomUpgrade", 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 []transfertypes.DenomTrace @@ -66,7 +67,7 @@ This is only necessary if there are DenomTraces in the store with incorrect trac For Transfer it is also necessary to [manually add store upgrades](https://docs.cosmos.network/v0.44/core/upgrade.html#add-storeupgrades-for-new-modules) for the transfer module and then configure the store loader to apply those upgrades in `app.go` if you wish to use the upgrade handler method above. ```go -// Here the upgrade name is just an example +// Here the upgrade name is just an example of the upgrade name set by the chain if upgradeInfo.Name == "supportSlashingDenomUpgrade" && !app.UpgradeKeeper.IsSkipHeight(upgradeInfo.Height) { storeUpgrades := store.StoreUpgrades{ Added: []string{transfertypes.StoreKey} From a6a8893b06fb9792d6904aa025cd5e724c7e641c Mon Sep 17 00:00:00 2001 From: Aditya Sripal Date: Thu, 9 Jun 2022 17:46:47 +0200 Subject: [PATCH 4/5] remove unnecessary store loader --- docs/migrations/support-slashed-denoms.md | 17 ----------------- 1 file changed, 17 deletions(-) diff --git a/docs/migrations/support-slashed-denoms.md b/docs/migrations/support-slashed-denoms.md index 60af9f57c60..7966708d151 100644 --- a/docs/migrations/support-slashed-denoms.md +++ b/docs/migrations/support-slashed-denoms.md @@ -62,23 +62,6 @@ app.UpgradeKeeper.SetUpgradeHandler("supportSlashingDenomUpgrade", This is only necessary if there are DenomTraces 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 slashed denominations runs this code for safety. -#### Add `StoreUpgrades` for Transfer module - -For Transfer it is also necessary to [manually add store upgrades](https://docs.cosmos.network/v0.44/core/upgrade.html#add-storeupgrades-for-new-modules) for the transfer module and then configure the store loader to apply those upgrades in `app.go` if you wish to use the upgrade handler method above. - -```go -// Here the upgrade name is just an example of the upgrade name set by the chain -if upgradeInfo.Name == "supportSlashingDenomUpgrade" && !app.UpgradeKeeper.IsSkipHeight(upgradeInfo.Height) { - storeUpgrades := store.StoreUpgrades{ - Added: []string{transfertypes.StoreKey} - } - - app.SetStoreLoader(upgradetypes.UpgradeStoreLoader(upgradeInfo.Height, &storeUpgrades)) -} -``` - -This ensures that the transfer module's stores are added to the multistore before the migrations begin. - ### 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. From 0217a235a1d295c3201ce2f75521a78275c9acbd Mon Sep 17 00:00:00 2001 From: Carlos Rodriguez Date: Mon, 13 Jun 2022 22:00:07 +0200 Subject: [PATCH 5/5] review comment, update migration code --- docs/migrations/support-slashed-denoms.md | 116 +++++++++++----------- 1 file changed, 60 insertions(+), 56 deletions(-) diff --git a/docs/migrations/support-slashed-denoms.md b/docs/migrations/support-slashed-denoms.md index 7966708d151..829849bc25a 100644 --- a/docs/migrations/support-slashed-denoms.md +++ b/docs/migrations/support-slashed-denoms.md @@ -1,4 +1,4 @@ -# Migrating from Unsupported Base Denom Slashes to Supporting Slashed BaseDenoms +# 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. @@ -9,24 +9,19 @@ There are four sections based on the four potential user groups of this document - Relayers - IBC Light Clients -**Note:** ibc-go supports golang semantic versioning and therefore all imports must be updated to bump the version number on major releases. -```go -github.com/cosmos/ibc-go/v3 -> github.com/cosmos/ibc-go/v4 -``` +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 slashed base denoms (e.g. v3.0.0) to a version that does (e.g. v3.1.0). +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. -If a chain receives a slashed denom 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"`. -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: "testcoin/testcoin", BaseDenom: "testcoin"` +This incorrect trace information must be corrected when the chain does upgrade to fully supporting denominations with slashes. -This incorrect trace information must be corrected when the chain does upgrade to fully supporting slashed denominations. - -To do so, chain binaries should include a migration script that will run when the chain upgrades from not supporting slashed base denominations to supporting slashed base denominations. +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 -### Transfer +### 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. @@ -34,33 +29,36 @@ The transfer module will now support slashes in base denoms, so we must iterate ```go // Here the upgrade name is the upgrade name set by the chain -app.UpgradeKeeper.SetUpgradeHandler("supportSlashingDenomUpgrade", +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 []transfertypes.DenomTrace - - transferKeeper.IterateDenomTraces(ctx, - func(dt transfertypes.DenomTrace) bool { - // check if the new way of splitting FullDenom - // into Trace and BaseDenom is the same as the current - // DenomTrace. - // If it isn't then store the new DenomTrace in the list of new traces. - newTrace := transfertypes.ParseDenomTrace(dt.GetFullDenomPath()) - - if !reflect.DeepEqual(newTrace, dt) { - append(newTraces, newTrace) - } - }) + 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 { - transferKeeper.SetDenomTrace(ctx, nt) + app.TransferKeeper.SetDenomTrace(ctx, nt) } - } -) + + return app.mm.RunMigrations(ctx, app.configurator, fromVM) + }) ``` -This is only necessary if there are DenomTraces 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 slashed denominations runs this code for safety. +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 @@ -69,29 +67,35 @@ If the chain chooses to add support for slashes in base denoms via genesis expor The migration code required may look like: ```go -func MigrateGenesis(appState genutiltypes.AppMap, clientCtx client.Context, genDoc tmtypes.GenesisDoc) (genutiltypes.AppMap, error) { - if appState[transfertypes.ModuleName] != nil { - transferGenState := &transfertypes.GenesisState - clientCtx.JSONCodec.MustUnmarshalJSON(appState[transfertypes.ModuleName], transferGenState) - - substituteTraces := make([]transfertypes.DenomTrace, len(transferGenState.Traces) - for i, dt := range transferGenState.Traces { - // replace all previous traces with the latest trace - // note most traces will have same value - newTrace := transfertypes.ParseDenomTrace(dt.GetFullDenomPath()) - - subsituteTraces[i] = newTrace - } - - transferGenState.Traces = substituteTraces - - // delete old genesis state - delete(appState, transfertypes.ModuleName) - - // set new ibc transfer genesis state - appState[transfertypes.ModuleName] = clientCtx.JSONCodec.MustMarshalJSON(transferGenState) - } - - return appState, nil +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 } -``` \ No newline at end of file +``` + +For a more detailed sample, please check out the code changes in [this pull request](https://github.com/cosmos/ibc-go/pull/1528). \ No newline at end of file