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 #1680

Merged
merged 37 commits into from
Jul 21, 2022
Merged
Show file tree
Hide file tree
Changes from 24 commits
Commits
Show all changes
37 commits
Select commit Hold shift + click to select a range
96f3c49
fix broken link
charleenfei Jun 10, 2022
250e45f
Merge branch 'main' of github.com:cosmos/ibc-go
charleenfei Jun 14, 2022
bf3b96b
Merge branch 'main' of github.com:cosmos/ibc-go
charleenfei Jun 16, 2022
ed4a153
fix: rm AllowUpdateAfter... check (#1118)
charleenfei Jun 14, 2022
8018627
Merge branch 'main' of github.com:cosmos/ibc-go
charleenfei Jun 16, 2022
08e71e7
Merge branch 'main' of github.com:cosmos/ibc-go
charleenfei Jun 20, 2022
05f50c4
Merge branch 'main' of github.com:cosmos/ibc-go
charleenfei Jun 23, 2022
169ead2
Merge branch 'main' of github.com:cosmos/ibc-go
charleenfei Jun 24, 2022
7c46b84
Merge branch 'main' of github.com:cosmos/ibc-go
charleenfei Jul 5, 2022
27998dd
erge branch 'main' of github.com:cosmos/ibc-go
charleenfei Jul 6, 2022
952b114
Merge branch 'main' of github.com:cosmos/ibc-go
charleenfei Jul 8, 2022
7a1d263
first commit
charleenfei Jul 8, 2022
a5b3ff6
register migrator service
charleenfei Jul 8, 2022
b33f4df
testing
charleenfei Jul 12, 2022
f673132
Merge branch 'main' of github.com:cosmos/ibc-go
charleenfei Jul 12, 2022
0b2f4c2
test
charleenfei Jul 14, 2022
78d4616
Merge branch 'main' of github.com:cosmos/ibc-go
charleenfei Jul 14, 2022
3084982
Merge branch 'main' into charly/denom_traces_migration_handler
charleenfei Jul 14, 2022
b1cd41f
test
charleenfei Jul 14, 2022
3c07902
update docs and tests
charleenfei Jul 14, 2022
6110232
update docs for migration handler
charleenfei Jul 15, 2022
87a97e6
fix for empty baseDenom
charleenfei Jul 18, 2022
ecbd10a
update docs
charleenfei Jul 18, 2022
35ea75e
Merge branch 'main' into charly/denom_traces_migration_handler
charleenfei Jul 18, 2022
7660328
Merge branch 'main' into charly/denom_traces_migration_handler
charleenfei Jul 19, 2022
cd8c867
Merge branch 'main' into charly/denom_traces_migration_handler
charleenfei Jul 19, 2022
0f91661
Merge branch 'main' into charly/denom_traces_migration_handler
charleenfei Jul 19, 2022
9a44480
Merge branch 'main' into charly/denom_traces_migration_handler
charleenfei Jul 19, 2022
96aabb5
Merge branch 'main' into charly/denom_traces_migration_handler
charleenfei Jul 19, 2022
b608e89
update tests, rm empty base denom in trace.go
charleenfei Jul 20, 2022
b86cdd8
Parse ICS20 denomination using channel identifier format (#1730)
colin-axner Jul 20, 2022
45fdc09
Merge branch 'main' into charly/denom_traces_migration_handler
Jul 21, 2022
60785a6
Update support-denoms-with-slashes.md
Jul 21, 2022
38c93bc
rm redundant err
charleenfei Jul 21, 2022
515e0e7
Merge branch 'main' into charly/denom_traces_migration_handler
charleenfei Jul 21, 2022
a2fe4ae
fix error
charleenfei Jul 21, 2022
d999103
Merge branch 'charly/denom_traces_migration_handler' of github.com:co…
charleenfei Jul 21, 2022
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
32 changes: 4 additions & 28 deletions docs/migrations/support-denoms-with-slashes.md
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Where/how is RunMigrations defined? Does this get run at the sdk level as a callback?

This one liner is very nice compared to previous implementation btw, nice job!

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

see docs. Core IBC has also had its "module version" incremented previously

})

```

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
52 changes: 52 additions & 0 deletions modules/apps/transfer/keeper/migrations.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,52 @@
package keeper

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

"github.com/cosmos/ibc-go/v4/modules/apps/transfer/types"
charleenfei marked this conversation as resolved.
Show resolved Hide resolved
)

// 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 {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is the intention that all transfer related migrations will happen here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes -- the cosmos sdk has all migrations in a separate directory but imo that might be overkill for us. wdyt?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think for us it makes sense to have them here, we can worry about additional directories if we end up with a large number of migrations.

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 {
var iterErr 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
// 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 := types.ParseDenomTrace(dt.GetFullDenomPath())
err := newTrace.Validate()
if err != nil {
iterErr = err
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just wondering if there's anything we should do if this (unlikely) scenario happens: if there's a corrupt denom trace and that blocks the migration to complete, is there anything we can do or document to help chains?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe it should just panic and then have the chain devs export a snapshot and manually address the situation? What do you think?

Copy link
Contributor Author

@charleenfei charleenfei Jul 18, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i think this is extremely unlikely to happen unless the migration happens from an exported genesis which was manually corrupted -- i think it could make sense rather than panicking the err here to simply pass back to the UpgradeHandler and have the app devs deal with it how they want... does that make sense?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In which case, couldn't we return the error immediately? Instead of continuing to do unnecessary work... I'm assuming that the upgrade handler and RunMigrations will likely bubble up the error and revert any changes so I think it seems unnecessary to continue parsing traces if we encounter an error for some unforeseen reason!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we do stop the parsing and then return the error in the current code, is there another way to return the error that you think would make more sense?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah sorry, I missed the return true underneath here due to the thread of comments!

return true
}
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 iterErr
crodriguezvega marked this conversation as resolved.
Show resolved Hide resolved
}

func equalTraces(dtA, dtB types.DenomTrace) bool {
return dtA.BaseDenom == dtB.BaseDenom && dtA.Path == dtB.Path
}
110 changes: 110 additions & 0 deletions modules/apps/transfer/keeper/migrations_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,110 @@
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() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🥇


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: "1", Path: "transfer/channel-0/gamm/pool",
})
},
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: "", Path: "transfer/channel-149/erc/0x85bcBCd7e79Ec36f4fBBDc54F90C643d921151AA",
})
},
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",
},
},
},
{
// the expected value will change with the implementation of the fix for #1698
"no change: 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: "customport/channel-7/uatom", Path: "transfer/channel-0/transfer/channel-1",
},
},
},
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know that this unlikely to happen in production, but should we add a test case for a denom trace that fails in the validation? If only, at least this will increase code coverage... :)
And maybe a second test, where the first denom trace is converted but the second fails in validation and see if the changes for the first one are rolled back or not?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

see below comment

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree this situation is unlikely. It can still be useful to add a test case to ensure there is an if statement checking that iterErr is non nil (I have seen those missing if statements go unnoticed before)


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)
})
}
}
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
3 changes: 3 additions & 0 deletions modules/apps/transfer/types/trace.go
Original file line number Diff line number Diff line change
Expand Up @@ -71,6 +71,9 @@ func (dt DenomTrace) GetFullDenomPath() string {
if dt.Path == "" {
return dt.BaseDenom
}
if dt.BaseDenom == "" {
return dt.Path
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why was this added? I think in this situation, there should be a panic

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i added it bc in the case that the incorrectly parsed BaseDenom is an empty string for whatever reason, the GetPrefix() method will actually append an additional / onto the end and the BaseDenom will end up looking like ie erc/0x00000000000etc/ at the conclusion of this logic. I can also set a panic, which do you think makes more sense?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we can cleanup a lot of this code when we look into refactoring transfer and improving upon it. For now, I think it'd be best to remove this check and ensure during migrations that the previously set denom trace passes the Validate() call. If it doesn't, we should probably panic

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

but i dont think some of the previous set denom traces would pass the Validate() call... ie something like transfertypes.DenomTrace{ BaseDenom: "1", Path: "transfer/channel-0/gamm/pool", })

wouldnt this make the point of the migration code redundant if we are trying to Validate before setting the new trace?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

previous invalid denom traces aren't possible in the current code as they would fail on packet Validation. The purpose of the migration code is to fix the stored trace info, not validate the denomination. denomTrace.Validate() should pass for all existing denominations

The specific example, "gamm/pool/1", should have failed due to port/channel identifier validation (insufficient length) when being received or sent in a packet

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

got it, that makes sense!

return dt.GetPrefix() + dt.BaseDenom
}

Expand Down