WitnessVersion type by dr-orlovsky · Pull Request #617 · rust-bitcoin/rust-bitcoin

dr-orlovsky · 2021-06-13T09:55:20Z

Reopening as a new PR to #562 (github does not allow to re-open force-pushed branches) since there might be changes during the review at this level unrelated to #563 (which will be in draft mode until this is merged - and rebased after the merge)

sgeisler

utACK 3645c6353757a1771799a7ec35bd1a29d34538c3

Looks pretty good. I really like the function documentation describing all error cases applicable to the function (especially when it's only a subset of the error enum).

src/util/address.rs

jrawsthorne

You mentioned here about using WitnessVersion in the is_* methods on Script. Don't know if you want to do that here.

src/util/address.rs

dr-orlovsky · 2021-06-13T21:59:08Z

@jrawsthorne well, tried that and it looks like it adds a lot more code instead of simplifying functions. So just made it for only is_witness_program and not the rest of is_* methods (which will require error testing instead of simple opcode equivalence test used now)

clarkmoody

I prefer this approach, since it "makes impossible states impossible." I've just got a couple preference nits.

clarkmoody · 2021-06-14T03:00:11Z

src/util/address.rs

Nit: use num instead of no for a number variable? Also, this function would probably be better named from_u8

It was originally named from_u8, but later I decided it is a bad name, since there are two different u8-based representations for witness version: by script byte (0, 0x51-0x60) and as version number (incremental from 0 to 16). So while I'm fine with using from_num instead of from_no, it will be confusing to use from_u8.

Possibly raw_num or raw_u8?

+1 for num instead of no

clarkmoody · 2021-06-14T03:01:42Z

src/util/address.rs

Nit: I would prefer into_u8

Commented above: the same reason for the naming here

Kixunil

Just a few doc suggestions.

Kixunil · 2021-06-14T12:32:08Z

src/util/address.rs

Did you mean to say "if a plain u8 type was used instead it would mean that version may be > 16, which is incorrect"?

src/util/address.rs

Kixunil · 2021-06-14T12:35:38Z

src/util/address.rs

I believe s.parse::<u8>() is considered idiomatic.

src/util/address.rs

Kixunil · 2021-06-14T12:49:27Z

src/util/address.rs

Not a native speaker but the original seemed correct. The rules check if the address is standard.

src/util/address.rs

src/blockdata/script.rs

dr-orlovsky · 2021-06-16T06:14:36Z

Addressed all suggestions with a separate commit

Kixunil · 2021-06-16T08:37:30Z

src/util/address.rs

Nit: "was be"

sanket1729

ack 4ecfd689d1fa8da4aed9d9a265f73c1ef3f1160f

dr-orlovsky · 2021-06-19T07:58:50Z

Needed rebase, merged master into this branch so the review will not require to go through all commits again. Also fixed @Kixunil nit in docs.

Ned re-ACK from @sanket1729 and review from @apoelstra before the merge since this is intrusive and important change

src/util/address.rs

sgeisler

utACK 409cb919c5586b5fb39bd72dba3b73e433c379ad

src/util/address.rs

sgeisler

utACK 8a99a3fddce43001223774fe0d58dfc1457394bb

apoelstra · 2021-07-07T20:43:56Z

Can you clean up the history by removing the merge commit and rebasing on top of master?

apoelstra · 2021-07-07T21:36:32Z

ack 8a99a3fddce43001223774fe0d58dfc1457394bb aside from history

dr-orlovsky · 2021-08-01T18:39:53Z

@apoelstra done

sanket1729

ACK a8571f24cb3d50289acc86a6fe72f4ecc615cee8

dr-orlovsky · 2021-08-10T08:35:24Z

@sanket1729 @sgeisler had to force-push due to conflicts with recent merges. Re-ACK will be highly appreciated

sgeisler

re-ACK ecc4008

Just looked at the diff-diff using git range-diff 8a99a3f...ecc400826c5eebf4c8bf3f28219af348a1ba0191

apoelstra · 2021-08-30T17:44:15Z

re-utACK ecc4008

sanket1729

ACK ecc4008

dr-orlovsky added the API break This PR requires a version bump for the next release label Jun 13, 2021

dr-orlovsky added this to the 0.27.0 milestone Jun 13, 2021

This was referenced Jun 13, 2021

WitnessVersion type #562

Closed

Taproot P2TR address #563

Merged

sgeisler previously approved these changes Jun 13, 2021

View reviewed changes

src/util/address.rs Outdated Show resolved Hide resolved

src/util/address.rs Outdated Show resolved Hide resolved

src/util/address.rs Outdated Show resolved Hide resolved

src/util/address.rs Outdated Show resolved Hide resolved

jrawsthorne reviewed Jun 13, 2021

View reviewed changes

src/util/address.rs Outdated Show resolved Hide resolved

src/util/address.rs Outdated Show resolved Hide resolved

dr-orlovsky dismissed sgeisler’s stale review via 7d1e2b4 June 13, 2021 21:44

clarkmoody reviewed Jun 14, 2021

View reviewed changes

Kixunil previously approved these changes Jun 14, 2021

View reviewed changes

sanket1729 reviewed Jun 15, 2021

View reviewed changes

src/util/address.rs Outdated Show resolved Hide resolved

src/blockdata/script.rs Outdated Show resolved Hide resolved

dr-orlovsky dismissed Kixunil’s stale review via 4ecfd68 June 16, 2021 06:14

dr-orlovsky requested review from clarkmoody, sanket1729 and sgeisler June 16, 2021 06:14

Kixunil reviewed Jun 16, 2021

View reviewed changes

src/util/address.rs Outdated

Copy link
Copy Markdown

Collaborator

Kixunil Jun 16, 2021

Choose a reason for hiding this comment

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

Nit: "was be"

sanket1729 previously approved these changes Jun 16, 2021

View reviewed changes

dr-orlovsky dismissed sanket1729’s stale review via 409cb91 June 19, 2021 07:57

Kixunil reviewed Jun 20, 2021

View reviewed changes

src/util/address.rs Outdated Show resolved Hide resolved

dr-orlovsky requested a review from sanket1729 June 27, 2021 12:43

sgeisler previously approved these changes Jun 27, 2021

View reviewed changes

src/util/address.rs Outdated Show resolved Hide resolved

dr-orlovsky dismissed sgeisler’s stale review via 8a99a3f June 27, 2021 13:23

sgeisler previously approved these changes Jun 27, 2021

View reviewed changes

Kixunil mentioned this pull request Jul 15, 2021

Moving bunch of stuff from my payjoin crate here #630

Open

dr-orlovsky dismissed sgeisler’s stale review via a8571f2 August 1, 2021 18:39

sanket1729 previously approved these changes Aug 1, 2021

View reviewed changes

dr-orlovsky added 2 commits August 10, 2021 10:34

WitnessVersion type

64c1ec0

Updating Script::is_witness_program to use new WitnessVersion

ecc4008

dr-orlovsky dismissed sanket1729’s stale review via ecc4008 August 10, 2021 08:34

sgeisler approved these changes Aug 10, 2021

View reviewed changes

dr-orlovsky mentioned this pull request Aug 26, 2021

Transfer WitnessVersion to rust-bitcoin BP-WG/descriptor-wallet#7

Closed

apoelstra approved these changes Aug 30, 2021

View reviewed changes

sanket1729 approved these changes Sep 7, 2021

View reviewed changes

apoelstra merged commit 2a655f4 into rust-bitcoin:master Sep 8, 2021

dr-orlovsky mentioned this pull request Sep 29, 2021

Tracking issue for Taproot #503

Closed

20 tasks

dr-orlovsky mentioned this pull request Jan 15, 2022

Bump version to v0.28 #785

Closed

Conversation

dr-orlovsky commented Jun 13, 2021

Uh oh!

sgeisler left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jrawsthorne left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

dr-orlovsky commented Jun 13, 2021

Uh oh!

clarkmoody left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dr-orlovsky Jun 14, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Kixunil left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

dr-orlovsky commented Jun 16, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sanket1729 left a comment

Choose a reason for hiding this comment

Uh oh!

dr-orlovsky commented Jun 19, 2021

Uh oh!

Uh oh!

sgeisler left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

sgeisler left a comment

Choose a reason for hiding this comment

Uh oh!

apoelstra commented Jul 7, 2021

Uh oh!

apoelstra commented Jul 7, 2021

Uh oh!

dr-orlovsky commented Aug 1, 2021

Uh oh!

sanket1729 left a comment

Choose a reason for hiding this comment

Uh oh!

dr-orlovsky commented Aug 10, 2021

Uh oh!

sgeisler left a comment

Choose a reason for hiding this comment

Uh oh!

dr-orlovsky Jun 14, 2021 •

edited

Loading