Ready for Review: Introduce util::key and deprecate util::privkey #183
Conversation
src/util/key.rs
Outdated
| #[inline] | ||
| pub fn is_compressed(&self) -> bool { | ||
| self.compressed | ||
| Address::p2pkh(self.public_key(secp), self.network) |
There was a problem hiding this comment.
Given the amount of ways you can go from a key to an address, I think it might be better to just have the explicit Address constructors and no getters here at all.
There was a problem hiding this comment.
What do you mean by this? Do you mean that we should eliminate PrivateKey::to_{legacy_,}address?
There was a problem hiding this comment.
Yeah. I mean there is p2pk, p2pkh, p2wpkh. Which one is "address" and which one is "legacy address"? Every time there is a new way to make an address from a key, this will get more confusing. The Address constructors are explicit and very clear.
If you really care about the getters on the key, I'd make them more explicit. Like to_p2pkh_address, ... But I think the constructors are sufficient.
There was a problem hiding this comment.
@stevenroose Actually... I've thought about it a little... It's a little un-ergonomic that we would have the use the constructor like so: Address::p2pk(sk.public_key(&secp), sk.network).
Perhaps we should do PrivateKey::to_p2pkh_address(&secp) etc., just for ergonomics sake.
There was a problem hiding this comment.
I feel like that's kind of a pity. I mean WIF is just an "import format", right? I feel like it would be perfectly acceptable to have wif just as a util/wif.rs thing for compatibility sake if it's actually being annoying wrt other things.
There was a problem hiding this comment.
A bit like how an Address is different from a PublicKey, a Wif could be different from a PrivateKey.
There was a problem hiding this comment.
We could do that. It would mean that you couldn't serialize a PrivateKey without converting to a WIf first, and you couldn't do that without knowing your network (which you'd need to obtain from somewhere)...and you'd then be unable to obtain an address from a PrivateKey without doing the same thing ... so the PrivateKey type becomes very unergonomic.
There was a problem hiding this comment.
Yeah it becomes basically useless because of secp256k1::key::SecretKey. But that's not necessarily a problem. Address::p2pkh(secp_priv.public_key(), network) doesn't seem wrong though (considering context-less priv-pub conversion).
And you really only need WIF strings when you actually want WIF strings, no? bitcoin::utils::wif::to_wif(my_pk, network) also not.
Question: will the secp256k1 SecretKey and PublicKey look different for Schnorr?
|
Can you please break this up into multiple commits? At least,
|
|
Should we future-proof this wrt Schnorr signatures coming in the (hopefully) near future? |
|
We cannot future proof against a future output version with semantics we don't know yet. |
0879209 to
3a7b7e6
Compare
3a7b7e6 to
84b16a0
Compare
|
So I think when we concluded discussions about this, my personal conclusion was that I'd take |
|
@stevenroose you mean take a network argument for encoding to WIF? What about decoding from WIF? |
|
@apoelstra This is ready for review BTW |
|
Decoding could just return a privkey and a network. I don't think the network has any relevance when decoding a WIF. Perhaps as a protection measure, but that's up to the user using the WIF stuff IMO. |
src/util/key.rs
Outdated
| @@ -32,13 +71,13 @@ pub struct PrivateKey { | |||
| /// The network on which this key should be used | |||
| pub network: Network, | |||
| /// The actual ECDSA key | |||
| pub key: SecretKey | |||
| pub key: secp256k1::SecretKey, | |||
| } | |||
|
|
|||
| impl PrivateKey { | |||
| /// Computes the public key as supposed to be used with this secret | |||
There was a problem hiding this comment.
how to grammar
not even sure what this is saying
|
What's the latest on the holdup for this one? |
58aff07 to
d6ecba0
Compare
|
Addressed nits from @instagibbs and rebased. |
d6ecba0 to
4dfab0e
Compare
|
Like I mentioned, I personally feel that an approach like #221 makes more sense. No hard feelings if this gets merged, though :) |
4dfab0e to
966597c
Compare
|
Rebased and ready for review! |
src/util/key.rs
Outdated
|
|
||
| /// Deserialize a public key from a slice | ||
| pub fn from_slice(data: &[u8]) -> Result<PublicKey, encode::Error> { | ||
| let key: secp256k1::PublicKey = secp256k1::PublicKey::from_slice(data) |
There was a problem hiding this comment.
Seems you don't need type ascription there (1.22, 1.29.2, stable)
Same for the io::Result and bool below (or are you doing it for code readability ?)
There was a problem hiding this comment.
I think you were looking at an outdated hunk, it's been changed :-)
There was a problem hiding this comment.
At top on 5fdf9e6, you can still prune bool and io::Result<()> type ascriptions
src/util/key.rs
Outdated
| //! | ||
| //! A private key represents the secret data associated with its proposed use | ||
| //! Structures and methods representing the public/secret data associated with | ||
| //! its proposed use |
There was a problem hiding this comment.
By "proposed use" what is mean there ? Being able to export it as WIF compressed pubkey ? Could be clarified :)
966597c to
5fdf9e6
Compare
|
Fixed doc nit from @ariard |
|
@tamasblummer @ariard Please re-ACK, perhaps https://git-scm.com/docs/git-range-diff would make your life easier too |
src/util/key.rs
Outdated
|
|
||
| /// Deserialize a public key from a slice | ||
| pub fn from_slice(data: &[u8]) -> Result<PublicKey, encode::Error> { | ||
| let key: secp256k1::PublicKey = secp256k1::PublicKey::from_slice(data) |
There was a problem hiding this comment.
At top on 5fdf9e6, you can still prune bool and io::Result<()> type ascriptions
|
@ariard Let's fix the ascription stuff after merge! |
- Rename privkey::PrivKey to privkey::PrivateKey - Remove unnecessary methods for privkey::PrivateKey - Modify tests to work with above changes
- Move util::privkey to util::key - Add PublicKey struct to util::key - Implement de/serialization methods for util::key::PublicKey
- Switch util::address::Payload::Pubkey variant to wrap util::key::PublicKey - Switch util::address::Address::p*k* constructors to use util::key::PublicKey - Fix tests for aforementioned switch - Add convenience methods for util::key::PublicKey to util::key::PrivateKey conversion - Switch BIP143 tests to use util::key::PublicKey
This change also moves the secp256k1::Error wrapper from util::Error to consensus::encode::Error, since we do not use it anywhere else. We can add it back to util::Error once we have instances of secp256k1::Error that are not related to consensus::encode.
a944c7f
5fdf9e6 to
a944c7f
Compare
|
ACK pending travis |
util::key::PublicKey.
@apoelstra @stevenroose I've introduced a few
.clone()'sthat I would like to remove, and am not entirely happy about the copying that happens inutil::key::PublicKey::serialize. Would love your opinions on how to design this better.