Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Distinguish between success and failure in Close message #242

Closed
phoebe-lew opened this issue Feb 20, 2024 · 26 comments
Closed

Distinguish between success and failure in Close message #242

phoebe-lew opened this issue Feb 20, 2024 · 26 comments
Assignees
Labels
enhancement New feature or request protocol related to the protocol spec

Comments

@phoebe-lew
Copy link
Contributor

can we provide a boolean property on the Close message indicating if the transaction was completed successfully or not. As a wallet app, all I get is Close and some String reason (again that could be in another language). I may want to add conditional logic to my app to display certain flows based on if the transaction completed successfully or not, and right now that's not possible without trying to programmatically decipher the Close.reason and determine if this String means things are good or not.

from @angiejones

@angiejones angiejones added enhancement New feature or request protocol related to the protocol spec labels Feb 21, 2024
@diehuxx
Copy link

diehuxx commented Feb 26, 2024

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.

@angiejones
Copy link
Member

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.

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

@phoebe-lew
Copy link
Contributor Author

@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.

@angiejones
Copy link
Member

@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

image

@diehuxx
Copy link

diehuxx commented Feb 27, 2024

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.

Proposal 1: Always be Closeing

The 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:

  1. Alice and PFI construct RFQ, Quote, and Order messages.
  2. The PFI issues some number of OrderStatus messages that Alice can poll while the payment(s) are pending.
  3. Once the payment(s) go through, the PFI will issue a 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.

Proposal 2: Always Bad Closeing

Add 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.

Proposal 3: Never be Closeing

This 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.

  • If OrderStatus.data.state = PENDING, Alice should keep polling.
  • If Alice or the PFI issue an OrderStatus with OrderStatus.data.state = TERMINATED, the exchange has been ended without a successful payment.
  • If Alice or the PFI issue an OrderStatus with OrderStatus.data.state = COMPLETE, the payment was successful and the exchange is complete.

Note on Naming

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.

@angiejones
Copy link
Member

@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.

@diehuxx
Copy link

diehuxx commented Mar 1, 2024

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

Recommended Proposal: Add 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.

@kirahsapong
Copy link
Contributor

kirahsapong commented Mar 1, 2024

@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.

@diehuxx
Copy link

diehuxx commented Mar 1, 2024

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?

@angiejones
Copy link
Member

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

@diehuxx
Copy link

diehuxx commented Mar 2, 2024

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?

@angiejones
Copy link
Member

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?

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...

@kirahsapong
Copy link
Contributor

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

@angiejones
Copy link
Member

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)

@KendallWeihe
Copy link
Contributor

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):

  1. We introduce a well-defined set of 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.
  2. We consider an abstraction layer on top of tbdex here, wherein we have specifications unique to given payment mediums (blockchains, credit cards, etc) where each is it's own specification with it's own well-defined set of status'.

@phoebe-lew
Copy link
Contributor Author

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.

@mistermoe
Copy link
Member

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:

  • not present == not applicable
  • false == error occurred. look to orderStatus messages for more detail
  • true == funds transfered

Examples:

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.

@mistermoe
Copy link
Member

@KendallWeihe, we can discuss what you raised in that separate github issue.

@mistermoe
Copy link
Member

@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

@kirahsapong
Copy link
Contributor

@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!

@angiejones
Copy link
Member

angiejones commented Mar 4, 2024

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:

  • not present == not applicable
  • false == error occurred. look to orderStatus messages for more detail
  • true == funds transfered

Examples:

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.

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

@KendallWeihe
Copy link
Contributor

KendallWeihe commented Mar 5, 2024

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)

@diehuxx
Copy link

diehuxx commented Mar 5, 2024

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.

@kirahsapong
Copy link
Contributor

kirahsapong commented Mar 5, 2024

@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. :)

@KendallWeihe
Copy link
Contributor

KendallWeihe commented Mar 6, 2024

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 boolean
  • Logic could be understood as following
if 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 Close is allowed after Order but before OrderStatus? @mistermoe @phoebe-lew @angiejones could you define the rule here? (edit below)

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)

@KendallWeihe
Copy link
Contributor

Proposal to close this ticket in favor of:

@phoebe-lew phoebe-lew closed this as not planned Won't fix, can't repro, duplicate, stale Mar 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request protocol related to the protocol spec
Projects
Status: Done
Development

No branches or pull requests

7 participants
@KendallWeihe @mistermoe @angiejones @phoebe-lew @diehuxx @kirahsapong and others