Make `Receiver` generic over its typestate #719

arminsabouri · 2025-05-28T13:58:14Z

This commit updates the v2::Receiver to be generic over its typestate (e.g., UncheckedProposal, InputsSeen, etc.). The motivation:

Improves readability and maintainability: Each typestate now clearly belongs to the receiver session. This makes the code easier to reason about, without runtime cost. It also sets us up to define a shared pattern between v2, NS1R and future state machines.
Decouples typestate data from transition logic: NS1R typesates can now consume state structs without being tied to the transition machinery. This keeps state transitions encapsulated and makes the underlying session data easier to use in other contexts.

This commit renames the original Receiver struct to WithContext. WithContext type represents the receiver session once the HPKE context has been created. The Receiver name is re-purposed for the new generic wrapper over typestate.

For the UniFFI exported receiver we monomorphizes the exported structs over a specific typestate. UniFFI doesn't support exporting generic structs, so we expose a concrete type to the FFI.

coveralls · 2025-05-28T14:10:17Z

Pull Request Test Coverage Report for Build 15353666997

Details

81 of 90 (90.0%) changed or added relevant lines in 5 files are covered.
5 unchanged lines in 2 files lost coverage.
Overall coverage increased (+0.04%) to 84.592%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
payjoin-cli/src/app/v2/mod.rs	7	8	87.5%
payjoin/src/receive/v2/persist.rs	0	1	0.0%
payjoin/src/receive/v2/mod.rs	45	52	86.54%

Files with Coverage Reduction	New Missed Lines	%
payjoin/src/receive/multiparty/mod.rs	2	90.46%
payjoin/src/receive/v2/mod.rs	3	93.27%

Totals
Change from base Build 15352205919:	0.04%
Covered Lines:	6539
Relevant Lines:	7730

💛 - Coveralls

arminsabouri · 2025-05-28T18:15:11Z

Last push reverts changes made to payjoin-ffi crate.
Backed up those changes here

arminsabouri · 2025-05-28T18:38:26Z

Hm it seems like the ffi changes are neccecary to make the tests happy -- probably due to the patch in the cargo.toml. Since payjoin-ffi is pinned to 0.23 and excluded from the workspace does it make sense to exclude its test suite from ./contrib/test_local.sh? or Remove the patch?

My preference would be to just include it in the workspace (#721)

DanGould

Easy to follow and mostly drop in as far as all files except for src/receive/v2/mod.rs are concerned.

I request some minor changes that will reduce the diff here by implementing Deref{,Mut} on receiver and some multiparty related visibility tweaks.

DanGould · 2025-05-28T18:28:08Z

payjoin-cli/src/app/v2/mod.rs

-        session: &mut payjoin::receive::v2::Receiver,
-    ) -> Result<payjoin::receive::v2::UncheckedProposal> {
+        session: &mut payjoin::receive::v2::Receiver<payjoin::receive::v2::WithContext>,
+    ) -> Result<payjoin::receive::v2::Receiver<payjoin::receive::v2::UncheckedProposal>> {


nonblocking nit comment: imported don't fully qualify the generic (this had to be the first comment didn't it 😛 I promise later comments are more important)

Lots of full qualifications in this file to replace old qualifications, but they're not longer necessary and create noise.

There are a couple more fully qualified receiver types that exist on master. I will import those ones and remove the full qualification in a seperate commit on this PR

DanGould · 2025-05-28T18:31:51Z

payjoin/src/receive/v2/mod.rs

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


Since all of the Receiver State types include SessionContext I think we could do better with this name (Initialilzed?) but am happy to leave it for a name audit before our next release

I would prefer to do a big naming audit before the release if thats an option.

DanGould · 2025-05-28T18:36:22Z

payjoin/src/receive/v2/mod.rs

    ) -> Result<(Request, ohttp::ClientResponse), Error> {
-        if SystemTime::now() > self.context.expiry {
-            return Err(InternalSessionError::Expired(self.context.expiry).into());
+        if SystemTime::now() > self.0.context.expiry {


These self.0 calls add verbosity without increased clarity since the internal type is unnamed. Let's elide the 0s everywhere with a simple impl Deref.

impl<State> core::ops::Deref for Receiver<State> { type Target = State; fn deref(&self) -> &Self::Target { &self.0 } }

Even the assignments can use this syntax if we impl DerefMut for Receiver for example self.context.e = Some(e);

impl<State> core::ops::DerefMut for Receiver<State> { fn deref_mut(&mut self) -> &mut Self::Target { &mut self.0 } }

Great suggestion. I think this works most cases but if we are trying to move the value out of the struct
i.e self.0.v1.check_broadcast_suitability(min_fee_rate, can_broadcast)?; then we will hit a borrowing / move through deref issue. In those cases we will still need to move with direct access (self.0).

without increased clarity

Should we name the inner type?

Receiver<state> { state: state }

Leaving explicit self.0 on those cases is fine with me.

However, there are still a dozen or so instances of self.0 that can now be elided because of the Deref impls that I would like to see included to take advantage of the new impls before this is merged.

self.state is a bit more clear. I'd accept this PR without it but I do think it reads better than self.0.

Did a comb through of all cases where self.0 could be elided. I agree self.state is more clear. Infact that is what I originally had in the sketch branch. I'll make that update

payjoin/src/receive/v2/mod.rs

DanGould · 2025-05-28T18:45:26Z

payjoin/src/receive/v2/mod.rs

+    #[cfg(feature = "_multiparty")]
+    pub fn inner(&self) -> &UncheckedProposal { &self.0 }


This is a weird one that's not super discoverable. Why .inner() instead of making multiparty_proposal.add take a v2::Receiver<UncheckedProposal>? Does this only smell to me?

You're right. The design constraint we are working with is two fold. One inner state should be opaque to the application. Two NS1R typestates don't need to store v2 typestates only the state structs (uncheckedProposal).
The best path forward is:

multiparty_proposal.add to accept v2::Receiver<UncheckedProposal>

and pub crate() the inner receiver state only if the multiparty flag is on

I think the new change that changes v2::Receiver's visibility with a feature flag is even weirder than a dedicated method. I would prefer leaving it pub(crate) regardless of feature set, or just have the internal v2_proposals store the v2::Receiver struct directly. I think the former is the simplest, to leave the inner state `pub(crate) regardless of feature set.

payjoin/src/receive/v2/mod.rs

arminsabouri · 2025-05-29T13:00:22Z

Converting to draft till we fix #721

arminsabouri · 2025-05-29T17:59:07Z

The last push re-introduces the ffi changes.

payjoin-ffi/tests/bdk_integration_test.rs

arminsabouri · 2025-05-30T13:52:26Z

Drafting till I fix the uniffi build

arminsabouri · 2025-05-30T16:16:38Z

FWIW sender side is drafted here
Will open that PR once this one gets goes thru the review process.

This commit updates the `v2::Receiver` to be generic over its typestate (e.g., `UncheckedProposal`, `InputsSeen`, etc.). The motivation: * Improves readability and maintainability: Each typestate now clearly belongs to the receiver session. This makes the code easier to reason about, without runtime cost. It also sets us up to define a shared pattern between v2, NS1R and future state machines. * Decouples typestate data from transition logic: NS1R typesates can now consume state structs without being tied to the transition machinery. This keeps state transitions encapsulated and makes the underlying session data easier to use in other contexts. This commit renames the original `Receiver` struct to `WithContext`. This type now represents the receiver session once the HPKE context has been created. The Receiver name is re-purposed for the new generic wrapper over typestate. For the UniFFI exported receiver we monomorphizes the exported structs over a specific typestate. UniFFI doesn't support exporting generic structs, so we expose a concrete type to the FFI.

The full paths are often hard to read and create noise in this file.

DanGould

ACK 1b42be6

arminsabouri requested a review from DanGould May 28, 2025 13:58

arminsabouri force-pushed the receiver-generic-over-typestate branch from 93a5327 to ca1e333 Compare May 28, 2025 14:02

arminsabouri force-pushed the receiver-generic-over-typestate branch 2 times, most recently from 63d50d7 to 7f296ea Compare May 28, 2025 18:14

DanGould mentioned this pull request May 28, 2025

Bring payjoin-ffi into top level workspace #721

Closed

DanGould requested changes May 28, 2025

View reviewed changes

arminsabouri force-pushed the receiver-generic-over-typestate branch from 7f296ea to 35ecbb1 Compare May 29, 2025 12:59

arminsabouri marked this pull request as draft May 29, 2025 13:00

arminsabouri force-pushed the receiver-generic-over-typestate branch 2 times, most recently from 7df3d86 to 06635ad Compare May 29, 2025 17:58

arminsabouri marked this pull request as ready for review May 29, 2025 17:59

arminsabouri commented May 30, 2025

View reviewed changes

payjoin-ffi/tests/bdk_integration_test.rs Show resolved Hide resolved

arminsabouri force-pushed the receiver-generic-over-typestate branch 3 times, most recently from 7f12faf to dcd036c Compare May 30, 2025 13:30

arminsabouri requested a review from DanGould May 30, 2025 13:31

arminsabouri marked this pull request as draft May 30, 2025 13:52

arminsabouri force-pushed the receiver-generic-over-typestate branch from dcd036c to 857ac8d Compare May 30, 2025 14:06

arminsabouri marked this pull request as ready for review May 30, 2025 16:10

arminsabouri force-pushed the receiver-generic-over-typestate branch from 857ac8d to 922a224 Compare May 30, 2025 18:39

arminsabouri added 2 commits May 30, 2025 14:50

Remove fully qualified paths for receiver types

1b42be6

The full paths are often hard to read and create noise in this file.

arminsabouri force-pushed the receiver-generic-over-typestate branch from 922a224 to 1b42be6 Compare May 30, 2025 18:53

DanGould approved these changes May 30, 2025

View reviewed changes

DanGould merged commit 8053a75 into payjoin:master May 30, 2025
13 checks passed

DanGould mentioned this pull request Jun 2, 2025

Sender generic over typestate #728

Merged

		#[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)]
		pub struct WithContext {

		#[cfg(feature = "_multiparty")]
		pub fn inner(&self) -> &UncheckedProposal { &self.0 }

Make Receiver generic over its typestate #719

Make Receiver generic over its typestate #719

Uh oh!

Conversation

arminsabouri commented May 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

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

Uh oh!

Pull Request Test Coverage Report for Build 15353666997

Details

💛 - Coveralls

Uh oh!

arminsabouri commented May 28, 2025

Uh oh!

arminsabouri commented May 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DanGould left a comment

Choose a reason for hiding this comment

Uh oh!

DanGould May 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!

Choose a reason for hiding this comment

Uh oh!

DanGould May 29, 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!

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!

arminsabouri commented May 29, 2025

Uh oh!

arminsabouri commented May 29, 2025

Uh oh!

Uh oh!

arminsabouri commented May 30, 2025

Uh oh!

arminsabouri commented May 30, 2025

Uh oh!

DanGould left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Make `Receiver` generic over its typestate #719

Make `Receiver` generic over its typestate #719

arminsabouri commented May 28, 2025 •

edited

Loading

coveralls commented May 28, 2025 •

edited

Loading

arminsabouri commented May 28, 2025 •

edited

Loading

DanGould May 28, 2025 •

edited

Loading

DanGould May 29, 2025 •

edited

Loading