-
Notifications
You must be signed in to change notification settings - Fork 169
use hex-conservative 1.0 everywhere for decoding #822
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
|
Drafting. Might as well just wait for the real 1.0. |
fb0adc3 to
bea8cde
Compare
|
Published :) will update this tonight or tomorrow morning. |
bea8cde to
78e8f01
Compare
apoelstra
left a comment
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.
On bea8cde successfully ran local tests
tcharding
left a comment
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.
ACK 78e8f01
|
PR title and git log are both stale - they mention 'rc'. |
| } | ||
| } | ||
| } | ||
| use miniscript::hex; |
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.
Nice!
| let depo_tx: Transaction = deserialize(&Vec::<u8>::from_hex(hex_tx).unwrap()).unwrap(); | ||
| let depo_tx: Transaction = deserialize_hex(hex_tx).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.
This is kind of an unrelated change, bad Andrew no biscuit.
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.
How is it unrelated? We used to have a hex library that has from_hex and now we don't.
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.
oooph - I'm a spastic. I did not even look inside the function call to deserialize only at the function name changes - ouch.
| Vec::<u8>::from_hex("76a91479091972186c449eb1ded22b78e40d009bdf008988ac").unwrap()[..] | ||
| hex::decode_to_vec("76a91479091972186c449eb1ded22b78e40d009bdf008988ac").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.
Out of interest did you find yourself cursing the f***ing hex-conservative devs or were you happy enough during the upgrade?
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.
I was happy enough, but the lack of encoding support is idiotic.
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.
True that.
It turns out we only ever use do decoding of fixed test vectors. It would be nice to have a hex! macro for this. Eventually. In some cases we were able to use inherent methods on bitcoin types -- for example ScriptBuf::from_hex or deserialize_hex.
78e8f01 to
a1fa898
Compare
tcharding
left a comment
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.
ACK a1fa898
|
Going to merge this one -- in general, I don't want to move rust-miniscript to 1.0 dependencies on the main branch because I have been hyping up "Miniscript 13" to people out-of-band and want the next release to be compatible with bdk. But in the case of hex-conservative, the only "1.0 types" that are expose are error types, so it's okay. |
…ywhere for decoding
a1fa898f9f164baee2eca787b06fd89ea10f880d use hex-conservative 1.0 everywhere for decoding (Andrew Poelstra)
Pull request description:
It turns out we only ever use do decoding of fixed test vectors. It would be nice to have a hex! macro for this. Eventually.
In some cases we were able to use inherent methods on bitcoin types -- for example ScriptBuf::from_hex or deserialize_hex.
ACKs for top commit:
tcharding:
ACK a1fa898f9f164baee2eca787b06fd89ea10f880d
Tree-SHA512: 234fb4974e86b6b72d3f0aaefbe7f1ea73cfd90c0152c163d3f0b46e94d50f3e784f5499e00b058736b242b1c0dde39b41d5a2583319bf9c580a0db84dc24237
It turns out we only ever use do decoding of fixed test vectors. It would be nice to have a hex! macro for this. Eventually.
In some cases we were able to use inherent methods on bitcoin types -- for example ScriptBuf::from_hex or deserialize_hex.