Skip to content
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

Improvements to script methods related to Taproot #721

Merged
merged 2 commits into from Dec 15, 2021
Merged

Improvements to script methods related to Taproot #721

merged 2 commits into from Dec 15, 2021

Conversation

dr-orlovsky
Copy link
Collaborator

Extraction of a portion from #696 which can be done without changes in rust-secp256k1

src/util/address.rs Outdated Show resolved Hide resolved
@sanket1729
Copy link
Member

Concept ACK for the first commit. I think the second commit should be done cleanly after rust-secp256k1 release.

fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
fmt::LowerHex::fmt(&self.0, f)
}
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any reason not to implement upper hex?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It was absent since inner type does not implement it as well - and doing it should be a case of other PR. Here we just need to get the stuff needed for Taproot :)

src/util/schnorr.rs Outdated Show resolved Hide resolved
src/util/schnorr.rs Outdated Show resolved Hide resolved
@dr-orlovsky
Copy link
Collaborator Author

@sanket1729 sorry it seemed I hd removed the second commit, but i had got its way through. Will remove once again this evening.

@dr-orlovsky
Copy link
Collaborator Author

@sanket1729 sorry it seemed I had removed the second commit, but i had got its way through. Will remove once again this evening.

Kixunil
Kixunil previously approved these changes Dec 2, 2021
Copy link
Collaborator

@Kixunil Kixunil left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK 4b9b59f

@dr-orlovsky
Copy link
Collaborator Author

@sanket1729 second commit is removed

@dr-orlovsky dr-orlovsky added the trivial Obvious, easy and quick to review (few lines or doc-only...) label Dec 2, 2021
@Kixunil Kixunil added the one ack PRs that have one ACK, so one more can progress them label Dec 2, 2021
@RCasatta
Copy link
Collaborator

RCasatta commented Dec 10, 2021

utACK 4b9b59f but needs rebase

@RCasatta RCasatta added the waiting for author This can only progress if the author responds to a request. label Dec 10, 2021
@sanket1729
Copy link
Member

utACK, but needs rebase.

@dr-orlovsky
Copy link
Collaborator Author

Rebased. CI fails on fuzzing due to some network error unrelated to the PR.

Copy link
Collaborator

@Kixunil Kixunil left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK d0a87be

@Kixunil Kixunil removed the waiting for author This can only progress if the author responds to a request. label Dec 14, 2021
Copy link
Member

@sanket1729 sanket1729 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK d0a87be

@@ -97,4 +96,11 @@ impl TweakedPublicKey {
&self.0
}

/// Serialize the key as a byte-encoded pair of values. In compressed form
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: Would have liked to have a sentence about 32 byte xonly keys instead of compressed keys.

@sanket1729 sanket1729 merged commit d09ef6f into rust-bitcoin:master Dec 15, 2021
@dr-orlovsky dr-orlovsky added this to Done in Taproot Jan 7, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
one ack PRs that have one ACK, so one more can progress them trivial Obvious, easy and quick to review (few lines or doc-only...)
Projects
Development

Successfully merging this pull request may close these issues.

None yet

5 participants