all: remove use of errors.Wrap(nil, ...)

[leighmcculloch]

PR Checklist

PR Structure

• This PR has reasonably narrow scope (if not, break it down into smaller PRs).
• This PR avoids mixing refactoring changes with feature changes (split into two PRs
  otherwise).
• This PR's title starts with name of package that is most changed in the PR, ex.
  services/friendbot, or all or doc if the changes are broad or impact many
  packages.

Thoroughness

• This PR adds tests for the most critical parts of the new functionality or fixes.
• I've updated any docs (developer docs, .md
  files, etc... affected by this change). Take a look in the docs folder for a given service,
  like this one.

Release planning

• I've updated the relevant CHANGELOG (here for Horizon) if
  needed with deprecations, added features, breaking changes, and DB schema changes.
• I've decided if this PR requires a new major/minor version according to
  semver, or if it's mainly a patch change. The PR is targeted at the next
  release branch if it's not a patch change.

Summary

Remove use of errors.Wrap(err, ...) where err may be nil, replacing it with the canonical if err != nil { ... }.

Goal and scope

To improve code clarity, and to set precedence for not using this feature of github.com/pkg/errors.

The errors.Wrap(err, ...) function call has a magical feature that if err is nil the function returns nil, essentially negating the need to check the error. On the surface it feels good to use because it turns this code:

if err != nil {
    return nil, errors.Wrap(err, "it broke")
}
return thing, nil

Into this code:

return thing, errors.Wrap(err, "it broke")

The shortening of the code feels good as the writer, but it can be misleading to a reader. It's very easy to misread code that uses this feature because there are not clear code paths for success and failure.

Code clarity is near the top of the list of things we should value as it will help prevent us from making mistakes; when we don't understand the code we're reading, we make it do things we don't intend to. In the absence for compelling business critical reasons to write code that isn't clear, e.g. for performance critical code, we should use patterns that are easily recognizable.

The if err != nil { ... } while tedious is one of the most well known patterns in Go code. Recent attempts to remove that pattern from Go by adding new language features were met with a lot of push back, including this very popular issue: golang/go#32825.

Removing these few examples won't prevent us from using this feature, but hopefully it sets precedence. And in six months when the current version of Go (1.13) is the oldest version we support we can drop use of github.com/pkg/errors in favor of Go's built-in error wrapping. When we do this, we'll lose this magical feature anyway.

I'd write a check for this to prevent us from writing this code, but it's too hard, and short term removing our use of the package will be better when Go 1.14 is released.

Known limitations & issues

There may be other places where we use this feature, as it's difficult to grep for. I mostly wanted to open this change to create a discussion point, and at least fix the cases I'd seen.

What shouldn't be reviewed

N/A