Remove panics, add errors

[asalzmann]

Summary
Per this thread: cosmos/cosmos-sdk#11600 panics should only be used when a golang error (program halting) is about to occur, like a divide by zero or OOB error.

This PR removes all panics given we're misusing them (and a panic in an BeginBlocker / epoch will crash the chain - some of these panics were called by functions in epochs!)

Test plan
Integration tests

[sampocs]

Looks good!

One thing I was discussing with Riley yesterday that I think is worth being brought up again - in all these cases we generally override each error and pass a new one.

Ex:

err := someFunctionCall() // some more specific error about what actually went wrong
if err != nil {
   return sdkerrors...  // some new, but less specific error that we actually return (hiding the one above)
}

Should we be actually wrapping the original error instead?

err := someFunctionCall() // some more specific error about what actually went wrong
if err != nil {
   return sdkerrors.SomeNewWrapperError(err) // takes original error as param
}

Not sure if there's a way to wrap the error like that (or maybe I'm misunderstanding and this wrapping happens automatically?).
Either way - sounds like this is common practice in cosmos, but just want to confirm

[sampocs]

nit: probably want to assign this to err instead of sdkerror

[riley-stride]

Looks good!

One thing I was discussing with Riley yesterday that I think is worth being brought up again - in all these cases we generally override each error and pass a new one.

Ex:

err := someFunctionCall() // some more specific error about what actually went wrong
if err != nil {
   return sdkerrors...  // some new, but less specific error that we actually return (hiding the one above)
}

Should we be actually wrapping the original error instead?

err := someFunctionCall() // some more specific error about what actually went wrong
if err != nil {
   return sdkerrors.SomeNewWrapperError(err) // takes original error as param
}

Not sure if there's a way to wrap the error like that (or maybe I'm misunderstanding and this wrapping happens automatically?). Either way - sounds like this is common practice in cosmos, but just want to confirm

Well phrased. I'd vote we wither return err or something like the following, where we have an error type, a descriptive string, as well as err:

return sdkerrors.Wrapf(sdkerrors.ErrInvalidRequest, "Failed to SubmitTxs for %s, %s, %s | err=%v", zone.ConnectionId, zone.ChainId, msgs, err)

[riley-stride]

Looks good to me, thanks for actioning this! Only thing worth looking at before merge IMO is Sam's comment re: error conventions and my reply. Might be quick to reformat all our errors to that convention, but I'm not sure it's the best use of time right now, nor that we want to run with that convention. Wdyt?

[sampocs]

Looks good to me, thanks for actioning this! Only thing worth looking at before merge IMO is Sam's comment re: error conventions and my reply. Might be quick to reformat all our errors to that convention, but I'm not sure it's the best use of time right now, nor that we want to run with that convention. Wdyt?

I think we can progressively fix these as we add unit tests that test the error cases. I'll push up an example soon

[asalzmann]

Let's fix these progressively! Does the following achieve both the goal of passing the low-level error up the stack, as well as surfacing something more meaningful in relation to how the enclosing function is erroring?

func (k Keeper) DoSomethingSpecific {
    err = k.someKeeperFunction(ctx, ...)
    if err != nil {
	    k.Logger(ctx).Info("Failed to DoSomethingSpecific")
	    return sdkerrors.Wrapf(err, "Failed to DoSomethingSpecific")
    }
}

The only advantage of surfacing a custom error that I can think of is that it has an attached error code, which might be easier for external services to parse than an arbitrary string

It does seem like common practice in Cosmos to surface new, custom errors, although I'm not sure why. If I recall correctly, some errors are redacted in the standard cosmos-sdk (we've removed this code in our fork), it's possible the only way to surface errors in the standard sdk is to create new error messages? Not sure

Wdyt?

[sampocs]

Yup, I think that's perfect!

The way I've been thinking about it, is there are some cases where we identify the error ourselves, and there are cases where we receive an error from a function we called. I think custom errors make sense in the first case, but less so in the second.

Ex:

someSpecificConditionWeCheck := ...
if !someSpecificConditionWeCheck {
    return CustomError
}

vs

err := someFunctionCall()
if err := nil {
   return WrappedError(err)
}

[sampocs]

That said, I think most of the cases I've seen are covered with some of the provided sdkerrors out of the box. One I noticed this morning was:

strideEpochTracker, found := k.GetEpochTracker(ctx, epochtypes.STRIDE_EPOCH)
if !found {
	k.Logger(ctx).Error("failed to find epoch")
	return nil, sdkerrors.Wrapf(types.ErrInvalidLengthEpochTracker, "no number for epoch (%s)", epochtypes.STRIDE_EPOCH)
}

Where we could instead do:

strideEpochTracker, found := k.GetEpochTracker(ctx, epochtypes.STRIDE_EPOCH)
if !found {
	k.Logger(ctx).Error("failed to find epoch")
	return nil, sdkerrors.Wrapf(sdkerrors.ErrNotFound, "no number for epoch (%s)", epochtypes.STRIDE_EPOCH)
}

Although I hear ya on the event code. I think if we make the wrapped message specific enough though, it should be easy enough to find? I do think covering up an error makes it very difficult to trace - I noticed that first hand yesterday while writing the unit tests.

[sampocs]

Also (side note) we should make sure we're logging these with an Error status instead of Info

[asalzmann]

Good call, updated Infos to Errors where appropriate