Remove ToHex

[Kixunil]

The ToHex trait was replaced by either simple Display/LowerHex where appropriate or DisplayHex from bitcoin_internals which is faster.

This change replaces the usages and removes the trait.

[tcharding]

ACK 1b09888

[apoelstra]

ACK 1b09888

[benthecarman]

Would love if this was brought back. This makes things really hard to work with, basically need to memorize which things are encoded to hex with .to_string() and which aren't, also need to guarantee that for all future versions. This basically forces us to use the hex crate

[Kixunil]

All types that are naturally displayed as hex are displayed as hex. :) Not so hard. All hashes, public keys... The only one which I think is weird is Script, which displays ASM, but that one does have as_hex method.

And yes, we do guarantee it for those important types, there's no reason to change it.

[benthecarman]

It still makes the code harder to read without that knowledge, we also lost the ability to call .to_hex() on types like [u8; 32]

[Kixunil]

Why not create a strong type? We have hash_newtype! macro for instance. And if you have a 32B object that is neither a public key nor a hash (which I don't remember existing in Bitcoin) then I'm interested to know if we should add it to the library. And if you don't want to wait and don't want hex (which is understandable, there's a reason we don't depend on it) we have the hex-conservative crate which backs our conversions and was released for people who need this.

[apoelstra]

@Kixunil I've gotten complaints from many people about this. It does make things harder to use. To tell whether something is being hex-encoded you now need to infer its type and also know what's "natural" about it, vs using to_hex where you don't need to know either.

The change also forces people to change their code all over the place, and it's not a mechanical change because you need to think about whether to_string is appropriate or if they need to use as_hex and dig out the appropriate trait for it. The resulting code is harder to read, and it makes people resentful because the change really wasn't necessary.

We should just bring the trait back. Nobody has to use it if they don't want to, it can exist alongside the "proper" traits. And we can taboo them within rust-bitcoin projects.

[tcharding]

This one is really quite curly, I flat out agree that the DisplayHex trait is hard/annoying to use, however I also flat out agree that it is more correct.

We have a prelude to make it more ergonomic to import the trait, this is even better than it was when we had ToHex and FromHex

I.e,

use bitcoin::hex::prelude::*

It still makes the code harder to read without that knowledge, we also lost the ability to call .to_hex() on types like [u8; 32]

We implement DisplayHex for various array sizes so you can call the longer but more descriptive to_lower_hex_string.

impl_array_as_hex!(
    1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16, 20, 32, 33, 64, 65, 128, 256, 512, 1024,
    2048, 4096
);

[tcharding]

We should just bring the trait back. Nobody has to use it if they don't want to, it can exist alongside the "proper" traits. And we can taboo them within rust-bitcoin projects.

We'd still have to litter the rust-bitcoin crates with code supporting it, I'm not super keen on this.

[Kixunil]

OK, if people really need to be explicit about things being hex, I suggest adding inherent methods instead of a trait. They obviate the need to import anything which I previously found annoying (part of the motivation for removing it). But rather than to_hex it should be to_hex_string and as_hex to nudge people into not allocating strings just to display them.

There's one thing I find weird though: reversed hashes. Should they be still reversed? Both options feel wrong TBH.

[apoelstra]

We have a prelude to make it more ergonomic to import the trait, this is even better than it was when we had ToHex and FromHex

I don't see how this is better. It looks significantly worse. Now you have unreadable code and you've hidden its origin behind a wildcard.

We implement DisplayHex for various array sizes so you can call the longer but more descriptive to_lower_hex_string.

Just adding an alias to_hex for to_lower_hex_string would go a long way. And/or we could maybe add a deprecated ToHex trait saying to use DisplayHex instead which simply passes through to this (and has a blanket impl). Then the majority of existing to_hex code will have a clear migration path.

[Kixunil]

If people have problem migrating specifically because of missing to_hex then I'm OK with adding a deprecated one, even backport it. But otherwise I would prefer not to add deprecated items.

[apoelstra]

Somebody just asked on IRC how to hex-serialize Transaction and I had to look it up because I can't remember how to use this. To hex-serialize a transaction you need to use consensus::serialize_hex, which has been true forever. We need to add a more discoverable method (I think you've suggested using Display, Kix, which we should definitely do.)

I think we should bring to_hex back and not even deprecate it.

[apoelstra]

Actually, what if we added serialize(&T) -> Vec<u8> and serialize_hex(&T) -> String to the Encodable trait? Maybe we can even create an efficient as_hex() method which returns a hex-encoding Display object. This would cover transactions, blocks and hashes, which is probably half the stuff people want to hex-encode. (The other half are signatures and pubkeys, which I think should implement Encodable, but we can debate that elsewhere.)

This would still be a clean/correct API and also show up when people search hex on the docs page for these various types.

I'd still like to add a deprecated to_hex/ToHex so peoples' existing code wouldn't break without a friendly upgrade path. But I think we can drop it after a version or two, if we had this extended Encodable API.

[tcharding]

That sounds like a good idea. I can have a play with it when I'm back off break.

[Kixunil]

Actually, what if we added serialize(&T) -> Vec<u8> and serialize_hex(&T) -> String to the Encodable trait?

Yeah, I was wondering why they're not methods before and IIRC there was some reason actually but I can't remember. We'll see when we try. :) I thought I added it in push_decode but don't see it now.

But for vec one I'd prefer it to mention vec. And probably mention consensus. consensus_to_hex, consensus_to_vec or something.

This would still be a clean/correct API and also show up when people search hex on the docs page for these various types.

Yes.

Also Transaction is serialized to hex so often I'd be fine with adding an inherent method.