Limit and remove panic usage in the code.

[robert-zaremba]

Summary

There are many (a lot!) places in the SDK code which are susceptible for panics.
The general Go recommendation is to not use panics whenever possible.

Problem Definition

We should avoid panics whenever possible. The Go team deliberately limited the exception mechanism for good reasons: they are not designed to rigorously check code on each step and can leave an app in a wrong state if not managed properly.

From Go FAQ:

Why does Go not have exceptions?

We believe that coupling exceptions to a control structure, as in the try-catch-finally idiom, results in convoluted code. It also tends to encourage programmers to label too many ordinary errors, such as failing to open a file, as exceptional.
Go takes a different approach. For plain error handling, Go's multi-value returns make it easy to report an error without overloading the return value. A canonical error type, coupled with Go's other features, makes error handling pleasant but quite different from that in other languages.

Rob Pike on a "Proposal for an exception-like mechanism"

This is exactly the kind of thing the proposal tries to avoid. Panic and recover are not an exception mechanism as usually defined because the usual approach, which ties exceptions to a control structure, encourages fine-grained exception handling that makes code unreadable in practice. There really is a difference between an error and what we call a panic, and we want that difference to matter. Consider Java, in which opening a file can throw an exception. In my experience few things are less exceptional than failing to open a file, and requiring me to write inside-out code to handle such a quotidian operation feels like a Procrustean imposition.

Proposal

Look through the code and remove panics wherever possible.

• in test code, reuse testing.T.Fail / Fatal methods.
• in SDK code return errors. Search for:
  • panic
  • log.Fatal

Discussions

• RegisterInterfaces registers service Msg type_urls #7671 (comment)
• Re-enable errcheck linter check  #7674 (comment)

Incremental issues

• Refactor store keys for variable-length addresses #8363 (comment)

---

For Admin Use

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

[alexanderbez]

in test code, reuse testing.T.Fail / Fatal methods.

Where do we explicitly panic in tests?! We shouldn't be doing this. I'm not aware of any places where we do this.

in SDK code return errors.

Yes, obviously returning errors as far upstream as possible is ideal, and there are a few places where we panic that we should not (e.g. CLI, client-code, REST handlers maybe, etc...). BUT, the panics found in the modules (i.e. state-machine) are explicit and meant to panic! Those are not to be removed.

[robert-zaremba]

Here is a list of places where we panic in tests: https://pastebin.com/jKmQv3Ug

Here is the list of all places where we panic. It's huge. Mostly in modules, but also in baseapp, codec, crypto (sic!)... https://pastebin.com/TkNEyq9x

[alexanderbez]

Cool, thanks for compiling that. We should definitely address the tests.

[tac0turtle]

closing in favour of #15555