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

Add slash migration guide #1518

Merged
Changes from 1 commit
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
113 changes: 113 additions & 0 deletions docs/migrations/support-slashed-denoms.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,113 @@
# Migrating from Unsupported Base Denom Slashes to Supporting Slashed BaseDenoms

This document is intended to highlight significant changes which may require more information than presented in the CHANGELOG.
Any changes that must be done by a user of ibc-go should be documented here.

There are four sections based on the four potential user groups of this document:
- Chains
- IBC Apps
- Relayers
- IBC Light Clients

**Note:** ibc-go supports golang semantic versioning and therefore all imports must be updated to bump the version number on major releases.
```go
github.com/cosmos/ibc-go/v3 -> github.com/cosmos/ibc-go/v4
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe we should remove this part to avoid confusion...

Copy link
Member

Choose a reason for hiding this comment

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

Maybe a asterisk note with special case for this release. I think this is part of the migrations template right?

```

This document is necessary when chains are upgrading from a version that does not support slashed base denoms (e.g. v3.0.0) to a version that does (e.g. v3.1.0).
Copy link
Contributor

Choose a reason for hiding this comment

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

q: shouldn't this be v4.0.0 if you are bumping the go version?

Copy link
Member Author

Choose a reason for hiding this comment

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

I believe the plan is to do a minor release and backport to v1 and v2. cc: @crodriguezvega

Copy link
Member Author

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, the idea is to release this fix in v3.1.0, but also as a minor backport in v2.3.0 and v1.5.0, so that chains not yet on v3 can still benefit from the fix. I think this is the fastest way to get this fix out.

Alternatively, we could release this fix only with v4.0.0. That would probably make it easier for us to communicate this fix, because we can add this migration guide to the v3-to-v4 migration guide, and all chains migrating to v4.0.0 will need to run this migration. But v4.0.0 needs to come out with fee middleware and it could still take easily another 3 weeks before we cut the final release. So the viability of this option depends on how soon Evmos needs to have this fix.

Having a quick look at mintscan and it looks like most of the chains are still on v2, so if we release this only on v4 and those chains would like to benefit from the fix, then we are forcing them to run the migrations to upgrade to v3 and v4, which might be a bit too much to ask?

Does Evmos expect their slashed-denom tokens to be transferred to a particular set of chains or they could just go to any chain? Because if most likely the receiving chains of those tokens are going to be a handful of chains, maybe we could coordinate with an upgrade to v4?

@fedekunze @AdityaSripal what of these two options you think it's best?

Copy link

@danburck danburck Jun 10, 2022

Choose a reason for hiding this comment

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

@crodriguezvega The idea is that slashed-denom tokens are supported on any cosmos chain and that there is no specific set of chains.

As this functionality is blocking the adoption of our x/erc20 module, it would be great to go for the first option and release it in v3.1.0 as it is the faster option and requires less coordination.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for the feedback, @danburck! Yes, I think that it's the option that makes most sense. I will try to release is asap!


If a chain receives a slashed denom 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: "testcoin/testcoin", BaseDenom: "testcoin"`
Copy link
Contributor

Choose a reason for hiding this comment

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

The trace information stored will actually be transfer/{channel-id}/testcoin/testcoin.


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

To do so, chain binaries should include a migration script that will run when the chain upgrades from not supporting slashed base denominations to supporting slashed base denominations.

## Chains

### Transfer

The transfer module will now support slashes in base denoms, so we must iterate over current traces to check if any of them are incorrectly formed and correct the trace information.

### Upgrade Propsoal
AdityaSripal marked this conversation as resolved.
Show resolved Hide resolved

```go
app.UpgradeKeeper.SetUpgradeHandler("v3.1.0",
AdityaSripal marked this conversation as resolved.
Show resolved Hide resolved
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 []transfertypes.DenomTrace

transferKeeper.IterateDenomTraces(ctx,
func(dt transfertypes.DenomTrace) bool {
// check if the new way of splitting FullDenom
// into Trace and BaseDenom is the same as the current
// DenomTrace.
// If it isn't then store the new DenomTrace in the list of new traces.
newTrace := transfertypes.ParseDenomTrace(dt.GetFullDenomPath())

if !reflect.DeepEqual(newTrace, dt) {
append(newTraces, newTrace)
}
})

// replace the outdated traces with the new trace information
for _, nt := range newTraces {
transferKeeper.SetDenomTrace(ctx, nt)
}
Copy link
Member

Choose a reason for hiding this comment

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

Is it worth adding a call to Validate on new traces?

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe we could add it in if !reflect.DeepEqual(newTrace, dt) as an extra condition. But if the validation fails, should we keep then the existing denom trace?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, I think we can add the Validate on that if condition and if it doesn't pass, then we just leave that denom trace as it is...

}
)
```

This is only necessary if there are DenomTraces 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 slashed denominations runs this code for safety.

#### Add `StoreUpgrades` for Transfer module

For Transfer it is also necessary to [manually add store upgrades](https://docs.cosmos.network/v0.44/core/upgrade.html#add-storeupgrades-for-new-modules) for the transfer module and then configure the store loader to apply those upgrades in `app.go` if you wish to use the upgrade handler method above.

```go
// Here the upgrade name is just an example
if upgradeInfo.Name == "supportSlashingDenomUpgrade" && !app.UpgradeKeeper.IsSkipHeight(upgradeInfo.Height) {
storeUpgrades := store.StoreUpgrades{
Added: []string{transfertypes.StoreKey}
}

app.SetStoreLoader(upgradetypes.UpgradeStoreLoader(upgradeInfo.Height, &storeUpgrades))
}
```

This ensures that the transfer module's stores are added to the multistore before the migrations begin.
AdityaSripal marked this conversation as resolved.
Show resolved Hide resolved

### Genesis Migration

If the chain chooses to add support for slashes in base denoms via genesis export, then the trace information must be corrected during genesis migration.

The migration code required may look like:

```go
func MigrateGenesis(appState genutiltypes.AppMap, clientCtx client.Context, genDoc tmtypes.GenesisDoc) (genutiltypes.AppMap, error) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This code also needed some adjustments to compile:

func MigrateGenesis(appState genutiltypes.AppMap, clientCtx client.Context, genDoc *tmtypes.GenesisDoc) (genutiltypes.AppMap, error) {
	if appState[ibctransfertypes.ModuleName] != nil {
		transferGenState := &ibctransfertypes.GenesisState{}
		clientCtx.Codec.MustUnmarshalJSON(appState[ibctransfertypes.ModuleName], transferGenState)

		substituteTraces := make([]ibctransfertypes.DenomTrace, len(transferGenState.DenomTraces))
		for i, dt := range transferGenState.DenomTraces {
			// replace all previous traces with the latest trace
			// note most traces will have same value
			newTrace := ibctransfertypes.ParseDenomTrace(dt.GetFullDenomPath())

			substituteTraces[i] = newTrace
		}

		transferGenState.DenomTraces = substituteTraces

		// delete old genesis state
		delete(appState, ibctransfertypes.ModuleName)

		// set new ibc transfer genesis state
		appState[ibctransfertypes.ModuleName] = clientCtx.Codec.MustMarshalJSON(transferGenState)
	}

	return appState, nil
}

Copy link
Contributor

Choose a reason for hiding this comment

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

I haven't been able to test this migration because the app state export panics.

Copy link
Contributor

Choose a reason for hiding this comment

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

I did a trick (I skipped the app state export, but instead I manually modified the genesis.json file to add malformed denom traces) and I was able to test this migration functionality. It also works as expected.

if appState[transfertypes.ModuleName] != nil {
transferGenState := &transfertypes.GenesisState
clientCtx.JSONCodec.MustUnmarshalJSON(appState[transfertypes.ModuleName], transferGenState)

substituteTraces := make([]transfertypes.DenomTrace, len(transferGenState.Traces)
for i, dt := range transferGenState.Traces {
// replace all previous traces with the latest trace
// note most traces will have same value
newTrace := transfertypes.ParseDenomTrace(dt.GetFullDenomPath())

subsituteTraces[i] = newTrace
}

transferGenState.Traces = substituteTraces

// delete old genesis state
delete(appState, transfertypes.ModuleName)

// set new ibc transfer genesis state
appState[transfertypes.ModuleName] = clientCtx.JSONCodec.MustMarshalJSON(transferGenState)
Copy link
Member

Choose a reason for hiding this comment

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

nit: indentation

Suggested change
// delete old genesis state
delete(appState, transfertypes.ModuleName)
// set new ibc transfer genesis state
appState[transfertypes.ModuleName] = clientCtx.JSONCodec.MustMarshalJSON(transferGenState)
// delete old genesis state
delete(appState, transfertypes.ModuleName)
// set new ibc transfer genesis state
appState[transfertypes.ModuleName] = clientCtx.JSONCodec.MustMarshalJSON(transferGenState)

}

return appState, nil
}
```