feat(pkg/scale): add use of pkg/error Wrap for error handling

[edwardmack]

Changes

• When preparing to address issue pkg/scale: define sentinel errors and add error wrapping #2631 I found this article regarding error handling https://dave.cheney.net/2016/04/27/dont-just-check-errors-handle-them-gracefully, which suggests that the use of Sentinel errors should be avoided for a number of reasons, so I decided to follow his advice and address this issue using github.com/pkg/errors package.

Tests

go test github.com/ChainSafe/gossamer/pkg/scale

Issues

will close #2631

Primary Reviewer

@timwu20
@qdm12

[qdm12]

Nice 👍

Mostly tiny nits you can batch commit 😉

[timwu20]

I'm not sure what value we're adding in this PR by adding these error types to the public interface. Dave Cheney always talks about keeping your errors opaque, and I don't see anywhere in the Gossamer codebase where we need to distinguish what type of error is being returned the scale package.

[qdm12]

@timwu20 pkg/scale is the most stable exported package (afaik) so some users other than Gossamer might need it to handle the error programmatically. And there is such usage in Gossamer, for example in the runtime Version() call where we check for the EOF string, where really we should use errors.Is (and also in some trie code, I'm using scale error strings to wrap a parent error which feels kinda wrong).

Dave Cheney hasn't written anything on errors using the Go 1.13 syntax, so I wouldn't pay too much attention to his say since it was a different story before 1.13.

And there are few custom error packages such as pkg/errors, but I would rather use the standard library for simplicity and also since we intend to make it a public Go library. You can check all big Go repos use go 1.13 error wrapping (moby, prometheus, etc.)

[timwu20]

And there is such usage in Gossamer, for example in the runtime Version() call where we check for the EOF string, where really we should use errors.Is (and also in some trie code, I'm using scale error strings to wrap a parent error which feels kinda wrong).

Can you link this code? Do you handle things differently whether it's an error produced by scale vs a passed along error like io.EOF.

Dave Cheney hasn't written anything on errors using the Go 1.13 syntax, so I wouldn't pay too much attention to his say since it was a different story before 1.13.

What has changed? The added his wrapping to fmt.Sprintf and errors.Is and errors.As. The arguments of whether or not keep your errors opaque are still valid.

[qdm12]

gossamer/lib/runtime/wasmer/exports.go

Line 45 in d3e0432

if strings.Contains(err.Error(), "EOF") {

https://go.dev/blog/go1.13-errors

The arguments of whether or not keep your errors opaque are still valid.

What do you mean by opaque?

The problem with using only error strings is that you would break compatibility about handling the error programmatically whenever one of the wrapping context is changed. It's also not really robust (using strings.HasSuffix etc.).
The alternative to use fmt.Errorf + errors.Is is better and allows you change even the bottom wrapped error string without breaking compatibility (as long as you don't rename the sentinel error variable name).

[qdm12]

Let's figure out how we want to handle errors on ChainSafe/engineering-handbook#62 before moving forward here 😉

[qdm12]

Note this PR should make some error wrapping code pretty instead of ugly in #2673

[edwardmack]

Let's figure out how we want to handle errors on ChainSafe/engineering-handbook#62 before moving forward here wink

That makes sense, what steps should we take to address updating the handbook?

[qdm12]

what steps should we take to address updating the handbook?

You can just review the PR I'd say and complain/question things there.

To be fair I noticed a few well known Go repositories using just simple strings for errors without sentinel errors. So I'm quite shared between:

1. Have a smaller package Go API with no sentinel errors (credits to @timwu20 for mentioning that aspect). But then developers might just end checking the error string programmatically (in tests and/or prod code) since there is no sentinel error to use with errors.Is
2. Have exported sentinel errors (at the bottom only, where the error gets created) so developers can use them with errors.Is. They could use string checking still, but that's on them not using the errors.Is way. But that adds some exported errors to the Go API that may or may not be needed. I'd still prefer that point, but not by much much either.

[timwu20]

@edwardmack I think you force pushed the wrong branch on to this. This is your PopChannel stuff now.

[edwardmack]

@edwardmack I think you force pushed the wrong branch on to this. This is your PopChannel stuff now.

@timwu20 yes, I've force-pushed the correct branch.

[qdm12]

Nice 💯

Made a bunch of commit suggestions you can batch and commit here 😉

[github-actions]

🎉 This PR is included in version 0.7.0 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀