Skip to content

Commit

Permalink
chore: denom traces migration handler (backport #1680) (#1753)
Browse files Browse the repository at this point in the history
* chore: denom traces migration handler (#1680)

* update code & test

* register migrator service

(cherry picked from commit be5ccf3)

# Conflicts:
#	CHANGELOG.md
#	docs/migrations/support-denoms-with-slashes.md
#	docs/migrations/v3-to-v4.md
#	modules/apps/transfer/types/trace.go

* fix conflicts

* fix go version package

Co-authored-by: Charly <charly@interchain.berlin>
Co-authored-by: crodriguezvega <carlos@interchain.io>
  • Loading branch information
3 people committed Jul 22, 2022
1 parent e468300 commit a061fab
Show file tree
Hide file tree
Showing 7 changed files with 247 additions and 63 deletions.
3 changes: 3 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,9 @@ Ref: https://keepachangelog.com/en/1.0.0/

### Improvements

* (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/v2/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/v2/modules/apps/transfer/keeper"
transfertypes "github.com/cosmos/ibc-go/v2/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 @@ -126,6 +126,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 @@ -145,7 +150,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
31 changes: 22 additions & 9 deletions modules/apps/transfer/types/trace.go
Original file line number Diff line number Diff line change
Expand Up @@ -7,11 +7,12 @@ import (
"sort"
"strings"

sdk "github.com/cosmos/cosmos-sdk/types"
sdkerrors "github.com/cosmos/cosmos-sdk/types/errors"
tmbytes "github.com/tendermint/tendermint/libs/bytes"
tmtypes "github.com/tendermint/tendermint/types"

sdk "github.com/cosmos/cosmos-sdk/types"
sdkerrors "github.com/cosmos/cosmos-sdk/types/errors"
channeltypes "github.com/cosmos/ibc-go/v2/modules/core/04-channel/types"
host "github.com/cosmos/ibc-go/v2/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 a061fab

Please sign in to comment.