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
Return error when constructing pubkey from slice #2576
Conversation
Constructing a pubkey using `PublicKey::from_slice` can fail for reasons other than just incorrect length - we should not be using `expect` but rather returning the error. A purist might argue that we are now returning a nested error type with an unreachable variant: `ParsePublicKeyError::Encoding(FromSliceError::InvalidLength)` Is this acceptable or do we want to further improve this?
Mad props to @Sh0g0-1758 for finding this bug literally days after I introduced it - amazing, that could easily have gotten through to the next release. |
Pull Request Test Coverage Report for Build 8256499696Details
💛 - Coveralls |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ACK 6ecc41d
Personally I think the unreachable variant is fine. It'd be really hard and annoying to restructure this code to avoid it. Plausibly upstream we should add a pair of |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ACK 6ecc41d
Andrew, kick your merge bot for me bro, I can't reach it from here :) |
This PR fixes a bug introduced by me in #2473, and uncovered by #2563 - amazing that it was found so quickly!
Constructing a pubkey using
PublicKey::from_slice
can fail for reasons other than just incorrect length - we should not be usingexpect
but rather returning the error.A purist might argue that we are now returning a nested error type with an unreachable variant:
ParsePublicKeyError::Encoding(FromSliceError::InvalidLength)
Is this acceptable or do we want to further improve this?