Correct the pjos parameter parity by Gmin2 · Pull Request #546 · payjoin/rust-payjoin

Gmin2 · 2025-02-20T23:05:53Z

altered and corrected the logic for pjos parameter

fixes #545

DanGould · 2025-02-21T21:42:12Z

I think this fix is correct but I want to ask @spacebear21 if the field should be renamed to enable_output_substitution because that reflects the orientation of pjos, where disable_output_substitution has the opposite orientation.

coveralls · 2025-02-21T21:45:14Z

Pull Request Test Coverage Report for Build 14041324313

Details

122 of 134 (91.04%) changed or added relevant lines in 11 files are covered.
2 unchanged lines in 2 files lost coverage.
Overall coverage increased (+0.3%) to 81.124%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
payjoin/src/receive/optional_parameters.rs	3	4	75.0%
payjoin/src/receive/v2/mod.rs	4	5	80.0%
payjoin/src/send/v1.rs	6	8	75.0%
payjoin/src/send/v2/mod.rs	6	8	75.0%
payjoin/src/uri/mod.rs	44	50	88.0%

Files with Coverage Reduction	New Missed Lines	%
payjoin/src/send/v2/mod.rs	1	96.62%
payjoin/src/uri/mod.rs	1	84.0%

Totals
Change from base Build 14041215432:	0.3%
Covered Lines:	5007
Relevant Lines:	6172

💛 - Coveralls

DanGould

I may have spoken too soon. Although I believe your change mechanics are correct, and I dig the test, I request a slight change to make the code semantics more correct

spacebear21 · 2025-02-26T17:30:17Z

I think this fix is correct but I want to ask @spacebear21 if the field should be renamed to enable_output_substitution because that reflects the orientation of pjos, where disable_output_substitution has the opposite orientation.

I think disable_output_substitution better reflects that "they MUST disallow payment output substitution".
Also, when allowing output substition I think we just shouldn't set pjos at all since it's implied to be allowed and would slightly shorten the URI.

DanGould · 2025-03-17T19:10:28Z

We've got to fix this bug before next release. @Gmin2 I'm going to push the recommended changes to this branch and merge as co-author, so no pressure

spacebear21 · 2025-03-17T19:29:37Z

I think disable_output_substitution better reflects that "they MUST disallow payment output substitution".

I changed my mind on this, I prefer allow_output_substitution to better match the pjos direction.

DanGould · 2025-03-18T00:00:53Z

In the spirit of Towards Impeccable Rust I'm inclined to replace the boolean with an enum OutputSubstitution {Enabled, Disabled}. That makes misuse much more difficult. The enum may be used across Uri and Params.

DanGould · 2025-03-18T00:54:26Z

The one thing I'm on the fence about is whether or not is_output_substitution_disabled() on PayjoinExtras should live or die. As I write this, i think it should die, because it simplifies the feature gating (all _core features would have it) and then downstream users would need to match explicitly on the enum, too.

spacebear21

ACK

There is a typo in the second commit message:

`pjos` must come before `pjos` for optimistic alphanumeric QR encoding.
                        ^ should be `pj`

spacebear21 · 2025-03-18T01:32:13Z

+    /// Whether the receiver is allowed to substitute original outputs or not.
+    pub fn output_substitution(&self) -> OutputSubstitution { self.v1.output_substitution() }


I don't remember why this is public on WantsOutputs and PayjoinProposal but maybe it doesn't need to be? It's checked in some tests but I don't know why a user would need it

WantsOutputs is should be better to handle an OutputSubstitutionError::OutputSubstitutionDisabled variant, assuming that variant, agreed. I'm not sure why it would be on PayjoinProposal.

Similarly, I'm confused why there are multiple OutputSubstitutionDisabled(&str) variants instead of distinct variants. This part is going to become a new issue.

Hm, though if they're removed, the OutputSubstitution Enum again only makes sense to expose publicly within the context of receive::v1::exclusive

A v1 receiver on an unsecured server MUST disallow output substitution, but that doesn't preclude a v2 receiver disabling it for other reasons?

A v2 receiver is always going to serialize pjos=0 to prevent A v1 sender from trying output substitution over the directory in plaintext messages, but a v2 sender may use disableoutputsubstitution Params to prevent the receiver from doing so. A receiver is really responding to preferences of the sender in v2 because they're always using an authenticated protocol.

spacebear21

utACK 1bebd6a

Gmin2 · 2025-03-18T20:56:03Z

We've got to fix this bug before next release. @Gmin2 I'm going to push the recommended changes to this branch and merge as co-author, so no pressure

Thanks @DanGould , i was occupied a bit

The inverse had been implemented, serializing "1" by mistake.

`pjos=1` is implied when it is not present. `pjos` must come before `pj`, which should be last since it contains the most data that can benefit from the compression of alphanumeric QR encoding.

DanGould · 2025-03-24T17:02:45Z

rebased on #590

An enum makes misuse more difficult. Since we were fixing our own misuse, this seemed like a prime area to use an enum instead of a bool. An enum also catches a bug caught in test_serialize_pjos, where elided pjos=1 should imply enabled output substitution. Vestigial is_output_substitution_disabled removed. The method was included before the state machine checked OutputSubstitutionErrors. Now those errors can replace the boolean.

spacebear21

tACK 4889b1e

Ran the payjoin-cli receiver with defaults and confirmed the pjos parameter is omitted (i.e. output substitution allowed) by default. Also ran a patched v1 receiver with hardcoded OutputSubstitution::Disabled to confirm that pjos=0 in the BIP21 uri

DanGould requested changes Feb 22, 2025

View reviewed changes

Comment thread payjoin/src/uri/mod.rs Outdated

benalleng reviewed Feb 25, 2025

View reviewed changes

Comment thread payjoin/src/uri/mod.rs

spacebear21 reviewed Feb 26, 2025

View reviewed changes

Comment thread payjoin/src/uri/mod.rs

benalleng mentioned this pull request Mar 14, 2025

Add additional tests for uri module #578

Merged

benalleng reviewed Mar 17, 2025

View reviewed changes

Comment thread payjoin/src/uri/mod.rs Outdated

DanGould added the bug Something isn't working label Mar 17, 2025

DanGould force-pushed the pjos-to-0 branch from a952911 to 53b4341 Compare March 18, 2025 00:51

DanGould force-pushed the pjos-to-0 branch 2 times, most recently from e7c149f to bd311e6 Compare March 18, 2025 01:10

spacebear21 reviewed Mar 18, 2025

View reviewed changes

DanGould mentioned this pull request Mar 18, 2025

Why are there multiple InternalOutputSubstitutionError::OutputSubstitutionDisabled(& str) variants? #588

Closed

DanGould force-pushed the pjos-to-0 branch 2 times, most recently from fad76bd to 1bebd6a Compare March 18, 2025 03:20

spacebear21 approved these changes Mar 18, 2025

View reviewed changes

Gmin2 and others added 2 commits March 24, 2025 13:00

Serialize pjos=0 when disabled

85dd6f4

The inverse had been implemented, serializing "1" by mistake.

Elide pjos URI param unless disabled

c9e9cb9

`pjos=1` is implied when it is not present. `pjos` must come before `pj`, which should be last since it contains the most data that can benefit from the compression of alphanumeric QR encoding.

DanGould force-pushed the pjos-to-0 branch from 1bebd6a to 4889b1e Compare March 24, 2025 17:03

spacebear21 approved these changes Mar 24, 2025

View reviewed changes

DanGould changed the title ~~fix: corrected the pjos parameter~~ Correct the pjos parameter parity Mar 24, 2025

DanGould merged commit f5982cf into payjoin:master Mar 24, 2025
7 checks passed

spacebear21 mentioned this pull request Apr 1, 2025

Release payjoin 0.23 #618

Closed

17 tasks

		/// Whether the receiver is allowed to substitute original outputs or not.
		pub fn output_substitution(&self) -> OutputSubstitution { self.v1.output_substitution() }

Conversation

Gmin2 commented Feb 20, 2025

Uh oh!

DanGould commented Feb 21, 2025

Uh oh!

coveralls commented Feb 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pull Request Test Coverage Report for Build 14041324313

Details

💛 - Coveralls

Uh oh!

DanGould left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

spacebear21 commented Feb 26, 2025

Uh oh!

Uh oh!

Uh oh!

DanGould commented Mar 17, 2025

Uh oh!

spacebear21 commented Mar 17, 2025

Uh oh!

DanGould commented Mar 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DanGould commented Mar 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

spacebear21 left a comment

Choose a reason for hiding this comment

Uh oh!

spacebear21 Mar 18, 2025

Choose a reason for hiding this comment

Uh oh!

DanGould Mar 18, 2025

Choose a reason for hiding this comment

Uh oh!

DanGould Mar 18, 2025

Choose a reason for hiding this comment

Uh oh!

spacebear21 Mar 18, 2025

Choose a reason for hiding this comment

Uh oh!

DanGould Mar 18, 2025

Choose a reason for hiding this comment

Uh oh!

spacebear21 left a comment

Choose a reason for hiding this comment

Uh oh!

Gmin2 commented Mar 18, 2025

Uh oh!

DanGould commented Mar 24, 2025

Uh oh!

spacebear21 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

coveralls commented Feb 21, 2025 •

edited

Loading

DanGould commented Mar 18, 2025 •

edited

Loading

DanGould commented Mar 18, 2025 •

edited

Loading