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

chore: denom traces migration handler (backport #1680) #1754

Merged
merged 4 commits into from
Jul 22, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
6 changes: 4 additions & 2 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -47,8 +47,10 @@ Ref: https://keepachangelog.com/en/1.0.0/

### Improvements

* (core/02-client) [\#1570](https://github.com/cosmos/ibc-go/pull/1570) Emitting an event when handling an upgrade client proposal.

* (core/02-client) [\#1570](https://github.com/cosmos/ibc-go/pull/1570) Emitting an event when handling an upgrade client proposal.
* (app/20-transfer) [\#1680](https://github.com/cosmos/ibc-go/pull/1680) Adds migration to correct any malformed trace path information of tokens with denoms that contains slashes. The transfer module consensus version has been bumped to 2.
* (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.

### Features

### Bug Fixes
Expand Down
29 changes: 4 additions & 25 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,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

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/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
}
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/v3/modules/apps/transfer/keeper"
transfertypes "github.com/cosmos/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())
})
}
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/v3/modules/core/04-channel/types"
host "github.com/cosmos/ibc-go/v3/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