Propagate/wrap invalid account errors

[ashleyvega]

This PR slightly improves error handling by propagating the ID of invalid account.

It also uses errors.Wrap(err, "info") to show where some errors came from.

[jeremyletang]

Can we avoid using errors.Wrap, and use fmt.Errorf("... %w ...") iunstead?

[ashleyvega]

@jeremyletang I think we should switch to wrapping errors in structs instead of in strings.

See https://dave.cheney.net/2016/04/27/dont-just-check-errors-handle-them-gracefully for details.

[jeremyletang]

@jeremyletang I think we should switch to wrapping errors in structs instead of in strings.

See https://dave.cheney.net/2016/04/27/dont-just-check-errors-handle-them-gracefully for details.

I think we should use the %w flag, which build errors that can be wrapped and unwrapped with std library. https://golang.org/pkg/fmt/#Errorf, and that we can use with errors.As / errors.Is.

[ashleyvega]

$ find -name vendor -prune -o -name '*.go' -print | xargs grep errors.Wrap |wc -l
60

$ find -name vendor -prune -o -name '*.go' -print | xargs grep -E 'fmt.Errorf.*?%w' |wc -l
31

$ find -name vendor -prune -o -name '*.go' -print | xargs grep fmt.Errorf |wc -l
462

$ find -name vendor -prune -o -name '*.go' -print | xargs grep 'return.*Err[A-Z]' |wc -l
510

There is already a mixture of error reporting going on in the app atm. Perhaps the debate on the One Correct Way can be postponed for now.

[jeremyletang]

$ find -name vendor -prune -o -name '*.go' -print | xargs grep errors.Wrap |wc -l
60

$ find -name vendor -prune -o -name '*.go' -print | xargs grep -E 'fmt.Errorf.*?%w' |wc -l
31

$ find -name vendor -prune -o -name '*.go' -print | xargs grep fmt.Errorf |wc -l
462

$ find -name vendor -prune -o -name '*.go' -print | xargs grep 'return.*Err[A-Z]' |wc -l
510

There is already a mixture of error reporting going on in the app atm. Perhaps the debate on the One Correct Way can be postponed for now.

Well ultimately they are all valid ones, just the 2 first ones serves the same purpose. And we'll go with the second one which provides same features from the standard library. So let's just not add more work for the future us at replacing errros.Wrap maybe?

[EVODelavega]

We currently have a mix of error reporting stuff because errors.Wrap was added in version 1.13. Some errors, mainly the client facing ones are declared as variables (hence return Err....). That's perfectly valid/fine. Some errors are constructed on the fly (fmt.Errorf). There's no definitive way to return errors to be discussed. Wrapping errors in a struct is kind of pointless, an error is just any type with the interface Error() string. A struct wouldn't add any more valuable information unless we manually set it. Using %w or other format specifiers are now built-in, so going forwards, we should be using that, rather than our own structs we have to maintain.

[pennyandrews]

Hey @jeremyletang @ashleyvega @EVODelavega, looks like an interesting debate here and one that would be a good candidate to discuss and agree a standard approach as a team. For right now, we need to get this work merged (!) and as it seems there is already a mix of approaches I would suggest going for the one that is already done rather than changing it now, to get it through and then I propose we use the retrospective time tomorrow afternoon to see if there is a standard approach we could all get onboard with going forward. Sound OK?