Abstract common sender functionality, not heirarchy #884

DanGould · 2025-07-16T16:15:21Z

{v1,v2,multiparty}::Sender{,Builder} types have been built off of a heirarchy where multiparty:: has a v2:: and v2:: has a v1::, which was done for quick convenience and not because that relationship actually makes functional sense.

This PR is the first in a few which I believe are necessary to rectify this distinction. It does so by drawing the separation between the sender's PsbtContext checks / response processing and the version-specific serialization for networked messaging. However, I don't think it goes far enough.

For one, it only really rectifies this issue between v1 and v2. Multiparty is left abstract over v2. Second, There are still distinct SenderBuilders that can't tell whether or not they're handling a v1 or v2 URIs. Since the information necessary to distinguish between a v1/v2 URI is in the URI itself, it seems that ought to be the first order of business for the sender to do even before calling SenderBuilder::new. The lack of this distinction leads to a problem with a hacky solution where downstream users need to wait all the way until they attempt to create a v2 request and handle an error there in order to figure out the version. The SenderBuilder also ought to behave differently for each version, and I'm not sure our current fix of #847 does this completely (Does a v2 SenderBuilder sending to v1 URI honor pjos? it should). In order to do so I reckon we could create an actual PjUri type, rather than an alias, that enumerates over the versions when check_pj_supported or its replacement is called. In order to do that effectively by making sure the correct parameters are there and we're not just switching on the presence of a fragment, I think UrlExt also needs to check for the parameter presence and validity.

The other issue with v1::Sender flow is that it doesn't use the generic typestate machine pattern to match v2, which would be nice as well but out of the scope of this PR.

re: #809

The PsbtContext is used in both v1 and v2 and can be shared.

The distinction beetween 1s1r async payjoin and ns1r is that the sender checks the proposal before the receiver signs in the first round of communication, so the receiver inputs won't yet be finalized. Therefore, in order to share functionality, the receiver input finalized check needs to be triggered by a condition. This does that.

The Version type exists for this less-prone-to-error purpose.

coveralls · 2025-07-16T16:18:38Z

Pull Request Test Coverage Report for Build 16378664856

Details

306 of 317 (96.53%) changed or added relevant lines in 5 files are covered.
7 unchanged lines in 3 files lost coverage.
Overall coverage decreased (-0.02%) to 85.777%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
payjoin/src/core/send/mod.rs	152	155	98.06%
payjoin/src/core/send/v1.rs	40	48	83.33%

Files with Coverage Reduction	New Missed Lines	%
payjoin/src/core/send/multiparty/mod.rs	1	97.62%
payjoin/src/core/send/v2/mod.rs	1	90.84%
payjoin/src/core/send/v1.rs	5	90.09%

Totals
Change from base Build 16377741477:	-0.02%
Covered Lines:	7756
Relevant Lines:	9042

💛 - Coveralls

DanGould · 2025-07-16T16:21:39Z

Seeking concept N/ACK @nothingmuch

nothingmuch

cACK

This seems like the right approach to me.

I would further suggest separating out the URI parsing, for the sender side and returns an enum with variants for the different versions. This enum should be non-exhaustive, and should map easily to the (feature enabled) senderbuilders in a statically typed way.

This would also address the outstanding comment in the payjoin-cli impl that suggests that the URI should not be stored in an event but should precede the sender state machine altogether, that makes intuitive sense to me.

nothingmuch · 2025-07-16T16:56:43Z

payjoin/src/core/send/mod.rs

 ) -> Url {
    let mut url = endpoint;
-    url.query_pairs_mut().append_pair("v", version);
+    url.query_pairs_mut().append_pair("v", &version.to_string());


nit, i think we can have &'static strs for this, implementing an as_str() method and then provide an Into<&str> impl that calls that

Introduce PsbtContextBuilder which has no ouptut_substitution field, allowing parent consumer SenderBuilders to handle output_substitution and the creation of types relevant to their independent state machines. Move common v1 logic used in both v1 and v2 flows to the send module.

DanGould · 2025-07-19T17:18:47Z

rebased so CI passes. Following up with the an enumerated, versioned Uri/SenderBuilder/Sender[] approach.

DanGould · 2025-07-21T05:09:05Z

payjoin/src/core/send/v2/mod.rs

    /// Helper function that takes a V1 sender build result and wraps it in a V2 Sender,
    /// returning the appropriate state transition.


The comment here is incorrect since this hierarchy is removed, but this function going to be completely replaced in a follow-up this week.

nothingmuch

Sorry this took me so long to finish reviewing.

FWIW I would have an easier time reviewing incrementally if the commits were a bit smaller, reviewing commit by commit was still a bit tricky since kinda boilerplate-y changes like moving things around were intermixed with more semantic changes like mut self -> self or the output substitution changes, so I felt like I had to reread a lot of stuff to double check that I didn't miss anything, and even then my confidence is not super high that inamongst the boilerplate-y changes I didn't miss anything important

nothingmuch · 2025-07-24T15:18:45Z

payjoin/src/core/send/v1.rs

+    pub fn always_disable_output_substitution(self) -> Self {
+        Self { output_substitution: OutputSubstitution::Disabled, ..self }


in principle, why is this method still needed?

i don't agree that it:

may prevent the receiver from doing advanced operations such as opening LN channels

maybe there's other situations but for the LN case, including concretely with CLN's PSBT based RPC as an example, it's arguably more appropriate for the receiver to initiate the LN side of things after modifying the PSBT and introducing the channel funding output, to obtain the backout tx etc and then respond to the PJ sender

i can only come up with somewhat contrived scenarios that would actually require this, more situations may come up with covenant support in the future where addresses and amounts could be more tightly coupled as a matter of course but for now i think we could consider literally always allowing substitution

I think if the sender disallows funds to go to a channel funding output, not the one in the bip21, then the payjoin would fail.

I don't believe anyone downstream is disabling output substitution everywhere. I think this is vestigial from kix's v1 implementation and agree it can be removed.

nothingmuch · 2025-07-24T15:32:36Z

payjoin/src/core/send/mod.rs

+/// Construct serialized V1 Request and Context from a Payjoin Proposal
+pub(crate) fn create_v1_post_request(endpoint: Url, psbt_ctx: PsbtContext) -> (Request, V1Context) {


it feels a bit off that this is not in the v1 module, which has more to do with feature flags than with functionality

this is not a critique of this PR, just thinking outloud, but this got me thinking that we also don't really have a mechanism for blanket disabling v1 support, e.g. a v2 receiver that ignores v1 senders' requests, or a v2 sender that refuses to send v1 requests. i'm not sure there is sufficient motivation for either behavior, but depending on the answer we might want need to reconsider, currently --features v1 means "compile code that supports sending/receiving only using bip 78" and --features v2 means "compile code that supports sending/receiving both bip 78 and bip 77". v2 could onstensibly mean bip 77 only unless v1 is also enabled, in which case this code would actually belong in the v1 module

nothingmuch · 2025-07-24T15:35:22Z

payjoin/src/core/send/mod.rs

+/// Data required to validate the response.
+///
+/// This type is used to process a BIP78 response.


Suggested change

/// Data required to validate the response.

///

/// This type is used to process a BIP78 response.

/// Data required to validate and process a BIP78 response.

spacebear21 · 2025-07-24T17:16:35Z

payjoin/src/core/send/multiparty/mod.rs

        let body = serialize_v2_body(
-            &self.0.v1.psbt,
-            self.0.v1.output_substitution,
-            self.0.v1.fee_contribution,
-            self.0.v1.min_fee_rate,
+            &self.0.state.psbt_ctx.original_psbt,
+            self.0.state.psbt_ctx.output_substitution,
+            self.0.state.psbt_ctx.fee_contribution,
+            self.0.state.psbt_ctx.min_fee_rate,
        )?;


This seems like an opportunity to refactor serialize_v2_body and maybe serialize_url to just take a &PsbtContext as argument.

Yes this is in my working branch 😎

DanGould added 3 commits July 14, 2025 12:49

Share PsbtContext between Sender structs

109f85d

The PsbtContext is used in both v1 and v2 and can be shared.

Use crate::Version instead of string

abf2a63

The Version type exists for this less-prone-to-error purpose.

nothingmuch reviewed Jul 16, 2025

View reviewed changes

DanGould force-pushed the refactor-sender branch from 949a58e to 8ceccaa Compare July 18, 2025 19:22

DanGould marked this pull request as ready for review July 19, 2025 14:29

DanGould requested a review from nothingmuch July 19, 2025 17:17

DanGould commented Jul 21, 2025

View reviewed changes

spacebear21 mentioned this pull request Jul 22, 2025

Cargo mutants version update #882

Merged

nothingmuch approved these changes Jul 24, 2025

View reviewed changes

spacebear21 reviewed Jul 24, 2025

View reviewed changes

DanGould merged commit 5ecfed7 into payjoin:master Jul 24, 2025
15 checks passed

		/// Helper function that takes a V1 sender build result and wraps it in a V2 Sender,
		/// returning the appropriate state transition.

		pub fn always_disable_output_substitution(self) -> Self {
		Self { output_substitution: OutputSubstitution::Disabled, ..self }

		/// Construct serialized V1 Request and Context from a Payjoin Proposal
		pub(crate) fn create_v1_post_request(endpoint: Url, psbt_ctx: PsbtContext) -> (Request, V1Context) {

Abstract common sender functionality, not heirarchy #884

Abstract common sender functionality, not heirarchy #884

Uh oh!

Conversation

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

Uh oh!

Uh oh!

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

Uh oh!

Pull Request Test Coverage Report for Build 16378664856

Details

💛 - Coveralls

Uh oh!

DanGould commented Jul 16, 2025

Uh oh!

nothingmuch left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DanGould commented Jul 19, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nothingmuch left a comment

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

DanGould commented Jul 16, 2025 •

edited

Loading

coveralls commented Jul 16, 2025 •

edited

Loading