Improve Handler interface

[ethanfrey]

Summary

With #4821, I provided a nice error library with stacktrace support for cosmos-sdk. This addressed #4708 to allow developers to get stack traces on test failures, if they use the new errors.

However, once we hit the Handler interface, we can no longer use the error interface, but rather fit it into a sdk.Result: type Handler func(ctx Context, msg Msg) Result. This encourages many functions to return sdk.Error, then return err.Result() at the Handler interface. Which discourages use of richer error structs.

Problem Definition

Passing a consistent, stacktrace-enriched error class allows much better test-ability at all layers, especially at Handler-level or app-level integration tests. Let's make these errors first class citizens.

I have discussed this with @marbar3778 and he liked these changes. I would like to get more opinion from the other devs. I am happy for feedback here early on, and if you like the goal, but want a smoother migration path, I can provide a proposal for backwards compatibility here as well.

Proposal

I propose a few steps, some breaking, some non-breaking.

1. Add sdk.ResultFromError(err error). If err implements sdk.Error, then we call the Result method on it. If not, then we call types/errors.ABCIInfo() to get the info and generate an sdk.Result from that. We can standardize on this usage, and allow an incremental migration from existing error package

2. Change type Handler func(ctx Context, msg Msg) Result to type Handler func(ctx Context, msg Msg) (*Result, error). And remove Code and Codespace from Result. Result should only be returned on success, error on failure. runTx/runMsg in BaseApp can construct an ABCIResult properly on success or failure. In practice, this may be done in multiple steps to be less breaking, but should be after v0.36 in any case. (Note that we can still use sdk.Error and just return it without .Result() as a first step, only changing the error creation piece by piece through the modules that desire it)

Advantages:

• Pass go error interface not ABCI-like Result struct to allow stacktrace info to flow over all levels of the code
• Allow go-standard error checking rather than sdk-specific code
• Encourage go-standard extensions using (*Result error) design rather than returning sdk.Result or sdk.Error down into the Keeper and below.

Current:

res := h.keeper.DoStuff()
if !res.IsOK() {
  return res
}

Proposed:

res, err := h.keeper.DoStuff()
if err != nil {
  return nil, err
}

---

For Admin Use

• Not duplicate issue
• Appropriate labels applied
• Appropriate contributors tagged
• Contributor assigned/self-assigned

[alexanderbez]

I'm generally in favor of these improvements 👍

[tac0turtle]

Is there an update to this issue? Would love to see this incorporated into the next breaking release

[alexanderbez]

@marbar3778, not really. Would like for someone to start taking a stab at this...it'll be a pretty hefty PR from what I can imagine though.

[ethanfrey]

There is always a way to make this a gentle, non-breaking change.
Just splitting the work over multiple PRs.

• Define a new type: type MsgHandler func(ctx Context, msg Msg) (*Result, error)
• Add a wrapper func ToMsg(h Handler) MsgHandler that will recover an error (minus stack trace) from "legacy" Handlers
• Update baseapp.Router to handle this:
  • Now store routes map[string]sdk.MsgHandler
  • Add a new method to baseapp.Router: func (rtr *router) AddMsgRoute(path string, h sdk.MsgHandler) sdk.Router to add those directly
  • AddRoute will now call ToMsg(h) and then dispatch to AddMsgRoute (or other name you prefer)
  • func Router.Route() sdk.MsgHandler returns new handler
• Code in baseapp, upstream of router will use the new error interface

This lets any new module take advantage of the new interface, and any legacy handlers keep working. Once that is done, the x package can be migrated piece by piece. This also gives external teams time to migrate their handlers.

After a few releases, support for the Handler interface can be dropped, and just use MsgHandler.

Or course the names above may not be so great, please change them. But the process is a tried and true way of gently upgrading by deprecating but not removing functionality.

[alexanderbez]

That line of reasoning seems fair at initial glance. Any chance you can take this one @ethanfrey? If not, I can do it.

[ethanfrey]

I would be happy to start a PR with most of those points. But I prefer not to touch baseapp. I would leave this to you to finish on that PR if that is okay with you:

Code in baseapp, upstream of router will use the new error interface

And of course, actually migrating the modules... I don't volunteer for that. But that is not urgent. this PR would just be to allow the interfaces.

[tac0turtle]

would recommend moving this into 0.38 milestone, tendermint will be removing cmn.Error in the next major release (0.33), and upgrade TM in the sdk will be blocked on this work or at least moving off cmn.Error

[ethanfrey]

I would like one sdk release where both the new error format and the old (sdk.Error off cmn.Error) code work. So one can upgrade there without breaking changes, and then migrate the app on that dependency, before making the breaking upgrade.

In general, I recommend to leave 1 or 2 releases where new and old versions both work before removing support for the old version, for nice upgradeability for everyone downstream.

That said, I will look at making a PR to make the error way usable

[alexanderbez]

For the record, we already have ResultFromError, in fact, I think you added that function @ethanfrey. I've gone ahead and started with #5072. I believe we'll keep the Result object as-is for now.