BIP78: spelling and grammar updates #1623

satsie commented at 2:30 AM on June 20, 2024: contributor

This PR proposes a number of spelling and grammar updates to BIP78. Nothing here should change the actual content of the BIP, though I'm new to Payjoin and would appreciate a sanity check :)

cc the BIP champion, @NicolasDorier

in bip-0078.mediawiki:270 in 640c712617 outdated

 271 | +** If it is one of the receiver's inputs:
 272 |  *** Verify the PSBT input is finalized
 273 |  *** Verify that <code>non_witness_utxo</code> or <code>witness_utxo</code> are filled in.
 274 | -** Verify that the payjoin proposal did not introduced mixed input's sequence.
 275 | -** Verify that the payjoin proposal did not introduced mixed input's type.
 276 | +** Verify that the payjoin proposal did not mix the sequence of the inputs.

DanGould commented at 2:38 AM on June 20, 2024:

I believe this rule actually specifies that all inputs have identical nSequence values. Perhaps a more precise comment than the original would be

** Verify that the payjoin proposal inputs all specify the same sequence value.

satsie commented at 3:17 AM on June 20, 2024:

Ooo thanks for the clarification! I did not realize this was referring to nSequence

Accepted both suggestions and rebased. Appreciate you taking a look at this!

in bip-0078.mediawiki:291 in 640c712617 outdated

 286 | @@ -287,8 +287,8 @@ The sender must be careful to only sign the inputs that were present in the orig
 287 |  
 288 |  Note:
 289 |  * The sender must allow the receiver to add/remove or modify the receiver's own outputs. (if payment output substitution is disabled, the receiver's outputs must not be removed or decreased in value)
 290 | -* The sender should allow the receiver to not add any inputs. This is useful for the receiver to change the paymout output scriptPubKey type.
 291 | -* If no input have been added, the sender's wallet implementation should accept the payjoin proposal, but not mark the transaction as an actual payjoin in the user interface.
 292 | +* The sender should allow the receiver to not add any inputs. This is useful for the receiver to change the payment output scriptPubKey type.
 293 | +* If no additional inputs have been added, the sender's wallet implementation should accept the payjoin proposal, but not mark the transaction as an actual payjoin in the user interface.

DanGould commented at 2:40 AM on June 20, 2024:

This works as-is, but could be more specific by adding a subject:

* If the receiver added no inputs, the sender's wallet implementation should accept the payjoin proposal, but not mark the transaction as an actual payjoin in the user interface.

DanGould commented at 2:42 AM on June 20, 2024: contributor

This clears up the language quite a bit.

I left one comment about the mixed sequence number validation step.

satsie force-pushed on Jun 20, 2024

in bip-0078.mediawiki:192 in eefc15dd31 outdated

 188 | @@ -189,7 +189,7 @@ The well-known error codes are:
 189 |  
 190 |  The receiver is allowed to return implementation specific errors which may assist the sender to diagnose any issue.
 191 |  
 192 | -However, it is important that error codes that are not well-known and that the message do not appear on the sender's software user interface.
 193 | +However, it is important that error codes are not well-known and that the messages do not appear on the sender's software user interface.

jonatack commented at 1:12 PM on June 20, 2024:

I'm not sure that this is the intended meaning (see the lines immediately following this one). It looks to me like the idea is that error codes that are not well known should not appear in the UI and be printed in the debug log only. Also, this change would require author sign-off to ensure it doesn't alter the intended meaning of the text.

DanGould commented at 4:49 AM on June 21, 2024:

Every spec I've seen has separate well-known error codes with templates to display to users, and a generic unknown error code for which messages they contain are only printed to debug logs. I missed that this changed themeaning in my first review. It should be reworded not to change the meaning in my view.

remcoros commented at 10:53 PM on July 1, 2024:

suggestion: However, it is important that error codes and messages that are not well-known, do not appear on the sender's software user interface.

satsie commented at 8:48 PM on July 3, 2024:

Thanks @remcoros! I like that suggestion. I think it brings more clarity while still preserving the meaning. Admittedly, this sentence has been a bit of a head scratcher for me. If I adopt remcoros's suggestion, I don't think it would it need BIP author sign off, right?

For now I rebased and just left this sentence alone. If there is consensus on how to reword it without changing the meaning, I am happy to open up a follow up PR.

jonatack commented at 1:17 AM on July 4, 2024:

@satsie Happy to review your future follow-up to reword this confusing passage in a way that makes sense without changing the intended meaning.

in bip-0078.mediawiki:193 in eefc15dd31 outdated

 188 | @@ -189,7 +189,7 @@ The well-known error codes are:
 189 |  
 190 |  The receiver is allowed to return implementation specific errors which may assist the sender to diagnose any issue.
 191 |  
 192 | -However, it is important that error codes that are not well-known and that the message do not appear on the sender's software user interface.
 193 | +However, it is important that error codes are not well-known and that the messages do not appear on the sender's software user interface.
 194 |  Such error codes or messages could be used maliciously to phish a non technical user.

jonatack commented at 1:12 PM on June 20, 2024:

Such error codes or messages could be used maliciously to phish a non-technical user.

jonatack commented at 1:16 PM on June 20, 2024: contributor

Mostly looks good modulo my comment. However, any changes that could alter the meaning of the text would need to be signed off by the BIP author. Perhaps split those out to a separate commit or PR.

in bip-0078.mediawiki:190 in eefc15dd31 outdated

 188 | @@ -189,7 +189,7 @@ The well-known error codes are:
 189 |  
 190 |  The receiver is allowed to return implementation specific errors which may assist the sender to diagnose any issue.

jonatack commented at 1:21 PM on June 20, 2024:

The receiver is allowed to return implementation-specific errors which may assist the sender to diagnose any issue.

in bip-0078.mediawiki:194 in eefc15dd31 outdated

 188 | @@ -189,7 +189,7 @@ The well-known error codes are:
 189 |  
 190 |  The receiver is allowed to return implementation specific errors which may assist the sender to diagnose any issue.
 191 |  
 192 | -However, it is important that error codes that are not well-known and that the message do not appear on the sender's software user interface.
 193 | +However, it is important that error codes are not well-known and that the messages do not appear on the sender's software user interface.
 194 |  Such error codes or messages could be used maliciously to phish a non technical user.
 195 |  Instead those errors or messages can only appear in debug logs.

jonatack commented at 1:22 PM on June 20, 2024:

Perhaps "might" here, if this is a suggestion and not a hard rule (would need BIP author sign-off along with the proposed change in line 192). I'm not sure if this section is describing a SHOULD or a MUST.

Instead, those errors or messages might only appear in debug logs.

murchandamus added the label Proposed BIP modification on Jun 20, 2024

murchandamus added the label Pending acceptance on Jun 20, 2024

satsie commented at 3:31 AM on June 21, 2024: contributor

Thanks for the review @jonatack! I will create a separate PR with the changes that are definitely just spelling or grammar related and rebase this (since it has comment history) to only include the ones that would need sign off from the BIP author.

jonatack commented at 2:18 PM on June 21, 2024: contributor

@satsie following up on the thread #1623 (review), perhaps keep all the changes in this PR if while updating your change there it becomes clear that they are editorial-only and don't alter the meaning of the text. I'm not sure the BIP author is active in providing review feedback here at the moment.

5twelve approved

satsie force-pushed on Jul 3, 2024

in bip-0078.mediawiki:347 in b3445ea8e9 outdated

 343 | @@ -344,7 +344,7 @@ On top of this the receiver can poison analysis by randomly faking a round amoun
 344 |  
 345 |  ===<span id="output-substitution"></span>Payment output substitution===
 346 |  
 347 | -Unless disallowed by sender explicitly via `disableoutputsubstitution=true` or by the BIP21 url via query parameter the `pjos=0`, the receiver is free to decrease the amount, remove, or change the scriptPubKey output paying to himself.
 348 | +Unless disallowed by sender explicitly via <code>disableoutputsubstitution=true</code> or by the BIP21 url via query parameter the <code>pjos=0</code>, the receiver is free to decrease the amount, remove, or change the scriptPubKey output paying to himself.

jonatack commented at 9:17 PM on July 3, 2024:

Unless disallowed by the sender explicitly via <code>disableoutputsubstitution=true</code> or by the BIP21 url via the query parameter <code>pjos=0</code>, the receiver is free to decrease the amount, remove, or change the scriptPubKey output paying to himself.

edit: maybe also s/url/URL in both places in this BIP (as URI is also capitalized).

jonatack commented at 9:18 PM on July 3, 2024: contributor

LGTM modulo this line, WDYT?

satsie force-pushed on Jul 4, 2024

BIP78: spelling and grammar updates

Co-authored-by: Dan Gould <d@ngould.dev>
Co-authored-by: Jon Atack <jon@atack.com>

5700a230dc

satsie force-pushed on Jul 4, 2024

satsie commented at 1:02 AM on July 4, 2024: contributor

LGTM modulo this line, WDYT?

Oh that is a good catch! I accepted the suggestion and also took the advice to replace "url" with "URL".

jonatack approved

jonatack commented at 1:14 AM on July 4, 2024: contributor

ACK, thanks for updating.

jonatack merged this on Jul 4, 2024

jonatack closed this on Jul 4, 2024

satsie deleted the branch on Jul 4, 2024