What's up with `send::v2::Sender`

[DanGould]

First, if there's no common factor between states, why even have a generic container type and not use the State types directly as we once did. It seems like the enum handles what the Sender once did. IMO this is distinct from Receiver where SessionContext is shared between each Receive state.

#[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)]
pub struct Sender<State> {
    pub(crate) state: State,
}

Second, why are we missing a V2PostContext state with ProposalReceived? That one variant strays from the patterns the rest use

pub enum SendSession {
    Uninitialized,
    WithReplyKey(Sender<WithReplyKey>),
    V2GetContext(Sender<V2GetContext>),
    ProposalReceived(Psbt),
    TerminalFailure,
}

[arminsabouri]

I'll address the second point first. V2PostContext is not a typestate and has no place in the session sumtype. I refer to the commit notes

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.

It seems like the enum handles what the Sender once did

I'm not sure what you mean. The Sender use to be a type state created off the builder. The enum / sumtype reprensents a particular state of a sender session.

State types directly as we once did.

1. There is a common field. Isnt v1 a common? Its not currently part of Sender the same way session context isnt but it could be.
2. This is an idomatic typestate construction. The previous sender session state machine was a collection of structs that are seemingly unreleated and unrelated with the bip77 sender session. If you go from a Sender<GetContext> to a Sender<PostContext> its obvious that you progressed the sender session state machine from one state to another. And we get this benefit at virutally no compile or runtime cost -- or complexity to the FFI layer which was the previously stated reason as to why we didn't implemented the idomatic pattern in the first place. We should aim to use idomatic patterns where we can. And we should maintain the same pattern across both sender and reciever. Both of which reduces developer onboarding.

[DanGould]

V2PostContext is not a typestate and has no place in the session sumtype

understood. Thanks for clearing that up.

Isnt v1 a common?

No. Only WithReplyKey has a v1::Sender:

#[derive(Debug, Clone, Serialize, Deserialize, PartialEq, Eq)]
pub struct V2GetContext {
    /// The endpoint in the Payjoin URI
    pub(crate) endpoint: Url,
    pub(crate) psbt_ctx: PsbtContext,
    pub(crate) hpke_ctx: HpkeContext,
}

I've stated before that endpoint/psbt_ctx could be shared in v1::Sender, and this might be the time to do it.

[DanGould]

I think the way to clean the separation between v1/v2/multiparty is to acknowledge the the PsbtContext rules are largely the same, so keep those. Rather than have v2 inherit v1, they both just share PsbtContext utilities. Perhaps PsbtContext gets a builder itself and then each {v1,v2}::SenderBuilder calls on this PsbtContext builder.

Payjoin is operates in two layers: 1. the transaction negotiation and 2. the serialization and networking and this distinguishes the roles. v2 does not inherit v1's networking, so it makes more sense to me to distinguish at the serialization/network layer instead of have a v2/v1 inheritance relationship.

[DanGould]

Another oddity I found is that the v2::SenderBuilder produces both an event (which can itself be success or failure), AND the sender itself, although the successful SessionEvent also contains the sender.

It seems to me that the only thing that ought to be persisted out of a v2::SenderBuilder is the success event containing the v2::Sender with a reply key. The BuildSenderError error can be shown to the send client immediately, and does not need to be persisted as it is now at the time of writing.

The WithReplyKey and V2GetContext states also structure ~the same data, since V2GetContext's HpkeContext may be constructed immediately from the reply_key and URI. The reason to have distinct states is to expose different functional behavior, but I think the actual data can be consistent across the Sender<State> types. Though at the moment the data structure is duplicated in distinct State definitions.

[arminsabouri]

The BuildSenderError error can be shown to the send client immediately, and does not need to be persisted as it is now at the time of writing.

This is what what the persister currently does.

rust-payjoin/payjoin/src/core/persist.rs

Line 439 in 3cd1c3c

fn save_maybe_bad_init_inputs<NextState, Err>(

This is why first state transitions use a custom state transition object (MaybeBadInitInputsTransition).

[DanGould]

I suppose the transition is necessary to go from SendSession::Uninitialized to the next state. Understood.

[DanGould]

if the error part of MaybeBadInitInputsTransition is not persisted then I don't think we ever need to include it as part of the transition. The error can be handled by the sender/receiver constructor, and then a transition/SessionEvent can be made from the result, which itself is persisted. I think that's why this confused me at first. I understand that the Sender/Receiver in initial state needs to go through save, but that can still be opaque without also encapsulating the Error type to be propagated after the builder and Before the transition is saved.

Seems that the initial transition could just be

pub struct InitialTransition<Event, NextState>(AcceptNextState<Event, NextState>);

I've got a draft of this idea taking shape on this branch

The other thing that's odd to me is the NextState being saved in the struct rather than being extractable from the Event itself.

Wouldn't it make more sense for

/// Wrapper that marks the progression of a state machine
pub struct AcceptNextState<Event, NextState>(pub(crate) Event, pub(crate) NextState);

to be

/// Wrapper that marks the progression of a state machine
pub struct AcceptNextState<Event, NextState>(pub(crate) Event);

impl AcceptNextState<Event, NextState> {
    fn next_state(self) -> NextState { event.next_state() }
}

[arminsabouri]

Seems that the initial transition could just be
pub struct InitialTransition<Event, NextState>(AcceptNextState<Event, NextState>);
I've got a draft of this idea taking shape on this branch

Ack. I don't see any glaring issues with this idea. Seems like a marginal improvement to API ergonomics.

The other thing that's odd to me is the NextState being saved in the struct rather than being extractable from the Event itself.

A session event only stores new information attained from a state machine transition. I am not certain how you would obtain the next typestate without knowing previous events or typestates. If you had the previous typestate you could use the apply_* method to get the next state but then you have a downcasting issue.

[DanGould]

i see. The InitialTransition state is a special case where both the event and NextState are ~the same data.