rust-bitcoin / rust-bitcoin

Rust Bitcoin library
Creative Commons Zero v1.0 Universal
2.13k stars 691 forks source link

rust-bitcoincore-rpc: `HexBytes` are not very convenient #200

Closed dpc closed 5 years ago

dpc commented 5 years ago

In APIs like sign_raw_transaction taking HexBytes is not very convenient. The caller will either have Transaction or String or Vec<u8> and only the last one is somewhat easily convertible. In none of the APIs we actually return HexBytes, and the fact that it is serializing to hex string, should be an implementation detail.

I think we should have be taking tx: T where T: AsRef<HexBytes> or something like that, so we can impl AsRef<HexString> for Transaction, String and Vec<u8> and &[u8].

We could potentially use the same technique in other places in which it's unclear which type exactly to return. Eg we could return T : From<TxString> and then implement it for String, Vec<u8> and Transaction. The downside of it, is that in some cases, it will be unclear which type is to be returned and user will have to do the turbofish.

Ping @stevenroose

dpc commented 5 years ago

We could also have an opaque struct Tx struct Hash struct Address with into() and from() implemented for stuff like Vec<u8> String and corresponding rust-bitcoin type, and user would just use into manually. These way the actuall conversions between types could sometimes be completely avoided. Eg create_raw_transaction would return Tx that could be passed directly to sign_raw_transaction and there would be no encoding/decoding at any point.

stevenroose commented 5 years ago

I think this issue in on the wrong repo. I agree that HexBytes could be made more ergonomic. The main reason I introduced that was so that users that didn't have a Transaction struct wouldn't have to parse into it for it to be serialized again after.

Let me look into your AsRef idea.

dpc commented 5 years ago

@stevenroose https://github.com/rust-bitcoin/rust-bitcoincore-rpc has issues disabled, so I thought all issues shoud go into "main" repo or something.

stevenroose commented 5 years ago

@dpc ah weird, probably initial settings or so. @apoelstra setup this repo.

dongcarl commented 5 years ago

See: https://github.com/rust-bitcoin/rust-bitcoincore-rpc/issues/19