Replace `Persister` with `SessionPersister` for v2 Sender by arminsabouri · Pull Request #789 · payjoin/rust-payjoin

arminsabouri · 2025-06-23T16:16:33Z

The following replaces the existing implemention of the Persister trait with the new SessionPersister.
Please take a look at individual commits for a more complete description of the changes.
commits messages prefixed'd with "squash" indicates that they are meant to be squash'd with the previous commit before this PR gets merged.

arminsabouri · 2025-06-23T16:18:32Z

payjoin-ffi/src/error.rs

 }
+
+#[macro_export]
+macro_rules! impl_persisted_error_from {


This macro ended up being uglier than I expected. I think it would be fine to have two seperate macros both in the send and receive mods.

I'm strongly in favor of readability/maintainability > DRY. I would even consider dropping the macro entirely since it's just 3 From implementations afaict.

I agree. This macro was removed and I impl From for the sender errors. The reciever already uses its own macro so I left that one alone

The commit message for 8b3abb9 still contains a reference to the macro. otherwise lgtm.

Good eye. Fixed. Thanks

payjoin/src/persist.rs

coveralls · 2025-06-23T16:20:56Z

Pull Request Test Coverage Report for Build 15855900356

Details

384 of 458 (83.84%) changed or added relevant lines in 6 files are covered.
1 unchanged line in 1 file lost coverage.
Overall coverage decreased (-0.08%) to 86.01%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
payjoin-cli/src/db/v2.rs	63	64	98.44%
payjoin/src/send/v2/session.rs	21	35	60.0%
payjoin-cli/src/app/v2/mod.rs	65	88	73.86%
payjoin/src/send/v2/mod.rs	86	122	70.49%

Files with Coverage Reduction	New Missed Lines	%
payjoin-cli/src/app/v2/mod.rs	1	80.67%

Totals
Change from base Build 15787087477:	-0.08%
Covered Lines:	7814
Relevant Lines:	9085

💛 - Coveralls

spacebear21

concept ACK and minor comments and questions.

commits messages prefixed'd with "squash" indicates that they are meant to be squash'd with the previous commit before this PR gets merged.

Personally, I'd prefer PRs to be presented "as-is" when marked ready for review (i.e. this is the exact set of commits the author intends to get merged). As a reviewer I try to put myself in the shoes of a future contributor who wants to understand the historical context for a change. If they'll see two commits squashed as one that's what I'd like to see too. Especially in the case of FFI it's pretty easy to review separately anyways since it lives in a different workspace.

payjoin-cli/src/db/v2.rs

spacebear21 · 2025-06-23T20:03:41Z

payjoin/src/persist.rs


+/// Handles cases where the transition either succeeds with a final result that ends the session, or hits a static condition and stays in the same state.
+/// State transition may also be a fatal error or transient error.
+pub struct MaybeSuccessTransitionWithNoResults<Event, SuccessValue, CurrentState, Err>(


This seems to have been added explicitly for the Sender, but shouldn't the Receiver also support no-change state transitions (e.g. the receiver also calls process_res on get requests which may be empty).

process_res in unchecked proposal does use a similar state transition object: MaybeFatalTransitionWithNoResults .

The difference is in the success case MaybeSuccessTransitionWithNoResults will close the session and return a result value while MaybeFatalTransitionWithNoResults leaves the session open and transitions to the next state

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

spacebear21 · 2025-06-23T20:14:05Z

payjoin/src/send/v2/mod.rs

+pub enum SenderTypeState {
+    Uninitialized(),
+    WithReplyKey(Sender<WithReplyKey>),
+    V2GetContext(Sender<V2GetContext>),


Making a note to revisit this state name in naming audit.

spacebear21 · 2025-06-23T20:17:59Z

payjoin/src/send/v2/mod.rs

+            SessionEvent::ProposalReceived(processed_proposal),
+        )
    }
+    pub fn endpoint(&self) -> &Url { &self.endpoint }


This seems new/unrelated to other changes, what's the rationale for adding it here?

This is used when getting the ohttp relay.

let (req, ctx) = session.extract_req( self.unwrap_relay_or_else_fetch(Some(session.endpoint().clone())).await?, )?;

A better solution here could be to add v1 to the top level sender and add an endpoint method instead of methods at all the specific type states. As a side note: Is the endpoint field is duplicated? It seems like it lives in v1 structs and v2 seperately?

let sender<state> { state: state, v1: send::v1, } impl<state> Sender<state> { pub fn endpoint(&self) -> &Url{ self.v1.endpoint } }

spacebear21 · 2025-06-23T20:25:41Z

payjoin-ffi/src/error.rs

 }
+
+#[macro_export]
+macro_rules! impl_persisted_error_from {


I'm strongly in favor of readability/maintainability > DRY. I would even consider dropping the macro entirely since it's just 3 From implementations afaict.

This state transition object handles cases where the transition either succeeds with a final result that ends the session, or hits a static condition and stays in the same state. An example of this type of state transition would be the sender's process_res on the get request typestate.

This commit updates the v2 Sender state transition methods to return state transition objects. Each state transition produces different outcomes based on the current state. When persisted, a state transition object returns the next valid state. Changes to `v2::receiver` `persist.rs` This file now contains the core logic for the session history manager, receiver session events, and associated error types. `send/v2/mod.rs` Introduced a sum type that wraps all possible sender session states. This type processes session events by checking if the event is valid for the current state. If valid, it uses the state’s `apply` method to move to the next state. The `apply` method acts as a shorthand for handling state transitions based on session events. Updated all state transition methods to return a transition object. Removed the `sender_ser_de_roundtrip` test since session events no longer store the data needed to reconstruct this typestate. Removed `V2PostContext` as a sender session typestate because it broke the typestate pattern and introduces risk of OHTTP key reuse. Other typestates follow a clear structure: construct a request, process a response, and transition only after learning new information. Information that we log as a session event. `V2PostContext` deviated from this by not representing any new session information. Re-generating the POST request (`extract_v2`) simply produces the same request. The only variation comes from the ephemeral OHTTP keys that are created per request. Supporting `V2PostContext` as its own typestate would require storing the request in the session event. That approach would allow applications to reuse the same ephemeral OHTTP keys, linking requests and undermining privacy. Instead, this commit updates the flow to let applications resume a session by re-extracting the v2 payload. The state only advances after successfully processing the response. Multiparty changes: Removed the `Persister` trait support from the sender’s multiparty module. Calls to the wrapped v2 typestate now use a noop persister and forward API errors. Storage-related errors are unwrapped with `expect()` since they're currently infallible. Future work will add proper session persistence to both sender and receiver multiparty modules. Changes to Payjoin-CLI `app/v2/mod.rs` Updated app logic to handle resumed sender sessions that may be in any of the typestates. Renamed `long_poll_post` to `post_original_proposal`, which now tries to advance the state machine after receiving a proposal. Database Removed the old `Persister` implementation in favor of `SessionPersister`. Also dropped the `close` and `get_session` methods since the current flow no longer uses them. FFI changes This commit monomorphizes each state transition object to be specific to its corresponding state transition, removing generic parameters to enable UniFFI exposure. Each state transition object is wrapped in an `Arc<RwLock<Option<Object>>>` to allow FFI bindings to reference self safely. UniFFI does not enforce Rust’s strong borrowing guarantees and operates more predictabily with `&self`. To prevent a replay error naming comflict this commit prefixes the receiver's error with "Reciver_". Additionally, this commit introduces `JsonSenderSessionPersister`, enabling applications to persist and load session events using exposed to_json and from_json methods on `SenderSessionEvent`.

`PersistedError` encapsulates the api side errors as well as storage related errors that can occur during a state transition. The following commit updates all the sender side transitions objects to return `PersistedError`. There was a naming conflict with the reciever type. Both error types had to be renamed and prefixed with "Sender|Receiver_...".

spacebear21

tACK 2340906

Tested by running the payjoin-cli receiver and sender and interrupting at each step to test the resume commands on both sides. I should note I had to delete my local payjoin.sled DBs to avoid errors from old/stale sessions (Error: Failed to replay receiver event log: ReplayError(PersistenceFailure(Deserialize(Error("missing field events", line: 1, column: 833))))). AFAICT payjoin-cli doesn't currently clean up expired sessions currently, that would address these breaking changes in the future.

Renamed `persist.rs` to `session.rs` as the file no longer manages persistence logic. It now handles session replays and history management.

This trait has been replaced by the new `SessionPersister`.

arminsabouri · 2025-06-24T16:20:10Z

Last push fixes a commit message e8c000f related to this comment #789 (comment)

arminsabouri · 2025-06-24T16:24:55Z

Seems like I did not dismiss the previous approve (?). Going to wait for CI to pass and merge :)

arminsabouri requested a review from spacebear21 June 23, 2025 16:16

arminsabouri commented Jun 23, 2025

View reviewed changes

payjoin/src/persist.rs Outdated Show resolved Hide resolved

spacebear21 reviewed Jun 23, 2025

View reviewed changes

arminsabouri added 2 commits June 24, 2025 08:14

arminsabouri force-pushed the sender-persist-session-events branch from 3b4c3b4 to 2340906 Compare June 24, 2025 12:15

arminsabouri requested a review from spacebear21 June 24, 2025 12:18

arminsabouri mentioned this pull request Jun 23, 2025

Session Persistence Tracking Issue (0.24) #785

Closed

13 tasks

spacebear21 approved these changes Jun 24, 2025

View reviewed changes

arminsabouri added 2 commits June 24, 2025 12:17

Rename sender persist.rs to session.rs

de94e66

Renamed `persist.rs` to `session.rs` as the file no longer manages persistence logic. It now handles session replays and history management.

Remove deprecated Persister trait

3b157b0

This trait has been replaced by the new `SessionPersister`.

arminsabouri force-pushed the sender-persist-session-events branch from 2340906 to 3b157b0 Compare June 24, 2025 16:17

arminsabouri requested a review from spacebear21 June 24, 2025 16:20

spacebear21 merged commit fec74d7 into payjoin:master Jun 24, 2025
13 checks passed

Conversation

arminsabouri commented Jun 23, 2025

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!

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

Uh oh!

Pull Request Test Coverage Report for Build 15855900356

Details

💛 - Coveralls

Uh oh!

spacebear21 left a comment

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!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

arminsabouri Jun 24, 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!

spacebear21 left a comment

Choose a reason for hiding this comment

Uh oh!

arminsabouri commented Jun 24, 2025

Uh oh!

arminsabouri commented Jun 24, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

coveralls commented Jun 23, 2025 •

edited

Loading

arminsabouri Jun 24, 2025 •

edited

Loading