Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

sdkerrors: make Wrap compatible with Go standard library errors extractors #7846

Closed
1 of 4 tasks
odeke-em opened this issue Nov 6, 2020 · 4 comments
Closed
1 of 4 tasks

Comments

@odeke-em
Copy link
Collaborator

odeke-em commented Nov 6, 2020

Coming here from tip 5903586 and PR #7770 in which I tried to invoke

sdkerrors.Wrap(sdkerrors.ErrTxDecode, err)

where err was an io.UnexpectedEOF, and then later I tried to do

errors.Is(err, io.UnexpectedEOF)

but it fails because sdkerrors stringifies content, thus to perform the check I had to resort to a hack

require.Contains(t, err.Error(), io.ErrUnexpectedEOF.Error())

This issue isn't critical, but just a nice-to-have.


For Admin Use

  • Not duplicate issue
  • Appropriate labels applied
  • Appropriate contributors tagged
  • Contributor assigned/self-assigned
@alexanderbez
Copy link
Contributor

You mean sdkerrors.Wrap(sdkerrors.ErrTxDecode, err.Error()), as you're not wrapping the ErrUnexpectedEOF, you're wrapping ErrTxDecode.

Regardless, I believe @ethanfrey brought over the errors pkg from Weave so we can expose stracktraces? I would be ideal to just use the stdlib wrapping while keeping our ABCI error types.

mergify bot added a commit that referenced this issue Nov 9, 2020
…n an error (#7770)

* unknownproto: check result from protowire.ConsumeFieldValue and return error

Given that protowire.ConsumeFieldValue returns -1 when it encounters an
error, perform a check for n < 0 and return the respectively obtained
error with context about the details.

Fixes an issue identified from a go-fuzz session, thanks to Ethan
Buchman and the IBC auditors from Informal Systems et al.

Fixes #7739.

* Address AlexanderBez's suggestions

* Use require in tests

* Add issue #7846 to TODO

Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
@ethanfrey
Copy link
Contributor

Yeah, I wanted something like pkg/errors - the stacktraces are very useful - that also contained the abci code info. This started from what I did at weave and evolved a bit to make it fit the sdk structure. It can evolve again. However:

sdkerrors.Wrap(sdkerrors.ErrTxDecode, err) is wraping sdkerrors.ErrTxDecode with the error text.

err = sdkerrors.Wrap(io.UnexpectedEOF, "Some useful context") should later match on errors.Is(err, io.UnexpectedEOF)

I haven't touched this in about a year, but my guess is usage error (which needs a better API/docs). The main point is if you wrap std errors, you have no abci code and it all ends up as 1 - Internal (and redacted at the abci boundary). You are supposed to use a proper sdkerror and add the context and compare to that - this will have a proper error code at the abci boundary.

nb: I have no idea if anyone ever uses those error codes in any client, but that is a different story

@ethanfrey
Copy link
Contributor

Also, there is sdkerrors.Is() which does the same compare - the std versions came after this code, and I am happy to adjust them to be compatible if there is a way to do so without breaking the fundamental functionality

@tac0turtle
Copy link
Member

this was done by Aaron in the new errors module.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants