P2tr fixes #707
P2tr fixes #707
Conversation
sanket1729
requested review from
dr-orlovsky and
Kixunil
and removed request for
dr-orlovsky
|
I don't know why it says that I removed the review request from @dr-orlovsky. Obviously, I want you to review this :) . |
|
There might be some discussion here since dr-orlovsky previously had (perhaps intentionally?) As an extereme, I don't think passing an |
|
To the best of my knowledge, I'd agree with making exception for If you have some resources that indicate something else I'd be happy to learn. |
|
By asking on rust-lang discord I am getting mixed responses. From https://stackoverflow.com/questions/38065331/should-i-borrow-or-copy-my-small-data-types, it also looks like it is opinion-based. All of secp API is full of references for types that are |
|
There is another argument that references is that they are consistent with other APIs. Maybe I will to write a benchmark |
|
My benchmarks say that it does not matter. They are very close in performance and within error of measurement to say if it really matters or not. However semantically, it makes sense to not consume the data because none of the underlying methods require references. I would 100% support this provided we have underlying APIs that require passing by value. What do you think? |
|
Created a new issue to get new input. People might miss this in the taproot PRs |
sanket1729
force-pushed
the
pt2r_fixes
branch
2 times, most recently
from
4002c78
to
347bd11
Compare
|
In the interest of moving things forward (even though I disagree), I have made this pass by value. |
|
Based on your last comment on #708, I don't care whether it's by reference anymore but let's not turn this into refactoring ping-pong. :) |
src/util/address.rs
Outdated
| @@ -1196,7 +1196,8 @@ mod tests { | |||
| fn p2tr_from_untweaked(){ | |||
| //Test case from BIP-086 | |||
| let internal_key = schnorrsig::PublicKey::from_str("cc8a4bc64d897bddc5fbc2f670f7a8ba0b386779106cf1223c6fc5d7cd6fc115").unwrap(); | |||
| let address = Address::p2tr(Secp256k1::new(), internal_key,None, Network::Bitcoin); | |||
| let secp = Secp256k1::verification_only(); | |||
| let address = Address::p2tr(&secp, internal_key,None, Network::Bitcoin); | |||
There was a problem hiding this comment.
If there's another round of changes, there could be space added before None but please don't bother with it otherwise.
| @@ -1196,7 +1196,8 @@ mod tests { | |||
| fn p2tr_from_untweaked(){ | |||
| //Test case from BIP-086 | |||
| let internal_key = schnorrsig::PublicKey::from_str("cc8a4bc64d897bddc5fbc2f670f7a8ba0b386779106cf1223c6fc5d7cd6fc115").unwrap(); | |||
| let address = Address::p2tr(Secp256k1::new(), internal_key,None, Network::Bitcoin); | |||
| let secp = Secp256k1::verification_only(); | |||
There was a problem hiding this comment.
Probably long term - use some kind of lazy static?
| /// Returns a reference to underlying public key | ||
| pub fn as_inner(&self) -> &PublicKey { | ||
| &self.0 | ||
| } | ||
| } |
There was a problem hiding this comment.
If there's another round a newline would be nice.
sanket1729
force-pushed
the
pt2r_fixes
branch
from
347bd11
to
e4774e7
Compare
|
Sorry for the delay. Fixed the nits |
There was a problem hiding this comment.
utACK e4774e7
Sorry for the delay. Will merge in several minutes
This was my bad for not clearly stating the expected spec #687 . Changed values to references so that we only take ownership where it is required.
This should simplify the #697