Change `FromHex` trait to have associated type Error instead of hex::Error

[sanket1729]

The hex::Error only has limited enums that make it difficult to implement the trait outside of this crate. I think it would be cleaner to have an associated Error type like FromStr.

This will be a breaking change that will impact all implementations of this trait. Looking for some feedback before implementation.

[apoelstra]

I think for the specific issue you're running into (wanting to reject an all-zeroes value) it might make more sense to just add a OutOfRange or InvalidValue variant to hex::Error.

Using an associated error type puts more of a burden downstream to get all the types to line up, and we'd need to require some sort of trait/compatibilty with our existing error returns.

[stevenroose]

Yeah or a Custom(Box<fmt::Display>) or Custom(&'static str) or something, like serde has.

[Kixunil]

Could also do something like this:

use core::convert::Infallible;

trait FromHex: FromHexRestricted<InvalidValueError=Infallible> {
// same as current FromHex
}

// please suggest a better name
pub enum MaybeInvalidValue<E> {
    InvalidValue(E),
    // Could also duplicate the single variant to make some things easier
    Encoding(HexError),
}

trait FromHexRestricted {
    type InvalidValueError;

    // same API as FromHex, except the error is MaybeInvalidValue<Self::InvalidValueError>
}

impl<T: FromHex> FromHexRestricted for T {
    type Error = Infallible;

    // just call the other trait and map error to MaybeInvalidValue::Encoding
}

• Fully backwards-compatible thanks to implications.
• Crates can loosen the trait bounds by replacing FromHex with FromHexRestricted at any time, old code still works because of implication, the errors just need to be handled differently after change
• Maybe there are potential extension traits to make errors easier

Or a lighter version that breaks exhaustive matching, is a bit annoying regarding conversions but involves one less type:

enum Error<InvalidValue = Infallible> {
    InvalidLength(usize, usize), // or if we're already breaking this InvalidLength { expected: usize, provided: usize, }
    InvalidValue(InvalidValue),
}

[Kixunil]

I was thinking about this and came to conclusion we should probably remove FromHex trait:

• Things that are always parsed as hex should have FromStr instead.
• Things that may be parsed as hex but not always are integers and those are not implemented by bitcoin_hashes.

This is related to #169

[apoelstra]

We use the impl on Vec<u8> all the time, and we cannot impl FromStr on Vec<u8>.

[Kixunil]

From what I've seen it's mainly (only?) used in tests, where hex!() may work better. But anyway HexIterator::new(x)?.collect()? should work. Possibly having StrExt trait on str with parse_hex(&self) -> Vec<u8> may be more convenient.

[Kixunil]

Or to put it differently, FromHex is essentially FromIterator<Result<u8, hex::Error>> so if we want convenient FromHex we may change it to this:

trait StrExt: AsRef<str> {
    fn parse_hex<T, E>(&self) -> Result<T, E> where Result<T, E>: FromIterator<Result<u8, hex::Error>>, E: From<hex::Error> {
        HexIterator::new(self.as_ref())?.collect()
    }
}

impl<T: AsRef<str>> StrExt for T {}

[apoelstra]

I like it :)

[apoelstra]

Closing. This crate is moving to rust-bitcoin and we have a number of hex discussions over there.