Closed phoebe-lew closed 8 months ago
I would argue a Close
is always "bad". To know if an exchange is successful we use OrderStatus
, not Close
. Currently, in tbdex-js
, tbdex-kt
, and the spec, an exchange is terminal either with Close
or OrderStatus
where Close
indicates a premature termination. In the existing implementations, after an OrderStatus
, you actually cannot add a Close
message.
From the spec section for Close
:
Alice -> PFI: "Not interested anymore." or "oops sent by accident"
PFI -> Alice: "Can't fulfill what you sent me for whatever reason (e.g. RFQ is erroneous, don't have enough liquidity etc.)" or "Your exchange is completed"
a Close can be sent by Alice or the PFI as a reply to an RFQ or a Quote. It indicates a terminal state. No messages can be added to an exchange after a Close.
I would argue a
Close
is always "bad". To know if an exchange is successful we useOrderStatus
, notClose
. Currently, intbdex-js
,tbdex-kt
, and the spec, an exchange is terminal either withClose
orOrderStatus
whereClose
indicates a premature termination. In the existing implementations, after anOrderStatus
, you actually cannot add aClose
message.From the spec section for
Close
:Alice -> PFI: "Not interested anymore." or "oops sent by accident"
PFI -> Alice: "Can't fulfill what you sent me for whatever reason (e.g. RFQ is erroneous, don't have enough liquidity etc.)" or "Your exchange is completed"
a Close can be sent by Alice or the PFI as a reply to an RFQ or a Quote. It indicates a terminal state. No messages can be added to an exchange after a Close.
But there are multiple OrderStatus messages and who knows what the text of the message will be. This isn't better than using Close
to try to decipher if the transaction completed successfully or not. A PFI can indicate "ok", "good", "done", "bueno", "1", ANYTHING. The request is for a boolean field so nothing is lost in translation and wallet apps don't need to look up every PFI's messaging scheme to figure out how to handle the end state of transactions
@diehuxx
In the existing implementations, after an OrderStatus, you actually cannot add a Close message.
This should not be the case...we're currently using Close
as equivalent to "complete" as well, so the client knows to stop polling. Close does not necessarily mean premature termination or error. Alice saying she isn't interested in the quote is terminal but not erroneous.
@diehuxx
In the existing implementations, after an OrderStatus, you actually cannot add a Close message.
This should not be the case...we're currently using
Close
as equivalent to "complete" as well, so the client knows to stop polling. Close does not necessarily mean premature termination or error. Alice saying she isn't interested in the quote is terminal but not erroneous.
Yes, correct. @diehuxx we tell customer agents to continue polling until a Close message has been written to the exchange
In the existing implementations, after an OrderStatus, you actually cannot add a Close message.
This should not be the case...we're currently using
Close
as equivalent to "complete" as well
I see, thanks for clarifying @angiejones @phoebe-lew ! There are few possible solutions that come to mind. Let me know if any of these appeal to you.
Close
ingThe simplest solution that comes to mind is to alter the Exchange
state machine to make Close
the only terminal message kind, and add boolean complete
to Close
data.
The PFI would use Close.data.success
to indicate whether the payment is successful. A successful exchange would be structured like this:
OrderStatus
messages that Alice can poll while the payment(s) are pending.Close
message with Close.data.complete = true
.A failed payment could follow the same sequence of messages except that the final Close
has Close.data.complete = false
.
Close
ingAdd boolean paymentSuccessful
to OrderStatus
data. Decide that both OrderStatus
and Close
can terminate an exchange. A Close
denotes that the exchange was unsuccessful. An OrderStatus
is terminal if and only if OrderStatus.data.paymentSuccessful = true
. If OrderStatus.data.paymentSuccessful
, then Alice should keep polling.
Close
ingThis is the sharpest departure from our current system. It's not my favorite, but I don't hate it.
Remove the concept of Close
messages. Alter OrderStatus
data to have enum string property state
which can be PENDING
, COMPLETE
, or TERMINATED
and string property detail
which is an unstructured string.
OrderStatus.data.state = PENDING
, Alice should keep polling.OrderStatus
with OrderStatus.data.state = TERMINATED
, the exchange has been ended without a successful payment.OrderStatus
with OrderStatus.data.state = COMPLETE
, the payment was successful and the exchange is complete.I've used names complete
or paymentSuccessful
in the above proposals. I have no attachment to those particular names and am open to any suggestions.
@diehuxx Close
is the terminal message, I think we should keep it that way. Adding Close.data.success
as a boolean field would solve the issue.
Close is the terminal message, I think we should keep it that way.
@angiejones I looked at the spec again and it looks like Close
is a terminal message, but it is not the only terminal message according to the spec and all existing implementations. Close
is only allowed directly after an RFQ
or a Quote
. From the spec:
a Close can be sent by Alice or the PFI as a reply to an RFQ or a Quote. It indicates a terminal state. No messages can be added to an exchange after a Close.
If you are interested in avoid breaking changes, we should avoid Proposal 1. Proposal 1 would constitute a breaking change in both message structure and the exchange state machine.
After some additional thought, I recommend an approach that combines aspects of Proposals 2 and 3
state
enum to OrderStatus
.The existing exchange state machine seems sufficient. Close
messages can terminate an exchange directly after an RFQ
or a Quote
. After an Order
, there can be any number of OrderStatus
messages.
To denote whether the payment was successful or not in an OrderStatus
, we should add an optional field called OrderStatus.data.state
which can have three values: COMPLETE
, PENDING
, and TERMINATED
. We should rename the existing OrderStatus.data.orderStatus
to OrderStatus.data.detail
. It remains useful to have an unstructured string to contain additional information that is dependent on the type and context of the payment.
@mistermoe @jiyoontbd @phoebe-lew @KendallWeihe @kirahsapong Seeking review on this proposal.
@diehuxx to your point I think the spec is a bit ambiguous around this. All of the following can be found in the spec:
a Close can be sent by Alice or the PFI as a reply to an RFQ or a Quote.
PFI -> Alice: "Can't fulfill what you sent me for whatever reason (e.g. RFQ is erroneous, don't have enough liquidity etc.)" or "Your exchange is completed"
an explanation of why the exchange is being closed/completed
We should prob aim to clarify this in the text.
In what you've described it sounds like the effect would be that a Close
message cannot be sent after an Order
or OrderStatus
, and the last message in a successful exchange must be an OrderStatus
. In that exchange, you wouldn't find a Close
message at all.
I understand why Alice should not be able to send a Close
after an order, and also why a PFI should not be able to send one immediately after an order. Once Alice submits an order, the PFI should start processing it, so Alice shouldn't be able to takesies backsies. Once the PFI receives an order, it should immediately send an OrderStatus
indicating that it's been received/ is processing. But if Close
is a validNext
message on OrderStatus
, then what's to stop Alice from sending a Close
message at that point.
-edited to add:-
^^ one other thing to note is if OrderStatus
is the only validNext
on an OrderStatus
, what's to stop anyone from sending an OrderStatus
after a "terminal" OrderStatus
message. Close
being our only terminal message feels like a good way to avoid this.
-end of edit-
What I like about having a Close
message exist in all exchanges is it gives our entire exchange a boolean state of open
or closed
. We get the same effect with OrderStatus.data.state === OrderStatus.Terminated || OrderStatus.data.state === OrderStatus.Complete
or OrderStatus.data.state !== OrderStatus.Pending
, even if a bit more verbose.
TBF about breaking changes though, I think all of our reference implementations treat Close
as the final message in all exchanges (pretty sure, not 100% sure), so this would not avoid a breaking change. The only approach that doesn't introduce a breaking change is:
Adding Close.data.success as a boolean field would solve the issue.
Implementations don't have to consume the new boolean field on Close
, and it wouldn't be a breaking change. But if we want to change to RFQ > Quote > Order > OrderStatus
as indicating a complete exchange, a state enum on OrderStatus
is a good way to go.
This was long. Anyway I like Occam's Razor, which says that the simplest approach is often the most correct one.
What I like about having a Close message exist in all exchanges is it gives our entire exchange a boolean state of open or closed.
This is risky when dealing with reversible payments. A broader discussion of refunds is outside the scope of this issue, but it is worth considering the possibility of a refund/chargeback/failure after the fact. It's not clear to me that we need to mark all exchanges as permanently closed once the payments are complete. If a successful exchange is permanently closed, what is the expected behavior when an exchange needs to be "reopened"?
We may not want to require that every exchange eventually be closed.
I'd like to discuss more about this line of the PR description:
I may want to add conditional logic to my app to display certain flows based on if the transaction completed successfully or not
What flows do we have in mind? E.g. If Alice receives a notification from her lightning wallet that she was paid, does she also need a TBDex message telling her she got paid?
I dont think we need to overcomplicate this. Again "Adding Close.data.success as a boolean field" is a very simple solution that doesn't break the current implementation or message flow
Adding the field to Close
does not break the message flow but altering the meaning of Close
to be required at the end of all message flows to denote success does break the message flow. If the spec does not currently contradict that setup, then it is at least ambiguous and the implementations fail to reflect that setup. It's not overcomplicating it to consider the how TBDex will be used.
In addition to the broader questions I raised in my previous comment (payment reversal, whether "closing" successful exchanges is necessary or desirable), we also need to address the details of adding a success boolean to Close
. Is the boolean optional? When is it required? Does false
mean something different than absent? If an RFQ is followed by a Close
with success == true
what does that mean?
Adding the field to
Close
does not break the message flow but altering the meaning ofClose
to be required at the end of all message flows to denote success does break the message flow. If the spec does not currently contradict that setup, then it is at least ambiguous and the implementations fail to reflect that setup. It's not overcomplicating it to consider the how TBDex will be used.In addition to the broader questions I raised in my previous comment (payment reversal, whether "closing" successful exchanges is necessary or desirable), we also need to address the details of adding a success boolean to
Close
. Is the boolean optional? When is it required? Doesfalse
mean something different than absent? If an RFQ is followed by aClose
withsuccess == true
what does that mean?
Ah, I see your point. I have always viewed Close as the single final message of an exchange. Let me read everything again, including the spec, and come back...
If the spec does not currently contradict that setup, then it is at least ambiguous and the implementations fail to reflect that setup
i think until now the interpretation and reference implementations have followed the guidance that Close
is the final message in an exchange. but +1 we need to clarify this point among ourselves and in the spec.
i do see the PR in the js implementation from 2 weeks ago that went in to add the validNext
check (nice work!) and make orderstatus
the only validNext
message on an OrderStatus
, but I also see a comment about testing for messages that can't come after an orderstatus
orderstatus: cannot add rfq, quote, order after orderstatus
so many crossed wires. clarity will be nice
ok I read everything again. I don't see a different interpretation of "Close is the final message of the exchange".
a Close can be sent by Alice or the PFI as a reply to an RFQ or a Quote. It indicates a terminal state. No messages can be added to an exchange after a Close.
I assume even if no Order is ever placed, once the Quote expires, a Close message will be sent. I'll ask Frank and Moe to weigh in so that we're all on the same page (even without the proposed 'success' flag)
Hey hey all, just catching up to this, apologies, I would've weighed in earlier had I known about it
I just spent some time trying to dig through history to find the references to what I'm about to say, but I was unsuccessful ☹️ Nevertheless, so the original intent of Close
and OrderStatus
was as followed. Close
is a terminal state which is non-successful, and OrderStatus
is a status of a given Order
, one-or-more of which may be considered terminal. I originally advocated Close
to be called Cancel
or Reject
.
Granted, the spec may not reflect what I have said here, and/or these semantic definitions may have evolved since I last touched this subject. Nevertheless, we have a problem to be solved here in the current moment, and solve it we shall!
I'm hesitant to vote for the proposal here (@angiejones in your comment above):
Adding Close.data.success as a boolean field would solve the issue
Because it's a slight conflation between the two semantic use cases. In an abstract sense, we need one message type which will deterministically terminate the exchange during a premature phase, and then we also need a separate message type which is an application-defined generic space to represent the state of the given Order
. Venturing into the use case of chargebacks is actually relevant here. Because, what does it mean for a financial exchange to be "final"? It's different depending on the given financial rails being used; Bitcoin transactions are irreversible, but a credit card payment is not. Ultimately it's a matter of jurisdictional legalese. And so through that lens, there cannot be a protocol-prescribed standard of finality, because it's not something the protocol can enforce. That said, prematurely terminating an exchange is a state of finality which is standard across all payment mechanisms. Which is to say, all exchanges, no matter the medium, can be terminated prior to an agreed-upon execution (that is, the Order
message is the state change in which the exchange transforms from being able to be terminated prematurely to being in the amorphous phase of finality).
Alright, a lot of words, I'm sorry, payments are difficult.
The next thing I would say to the above is, "but Kendall, sure there is no standard state of finality, but every transaction, even those which are reversible, does enter a state wherein it's considered to be successful and we can all go about our lives, albeit in some mediums that state can be reversed, the state still exists in standard!" So, I'll put forth two possible proposals (either or, but not both):
OrderStatus
values into the spec. One of which may be something like SUBJECTIVELY_SUCCESSFUL
. This set of status' may or may not be strictly required, let's debate. Either they're "recommended" or they're "required." Either way, regardless of it they're required or not, the spec must make clear that the OrderStatus
set of potential status' can be extended to more than just what's defined here.There's a lot of tangential conversations happening in the GH issue now. Essentially, Close
should always be the terminal message of an exchange. If a PFI is unable to provide a Quote
response to an RFQ
, it sends a Close
. If a Quote
has expired, the PFI should (IIRC we've cut corners on this in the past) add a Close
message.
Close
should be the terminating message for tbdex exchanges. this is necessary given that there isn't a prescribed enumeration of order statuses by design. different PFIs will use different statuses based on their own backend systems. there may emerge conventional statuses for specific currency pairs but we're a long way from that.
At the protocol level, I think it's a bad idea to worry about PFI specific state machines. Necessitating a Close
at the end gives us a guaranteed way to know that an exchange is finished.
Close.data.completed
feels redundant given that Close
inherently means "completed", "finished" "done". I think Close.data.success
makes more sense. Proposing that it be an optional field where:
false
== error occurred. look to orderStatus
messages for more detailtrue
== funds transferedExamples:
Alice: RFQ
Alice: Close (whoops jk, don't send me a quote)
Close.data.success
is not applicable here and therefore not present
Alice: RFQ
PFI: Quote
Alice: Order
PFI: OrderStatus x n
PFI: Close
Close.data.success
is applicable here and therefore present. true
would imply that funds were transferred etc. false
would imply sad times. either way, the wallet now knows that it can stop polling and what the outcome of the exchange is without having to impossibly interpret different order statuses
Going to create a separate github issue to discuss all possible usages of Close
so that we can be far more specific in the spec and in our guides.
@KendallWeihe, we can discuss what you raised in that separate github issue.
@angiejones
I assume even if no Order is ever placed, once the Quote expires, a Close message will be sent. I'll ask Frank and Moe to weigh in so that we're all on the same page (even without the proposed 'success' flag)
will raise in that separate github issue
@KendallWeihe
Because, what does it mean for a financial exchange to be "final"? It's different depending on the given financial rails being used; Bitcoin transactions are irreversible, but a credit card payment is not. Ultimately it's a matter of jurisdictional legalese. And so through that lens, there cannot be a protocol-prescribed standard of finality, because it's not something the protocol can enforce.
this would be new to me to hear that a reversible transaction is one single transaction which has been reversed, and that transactions don't typically have finality. my understanding is that transactions always have finality, and a reversal would happen as a separate, self-contained transaction (even if it looks to the customer like a given transaction was "reversed"). if we think of traditional ledgers, the original transaction doesn't get modified or erased, but rather followed up with a second transaction to show its reversal. so a reversal would be two transactions that zero out. but super interested to hear where you learned about this and to learn more about that approach!
@mistermoe @angiejones and all
I mentioned in this comment a recent change went into the latest release where Close
is no longer permitted to be the last message, (orderstatus
was made the only possible validNext
message on an OrderStatus
and the isValidNext
check was implemented to addMessage
to an exchange) so current reference implementations we have in the wild (the ones that expect a Close
to be the final message after an OrderStatus
) won't work on 0.26.1.
Once we have clarity we'll need a follow up issue to address, either in the tbdex SDK implementations, or in the docs + reference implementations. Just raising for viz!
Close
should be the terminating message for tbdex exchanges. this is necessary given that there isn't a prescribed enumeration of order statuses by design. different PFIs will use different statuses based on their own backend systems. there may emerge conventional statuses for specific currency pairs but we're a long way from that.At the protocol level, I think it's a bad idea to worry about PFI specific state machines. Necessitating a
Close
at the end gives us a guaranteed way to know that an exchange is finished.
Close.data.completed
feels redundant given thatClose
inherently means "completed", "finished" "done". I thinkClose.data.success
makes more sense. Proposing that it be an optional field where:
- not present == not applicable
false
== error occurred. look toorderStatus
messages for more detailtrue
== funds transferedExamples:
Alice: RFQ Alice: Close (whoops jk, don't send me a quote)
Close.data.success
is not applicable here and therefore not presentAlice: RFQ PFI: Quote Alice: Order PFI: OrderStatus x n PFI: Close
Close.data.success
is applicable here and therefore present.true
would imply that funds were transferred etc.false
would imply sad times. either way, the wallet now knows that it can stop polling and what the outcome of the exchange is without having to impossibly interpret different order statusesGoing to create a separate github issue to discuss all possible usages of
Close
so that we can be far more specific in the spec and in our guides.
This is perfect. Thank you, Moe! Only thing I would add is that for the below, they can also consult Close.data.reason
false == error occurred. look to orderStatus messages for more detail
Do we allow Close
after Order
but before OrderStatus
?
Edit: we have a consensus here on this ticket, disregard this comment (created a new ticket)
this would be new to me to hear that a reversible transaction is one single transaction which has been reversed, and that transactions don't typically have finality. my understanding is that transactions always have finality, and a reversal would happen as a separate, self-contained transaction (even if it looks to the customer like a given transaction was "reversed"). if we think of traditional ledgers, the original transaction doesn't get modified or erased, but rather followed up with a second transaction to show its reversal. so a reversal would be two transactions that zero out. but super interested to hear where you learned about this and to learn more about that approach!
@kirahsapong Reversibility exists in many many types of fiat payments. The traditional payments system does not have finality, or at least finality can take very long -- on the order of weeks or even months. Satoshi discusses some of the tradeoffs of reversibility in the Introduction to the bitcoin whitepaper. Reversibility is also discussed in section 6.0.2 of the TBDex whitepaper. Payment reversal is a significant risk that financial institutions take on when they process payments.
This is one reason why exchanging fiat for bitcoin is risky: Alice sends Bob bitcoin, Bob sends Alice dollars, Bob calls his bank to issue a chargeback, and now Alice is out of luck. There is trust inherent in the transaction. Alice must trust Bob and Bob's bank not to leave her emptyhanded. TBDex represents a trust layer that can facilitate decentralized exchange of fiat and crypto. In this case there are three "money movements": bitcoin payment, dollar payment, and dollar chargeback. But since Bob initiated both the dollar payment and chargeback, it is useful to think of that as a single transaction.
In a ledgering software services I have used, you're right that there are typically two database rows representing a money movement are present when a transaction is reversed. It is also common for those two rows to have the same transaction_id
or even another table called transactions
which both money_movement
rows. Naturally different implementations of a ledger might use different terms or database schemas, but the concept applies to many of them.
@diehuxx i didn't dispute the concept of reversibility in payments.
i disputed:
that a reversible transaction is one single transaction
and
that transactions don't typically have finality
distinct from reversibility of payments, to be clear.
coming from a banking background and merchant services baked into my childhood, i'm just sharing my perspective. but let me know what you decide. :)
Lot's of killer discourse, but for the sake of implementation focus, summarizing the new features here:
Close
should always exist for all exchanges, it's the terminal message (as in, it always exist at the end)Close
is valid after all messages with the exception of Order
Close
has a new property success
of type booleanif Close.success == null then that means Close was sent prior-to the Order execution
else if Close.success == true then Close was sent after a successful Order execution
else if Close.success == false then Close was sent after a failed Order execution
With regards to how this state machine can drive wallet (GUI) experience, see comment here
TODO: unclear to me if (edit below)Close
is allowed after Order
but before OrderStatus
? @mistermoe @phoebe-lew @angiejones could you define the rule here?
LMK if I have anything incorrect!
Edit: this PR makes it clear to me, we have a consensus that Close
is not allowed immediately after an Order
(as in, prior-to an OrderStatus
)
Proposal to close this ticket in favor of:
from @angiejones