Skip to content

Require taproot tree depth argument always to be u8#925

Merged
sanket1729 merged 1 commit intorust-bitcoin:masterfrom
BP-WG:taproot/depth
Apr 1, 2022
Merged

Require taproot tree depth argument always to be u8#925
sanket1729 merged 1 commit intorust-bitcoin:masterfrom
BP-WG:taproot/depth

Conversation

@dr-orlovsky
Copy link
Copy Markdown
Collaborator

@dr-orlovsky dr-orlovsky commented Mar 31, 2022

There is an inconsistency in depth type: some places uses u8 (which is reasonable, since the depth can't be >127), others use usize. This PR makes API consistent.

I think this should be a RC fix, since it affects newly introduced API in this release and it will be bad to make a breaking changes the next version after its introduction.

@dr-orlovsky dr-orlovsky added this to the 0.28.0 milestone Mar 31, 2022
@dr-orlovsky dr-orlovsky mentioned this pull request Mar 31, 2022
Closed
@tcharding
Copy link
Copy Markdown
Member

tcharding commented Mar 31, 2022

ACK e3f173e

@tcharding
Copy link
Copy Markdown
Member

tcharding commented Mar 31, 2022

Hey @sanket1729, I know you pointed me already to bitcoin-maintainer-tools for acking PRs but I'm not finding how to do it. If you get a sec can you tell me like I'm 5 please. I used gh pr comment 925 --body "ACK e3f173e" to ack this but it does not get picked up as an 'approve'. Thanks

@sanket1729
Copy link
Copy Markdown
Member

sanket1729 commented Mar 31, 2022

@tcharding , I only use the github-merge.py script there. I use the GitHub UI to manually ACK the PR. Maybe others use the pr comment tool here.

The point of the tool is that it checks the ACKs commit hash against the PR tip, allows to running of tests on the merge commit. It also serves as a record for assigning blame (:P) to reviewers as they are saved in the commit message.

sanket1729
sanket1729 approved these changes Mar 31, 2022
View reviewed changes
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 e3f173e

@dr-orlovsky
Copy link
Copy Markdown
Collaborator Author

dr-orlovsky commented Apr 1, 2022

I assume @tcharding has troubles with github merge tool, so not to merge my own PR myself, can I pls ask you @sanket1729 to do the merge?

@sanket1729 sanket1729 merged commit c22f40f into rust-bitcoin:master Apr 1, 2022
@apoelstra
Copy link
Copy Markdown
Member

apoelstra commented Apr 1, 2022

I also use the github UI to ACK PRs. If somebody knows how to do this with gh pr (commenting is insufficient) that would be amazing. Especially annoying since you have to click to the "Changes" tab to get the "Approve" button, which is the slowest tab to load.

@tcharding
Copy link
Copy Markdown
Member

tcharding commented Apr 2, 2022

I assume @tcharding has troubles with github merge tool

I did not think I was in the role of merging PRs yet, I've barely started ack'ing :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@sanket1729 sanket1729 sanket1729 approved these changes

@tcharding tcharding tcharding approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

0.28.0

Development

Successfully merging this pull request may close these issues.

4 participants

@dr-orlovsky @tcharding @sanket1729 @apoelstra