Update rust-bitcoin version and general updates #8
Conversation
|
I don't even have a local fork of this crate and it has no dependants listed on crates.io - is this crate used at all? Is the effort worth it? |
|
Also my question :). I must have heard of this because it's in the org, but I can't recall when it moved here or anything about it. |
|
Concept ACK from me for keeping this updated. Not much maintenance is required. |
|
Yeah, I started to use this library in another project I'm working on when I realized it is using a very outdated version of rust-bitcoin. It's not much code so I just went ahead and updated it to the latest. The package is under rust-bitcoin and listed on the rust-bitcoin main organization page which is why I brought it up. I don't think it will take much work to keep it up to date, and even if I'm just using it, it's probably more visible being under rust-bitcoin than hosted under myself. I'm not sure if you all could add me to the repo. |
tcharding
left a comment
tcharding
left a comment
There was a problem hiding this comment.
Please feel free to ignore my review. FWIW I have no way of reviewing this meaningfully because there is too much going on, I'd have to spend a while learning the crate which I don't want to do.
If you split it up into individual patches then I (and others) can offer a more meaningful review.
(Please accept my apology if I'm terse. Its me not you.)
| // along with this software. | ||
| // If not, see <http://creativecommons.org/publicdomain/zero/1.0/>. | ||
|
|
||
| //! # Rust BIP47 Library |
There was a problem hiding this comment.
Why are the crate level docs removed?
| secp256k1::{self, Secp256k1}, | ||
| CompressedPublicKey, Network, NetworkKind, OutPoint, PrivateKey, PublicKey, ScriptBuf, | ||
| Transaction, | ||
| }; |
There was a problem hiding this comment.
Changing the import style is unrelated to the patch. None of the reviewers are likely familiar with this crate to start with so adding noise to the PR is only going to make the review quality worse.
|
|
||
| const PAYMENT_CODE_BIN_LENGTH: usize = 80; | ||
| const LETTER_P: u8 = 0x47; | ||
| const ENCODED_PCODE_PREFIX: [u8; 3] = [0x6a, 0x4c, 0x50]; // OP_RETURN + PUSHDATA1 + 80-byte length |
There was a problem hiding this comment.
Its not obvious what this has to do with upgrading rust-bitcoin?
| ))?), | ||
| None => Err(bip32::Error::InvalidDerivationPathFormat), | ||
| }, | ||
| } |
There was a problem hiding this comment.
I can't immediately tell if this is because of rust-bitcoin or not.
| let sk_scalar = secp256k1::Scalar::from(sk.inner); | ||
| let tweaked = pk.0.mul_tweak(&Secp256k1::new(), &sk_scalar)?; | ||
| let compressed = &tweaked.serialize()[1..]; |
There was a problem hiding this comment.
Its not obvious to me right now why the slice is indexed.
|
|
||
| use bitcoin::hashes::{self, sha512, HashEngine, Hmac}; | ||
| let mut hmac = hashes::hmac::HmacEngine::<sha512::Hash>::new(&encoded_utxo); | ||
| hmac.input(&pk.serialize()[1..]); |
There was a problem hiding this comment.
Oh I guess its from down here?
4 participants
General update from prior rust-bitcoin version to the latest. This required quite a bit of rework.
Fix failing tests.
Update private code from seed to take into account the chain id, as well as give the option for user specified derivation paths.
I'm happy to continue to maintain this repo and give periodic updates as it seems like it has been abandoned (the original author no longer even has a GitHub).
@apoelstra @TheBlueMatt @elichai @sanket1729 @tcharding