Name audit for 0.24 release

[DanGould]

No description provided.

[coveralls]

Pull Request Test Coverage Report for Build 15893586163

Details

• 14 of 15 (93.33%) changed or added relevant lines in 5 files are covered.
• No unchanged relevant lines lost coverage.
• Overall coverage remained the same at 86.155%

---

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
payjoin/src/send/v2/mod.rs	0	1	0.0%

Totals
Change from base Build 15884774436:	0.0%
Covered Lines:	7909
Relevant Lines:	9180

---

💛 - Coveralls

[spacebear21]

Since send::v2::V2PostContext was removed, what do you think of renaming V2GetContext to something more relevant to the Sender state machine?

pub enum SenderTypeState {
    Uninitialized(),
    WithReplyKey(Sender<WithReplyKey>),
    V2GetContext(Sender<V2GetContext>), // ProposalPending? PollingProposal?
    ProposalReceived(Psbt),
    TerminalState,
}

[DanGould]

I have a big problem with both of the sumtypes. I don't think they're properly sumtypes as-is. I think they should be

pub enum AnySender {
    Uninitialized(Sender<Uninitialized>), // (or Sender(Uninitielized)
    WithReplyKey(Sender<WithReplyKey>),
    V2GetContext(Sender<V2GetContext>), // ProposalPending? PollingProposal?
    ProposalReceived(Sender<ProposalReceived>), // struct ProposalReceived(Psbt);
    TerminalState(Sender<TerminalError>)
}

Then they could even be reduced to

pub enum AnySender {
    Uninitialized(Uninitialized), // (or Sender(Uninitielized)
    WithReplyKey(WithReplyKey),
    V2GetContext(V2GetContext), // ProposalPending? PollingProposal?
    ProposalReceived(ProposalReceived), // struct ProposalReceived(Psbt);
    TerminalState(TerminalError)
}

impl AnySender {
    fn into_sender(self) -> Sender<State> {
        // ...
    }
}

And yes I think there is a more descriptive name of V2GetContext to PollingForProposal or similar

I think I remember Armin saying this can't work but I want to document why not if that is indeed the case.

[arminsabouri]

I have a big problem with both of the sumtypes. I don't think they're properly sumtypes as-is. I think they should be

pub enum AnySender {
    Uninitialized(Sender<Uninitialized>), // (or Sender(Uninitielized)
    WithReplyKey(Sender<WithReplyKey>),
    V2GetContext(Sender<V2GetContext>), // ProposalPending? PollingProposal?
    ProposalReceived(Sender<ProposalReceived>), // struct ProposalReceived(Psbt);
    TerminalState(Sender<TerminalError>)
}

Then they could even be reduced to

pub enum AnySender {
    Uninitialized(Uninitialized), // (or Sender(Uninitielized)
    WithReplyKey(WithReplyKey),
    V2GetContext(V2GetContext), // ProposalPending? PollingProposal?
    ProposalReceived(ProposalReceived), // struct ProposalReceived(Psbt);
    TerminalState(TerminalError)
}

impl AnySender {
    fn into_sender(self) -> Sender<State> {
        // ...
    }
}

And yes I think there is a more descriptive name of V2GetContext to PollingForProposal or similar

I think I remember Armin saying this can't work but I want to document why not if that is indeed the case.

I think it could work but I can't think of any case where we need the state struct but not the typestate. Take a look at how the sumtype is used in payjoin cli after resume. The state structs being the template for the data are useful for the multiparty constructions that impl their own typestates. And we don't really need a union / sumtype for that

[DanGould]

This is a sort of first-round name audit. There are some more things I'd like to change but wish to progress on these things that can be independently considered.

[DanGould]

rebased

[DanGould]

Take a look at how the sumtype is used in payjoin cli after resume.

This is one of my biggest bugbears with the existing design. The match session there isn't a loop or even a straightforward procedure that goes from one state to the next but actually has 2 places where each of the state transition calls (e.g. self.contribute_inputs) is called / can enter.

It seems possible for that match session to handle both the entry and the continued calls after rather than calling them in 2 separate places.

Investigating.

[spacebear21]

concept ACK.

I feel like Initialized could be more descriptive (what was initialized? ohttp keys? a directory mailbox? a db entry?) but don't have an immediate name suggestion.

[spacebear21]

Sender typestate goes Uninitialized -> WithReplyKey. Those should at least be consistent with one another.

[DanGould]

I'm following up with a Receiver that takes sessioncontext out of the state (because it remains for all states) where Uninitialized has no Receiver / state at all

#[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)]
pub struct Receiver<State: ReceiverState> {
    pub(crate) state: State,
    pub(crate) context: Arc<SessionContext>,
}

In that context this will make more sense.

[arminsabouri]

This is one of my biggest bugbears with the existing design. The match session there isn't a loop or even a straightforward procedure that goes from one state to the next but actually has 2 places where each of the state transition calls (e.g. self.contribute_inputs) is called / can enter.
It seems possible for that match session to handle both the entry and the continued calls after rather than calling them in 2 separate places.

There is nothing preventing a loop. In fact that is how it was implemented at onepoint IIRC. I simply find it more straightfoward have methods both do the state transition and continute progressing the state machine until you get to the desired state and then return (). Otherwise you have to return a sumtype and match again. But this point seems unrelated to the the first comment (?) . You still need a typestate to access the state transition methods

[DanGould]

Please bear with me as I audit what we merged in these PRs with a finer-toothed comb. The bugbear (if that's what "first comment" was referring to) is indeed unrelated to the first comment, but a direct response to your quote. I'm erring toward overcommunication here so that we can index our discussions.

When I said

I have a big problem with both of the sumtypes. I don't think they're properly sumtypes as-is.

I think I was just wrong. "Proper" there was just a preference and not related to a specific definition of sumtype or best practice. I was trying to find out why the heck the pattern is the way it is.

After more digging, the concrete reason we need the ReceiverTypeState is because of type erasure by enum as an alternative to dynamic dispatch with trait objects, which is even uglier. I will document this finding on the enum and likely rename it for more clarity as well.

I think it might be able to be addressed with a trait transition for each of the Receiver<State> types e.g.

pub trait ProcessEvent: Sized {
    /// Attempts to transition from the current state to the next by processing the relevant event.
    fn process_event(self, event: SessionEvent) -> Result<ReceiverTypeState, ReplayError>;
}

where the event matching could be done with a macro that handled the non-transition functions.

which makes the logic in the match more less verbose (just calls to receiver.transition(event) for most arms) keeping the logic in the actual Receiver impl, which is close to what we have with the apply_... functions but should be one less function per typestate since apply_... would become the process_event impl.

[arminsabouri]

Ack ba6f515

[arminsabouri]

Nit the commit says

shared SessionContext can be moved to Receiver and out of state field.

Is this justiification for the rename or something that should be done in this commit?

[DanGould]

Another PR that is coming imminently since it does affect the Serialization

[arminsabouri]

Meant to approve