This PR follows the same pattern as used by[0] and suggested in[1] the hex_conservative docs with a wrapper type around the reference to provide the DisplayHex::Display type. I'm not a huge fan of the end result, so if there are any suggestions on other methods to achieve the same result, I would appreciate them.

[0] https://docs.rs/hex-conservative/0.3.1/src/hex_conservative/display.rs.html#207-250
[1] https://docs.rs/hex-conservative/0.3.1/hex_conservative/display/trait.DisplayHex.html#associatedtype.Display

I can't tell whether you're putting hex-conservative 0.3 stuff into the public API. It looks like it because the as_hex and into_iter methods appear in the API files.

Your Display impls also just collect the entire object into a string and then displays it. If this is the approach you want to take then you shouldn't need to use hex-conservative at all.

I'm also not totally confident about implementing Display without FromStr on these types, though I agree that we ought to implement the formatting traits which output hex.

I can't tell whether you're putting hex-conservative 0.3 stuff into the public API

into_iter is used for the fmt_hex_exact! macro in Header, and for the BytesToHexIter in Transaction.
as_hex only exists because of the DisplayHex trait (which is from unstable hex-conservative). I can easily drop that trait implementation entirely, but I specifically included it because you called it out here: #1706 (comment)

Your Display impls also just collect the entire object into a string and then displays it.

I did it that way mainly because it was easy. If we want an alloc-less version which works purely off the BytesToHexIter, then I can do that instead. It will take a bit of care to make it work with formatting arguments though, which is why I went with the current method first.

I'm also not totally confident about implementing Display without FromStr on these types

I can extend this PR to include that trait too if you'd like.

I can easily drop that trait implementation entirely, but I specifically included it because you called it out here: #1706 (comment)

Yeah, sorry, I didn't realize that this would (apparently) leak into the public API.

I wonder if we want to back up and add some facility to consensus_encoding to do these hex displays. It seems like every consensus object will have pretty-much the same considerations when it comes to hex encoding.

At the very least we could make a wrapper type which is private to primitives and which holds an &T where T: Encodable and implement everything on that. Then questions of "what format args should be respected" and "should we try to avoid allocations" can be answered in one place.

Similarly we should do something for FromStr.

I think that as a first approach we should do this in private structs in primitives, and use those to impl Display and FromStr on Header and Transaction as you propose. I wouldn't worry about allocations or about format args. Then in followup PRs we can improve the wrapper types, and then (maybe) we can promote them to public types in consensus_encoding.

I've tried to implement the private wrapper struct.
While the wrapper struct works fine for the display types, I've introduced a public error for the FromStr result. I assume you don't want to introduce a new error type, so I'd appreciate any guidance and/or suggestions on how to solve this.

I also reverted the change to the Header Display implementation, since this private wrapper struct requires alloc, which the old Display implementation didn't.

I've introduced a public error for the FromStr result. I assume you don't want to introduce a new error type, so I'd appreciate any guidance and/or suggestions on how to solve this.

So, I think you will need to introduce a public error type no matter what, because as you observe there are (at least) two cases: a hex decoding error and a Decoder error. But both cases have ugly API questions:

• for the hex error, do we need to wrap a hex-conservative 0.x error? Hopefully not; hopefully the 1.0 API (which will let us decode arrays and vectors, but not ArrayVecs or slices, ugh) is somehow sufficient for this
• for the decoding error, do we make the ParsePrimitiveError type generic? In general users hate this. Or do we embed a Box<dyn Error> or something else that's awful. Or do we just return a nested Result?

do we need to wrap a hex-conservative 0.x error?

hex_conservative 1.0 has the same error types (OddLengthStringError and InvalidCharError) that we can, in theory, use. The problem is in converting from the unstable to the stable versions without losing information that could be used in Display. The HexToBytesIter yields unstable error types. We could just use decode_to_vec from stable hex for now, since it's functionally equivalent to what's there and would yield the stable errors for us.

do we make the ParsePrimitiveError type generic?

I specifically made it generic over the Decodable type because I thought that would the most intuitive way to understand it. A ParsePrimitiveError<T>::Decode is an error while decoding an object of type T. Box is less than ideal because it further pushes us into requiring alloc and will need to be removed eventually anyway if we go alloc-less.
The nested Result, I'm confused about. Any chance you could elaborate on that?

The nested Result, I'm confused about. Any chance you could elaborate on that?

Rather than having ParsePrimitiveError<T>, we have Result<Result<Object, T>, hex::Error>.

This is ugly and potentially confusing for the user who needs to keep track of which error is which (and there is no .transpose two swap them; there is a .transpose method on Results but it's `Option~-specific because Rust has no monads), but

• Result has a huge pile of accessory methods .and_then() .map_err() etc etc that our custom type does not
• Users are used to Result being generic; less so for actual error types

I'm leaning against doing this, but I wanted to remind people that it's an option.

I've sorted out the use of stable hex errors for the ParsePrimitiveError. I'm still using the generic for Decode though because while it's not nice, I don't personally think the nested Result is much better, and neither of us seem to be confident in a specific solution.

That said, since this is going into 1.0, maybe we should sit on this until we really lock down the error type.

And sorry for being so late to the party. Since I'm so late, @apoelstra please override me if you think I'm wrong.

If so I rekon a brain-dead simple implementation would be better even if it requires code duplication.

I had something like that initially, but @apoelstra suggested something like the private struct that we have now, specifically so that we have a single point at which to make decisions about the encoding style. The main problem that I can see is the error type. Even if we do manual implementations on Transaction or Header, we'd still need to have an error type like the ParsePrimitiveError for the FromStr implementations.

hmm, I had a hunch that might have happened. What about we do the same trick we did with hash_types::generic then define two separate error types (one for Transaction, one for Header)? This is an off-the-top-of-my-head idea and may be a wild goose chase. I'd suggest either waiting for @apoelstra to weigh in or only do the work with the that expectation.

Maybe we can do the generic error type, but keep it private, then expose specific opaque error types for Header and Transaction that wrap it?

fwiw ack 38fc6b5, I learned a bunch following along on this one.

Hot. Patch 1 made patch 2 a blast to review.

My vote goes for merge and follow up (if you feel like following up that is).

Sounds good! Can one of you file an issue to deal with the allocations in both encoding and decoding? They should be unnecessary, though undoubtedly they'll both be annoying to deal with (encoding may require finally adding encoding to hex-conservative, while decoding will involve a "decode hex to a buffer then pass to push_decode" loop).

Everything is encapsulated well so that users will not need to know or care about the change.

Rebased on master to fix conflicts, and changed the unaddressed comments from Tobin:

• pub(crate) for HexPrimitive
• hex_write_tx_with_case to hex_write_with_case
• Debug for ParsePrimitiveError Nevermind on this one. It does not build correctly.