Refactor Error Handling

[alexanderbez]

Removes the legacy types, interfaces and logic surrounding errors (types/errors.go) -- i.e. paying down technical debt.

To Reviewer:

I cannot stress how critical this PR is to the safety of the SDK. To aid in review, the work is split into two PRs.

1. This PR handles the critical and core changes to /types and /baseapp. Reviewers should primarily review changes to BaseApp logic, mainly runTx and runMsgs. The two critical changes (credit @ethanfrey):
   a. runTx: We now return GasInfo, *Result, error, where GasInfo is always returned. The happy path is a non-nil Result where tx execution succeeded and an error is returned for the sad path.
   b. runMsgs: We return now a *Result, error. The happy path is a non-nil Result where message execution succeeded and where an error is returned for the sad path.

2. A subsequent PR handles updates to the modules and clients.

closes: #4844
replaces: #5072

---

For contributor use:

• Targeted PR against correct branch (see CONTRIBUTING.md)
• Linked to Github issue with discussion and accepted design OR link to spec that describes this work.
• Code follows the module structure standards.
• Wrote unit and integration tests
• Updated relevant documentation (docs/) or specification (x/<module>/spec/)
• Added relevant godoc comments.
• Added a relevant changelog entry to the Unreleased section in CHANGELOG.md
• Re-reviewed Files changed in the Github PR explorer

---

For admin use:

• Added appropriate labels to PR (ex. WIP, R4R, docs, etc)
• Reviewers assigned
• Squashed all commits, uses message "Merge pull request #XYZ: [title]" (coding standards)

[alexanderbez]

@ethanfrey this is a revival of the original PR (I did not feel like going through and fixing all the conflicts). I've incorporated your feedback from the original PR for runTx and runMsgs. Do you mind taking another look at baseapp.go when you can spare some time?

[lgtm-com]

This pull request fixes 1 alert when merging a1c1439 into eae10b0 - view on LGTM.com

fixed alerts:

• 1 for Useless assignment to local variable

[alexanderbez]

@ethanfrey @AdityaSripal @fedekunze just to reiterate from my response #5421 (comment), in order to drastically reduce semantic confusion and flow, the following should always hold true:

• A *Result or error is returned from runMsgs, runTx, DeliverTx and CheckTx but never both.
• State transitions only occur if no error is returned on runMsgs.
• Even if a single message fails out of n in runMsgs, a single error is returned (context on successful messages are lost).
  • This is the only break to the current semantics but it makes things much easier to read and handle.

As a result, DeliverTx and CheckTx responses will return ABCI info from the error or the result, but never both. An error if gas issues occurred or message execution failed. Otherwise, a *Result is guaranteed.

[lgtm-com]

This pull request fixes 1 alert when merging e64d87f into 3c82b66 - view on LGTM.com

fixed alerts:

• 1 for Useless assignment to local variable

[codecov]

Codecov Report

Merging #5421 into master will decrease coverage by 0.14%.
The diff coverage is 55.21%.

@@            Coverage Diff             @@
##           master    #5421      +/-   ##
==========================================
- Coverage   54.51%   54.37%   -0.15%     
==========================================
  Files         318      313       -5     
  Lines       19131    18836     -295     
==========================================
- Hits        10430    10242     -188     
+ Misses       7915     7809     -106     
+ Partials      786      785       -1

Impacted Files	Coverage Δ
x/gov/keeper/keeper.go	45.61% <ø> (-0.94%)	⬇️
types/handler.go	0% <ø> (ø)	⬆️
x/mock/types.go	11.76% <0%> (ø)	⬆️
simapp/test_helpers.go	0% <0%> (ø)	⬆️
x/distribution/keeper/proposal_handler.go	0% <0%> (ø)	⬆️
store/iavl/store.go	73.07% <0%> (+0.69%)	⬆️
x/auth/keeper/keeper.go	55.55% <0%> (ø)	⬆️
x/slashing/internal/keeper/unjail.go	0% <0%> (ø)	⬆️
types/errors/abci.go	60% <0%> (-30.7%)	⬇️
x/distribution/keeper/fee_pool.go	0% <0%> (ø)	⬆️
... and 70 more

[tac0turtle]

should there be a followup PR from docs to update docs on this? I think in some places we are copying in code which uses the old way

[alexanderbez]

I updated some docs with respect to Handler and Querier, but I'm certain there are remnants of sdk.Error in docs somewhere. So we should definitely do a sweep through!

/cc @gamarin2