BIP-0322: polishing follow-up#2155
Conversation
guggero
force-pushed
the
bip-0322-follow-up
branch
from
b21f3ab to
d9feef3
Compare
jonatack
added
the
BIP Update by Owner
|
Thanks for following up @guggero! Here are some suggested edits. Feel free to pick and choose (or for the BIP names, adopt a different format to be used consistently): https://github.com/jonatack/bips/commits/2026-05-bip322-suggested-edits-or-areas-of-feedback/ |
guggero
force-pushed
the
bip-0322-follow-up
branch
from
d9feef3 to
6a61279
Compare
|
Thanks a lot for the review and suggested fixes, @jonatack! Took almost all of them, really appreciate the inputs. I also added a commit to clarify the use of the |
danielabrozzoni
left a comment
danielabrozzoni
left a comment
There was a problem hiding this comment.
Left some comments.
It's my first time reading BIP322, so I also left some comments outside of the current PR diff :)
|
|
||
| The [[#full-proof-of-funds|Proof of Funds]] variant allows demonstrating control | ||
| of a set of UTXOs in addition to the message being signed. | ||
| The list of UTXOs may or may not be related to the address being signed with (the |
There was a problem hiding this comment.
I'm a bit unsure about this sentence... I think this is an implementation detail that doesn't really belong in the motivation. Also, before reading the whole BIP, I found this sentence to be a bit confusing, it almost reads as if a signer could prove ownership of arbitrary UTXOs, including ones they don't control.
There was a problem hiding this comment.
You're right, this sounds weird in the motivation. I moved the section to the definition of the Proof of Funds variant and also attempted to make it more clear in what it means. What do you think?
| This specification is backwards compatible with the legacy signmessage/verifymessage specification | ||
| through the special case as described above. | ||
| This specification is backwards-compatible with the legacy signmessage/verifymessage specification | ||
| through the special case [#legacy|as described above]. |
There was a problem hiding this comment.
The [#legacy|as described above] is not a link on Github, and is shown as this:
I think you want it written as: [[#legacy|as described above]].
Also, you may want to move this to the second commit (it's in the fourth one at the moment).
murchandamus
left a comment
murchandamus
left a comment
There was a problem hiding this comment.
Good improvements. I just have one additional suggestion.
murchandamus
added
the
PR Author action required
guggero
force-pushed
the
bip-0322-follow-up
branch
from
6a61279 to
ae43dce
Compare
murchandamus
removed
the
PR Author action required
murchandamus
left a comment
murchandamus
left a comment
There was a problem hiding this comment.
Sorry for the piece-meal review: I noticed an open parenthesis that isn’t being closed on this read. Other than that all the changes looks good to me. Let’s get this shipped. :)
| This UTXO set is chosen freely by the signer and need not be associated with the signing address | ||
| (the <code>message_challenge</code>. For example, it may consist of outputs paid to that address, |
There was a problem hiding this comment.
There is at least a closing parenthesis missing here, but the first sentence reads a bit wonky.
4 participants
Addresses a couple of review comments in #2141 as a follow-up.