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

Make enabling slashes on transfers a one-liner #1542

Closed
3 tasks
ValarDragon opened this issue Jun 14, 2022 · 2 comments · Fixed by #1680
Closed
3 tasks

Make enabling slashes on transfers a one-liner #1542

ValarDragon opened this issue Jun 14, 2022 · 2 comments · Fixed by #1680
Assignees
Milestone

Comments

@ValarDragon
Copy link
Contributor

ValarDragon commented Jun 14, 2022

Summary

Lets make the upgrade handler code for chains to upgrade to support upon switching to IBC v3.1.0 a one-liner, that they import from the app-transfer module.

Problem Definition / Proposal

The code for the upgrade is currently including the following in your upgrade struct:

        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)
        }

Seems to me that this would be better being:

ibctransfertypes.UpgradeToSupportSlashes(ctx, app.TransferKeeper)

for chains to all adopt this more quickly. This would save several lines from the upgrade. Or you could make this use migrations on the TransferKeeper directly. This would be preferrred, since then the upgrade would just be a go.mod update!


For Admin Use

  • Not duplicate issue
  • Appropriate labels applied
  • Appropriate contributors tagged/assigned
@ValarDragon
Copy link
Contributor Author

Actually is it viable to make a new patch or minor release, and just bump the transfer consensus version to 2.0 https://github.com/cosmos/ibc-go/blob/main/modules/apps/transfer/module.go, and setup a migration handler? That way chains can update the import in the go.mod, and the rest will 'justwork.TM' :)

You'd do this by bumping consensus version to 2, and setting up a migration handler: https://github.com/cosmos/cosmos-sdk/blob/main/x/bank/module.go#L109-L112

@crodriguezvega
Copy link
Contributor

Thanks for the idea, @ValarDragon! This would be indeed a good improvement for the migrations!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

3 participants