Add new hex parse error variant #1584
Conversation
3c5b625 to
a705e76
Compare
bitcoin/src/bip32.rs
Outdated
| @@ -455,6 +455,8 @@ pub enum Error { | |||
| Base58(base58::Error), | |||
| /// Hexadecimal decoding error | |||
| Hex(hex::Error), | |||
| /// `PublicKey` hex should be 66 or 130 digits long. | |||
| InvalidPublicKeyHex(usize), | |||
There was a problem hiding this comment.
Why is this not called InvalidHexLength as below? That one is more clear.
Also, we have this problem everywhere: I think all enum variants should use named fields except for inner errors (e.g. hex::Error in this case) which are obvious enough.
There was a problem hiding this comment.
I put PublicKey in the name to future proof the varian because the 66/130 in the comments and output are pubkey specific.
There was a problem hiding this comment.
Can you explain a bit more what you're future-proofing against?
There was a problem hiding this comment.
I went digging into the code and my brain to try and remember what I was thinking when I wrote both this PR and the comment above. Seems I misspoke in the comment, I believe I chose the name because the error enum in the bip32 module is more general than just parsing keys, where as in thekey module the error enum is only related to keys. Moreover the InvalidHexLength is only returned when parsing pubkeys. I've changed the bip32 error variant to InvalidPublicKeyHexLength to better show that it is the same as key::InvalidHexLength but with pubkey in the variant to disambiguate in like other bip32::Error variants.
There was a problem hiding this comment.
Ah, I think I understand, makes sense.
a705e76 to
024f704
Compare
Recently we used an error type that holds only one expected hex string length when parsing but for `PublicKey`s we have two (66 and 130). Add a new error variant to express the error. Requires adding a variant to `bip32` for the same thing. Fix: rust-bitcoin#1281
024f704 to
877f9af
Compare
| @@ -473,6 +475,7 @@ impl fmt::Display for Error { | |||
| write!(f, "encoded extended key data has wrong length {}", len), | |||
| Error::Base58(ref e) => write_err!(f, "base58 encoding error"; e), | |||
| Error::Hex(ref e) => write_err!(f, "Hexadecimal decoding error"; e), | |||
| Error::InvalidPublicKeyHexLength(got) => write!(f, "PublicKey hex should be 66 or 130 digits long, got: {}", got), | |||
There was a problem hiding this comment.
I think we should use user friendly descriptions rather than type names in error messages but I guess there are other instances, so we may want to do complete audit and cleanup.
There was a problem hiding this comment.
Since we are on the topic, error messages should not be capitialised, right? I always forget and we have so many of both forms.
There was a problem hiding this comment.
Noted however not to use types in error messages. Will leave it as it is for now since upcoming error work is imminent.
There was a problem hiding this comment.
error messages should not be capitialised, right?
I think so.
Recently we used an error type that holds only one expected hex string length when parsing but for
PublicKeys we have two (66 and 130). Add a new error variant to express the error. Requires adding a variant tobip32for the same thing.Fix: #1281