Separate receiver::{v1,v2} abstraction cleanly #998

DanGould · 2025-08-27T01:35:33Z

There is a lot of movement here but nothing complex is added or removed. It's a pure shuffle.

The focus of this work is to untangle the v1/v2 logic so that there is a clean split between shared PSBT logic and isolated, version-specific serialization.

In essence:

receive::v1::exclusive becomes receive::v1 which is only compiled when feature = "v1".
receive::v1 becomes receive::common and includes types used when feature = "_core". Typestates defined here but only used when feature = "v1 are moved to receive::v1. It's always pub(crate) so there's no conditional visibility on whether or not feature = "v1", because that is completely separated into receive::v1.
v1 typestates are removed from the v2 state machine and module. v2 no longer depends on the v1 module, only common.
Tests are shuffled around so that v1-specificb behavior and common behavior are tested in the appropriate modules. At times, that means test fixture functions and visibility is rearranged. By the end, visibility is almost as private as it gets.

This PR stops short of a visibility, documentation, or naming audit. I may consider renaming receive::common to receive::states but open to suggestion. It does need to be a submodule in order to stay invisibile wrt the receive:: scope. It may go further with newtypes and even a reduction in v2 typestates as follow up, though I think those tasks can follow #995 where ReceiverBuilder is added and the Uninitialized state is removed as well.

Pull Request Checklist

Please confirm the following before requesting review:

A human has reviewed every single line of this code before opening the PR (no auto-generated, unreviewed LLM/robot submissions).
I have read CONTRIBUTING.md and rebased my branch to produce hygienic commits.

coveralls · 2025-08-27T01:40:27Z

Pull Request Test Coverage Report for Build 17276344135

Details

806 of 827 (97.46%) changed or added relevant lines in 6 files are covered.
2 unchanged lines in 1 file lost coverage.
Overall coverage increased (+0.09%) to 85.811%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
payjoin/src/core/receive/error.rs	5	6	83.33%
payjoin/src/core/receive/v1/mod.rs	83	86	96.51%
payjoin/src/core/receive/v2/mod.rs	29	34	85.29%
payjoin/src/core/receive/common/mod.rs	582	594	97.98%

Files with Coverage Reduction	New Missed Lines	%
payjoin/src/core/receive/v2/mod.rs	2	92.82%

Totals
Change from base Build 17275991760:	0.09%
Covered Lines:	8007
Relevant Lines:	9331

💛 - Coveralls

DanGould · 2025-08-27T14:18:22Z

payjoin/src/core/receive/common/v1/mod.rs

-        Original { psbt: PARSED_ORIGINAL_PSBT.clone(), params }
-    }
-
+    // TODO: restrict to v1 only since UncheckedProposal is a v1 only type


this is addressed later in the history

DanGould · 2025-08-27T14:21:21Z

payjoin/src/core/receive/mod.rs

 impl PsbtContext {
    /// Prepare the PSBT by creating a new PSBT and copying only the fields allowed by the [spec](https://github.com/bitcoin/bips/blob/master/bip-0078.mediawiki#senders-payjoin-proposal-checklist)
-    fn prepare_psbt(self, processed_psbt: Psbt) -> Psbt {
+    pub(crate) fn prepare_psbt(self, processed_psbt: Psbt) -> Psbt {


This function and finalize_proposal are eventually returned to private in this PR. This is temporary

DanGould · 2025-08-27T14:24:11Z

payjoin/src/core/receive/mod.rs


 #[derive(Debug, Clone, Serialize, Deserialize, PartialEq, Eq)]
-pub(crate) struct PsbtContext {
+pub struct PsbtContext {


The receive::v2::SessionEvent::ProvisionalProposal(PsbtContext) requires this visibility. It's possible to wrap this struct if desired, but PsbtContext is the actual event diff that we're using. Perhaps it could be pruned further, just to the bitcoin::Psbt that's the payjoin_psbt since we already have the original psbt at the UncheckedProposal stage.

IMO UncheckedProposal((Original, Optioncrate::HpkePublicKey)) deserves its own type as well instead of a tuple but that can come after. The UncheckedProposal tuple should probably at least be deconstructed out of the nested tuple to UncheckedProposal(Original, Option<crate::HpkePublicKey>)

DanGould · 2025-08-27T14:24:43Z

payjoin/src/core/receive/mod.rs

    ///   1. Remove all sender signatures which were received with the original PSBT as these signatures are now invalid.
    ///   2. Sign and finalize the resulting PSBT using the passed `wallet_process_psbt` signing function.
-    pub fn finalize_proposal(
+    pub(crate) fn finalize_proposal(


as stated above this can be made private later on when some more shuffling is done.

DanGould · 2025-08-27T14:26:49Z

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

-        self,
-        v1: common::ProvisionalProposal,
-    ) -> ReceiveSession {
+    pub(crate) fn apply_provisional_proposal(self, psbt_context: PsbtContext) -> ReceiveSession {


I can see making a NewType/new name for this, but it's not strictly necessary yet.

TODO: reduce pub(crate) fn visibility to private visibility for all of these apply state Receiver methods.

DanGould · 2025-08-27T14:27:55Z

payjoin/src/core/receive/v2/session.rs

+        let psbt_context = wants_fee_range.clone()._apply_fee_range(None, None).unwrap();
+        let psbt = psbt_context


This use of the private _apply_fee_range function is transient until further separation is done. you'll see.

I dont quite understand this comment.
_apply_fee_range is pub(crate)'d. Did you mean to remove this in the latest commit? FWIW this method would be apply_fee_range bc it is used by methods in the crate and apply_fee should be _apply_fee but I dont think a _ prefix is necessary eitherway.

The only problem is a naming conflict with v1 when both v1 and v2 feature flags are on.

It is pub(crate), by private I mean that it's only used internally and is not part of the public api. the _ prefix reinforces this.

On a second look I think I should just get rid of this and rely on the actually private apply_fee directly on both v1 and v2 implementations to avoid this confusion.

DanGould · 2025-08-27T14:31:09Z

payjoin/src/core/receive/v2/session.rs

+        let persister = NoopSessionPersister::<SessionEvent>::default();
+


The problem here is that the v1 typestate machine was awkwardly being used to produce the v2 types.

If we made helper transition methods that didn't save and just produced errors, we could test without any persister at all, and have the main transition functions call those helpers. But we don't really need that additional boilerplate and can just test using a NoopSessionPersister.

I tried to leave expects where there were expects and otherwise just unwrap().

DanGould · 2025-08-27T14:33:11Z

payjoin/src/core/receive/common/mod.rs

+    pub(crate) original_psbt: Psbt,
+    pub(crate) payjoin_psbt: Psbt,
+    pub(crate) params: Params,
+    pub(crate) change_vout: usize,
+    pub(crate) owned_vouts: Vec<usize>,


Because tests are still tangled across the common/v1 modules, this visibility change and others in this commit are required. It's reversed in a later commit which untangles the tests.

DanGould · 2025-08-27T14:34:39Z

payjoin/src/core/receive/error.rs

    /// Protocol-specific errors for BIP-78 v1 requests (e.g. HTTP request validation, parameter checks)
    #[cfg(feature = "v1")]
-    V1(crate::receive::common::RequestError),
+    V1(crate::receive::v1::RequestError),


Ahhh now you can sense some relief. Nice abstractions are coming soon :)

DanGould · 2025-08-27T14:37:14Z

payjoin/src/core/receive/common/mod.rs

+        let original = original_from_test_vector();
+        let wants_inputs = WantsOutputs::new(original, vec![0]).commit_outputs();


Simplified so as not to use the v1-exclusive UncheckedProposal state

DanGould · 2025-08-27T14:49:23Z

@arminsabouri There is a lot of movement here but nothing complex is added or removed. It's a pure shuffle.

I left comments commit by commit so please review in that way so the comments can help you.

The focus of this work is to untangle the v1/v2 logic so that there is a clean split between shared PSBT logic and isolated, version-specific serialization.

In essence:

receive::v1::exclusive is pulled out to become receive::v1 which is only compiled when feature = "v1".
receive::v1 becomes receive::common and includes types used when feature = "_core". Typestates defined here but only used when feature = "v1 are moved to receive::v1
v1 typestates are removed from the v2 state machine
Tests are shuffled around so that v1-specificb behavior and common behavior are tested in the appropriate modules. At times, that means test fixture functions and visibility is rearranged. By the end, visibility is almost as private as it gets.

This PR stops short of a visibility or naming audit. It may go further with newtypes and even a reduction in v2 typestates, though I think those can follow #995 where you add a ReceiverBuilder and remove the uninitialized state as well. Names and documentation probably need to be audited as a follow up as well.

I'm copying much of this message to OP because I think it better describes the issue than OP

DanGould · 2025-08-27T14:55:15Z

I left a bunch of comments on the source so that you can understand why intermediate changes were made. Please let me know if you need any more detail. I'd like to get this in ASAP since it's such a big move and rebase on this is gonna be tricky.

arminsabouri

Approach/utACK
Shuffling seems sane. I had a couple questions about some shared abstractions that could also live in common, some struct field renames, and a question about a apply_fee_range when both v1 and v2 feature flags are on. Otherwise had some nits and stylistic nits that can be ignored and/or followed up on if applying historical is too tricky/not worth the time.

arminsabouri · 2025-08-27T15:11:24Z

payjoin/src/core/receive/v2/session.rs

+        let original = original_from_test_vector();
+        let unchecked_proposal = unchecked_receiver_from_test_vector();
+        let maybe_inputs_owned =
+            unchecked_proposal.clone().assume_interactive_receiver().save(&persister).unwrap();


Nit: I have a prefrence over expects here. It describes exactly what error condition out of many we expect not to happen. e.g since assume_interactive_receiver is a next state only transition the only thing we expect not to happen is a storage error. Which is infallible for a noop persister. Marked as nit. Not mission critical.

arminsabouri · 2025-08-27T15:14:01Z

payjoin/src/core/receive/common/mod.rs

+use crate::output_substitution::OutputSubstitution;
+use crate::psbt::PsbtExt;
+use crate::receive::{InternalPayloadError, Original, PsbtContext};
+


I expected Orignial to live here as well as its a common abstraction. Any reason why its left out?

Same thing for PsbtContext

What you seem to be asking for is to empty receive/mod.rs in favor of common. But we want common specifically to hide types from the top level receive API where Original and PsbtContext are exported. It's not a catchall. receve::common is only for hidden shared types. Otherwise I would put everything in common into recieve/mod.rs.

Now, Original and PsbtContext almost definitely ought to become private in receive/mod.rs, but right now they're in the public interface of receive::v2::session::SessionEvent, so those variants need to encapsulate different or newtypes in order to get rid of that visibility in a follow-up.

arminsabouri · 2025-08-27T15:26:04Z

payjoin/src/core/receive/v1/mod.rs

-            change_vout: self.change_vout,
-            receiver_inputs: self.receiver_inputs,
+/// [`RequestError`] should only be produced here.
+fn validate_body(headers: impl Headers, body: &[u8]) -> Result<&[u8], RequestError> {


Stylistic Nit: This method is only used in UncheckedProposal but defined in the bottom of the file. I would expect to be defined along side build_v1_pj_uri.

Don't we typically put pub fn (like build_..) at the top of the file and fn at the bottom? I didn't write this, just moved it.

Fair enough. If that is consistent with the rest of the repo thats good with me

arminsabouri · 2025-08-27T15:30:40Z

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

 #[derive(Debug, Clone, PartialEq)]
 pub struct WantsOutputs {
-    v1: v1::WantsOutputs,
+    v1: common::WantsOutputs,


Since this is no longer wrapping v1 typestates.

Suggested change

v1: common::WantsOutputs,

inner: common::WantsOutputs,

arminsabouri · 2025-08-27T15:30:51Z

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

 #[derive(Debug, Clone, PartialEq)]
 pub struct WantsInputs {
-    v1: v1::WantsInputs,
+    v1: common::WantsInputs,


Same thing here

arminsabouri · 2025-08-27T15:31:03Z

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

 #[derive(Debug, Clone, PartialEq)]
 pub struct WantsFeeRange {
-    v1: v1::WantsFeeRange,
+    v1: common::WantsFeeRange,


arminsabouri · 2025-08-27T15:53:42Z

payjoin/src/core/receive/v1/mod.rs

-        output_contribution_weight
-    }
-
+impl crate::receive::common::WantsFeeRange {


Could this be problematic if both v1 and v2 feature flags are on? If both are on IIUC the apply_fee_range impl below would override the one in common.

As far as I can tell this would only be an API-side oddity. Applications cannot call the v1 apply_fee_range method if they have a v2 typestate.

I checked that this is not an issue with cargo doc --open --all-features. This impl is scoped to v1::WantsFeeRangeand does not apply tov2::WantsFeeRangewhich is public, only tocommon::WantsFeeRange*in the v1 scope*.common::WantsFeeRangeis not part of the public interface since this type is only ever publicly exported as part of thereceive::v1` scope.

arminsabouri · 2025-08-27T15:58:51Z

payjoin/src/core/receive/v2/session.rs

+        let psbt_context = wants_fee_range.clone()._apply_fee_range(None, None).unwrap();
+        let psbt = psbt_context


I dont quite understand this comment.
_apply_fee_range is pub(crate)'d. Did you mean to remove this in the latest commit? FWIW this method would be apply_fee_range bc it is used by methods in the crate and apply_fee should be _apply_fee but I dont think a _ prefix is necessary eitherway.

arminsabouri · 2025-08-27T16:04:14Z

payjoin/src/core/receive/v2/session.rs

+        let psbt_context = wants_fee_range.clone()._apply_fee_range(None, None).unwrap();
+        let psbt = psbt_context


The only problem is a naming conflict with v1 when both v1 and v2 feature flags are on.

Code that was only for v1 and NOT v2 was in the common folder.

The `exclusive` name does not make sense in this context since it's really the `v1` module now. That change comes next.

Make intenal WantsFeeRange::_apply_fee_range function and external v1-exclusive after which the v1 typestates can be used since the v2 sumtype can use the underlying type directly (or eventually encapsulate them itself).

- have the parameters reflect their new names - add a docstring

That's convention.

Untangle it. Call that conditioner.

cause it just tests WantsOutputs

DanGould · 2025-08-27T19:09:22Z

Ok I think I fixed the problems you brought up and made sure each and every commit was linted and formatted. I want to address the WantsFeeRange::apply_fee in a name audit follow up. It's just a matter of name conflict but we do want that method _apply_fee_range to exist (really as pub(super), not pub(crate) so that the callers can get the PsbtContext without needing to make common::WantsFeeRange's fields visible. If that's ok as a follow-up with you and the other changes you requested are addressed. I'd like to move forward.

as Armin prefers.

arminsabouri

utACK de883a6

DanGould · 2025-08-28T13:38:24Z

survived

arminsabouri mentioned this pull request Aug 27, 2025

Decouple wants outputs v1 and v2 typestates #992

Closed

2 tasks

Move FinalizeProposalError to error mod

1d1e378

DanGould force-pushed the refactor-receiver branch from 806fc20 to 58d7c6e Compare August 27, 2025 14:14

DanGould commented Aug 27, 2025

View reviewed changes

DanGould force-pushed the refactor-receiver branch from 58d7c6e to d8ced52 Compare August 27, 2025 14:36

DanGould commented Aug 27, 2025

View reviewed changes

DanGould force-pushed the refactor-receiver branch from d8ced52 to ccc7ff6 Compare August 27, 2025 14:43

DanGould requested a review from arminsabouri August 27, 2025 14:43

DanGould marked this pull request as ready for review August 27, 2025 14:55

arminsabouri approved these changes Aug 27, 2025

View reviewed changes

arminsabouri mentioned this pull request Aug 27, 2025

Make max fee rate sticky per payjoin-cli session #934

Closed

DanGould added 8 commits August 27, 2025 14:55

Move v1-exclusive code to appropriate module

34e6ab1

Code that was only for v1 and NOT v2 was in the common folder.

Move receive::v1 to receive::common

3a1fa44

The `exclusive` name does not make sense in this context since it's really the `v1` module now. That change comes next.

Move receive::common::exclusive to receive::common::v1

9ac5afc

Move receive::common test fn to receive module

993258a

Remove unnecessary PayjoinProposal field

ff70bef

Simplify visibility of core receive structs

68987d0

Abstract v1-exclusive ProvisionalProposal onward

66302af

Make intenal WantsFeeRange::_apply_fee_range function and external v1-exclusive after which the v1 typestates can be used since the v2 sumtype can use the underlying type directly (or eventually encapsulate them itself).

Use v2 logic in v2 tests

430db08

DanGould added 12 commits August 27, 2025 14:56

Make receive::common::v1::test private

f619017

Move receive::common::v1 to receive::v1

4e307d8

Refactor WantsOutputs::from_proposal to ::new

aa5fa74

- have the parameters reflect their new names - add a docstring

Move WantsOutputs::new to the top of impl

f6a835a

That's convention.

Move interleave_shuffle to reduce visibility

efdd88b

Untangle it. Call that conditioner.

Move test_pjos_disabled to common

9aa635a

cause it just tests WantsOutputs

Move test_avoid_uih_one_output to WantsOutputs containing mod

51b31d1

Move empty_candidate_inputs to common

668377a

Add test_multiple_contribute_inputs() to common

19b2491

Move rest of tests into common

6d75a4b

Move tests to reduce common struct/field/fn scopes

91b5316

Reduce PsbtContext method visibility

eac5e77

DanGould force-pushed the refactor-receiver branch from ccc7ff6 to eac5e77 Compare August 27, 2025 19:07

Make .save(&persister).unwrap()s .expect()s

de883a6

as Armin prefers.

DanGould requested a review from arminsabouri August 27, 2025 19:14

arminsabouri approved these changes Aug 28, 2025

View reviewed changes

DanGould merged commit 8b1bb2b into payjoin:master Aug 28, 2025
10 checks passed

DanGould deleted the refactor-receiver branch August 28, 2025 13:38

		let psbt_context = wants_fee_range.clone()._apply_fee_range(None, None).unwrap();
		let psbt = psbt_context

		let persister = NoopSessionPersister::<SessionEvent>::default();

		let original = original_from_test_vector();
		let wants_inputs = WantsOutputs::new(original, vec![0]).commit_outputs();

Separate receiver::{v1,v2} abstraction cleanly #998

Separate receiver::{v1,v2} abstraction cleanly #998

Uh oh!

Conversation

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

Uh oh!

Pull Request Checklist

Uh oh!

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

Uh oh!

Pull Request Test Coverage Report for Build 17276344135

Details

💛 - Coveralls

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!

DanGould Aug 27, 2025 • 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!

arminsabouri Aug 27, 2025 • 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!

DanGould Aug 27, 2025 • 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!

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

Uh oh!

Uh oh!

DanGould commented Aug 27, 2025

Uh oh!

arminsabouri 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!

DanGould Aug 27, 2025 • 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!

arminsabouri Aug 28, 2025 • 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!

DanGould commented Aug 27, 2025 •

edited

Loading

coveralls commented Aug 27, 2025 •

edited

Loading

DanGould Aug 27, 2025 •

edited

Loading

arminsabouri Aug 27, 2025 •

edited

Loading

DanGould Aug 27, 2025 •

edited

Loading

DanGould commented Aug 27, 2025 •

edited

Loading

DanGould Aug 27, 2025 •

edited

Loading

arminsabouri Aug 28, 2025 •

edited

Loading

arminsabouri Aug 27, 2025 •

edited

Loading