-
Notifications
You must be signed in to change notification settings - Fork 831
Remove the hex dependency #381
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
37b1ab8
to
865d4ae
Compare
This won't work until rust-bitcoin/bitcoin_hashes#71 will get merged because the |
Now that rust-bitcoin/bitcoin_hashes#71 is merged this should is mergeable. |
865d4ae
to
610174b
Compare
Codecov Report
@@ Coverage Diff @@
## master #381 +/- ##
==========================================
+ Coverage 85.15% 85.66% +0.51%
==========================================
Files 40 40
Lines 7478 7745 +267
==========================================
+ Hits 6368 6635 +267
Misses 1110 1110
Continue to review full report at Codecov.
|
@@ -84,7 +84,7 @@ fn bitcoin_genesis_tx() -> Transaction { | |||
|
|||
// Outputs | |||
let out_script = script::Builder::new() | |||
.push_slice(&Vec::<u8>::from_hex("04678afdb0fe5548271967f1a67130b7105cd6a828e03909a67962e0ea1f61deb649f6bc3f4cef38c4f35504e51ec112de5c384df7ba0b8d578a4c702b6bf11d5f").unwrap()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah that's really cool :) I guess Vec<u8>
is the only vec FromHex
is defined on?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It wouldn't make much sense on any other Vec :) (without getting into endianness lol )
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nicee
We manually implement a bunch of traits on `Miniscript` that pass through to the `node` field (e.g. `PartialEq`). We should do the same for `hash::Hash` instead of deriving it. Found by clippy. Fixes: rust-bitcoin#381
…script 4ac68c9 Implement hash::Hash for Miniscript (Tobin C. Harding) Pull request description: We manually implement a bunch of traits on `Miniscript` that pass through to the `node` field (e.g. `PartialEq`). We should do the same for `hash::Hash` instead of deriving it. Found by clippy. Fixes: rust-bitcoin#381 ACKs for top commit: apoelstra: ACK 4ac68c9 Tree-SHA512: 7920481034bee12fbfdbdfac2f38e9598e25a13a8a89b6b50cc7f63ce528c2de47e959ad073ac1c8f1ab64d4b0e432d264a5caf74ddb656c1a0ffbd8923faf6f
This removes the hex dependecy from the tests.
I had to leave the
hex_hash!
macro because here https://github.com/rust-bitcoin/rust-bitcoin/blob/master/src/util/bip143.rs#L143 I needed to use that instead ofSigHash::from_hex
because the latter requires the hex to be in reverse ordering.FYI currently even non tests will build
hex
because of#[cfg(any(test, feature = "serde"))] extern crate hex;
This is no longer true as of rust-bitcoin/bitcoin_hashes#71(I confued myself. even though the hex crate isn't used in non-test code it is still compiled and linked for that reason)Closes #332