Add slash migration guide

[AdityaSripal]

Description

Adds a migration guide for chain developers that are upgrading from a version that does not support slashed denominations to a version that does. Only necessary if the chain has malformed trace information in its state, but recommended to run for all upgrading chains regardless.

closes: #XXXX

---

Before we can merge this PR, please make sure that all the following items have been
checked off. If any of the checklist items are not applicable, please leave them but
write a little note why.

• Targeted PR against correct branch (see CONTRIBUTING.md)
• Linked to Github issue with discussion and accepted design OR link to spec that describes this work.
• Code follows the module structure standards.
• Wrote unit and integration tests
• Updated relevant documentation (docs/) or specification (x/<module>/spec/)
• Added relevant godoc comments.
• Added a relevant changelog entry to the Unreleased section in CHANGELOG.md
• Re-reviewed Files changed in the Github PR explorer
• Review Codecov Report in the comment section below once CI passes

[fedekunze]

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

[AdityaSripal]

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

[AdityaSripal]

https://github.com/cosmos/ibc-go/blob/main/RELEASES.md

[crodriguezvega]

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?

[danburck]

@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.

[crodriguezvega]

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

[crodriguezvega]

Thank so much, @AdityaSripal, for writing this migration guide!

I can do some testing of these migrations in the next couple of days.

[crodriguezvega]

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?

[crodriguezvega]

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

[crodriguezvega]

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

[damiannolan]

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

[damiannolan]

Nice! Guide looks pretty much good to me, left a nit or two, pretty minor!

[damiannolan]

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)

[damiannolan]

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

[crodriguezvega]

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?

[crodriguezvega]

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...

[damiannolan]

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

[crodriguezvega]

The code needed some adjustments to make it compile in the ibc-go simapp:

app.UpgradeKeeper.SetUpgradeHandler("supportSlashingDenomUpgrade",
	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 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 !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)
		}

		return app.mm.RunMigrations(ctx, app.configurator, fromVM)
	})

[crodriguezvega]

I tested the upgrade handler locally and it works.

Before migration:

denom_traces:
- base_denom: "1"
  path: transfer/channel-0/gamm/poolpoolpool
- base_denom: "1"
  path: transfer/channel-0/transfer/channel-1/gamm/testtesttest
pagination:
  next_key: null
  total: "0"

After migration:

denom_traces:
- base_denom: gamm/poolpoolpool/1
  path: transfer/channel-0
- base_denom: gamm/testtesttest/1
  path: transfer/channel-0/transfer/channel-1
pagination:
  next_key: null
  total: "0"  

[crodriguezvega]

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
}

[crodriguezvega]

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

[crodriguezvega]

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.

[damiannolan]

Suggested change

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.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 upgrade handler or in-place genesis migration described in this document are required when upgrading.

[damiannolan]

Quick question, with the additionof the reflect.DeepEqual check, doesn't this mean that non-malformed traces that were previously in the store will be essentially discard here?
Do we need the reflect.DeepEqual check?

This logic seems to differ from the Genesis Migration below.

[damiannolan]

Ah apologies, we're not setting the whole list and overriding, its looping below and setting individually

[damiannolan]

LGTM!