Replace `Persister` with `SessionPersister` for `v2::Receiver` #750

arminsabouri · 2025-06-06T21:18:16Z

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.

One open item before this is undrafted:

c7c31fc introduces a In memory receiver sesssion persister. It is accessible to the integration tests but not to sub mod unit tests (?) . Need to move this persister impl a common location or figure out why its not visibile to the unit tests.

payjoin-cli/src/db/v2.rs

payjoin-ffi/python/test/test_payjoin_integration_test.py

payjoin-test-utils/src/persist.rs

coveralls · 2025-06-06T21:22:42Z

Pull Request Test Coverage Report for Build 15764018768

Details

710 of 823 (86.27%) changed or added relevant lines in 8 files are covered.
5 unchanged lines in 3 files lost coverage.
Overall coverage decreased (-0.05%) to 86.087%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
payjoin-cli/src/db/v2.rs	57	58	98.28%
payjoin/src/receive/v1/exclusive/error.rs	0	1	0.0%
payjoin-cli/src/db/error.rs	0	2	0.0%
payjoin/src/persist.rs	0	12	0.0%
payjoin/src/receive/v2/session.rs	261	279	93.55%
payjoin-cli/src/app/v2/mod.rs	156	191	81.68%
payjoin/src/receive/v2/mod.rs	234	278	84.17%

Files with Coverage Reduction	New Missed Lines	%
payjoin/src/directory.rs	1	92.0%
payjoin-cli/src/app/v2/mod.rs	2	81.02%
payjoin/src/receive/v2/mod.rs	2	87.75%

Totals
Change from base Build 15736242427:	-0.05%
Covered Lines:	7617
Relevant Lines:	8848

💛 - Coveralls

payjoin/src/persist.rs

DanGould · 2025-06-07T21:11:52Z

payjoin-test-utils is really for those dependencies that we don't want to include in payjoin. This PR creates circular dependency by defining InMemoryTestPersister in payjoin-test-utils, which itself depends on payjoin, and using that as a test dependency for payjoin.

Rationale for InMemoryPersister is not yet in the commit log. So I had to do some serious digging to figure out why it was there. I suppose you want to play back the session event log for those unit tests and integration tests where NoopPersister will not actually hand you the history. It would seem to me then that this persister is defined in the payjoin test module where it is used, it may be exported behind a _test-utils feature flag for re-export in payjoin-test-utils if NoopSessionPersister is insufficient in integration tests.

edit: I did an implementation on this branch. There are some other optimizations to the [patch.crates-io] Cargo.toml sections that were also potential sources of discrepancy.

payjoin/src/receive/v2/session.rs

arminsabouri · 2025-06-09T14:02:32Z

payjoin-test-utils is really for those dependencies that we don't want to include in payjoin. This PR creates circular dependency by defining InMemoryTestPersister in payjoin-test-utils, which itself depends on payjoin, and using that as a test dependency for payjoin.

Rationale for InMemoryPersister is not yet in the commit log. So I had to do some serious digging to figure out why it was there. I suppose you want to play back the session event log for those unit tests and integration tests where NoopPersister will not actually hand you the history. It would seem to me then that this persister is defined in the payjoin test module where it is used, it may be exported behind a _test-utils feature flag for re-export in payjoin-test-utils if NoopSessionPersister is insufficient in integration tests.

Rationale should be in the commit that introduce it but also mentioned here fc4bbe7.

edit: I did an implementation on this branch. There are some other optimizations to the [patch.crates-io] Cargo.toml sections that were also potential sources of discrepancy.

Thanks for taking this on. I will apply this in the history.

arminsabouri · 2025-06-09T16:08:01Z

Rebased and resolved conflicts against master.
Implemented changes from this branch.

Going to wait for CI to pass and do comb thru before opening for review

payjoin-test-utils/src/persist.rs

arminsabouri · 2025-06-09T18:07:00Z

payjoin/src/receive/error.rs

+impl From<&ReplyableError> for JsonReply {
+    fn from(e: &ReplyableError) -> Self {


note to self: Can we revert this? Why was this neccecary?

This is because we persist the JSON replayable error in the terminal session event. And we take ownership in two places

rust-payjoin/payjoin/src/receive/v2/mod.rs

Lines 479 to 480 in 1063687

let inner = self.state.v1.commit_outputs();

Receiver { state: WantsInputs { v1: inner, context: self.state.context } }

Solution here:

Impl clone on the Replayable error

Impl From on a refrence of replayable error.

Impl From on a refrence to an error is not rusty idomatic we can also have a function on JsonReply that takes a refrence to the error

Do you intend on resolving this in this PR or as a follow-up?

I think this would be better left as a follow up. There is a refactor we would need to do to get error types persisted into terminal session events and remove the JSON replies all together and this can get resolved as part of that follow up. Leaving this comment open so can make an follow up issue

payjoin/src/receive/v1/exclusive/error.rs

payjoin/tests/integration.rs

payjoin/src/receive/v2/session.rs

payjoin/src/receive/v2/mod.rs

payjoin-ffi/python/test/test_payjoin_integration_test.py

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

payjoin-ffi/src/receive/mod.rs

payjoin-ffi/src/receive/uni.rs

arminsabouri

Noticed a couple things. Additionally, all the state transition methods can now be pub(crate)'d. That should be done in its own commit

payjoin-ffi/tests/bdk_integration_test.rs

payjoin/src/receive/v2/session.rs

payjoin/src/receive/v2/mod.rs

arminsabouri · 2025-06-16T16:07:53Z

Last push reverts the change that moved state transition save()'s internally. Keeping the type states concerned purely with computing the next state allows us to have first class support for async runtimes in the future.

payjoin/src/receive/v2/session.rs

spacebear21

IMO a95bdd8 does too many things and should be split up into smaller chunks to reduce the cognitive burden on reviewers and help future contributors understand what happened here. I would try to split these into standalone commits:

Rename persist.rs to session.rs
Rename NewReceiver to UninitializedReceiver
Introduce ReceiverTypeState and the apply functions
Introduce SessionHistory, replay_event_log and the ReplayError type
Remove receiver_ser_de_roundtrip - It's unclear to me what change this relates to, perhaps it was a leftover from when test_session_event_serialization_roundtrip was introduced in receive/persist.rs?
payjoin-cli changes
Remove the obsolete persist and load methods and, and probably the Persister trait and NoopPersister implementation too (I don't think they're needed anymore?).

The commit message says:

Changes to v1/mod.rs allow the session history manager to expose the PSBT at this specific state. This update addresses a requirement from the Liana integration, where the contributed PSBT must be saved to a separate database.

I don't see a change in v1/mod.rs regarding this. If there is such a change, it sounds like it should be a standalone commit.

spacebear21 · 2025-06-17T01:38:55Z

payjoin/src/receive/error.rs

+impl From<&ReplyableError> for JsonReply {
+    fn from(e: &ReplyableError) -> Self {


Do you intend on resolving this in this PR or as a follow-up?

payjoin/src/receive/v1/exclusive/error.rs

payjoin/tests/integration.rs

payjoin/src/receive/v2/mod.rs

arminsabouri · 2025-06-17T11:00:16Z

IMO a95bdd8 does too many things and should be split up into smaller chunks to reduce the cognitive burden on reviewers and help future contributors understand what happened here. I would try to split these into standalone commits:

Rename persist.rs to session.rs

Rename NewReceiver to UninitializedReceiver

Introduce ReceiverTypeState and the apply functions

Replay error type would need to get introduce here.

Introduce SessionHistory, replay_event_log and the ReplayError type

Remove receiver_ser_de_roundtrip - It's unclear to me what change this relates to, perhaps it was a leftover from when test_session_event_serialization_roundtrip was introduced in receive/persist.rs?

Not leftover bc we we're still doing persistence via the persister trait as of #760. a95bdd8 deprecates it and replaces with persisting session events.

payjoin-cli changes
Note that these changes would have to be in lock step with deprecating the old persister trait.

Remove the obsolete persist and load methods and, and probably the Persister trait and NoopPersister implementation too (I don't think they're needed anymore?).

Sender is still using these.

The commit message says:

Changes to v1/mod.rs allow the session history manager to expose the PSBT at this specific state. This update addresses a requirement from the Liana integration, where the contributed PSBT must be saved to a separate database.

I don't see a change in v1/mod.rs regarding this. If there is such a change, it sounds like it should be a standalone commit.

arminsabouri · 2025-06-17T12:45:25Z

One more commit added here to exported api side errors in ffi state transition methods
3096f39

DanGould

This keeps getting bigger!???!? It seems like it is in workable state but our best option may be to consider merging what we have at the end of the day and then listing the follow ups so that the work can be divided.

Sure, I'd (much) rather the commits were smaller to begin with but the facts on the ground are that I don't think either you @0xBEEFCAF3 or @spacebear21 are completely opposed to what we have here and our best course of action is probably to do systematic re-review once this PR is merged of a list of the priorities that need to get done so that we can ship this into the world.

I particularly like 623c40e because that match was stressing me out. I definitely have concerns with the way things are named in this PR, but again, we can solve that after.

And in considering the name changes I think I figured out why the TerminalState special case is wonky and shouldn't actually be a special case, which I touch on below.

I don't want to leave my ACK, because it's 02:19 and I really want Spacebear to look again before merge, but I think we're quite close to good enough for merge, have learned a significant amount we can apply to next time, and this gets us over the hump to actually roll the release.

Your perseverance is palpable ⭐️

payjoin/src/receive/v2/persist.rs

DanGould · 2025-06-19T05:45:30Z

payjoin/src/receive/v2/persist.rs

+    pub fn pj_uri<'a>(&self) -> Option<PjUri<'a>> {
+        self.events.iter().find_map(|event| match event {
+            SessionEvent::Created(session_context) => {
+                // TODO this code was copied from ReceiverWithContext::pj_uri. Should be deduped
+                use crate::uri::{PayjoinExtras, UrlExt};
+                let id = session_context.id();
+                let mut pj = subdir(&session_context.directory, &id).clone();
+                pj.set_receiver_pubkey(session_context.s.public_key().clone());
+                pj.set_ohttp(session_context.ohttp_keys.clone());
+                pj.set_exp(session_context.expiry);
+                let extras = PayjoinExtras {
+                    endpoint: pj,
+                    output_substitution: OutputSubstitution::Disabled,
+                };
+                Some(bitcoin_uri::Uri::with_extras(session_context.address.clone(), extras))
+            }
+            _ => None,
+        })
+    }
+
+    /// Payment amount from the Payjoin URI
+    pub fn pj_uri_amount(&self) -> Option<bitcoin::Amount> { self.pj_uri().map(|uri| uri.amount)? }

-    fn key(&self) -> Self::Key { ReceiverToken(self.context.id()) }
+    /// Payment address from the Payjoin URI
+    pub fn payment_address(&self) -> Option<bitcoin::Address<bitcoin::address::NetworkChecked>> {
+        self.pj_uri().map(|uri| uri.address)
+    }
+
+    /// Fallback txid from the original proposal
+    pub fn fallback_txid(&self) -> Option<bitcoin::Txid> {
+        self.events.iter().find_map(|event| match event {
+            SessionEvent::UncheckedProposal((proposal, _)) =>
+                Some(proposal.psbt.unsigned_tx.compute_txid()),
+            _ => None,
+        })
+    }
+
+    /// Fallback tx from the original proposal
+    pub fn fallback_tx(
+        self,
+    ) -> Option<Result<bitcoin::Transaction, bitcoin::psbt::ExtractTxError>> {
+        self.events.into_iter().find_map(|event| match event {
+            SessionEvent::UncheckedProposal((proposal, _)) => Some(proposal.psbt.extract_tx()),
+            _ => None,
+        })
+    }
+
+    /// Original psbt from the original proposal
+    pub fn original_psbt(&self) -> Option<bitcoin::Psbt> {
+        self.events.iter().find_map(|event| match event {
+            SessionEvent::UncheckedProposal((proposal, _)) => Some(proposal.psbt.clone()),
+            _ => None,
+        })
+    }
+
+    /// Proposed payjoin psbt from the payjoin proposal
+    pub fn proposed_payjoin_psbt(&self) -> Option<bitcoin::Psbt> {
+        self.events.iter().find_map(|event| match event {
+            SessionEvent::PayjoinProposal(proposal) => Some(proposal.psbt().clone()),
+            _ => None,
+        })
+    }
+
+    /// Psbt with receiver contributed inputs
+    pub fn psbt_with_contributed_inputs(&self) -> Option<bitcoin::Psbt> {
+        self.events.iter().find_map(|event| match event {
+            SessionEvent::ProvisionalProposal(proposal) => Some(proposal.payjoin_psbt.clone()),
+            _ => None,
+        })
+    }
+
+    /// Terminal error from the session if present
+    pub fn terminal_error(&self) -> Option<(String, Option<JsonReply>)> {
+        self.events.iter().find_map(|event| match event {
+            SessionEvent::SessionInvalid(err_str, reply) => Some((err_str.clone(), reply.clone())),
+            _ => None,
+        })
+    }


I'd like to prune the nonesential of these functions (anything that can be pulled from pj_uri) in an immediate follow-up. In the future, prune anything that's not used in the PR or we know to have immediate use downstream please it simplifies review.

Fair comment. The ones that are in use are

Pj_uri -> used to get the ohttp relay when we need to send the err req.

return Err(handle_recoverable_error( reply_error, session, &self .unwrap_relay_or_else_fetch(Some( session_history .pj_uri() .expect("Session should exist") .extras .endpoint() .clone(), )) .await?, )

psbt_with_contributed_inputs : is used by Liana specifically. Liana will only sign PSBTs in a dedicated Table. This method is used to extract the psbt with reciever inputs so we can save it in this table.

terminal_error: is used when we need to call extract_err_req

Note about fallback_tx: Right now the refrence impl is still using the method off UncheckedProposal. And Liana IIRC is saving the fallback psbt when it first receives it. I'd like to unify both implementations to use the session history fallback tx method. We can remove it now and re-introduce it when we use it in the refrence impl. This can be done in an immediate follow up.

my issue is more with the params off of pj_uri, the amount of the request and the address since both can be altered by output substitution their names are ~misleading and their fields are already available from the the PjUri type

psbt_with_contributed_inputs still seems like a hack since the caller has the inputs to manage itself, and Liana should probably handle liana-specific details, but no reason for a veto. maybe the name should actually be unsigned_proposal_psbt. We can revisit this when reviewing the liana integration

re: fallback tx docs: fallback tx is the transaction extracted from the Original PSBT in BIPS 77/78 parlance. There is no "Original proposal", there are Original PSBT and Proposal PSBT afaiu.

payjoin-ffi/python/test/test_payjoin_integration_test.py

payjoin-ffi/src/receive/mod.rs

DanGould · 2025-06-19T05:58:48Z

payjoin/src/receive/v2/mod.rs

+/// Each variant wraps a `Receiver` with a specific state type, except for `TerminalState` which
+/// indicates the session has ended or is invalid.
+#[derive(Debug, Clone, PartialEq)]
+pub enum ReceiverTypeState {


The name of this has been bugging me. AnyReceiver or ReceiverEnum might be more appropriate so that there's no confusion with Receiver<State>'s receiver::State.

I'm pretty sure making TerminalState a receiver::State could also fix the SessionInvalid error type shenanigans in SessionEvent. TerminalState could the error and can produce / remember the JsonError, which would follow the same pattern nearly every other SessionEventObject containing the state type. Perhaps even every state in SessionEvent could reflect an AnyReceiver variant's internal state.

DanGould · 2025-06-19T06:07:29Z

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

    }

-    fn process_v2_proposal(
+    #[allow(clippy::incompatible_msrv)]


Gotta open an issue to fix these incompatible_msrv exceptions. They're not MSRV compliant!

DanGould · 2025-06-19T06:10:16Z

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

+    async fn commit_outputs(
+        &self,
+        proposal: Receiver<WantsOutputs>,
+        persister: &ReceiverPersister,
+    ) -> Result<()> {
+        let proposal = proposal.commit_outputs().save(persister)?;
+        self.contribute_inputs(proposal, persister).await
+    }


I don't find way these are chained to be intuitive. Why does commit_outputs call contribute_inputs? contribute_inputs doesn't seem like a part of commit_outputs at all.

and so on for each of these transitions. Because of this, I don't find thier separation into functions to be improving readability and would prefer them to be called imperatively in a list.

Perhaps we just need a naming change. e.g process_commit_outputs_state.
Alternatively what can be done is to return the sumtype and iterate in process_receiver_proposal until the session fails or succeeds. I find this to be less intuitive.

I don't think a rename to process_commit_output_state resolves my concern here.

would a single function that looped over a match on the sumtype, where each branch called a process function work? The weird part is the nested chain even more than the fact that the function calls another function unrelated to the parent's name.

would a single function that looped over a match on the sumtype, where each branch called a process function work?

Yes this would work

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

This commit updates the `v2::Receiver` state transition functions 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` Serialization and Deserialization Error Code Terminal session events now store JSON replies. These events must derive `serde::Serialize` and `serde::Deserialize` to support serialization and deserialization. `v1/mod.rs` The session history object enables application developers to introspect a session and retrieve the Payjoin PSBT after inputs have been contributed. Changes to `v1/mod.rs` allow the session history manager to expose the PSBT at this specific state. This update addresses a requirement from the Liana integration, where the contributed PSBT must be saved to a separate database. `session.rs` This file now contains the core logic for the session history manager, receiver session events, and associated error types. `receive/v2/mod.rs` Introduced a sum type over all possible receiver states. This sum type processes a session event and, if the event is valid for the current state, progresses to the next state by calling the typestate's `apply` function. The `apply` function provides a shorthand mechanism to transition to the next state based on the session event. Replaced `NewReceiver` with `UninitializedReceiver` and introduced a `create_session` method, which takes the same parameters as `NewReceiver::new`. Updated each state transition method to match on specific errors. The system classifies errors as either transient (`ImplementationError`) or fatal (`SessionError`). Changes to Payjoin-CLI Database Introduced a session wrapper struct to persist session events. This wrapper will support both sender and receiver sessions. As session updates occur, the system deserializes the session wrapper, appends the new event log, and reserializes the session. Each session has a unique `u64` identifier, derived via `serde`. When a session completes (either successfully or due to a terminal error), the session persister records a `completed_at` timestamp. Application Refactored the receiver session handling to eliminate compile-time state assumptions. Introduced a unified handler that matches on any receiver state and drives the session to completion. Each state handler now exists as an individual method, and each method invokes the next state handler until the session completes successfully or encounters a terminal error. Lastly, replaced `try_contributing_inputs` with `contribute_inputs` for clarity and consistency.

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`. Additionally, this commit introduces `JsonReceiverSessionPersister`, enabling applications to persist and load session events using exposed to_json and from_json methods on `ReceiverSessionEvent`.

FFI state transitions objects were returning `ImplementationError` which made no use of our FFI exported receiver error types. The new `PersistedError` type will wrap around api side error if they occur and use `ImplementationError` in the case of an application storage related error

This commit refactors the extraction of error requests and the handling of responses. Previously, these methods were only available on the `UncheckedProposal` typestate. However, a session may fail at any typestate, and the receiver must send an error response to the sender if the failure is fatal. The extract_err_req and process_err_res functions are now implemented as pure functions. By replaying the session event log, applications can call `extract_err_req` at any point. If the session is in a terminal state, the function exposes the associated Request and OHTTP context.

This commit adds test coverage for `replay_receiver_event_log` and the session history object produced by replaying the logs. The tests persist session event logs and replay them to verify that the resulting state matches the expected state.

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

spacebear21

utACK. I think FFI in particular may need a closer look after this is merged, probably in parallel with the uniffi-dart integration, but I don't feel strongly enough about any particular thing to keep this PR stuck any longer.

I agree the best course of action would be to merge this and iterate on it as we prepare the release. I'm thinking the best way to do this is to start a new tracking issue for persistence/0.24 release?

spacebear21 · 2025-06-19T16:42:11Z

payjoin-ffi/src/receive/uni.rs

+    #[uniffi::constructor]
+    // TODO: no need for this constructor. `create_session` is the only way to create a receiver.
+    pub fn new() -> Self { Self {} }
+


Should this be removed?

The issue is create_session does not return Self and uniffi does not like this.

payjoin-ffi/python/test/test_payjoin_integration_test.py

spacebear21 · 2025-06-19T17:57:13Z

payjoin-ffi/src/receive/error.rs

+    /// Error that may occur when converting a some type to a URL
+    #[error("IntoUrl error: {0}")]
+    IntoUrl(Arc<IntoUrlError>),


Is this new variant necessary? Ideally the ffi API should map as closely as possible to the Rust API.

I think we can get around having to add this variant

arminsabouri commented Jun 6, 2025

View reviewed changes

payjoin-cli/src/db/v2.rs Outdated Show resolved Hide resolved

arminsabouri commented Jun 6, 2025

View reviewed changes

payjoin-ffi/python/test/test_payjoin_integration_test.py Outdated Show resolved Hide resolved

arminsabouri commented Jun 6, 2025

View reviewed changes

payjoin-test-utils/src/persist.rs Outdated Show resolved Hide resolved

arminsabouri commented Jun 6, 2025

View reviewed changes

payjoin/src/persist.rs Show resolved Hide resolved

DanGould reviewed Jun 7, 2025

View reviewed changes

payjoin/src/receive/v2/session.rs Outdated Show resolved Hide resolved

arminsabouri force-pushed the recv-state-transition-objects branch from f860f88 to 9d4b630 Compare June 9, 2025 16:00