Skip to content

Improvements to script methods related to Taproot#721

Merged
sanket1729 merged 2 commits intorust-bitcoin:masterfrom
BP-WG:taproot/script-improvements
Dec 15, 2021
Merged

Improvements to script methods related to Taproot#721
sanket1729 merged 2 commits intorust-bitcoin:masterfrom
BP-WG:taproot/script-improvements

Conversation

@dr-orlovsky
Copy link
Copy Markdown
Collaborator

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

@sanket1729
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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 :)

@dr-orlovsky
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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 4b9b59f2c8b6293e2b660f0e5813e80402c51898

@dr-orlovsky
Copy link
Copy Markdown
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
Copy Markdown
Collaborator

RCasatta commented Dec 10, 2021

utACK 4b9b59f2c8b6293e2b660f0e5813e80402c51898 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
Copy Markdown
Member

utACK, but needs rebase.

@dr-orlovsky
Copy link
Copy Markdown
Collaborator Author

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

Copy link
Copy Markdown
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
Copy Markdown
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

&self.0
}

/// Serialize the key as a byte-encoded pair of values. In compressed form
Copy link
Copy Markdown
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
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

None yet

Development

Successfully merging this pull request may close these issues.

5 participants