Add SessionPersister trait

[arminsabouri]

SessionPersister is an alternative to Persister designed to persist not just the initial state of a BIP78 type state machine, but the full sequence of state transitions. Its explicit purpose is to allow sessions to be replayed from their event history and to give applications the ability to introspect their session’s progress (#336).

A SessionPersister is scoped to a specific BIP78 sender or receiver session. Each session consists of discrete pieces of information obtained during execution called session events. These session events are stored via the save_event method. When an application resumes a session, it loads these events into a replayer (not included in this commit) to recover the latest session state. If the type state reaches its terminal state or a fatal error occurs, the session is marked as closed via close().

Application developers should not interact directly with SessionPersister. Instead, they interact indirectly by processing state transition objects. These objects represent the possible outcomes of attempting a state transition, and they must be processed for the type state to advance or terminate.

There are six kinds of BIP78 state transition outcomes:

• A successful state transition that always progresses.
• A transition that may result in a transient error. In these cases, the application should retry the session from its current state. These errors originate outside of rust-payjoin. E.g an ImplementationError triggered when executing an InputsSeen closure.
• A fatal transition that must close the session. These errors are unrecoverable, requiring the fallback transaction to be broadcast. Fatal transitions also include an event to be persisted for auditing purposes and may also contain a transient error.
• Transitions that are either fatal or have no result (interpreted as success). This is common for transitions handling responses to BIP78 subdirectory GET requests.
• A transition that marks the final, successful state of the type state.
• The initial transition, which is a special case. If it fails, there is no prior state to retry from and nothing to save or close.

State transitions are opaque to the application. Each transition method implements a save() function, which internally delegates to the appropriate InternalPersistedSession trait method. These internal methods are strongly typed to ensure correctness and are not accessible to the application layer. Only the save() methods are exposed.

Each save() call returns a Result indicating either the next state transition (if there is one) or a PersistedError, which captures all possible failure modes e.g., Fatal, Transient, etc.

[arminsabouri]

@nothingmuch I'm thinking #675 can be broken down into four PRs. This, a refactor to make receiver and sender idomatic type state, Type state return state transition objects and persited with SessionPersister and lastly uniffi bindings

[coveralls]

Pull Request Test Coverage Report for Build 15261618388

Details

• 582 of 597 (97.49%) changed or added relevant lines in 1 file are covered.
• No unchanged relevant lines lost coverage.
• Overall coverage increased (+1.1%) to 84.553%

---

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
payjoin/src/persist.rs	582	597	97.49%

Totals
Change from base Build 15195558696:	1.1%
Covered Lines:	6519
Relevant Lines:	7710

---

💛 - Coveralls

[arminsabouri]

This file is getting crowded. These state transition objects should live in their own file under a persist submod. This file should just define the various traits

[DanGould]

ACK 839fccd

I went through the structs and combed over the testing you wrote here and it all reads as sane.

Overall looking good. I understood what to review based on the commit message and my comments are fundamentally nits.

re: the comment and commit log

which internally delegates to the appropriate InternalPersistedSession trait
method.

I don't see an InternalPersistedSession trait. Do you mean SessionPersister?
or InternalSessionPersister? If the name is indeed InternalSessionPersister I would like to see the commit log reflect that.

[DanGould]

I'm not sure what's internal to the payjoin crate about this. Nor do I see a StorageError in contrast.

Prefer StorageError or ApplicationStorageError or ImplementationStorageError if
indeed this is produced externally to the payjoin crate in which it is written. Based on the docstring
I'm guessing ImplementerStorageError might be more appropriate.

[arminsabouri]

Has been changed to ImplementerStorageError.

[DanGould]

When I first came across the PersistedSuccessWithmaybeNoResults I figured the success transition it defines would be persisted. every single time. Based on this code that's not what happens. I'm glad that the transient success case appears not to be persisted, but I still found the name a bit confusing.

This also ties to some confusion with the MaybeFatalRejection name. Why not MaybeTransientRejection (I am not seriously suggesting this, but trying to understand why one variant was chosen over the other)? Would Rejection not be more appropriate with Fatal and Transient variants?

Perhaps a simplification to PersistedSuccessWithMaybeNoResults's name could also be something like:

pub enum Success<NextState, CurrentState> { // ~2xx codes = success
    Ok(NextState), // ~200
    Accepted(CurrentState), // ~202
}

or

pub enum Success<NextState, CurrentState> { // ~2xx codes = success
    Progress(NextState), // ~200
    Stasis(CurrentState), // ~202
}

My point with this long comment is that I imagine the purpose of the long variable names is to clarify, but I'm not so sure the long variable names convey what they are clearly yet.

[arminsabouri]

Progress and stasis makes sense. I'll make that change.

Would Rejection not be more appropriate with Fatal and Transient variants?

Not sure I understand this. Fatal and Transient wrappers should be suffixed or prefixed with _rejection. e.g
pub struct RejectFatal<Event, Err>(Event, Err);

My point with this long comment is that I imagine the purpose of the long variable names is to clarify, but I'm not so sure the long variable names convey what they are clearly yet.

Yeah its hard to capture every outcome in the var name without adding a load of Ors and And in the var name. I've attempted to standardize it where I can. For example, state transitions objects all end in _transition and perhaps we just need to do more of that. I'll think about this a bit more and I'm open to suggestions.

[DanGould]

Still a bit confused about PersistedSucccessWithMaybeNoResults (Which has 3 'c's in Succcess btw.

Is the Stasis condition persisted every single time as the name suggests?

[DanGould]

Would Rejection not be more appropriate with Fatal and Transient variants?

My point is that the commonality between the two variants is not that they're maybe fatal, but that they're rejections:

/// Wrapper representing a fatal error or a transient error
pub enum Rejection<Event, Err> {
    Fatal(RejectFatal<Event, Err>),
    Transient(RejectTransient<Err>),
}

"Wrapper" in the docstrings is a bit confusing to me as well because the docstring doesn't explain why the thing is wrapped. For instance:

/// Wrapper representing a fatal error
pub struct RejectFatal<Event, Err>(Event, Err);

Suggests RejectFatal is an error type but it's verb named like a trait would be and called a wrapper, presumably for some type safety. Is it not just a FatalRejectionError? Or is it not an error type?

These are definitely nits that don't need to be addressed to be merged, but our chance to get names right is here early so I figured I'd bring it up as it came to mind.

[arminsabouri]

Still a bit confused about PersistedSucccessWithMaybeNoResults (Which has 3 'c's in Succcess btw.

Is the Stasis condition persisted every single time as the name suggests?

I understand the confusion. No the stasis condition is not persisted. Perhaps a better name for this enum is simply TransitionOutcome.

[arminsabouri]

Is it not just a FatalRejectionError? Or is it not an error type?

Its not an error type. The error that gets returned from save() is the generic Err which is a public error rust-payjoin error type.

pub enum Rejection<Event, Err>

My intention with the "maybe" prefix was the indicate that it could be either one of the rejection variant. Looking back at it now its more confusing than helpful. "Rejection" covering both enum variants makes sense to me.

[arminsabouri]

re: the comment and commit log

which internally delegates to the appropriate InternalPersistedSession trait
method.

I don't see an InternalPersistedSession trait. Do you mean SessionPersister? or InternalSessionPersister? If the name is indeed InternalSessionPersister I would like to see the commit log reflect that.

That was a refrence to the old trait name. Thanks for catching that. Fixed.

[DanGould]

ACK aa79c0c

confirmed the only change from 839fccd in the last review was the variant rename

[arminsabouri]

Last push includes:

• Rename MaybeFatalRejection enum to Rejection
• Better rustdocs for state transition wrapper types
• Rename PersistedSucccessWithMaybeNoResults to OptionalTransitionOutcome

[arminsabouri]

Last push d70d64e also renames AcceptWithMaybeNoResults -> AcceptOptionalTransition so its on par with OptionalTransitionOutcome.

[DanGould]

ACK 839fccd

This is more readable thanks for the changes. Let's audit the names once all the pieces have been merged before the next release to make sure they make sense.