Skip to content

Commit

Permalink
chore: denom traces migration handler (#1680)
Browse files Browse the repository at this point in the history
* update code & test

* register migrator service
  • Loading branch information
charleenfei authored Jul 21, 2022
1 parent 5f5a287 commit be5ccf3
Show file tree
Hide file tree
Showing 8 changed files with 265 additions and 66 deletions.
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
34 changes: 5 additions & 29 deletions docs/migrations/support-denoms-with-slashes.md
Original file line number Diff line number Diff line change
Expand Up @@ -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.

Expand All @@ -28,41 +28,17 @@ 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 && !equalTraces(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)
})

func equalTraces(dtA, dtB ibctransfertypes.DenomTrace) bool {
return dtA.BaseDenom == dtB.BaseDenom && dtA.Path == dtB.Path
}

```

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

Expand Down
22 changes: 21 additions & 1 deletion docs/migrations/v3-to-v4.md
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,27 @@ No genesis or in-place migrations required when upgrading from v1 or v2 of ibc-g

## Chains

- No relevant changes were made in this release.
### Migration to fix support for base denoms with slashes

As part of [v1.5.0](https://github.com/cosmos/ibc-go/releases/tag/v1.5.0), [v2.3.0](https://github.com/cosmos/ibc-go/releases/tag/v2.3.0) and [v3.1.0](https://github.com/cosmos/ibc-go/releases/tag/v3.1.0) some [migration handler code sample was documented](https://github.com/cosmos/ibc-go/blob/main/docs/migrations/support-denoms-with-slashes.md#upgrade-proposal) that needs to run in order to correct the trace information of coins transferred using ICS20 whose base denom contains slashes.

Based on feedback from the community we add now an improved solution to run the same migration that does not require copying a large piece of code over from the migration document, but instead requires only adding a one-line upgrade handler.

If the chain will migrate to supporting base denoms with slashes, it must set the appropriate params during the execution of the upgrade handler in `app.go`:
```go
app.UpgradeKeeper.SetUpgradeHandler("MigrateTraces",
func(ctx sdk.Context, _ upgradetypes.Plan, fromVM module.VersionMap) (module.VersionMap, error) {
// transfer module consensus version has been bumped to 2
return app.mm.RunMigrations(ctx, app.configurator, fromVM)
})

```

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.

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"`.

This incorrect trace information must be corrected when the chain does upgrade to fully supporting denominations with slashes.

## IBC Apps

Expand Down
59 changes: 59 additions & 0 deletions modules/apps/transfer/keeper/migrations.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,59 @@
package keeper

import (
"fmt"

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

"github.com/cosmos/ibc-go/v4/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
}
123 changes: 123 additions & 0 deletions modules/apps/transfer/keeper/migrations_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,123 @@
package keeper_test

import (
"fmt"

transferkeeper "github.com/cosmos/ibc-go/v4/modules/apps/transfer/keeper"
transfertypes "github.com/cosmos/ibc-go/v4/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())
})
}
7 changes: 6 additions & 1 deletion modules/apps/transfer/module.go
Original file line number Diff line number Diff line change
Expand Up @@ -120,6 +120,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
Expand All @@ -139,7 +144,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 }

// BeginBlock implements the AppModule interface
func (am AppModule) BeginBlock(ctx sdk.Context, req abci.RequestBeginBlock) {
Expand Down
27 changes: 20 additions & 7 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 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
Loading

0 comments on commit be5ccf3

Please sign in to comment.