chore: denom traces migration handler #1680
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| 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" | ||
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 { | ||
|
There was a problem hiding this comment. is the intention that all transfer related migrations will happen here? There was a problem hiding this comment. yes -- the cosmos sdk has all migrations in a separate directory but imo that might be overkill for us. wdyt? There was a problem hiding this comment. 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 { | ||
|
|
||
| // 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 | ||
| } | ||
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| 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", | ||
| }, | ||
| }, | ||
| }, | ||
| } | ||
|
There was a problem hiding this comment. 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... :) There was a problem hiding this comment. see below comment There was a problem hiding this comment. 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) | ||
| }) | ||
| } | ||
| } | ||
|
|
||
| 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()) | ||
| }) | ||
| } | ||
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Where/how is
RunMigrationsdefined? Does this get run at the sdk level as a callback?This one liner is very nice compared to previous implementation btw, nice job!
There was a problem hiding this comment.
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