Sender generic over typestate #728

arminsabouri · 2025-06-02T00:42:43Z

This commit updates the v2::Sender to be generic over its
typestate (e.g., V2GetContext, V2PostContext, etc.). The
motivation is outlined in arminsabouri@c8b860d

This commit renames the original Sender struct to WithReplyKey.
This type now represents the v2::sender session once the HPKE context has been
created. The Sender name is re-purposed for the new generic wrapper
over typestate.

For the UniFFI exported receiver we monomorphize 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-06-02T00:46:14Z

Pull Request Test Coverage Report for Build 15381296070

Details

57 of 60 (95.0%) changed or added relevant lines in 6 files are covered.
1 unchanged line in 1 file lost coverage.
Overall coverage increased (+0.03%) to 84.355%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
payjoin/src/send/v2/persist.rs	0	1	0.0%
payjoin-cli/src/db/v2.rs	9	11	81.82%

Files with Coverage Reduction	New Missed Lines	%
payjoin-cli/src/db/v2.rs	1	94.59%

Totals
Change from base Build 15378234071:	0.03%
Covered Lines:	6438
Relevant Lines:	7632

💛 - Coveralls

DanGould

The one change I'll insist on is that Sender gets defined before the states, otherwise my comments are very similar to the #719 PR.

I see that send::v2::State has no explicit definition, and so the Sender here does not require explicit trait State {} type constraint. I noticed that wasn't done for Receiver either, and ask you to consider doing so or otherwise stating rationale for straying from the pattern.

Lastly, review of this brought into question our use of v1::Sender context in the state vs on the Sender struct, since each state contains that data, either as v1::Sender OR as endpoint and psbt_context. I wonder if even v1::Sender should have the psbt_context after noticing this distinction.

We could follow up with

elision of fully qualified names in ffi
better names for the states

Please let me know which of these changes you think should be made here besides the Sender/typestate definition reorder.

DanGould · 2025-06-02T19:43:23Z

payjoin-ffi/src/send/mod.rs


 #[derive(Clone)]
-pub struct Sender(payjoin::send::v2::Sender);
+pub struct WithReplyKey(payjoin::send::v2::Sender<payjoin::send::v2::WithReplyKey>);


Similar to my comment about WithContext in the receiver PR, I'd rather use a unique identifier for this state, especially as it gets its own type here in FFI, where it's not SenderWithReplyKey but just tacit WithReplyKey.

What do you think of Uninitialized/Initialized as NewSender/WithReplyKey renames?

I not exactly sure what you mean by "unique identifier for this state". The reason I went WithReplyKey is that is describes the diff between the current and last state.

FWIW we will need to introduce a Uninitalizsed sender state. Its used during session log replay.

DanGould · 2025-06-02T19:51:00Z

payjoin/src/send/v2/mod.rs

+pub struct Sender<State> {
+    pub(crate) state: State,
+}


State Appears not to be defined as a trait, so any type can Be State rather than requiring explicit impl State for V2PostContext {} etc. I think this was missed for Receiver as well. It's not strictly necessary but it would prevent the next engineer from making a Receiver<State> for any arbitrary State without explicitly marking it so.

I'd also definitely like to see it defined before the first typestate NewSender.

I'd also definitely like to see it defined before the first typestate NewSender.

I assuming you mean the Sender<state> def. If so can do

State Appears not to be defined as a trait, so any type can Be State rather than requiring explicit impl State for V2PostContext {} etc. I think this was missed for Receiver as well. It's not strictly necessary but it would prevent the next engineer from making a Receiver for any arbitrary State without explicitly marking it so.

This offers slightly better type safety and fool-proof. I also notice that it is defined as part of the pattern defined here. Happy to make this change. I'll make it a seperate commit for both sender and receiver in this PR

As a side note: the state trait will unfortuantely need to be pub due to Sender being pub. However if we use sealed traits they can be pub however the caller cannot impl

Yes of course it has to be pub if the type is pub. A sealed trait is appropriate in these cases.

DanGould · 2025-06-02T19:54:09Z

payjoin/src/send/v2/mod.rs

-pub struct Sender {
+pub struct WithReplyKey {
    /// The v1 Sender.
    pub(crate) v1: v1::Sender,


v1: fields are duplicated v1::Senders for two states, unlike receiver.state.v1 fields which are unique to each state. Might it make more sense to make v1: v1::Sender a field of Sender, or does that complicate the FFI translation or other machinery?

It makes me wonder if v1::sender should have its own PsbtContext versus the odd separation starting with V2PostContext where psbt_context and endpoint are separated out of v1::Sender, but all the information is still there.

No it shouldn't complicate FFI at all. FFI doesnt read v1 fields so FFI shouldn't need to change.

It would look something like this.
I would like to create an issue and catagorize this as techdebt and focus of the core functionality for now. Lmk if you agree

This commit updates the `v2::Sender` to be generic over its typestate (e.g., `V2GetContext`, `V2PostContext`, etc.). The motivation is outlined in c8b860d This commit renames the original `Sender` struct to `WithReplyKey`. This type now represents the v2::sender session once the HPKE context has been created. The `Sender` name is re-purposed for the new generic wrapper over typestate. For the UniFFI exported receiver we monomorphize the exported structs over a specific typestate. UniFFI doesn't support exporting generic structs, so we expose a concrete type to the FFI.

To prevent any struct from becoming a typestate we enforce that all type states implement a blank trait. Each "State" trait is unique to a state machine to prevent accidentaly crosswiring. i.e a reciever type state should never be used for a sender state machine.

arminsabouri · 2025-06-03T14:01:14Z

The one change I'll insist on is that Sender gets defined before the states, otherwise my comments are very similar to the #719 PR.

I see that send::v2::State has no explicit definition, and so the Sender here does not require explicit trait State {} type constraint. I noticed that wasn't done for Receiver either, and ask you to consider doing so or otherwise stating rationale for straying from the pattern.

I added a blank trait 722bfa9 to address this for both the receiver and sender typestates.

Lastly, review of this brought into question our use of v1::Sender context in the state vs on the Sender struct, since each state contains that data, either as v1::Sender OR as endpoint and psbt_context. I wonder if even v1::Sender should have the psbt_context after noticing this distinction.

I implemented this suggestion here. I would like to save this item for a follow up PR and create an issue labeled as tech debt.

We could follow up with

elision of fully qualified names in ffi

better names for the states

Please let me know which of these changes you think should be made here besides the Sender/typestate definition reorder.

Note: Sender type reorder is also pushed. Follow up items are:

de-dupping v1::Sender in the typestates
Better typestate names

DanGould

ACK 722bfa9

I think x:State more readable than x:XStateandmore readable than<State: XState> so I'd like to see that name change in a follow up as well

DanGould · 2025-06-03T14:43:12Z

payjoin/src/receive/v2/mod.rs

+pub trait ReceiverState {}
+
 #[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)]
-pub struct Receiver<State> {
+pub struct Receiver<State: ReceiverState> {


prefer receiver::State to receiver::ReceiverState, and then Receiver<State> remains valid. Can wait for rename.

DanGould · 2025-06-03T17:01:28Z

payjoin/src/send/v2/mod.rs

+pub struct Sender<State> {
+    pub(crate) state: State,
+}


Yes of course it has to be pub if the type is pub. A sealed trait is appropriate in these cases.

DanGould · 2025-06-03T17:03:41Z

Additionally, I find your recommendation to use sealed traits for these State traits appropriate

arminsabouri force-pushed the sender-generic-over-state branch from b5a0378 to eaf5b48 Compare June 2, 2025 14:44

arminsabouri changed the title ~~WIP - Sender generic over typestate~~ Sender generic over typestate Jun 2, 2025

arminsabouri marked this pull request as ready for review June 2, 2025 14:46

arminsabouri requested a review from DanGould June 2, 2025 14:46

DanGould requested changes Jun 2, 2025

View reviewed changes

arminsabouri added 2 commits June 3, 2025 09:08

arminsabouri force-pushed the sender-generic-over-state branch from eaf5b48 to 722bfa9 Compare June 3, 2025 13:29

arminsabouri requested a review from DanGould June 3, 2025 14:02

DanGould approved these changes Jun 3, 2025

View reviewed changes

arminsabouri merged commit affd9d9 into payjoin:master Jun 4, 2025
13 checks passed

arminsabouri deleted the sender-generic-over-state branch June 4, 2025 11:33

DanGould mentioned this pull request Jun 5, 2025

use sealed traits for State traits #747

Closed

DanGould mentioned this pull request Jul 13, 2025

Fix pjos handling in v2 receiver and sender #847

Merged

Sender generic over typestate #728

Sender generic over typestate #728

Uh oh!

Conversation

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

Uh oh!

Uh oh!

coveralls commented Jun 2, 2025

Pull Request Test Coverage Report for Build 15381296070

Details

💛 - Coveralls

Uh oh!

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

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!

arminsabouri Jun 3, 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 commented Jun 3, 2025

Uh oh!

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

DanGould commented Jun 3, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

arminsabouri commented Jun 2, 2025 •

edited

Loading

arminsabouri Jun 3, 2025 •

edited

Loading