[zealsham]

This renames the sender session events to describe what happened instead of what the event produced .

This is part of #1119

Pull Request Checklist

Please confirm the following before requesting review:

• I have disclosed my use of
  AI
  in the body of this PR.
• I have read CONTRIBUTING.md and rebased my branch to produce hygienic commits.

[coveralls]

Pull Request Test Coverage Report for Build 18203403187

Details

• 11 of 11 (100.0%) changed or added relevant lines in 2 files are covered.
• No unchanged relevant lines lost coverage.
• Overall coverage remained the same at 84.562%

---

Totals
Change from base Build 18202518786:	0.0%
Covered Lines:	8961
Relevant Lines:	10597

---

💛 - Coveralls

[benalleng]

Nice change, I have a nit that i won't push too hard on if others don't feel too strongly about ti but I think it would be nice to have every SessionEvent be syntactically the same with some object + action, i.e.

• ReplyKeyGenerated
• OriginalPsbtSubmitted
• ProposalReceived
• SessionInvalid

[DanGould]

On the receiver side we named the event to match the apply function. That'd mean reverting PollingForProposal or changing the apply_polling_for_proposal function to match the new name (PolledForProposal, apply_polled_for_proposal ? Is this state when polling happened past tense or when the original PSBT was POSTed as the new name suggests? If the latter, apply_original_payload_submitted makes the most sense for a name imo)

I don't think these names are going to be reflective of the actual events however, until the SessionEvents hold exclusively primary information.

[spacebear21]

The rule of thumb I used for the receiver events is to name the events after the method action that caused it to happen, e.g. identify_receiver_outputs -> IdentifiedReceiverOutputs. I think we should stick with that pattern here.

[zealsham]

On the receiver side we named the event to match the apply function. That'd mean reverting PollingForProposal or changing the apply_polling_for_proposal function to match the new name (PolledForProposal, apply_polled_for_proposal ? Is this state when polling happened past tense or when the original PSBT was POSTed as the new name suggests? If the latter, apply_original_payload_submitted makes the most sense for a name imo)

I don't think these names are going to be reflective of the actual events however, until the SessionEvents hold exclusively primary information.

If we are sticking with "what happened " and not what the event produced then the correct state is when original PSBT was posted .

[benalleng]

I will defer to spacebear's comment here about following the naming pattern by the event after the method action that caused it, I'd just like it to be consistent!

[spacebear21]

For maximum consistency with the Receiver events, ultimately I'd like to see:

pub enum SessionEvent {
    /// Sender was created with a HPKE key pair
    Created(WithReplyKey),
    /// Sender POSTed the Original PSBT, and is waiting to receive a Proposal PSBT
    PostedOriginalPsbt(),
    /// Sender received a Proposal PSBT
    RetrievedProposalPsbt(bitcoin::Psbt),
    /// Closed session
    Closed(SessionOutcome),
}

This is highly correlated with #1114 and #1128 and #1119 (comment), so the order in which those get merged matters.

[benalleng]

the mutants here are being ignored until we remove terminal_error()

[zealsham]

@benalleng looks like #1129 added the same changes for the sender session closing variant . i'll rebase

[spacebear21]

ACK, i noticed some formatting issues that I'm surprised weren't caught in CI.

[spacebear21]

Leftover import

[zealsham]

SessionOutcome isn't a leftover import here as it is needed for the different close sessionEvents variants . The current state of the code-base also refelects this change which i think was added by Ben since he worked on the close variant . i initially did the same thing which is the reason behind this comment #1125 (comment)

[spacebear21]

ACK 5a62fa9