Add guidelines on using sdk errors and module errors

[ryanchristo]

Summary

When to use what sdk errors is not clear and often we default to ErrInvalidRequest. We should have clear guidelines on using sdk errors and module specific errors. This could be included in the contributing guidelines (#1003) or in a separate document.

Problem Definition

Error messages are inconsistent and guidelines on when to use what errors would help with writing better error messages and as a result improve the user experience.

Proposal

Write guidelines on when to use what errors and then open a followup issue to update codebase according to those guidelines.

---

For Admin Use

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

[aaronc]

This should likely get upstreamed to the SDK

[aaronc]

@ryanchristo would you suggest modules don't use ErrInvalidRequest and define custom errors?

Or would it make sense if the SDK has a core set or general purpose errors like invalid request, unauthorized that modules are encouraged to use?

[ryanchristo]

I'm not entirely sure at the moment. I'll need to spend more time reviewing how we are currently using errors and how other sdk projects are using errors and maybe best practices for errors in general. We are sticking with using ErrInvalidRequest in many cases for now given error messages are not API or state machine breaking and we could make improvements following v4.0. It also makes sense to use ErrInvalidRequest when a field is empty, e.g. name cannot be empty: invalid request.

I think general purpose errors defined in the sdk are still nice but more specific module errors could be better utilized. One case in particular that I have found in our case is insufficient funds being used when we should be using insufficient credits.

[ryanchristo]

There are already guidelines in the sdk and it looks like we could do a better job of following them.

https://docs.cosmos.network/main/building-modules/errors.html

Specifically for the case I mentioned above, the sdk actually suggests something like:

    ErrEmptyName      = sdkerrors.Register(ModuleName, 2, "name is empty")

[ryanchristo]

Note to include use of sdkerrors.Wrap in guidelines and to avoid stacking the same error. See https://github.com/regen-network/regen-ledger/pull/1229#discussion_r913141103.

[ryanchristo]

Ref: https://github.com/regen-network/regen-ledger/pull/1317#pullrequestreview-1055837009

Functions that can be used outside the context of a request should not use ErrInvalidRequest but it may make sense to use in basic message validation that makes use of the function.

For example, we have a utility function that validates the format of a batch denom. The utility function that validates the string would return a parse error and the basic message validation that calls the utility function would return an invalid request error that wraps the parse error:

func ValidateBatchDenom(denom string) error {
    if denom == "" {
        return ecocredit.ErrParseFailure.Wrap("batch denom cannot be empty")
    }
    matches := regexBatchDenom.FindStringSubmatch(denom)
    if matches == nil {
        return ecocredit.ErrParseFailure.Wrap("invalid batch denom: expected format A00-000-00000000-00000000-000")
    }
    return nil
}

And when being used within basic message validation:

if err := core.ValidateBatchDenom(credit.BatchDenom); err != nil {
    return sdkerrors.ErrInvalidRequest.Wrap(err)
}

[ryanchristo]

Note, Register, Wrap, and Wrapf are now deprecated in github.com/cosmos/cosmos-sdk/types/errors and we need to use the new cosmossdk.io/errors package when defining sdk errors within our custom modules.

[ryanchristo]

Note, the new orm errors provide additional information about the field name(s) causing the error. In #1383, a quick fix was made that does not make use of the new orm errors but we take these into consideration when writing guidelines, i.e. whether we want to return the errors as is or continue customizing the error messages. For example:

expected: "credits already issued with tx id: 0x64: invalid request"
actual  : "\"regen.ecocredit.v1.OriginTxIndex\":[1 0x64 polygon]: already exists"