Conversation
benalleng
force-pushed
the
sender-closed-variant
branch
from
60b9209 to
88d1d1e
Compare
Pull Request Test Coverage Report for Build 18175422394Details
💛 - Coveralls |
benalleng
force-pushed
the
sender-closed-variant
branch
from
88d1d1e to
9da6ee4
Compare
arminsabouri
left a comment
arminsabouri
left a comment
There was a problem hiding this comment.
Approach Ack. Two small nits
spacebear21
left a comment
spacebear21
left a comment
There was a problem hiding this comment.
I think ReceivedProposalPsbt is a distinct event from a successful session closure. We might need to expose a function allowing implementers to mark a send session as closed successfully if monitoring won't be part of the sender state machinery?
payjoin/src/core/send/v2/session.rs
Outdated
| /// Closed successful or failed session | ||
| Closed(SessionOutcome), | ||
| } | ||
|
|
||
| /// Represents all possible outcomes for a closed Payjoin session | ||
| #[derive(Debug, Clone, serde::Serialize, serde::Deserialize, PartialEq, Eq)] | ||
| pub enum SessionOutcome { | ||
| /// Payjoin completed successfully | ||
| ReceivedProposalPsbt(bitcoin::Psbt), |
There was a problem hiding this comment.
I think ReceivedProposalPsbt should be a distinct SessionEvent from SessionOutcome::Success: we could receive the proposal psbt but the fallback transaction still got broadcasted in the meantime. That would result in a SessionEvent::ReceivedProposalPsbt and SessionEvent::Failure
There was a problem hiding this comment.
Should it be a different Closed Outcome or remain an active Event?
There was a problem hiding this comment.
We concluded that ReceivedProposalPsbt should not yet be treated as a Closed Event as a fallback could come in the meantime and thus the transaction would ultimately be successful but the payjoin would still be considered a failure.
benalleng
force-pushed
the
sender-closed-variant
branch
3 times, most recently
from
aed391c to
c63224d
Compare
By adding a Closed state for sender `SessionEvent`s we can consolidate the failed, succeded, and canceled states into one enum to keep eveyrting more concise and clear. The Session outcomes are as follows, `ReceivedProposalPsbt(bitcoin::psbt)` which represents the success state, `FailedSession(String)` wich represntes the failure state and accepts an error string, and `Cancel` which represents a manual cancel.
Adds a SessionStatus enum and accessor to get the current status of the Sender, this differs slightly from the receiver as it does not caluclate expired statuses at the moment.
benalleng
force-pushed
the
sender-closed-variant
branch
from
c63224d to
9666b08
Compare
4 participants
By adding a Closed state for sender
SessionEvents we can consolidatethe failed, succeded, and canceled states into one enum to keep
eveyrting more concise and clear.
The Session outcomes are as follows,
ReceivedProposalPsbt(bitcoin::psbt)whichrepresents the success state,
FailedSession(String)wich represntesthe failure state and accepts an error string, and
Cancelwhichrepresents a manual cancel.
Addresses some of #1119
Pull Request Checklist
Please confirm the following before requesting review:
AI
in the body of this PR.