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

adding orderinstructions message description #355

Merged
merged 3 commits into from
Jul 31, 2024
Merged

Conversation

jiyoonie9
Copy link
Contributor

@jiyoonie9 jiyoonie9 commented Jul 23, 2024

closes #335

  • adding orderinstructions message description to readme.
  • updated quote and message schema.
  • added orderinstructions schema.
  • updated quote test vector
  • add orderinstructions test vector

… and message schema. added orderinstructions schema.
@jiyoonie9 jiyoonie9 linked an issue Jul 23, 2024 that may be closed by this pull request
Copy link
Contributor

@KendallWeihe KendallWeihe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added some comments which I think should be addressed, but lgtm ✅

I don't see any issues which will arise out of this, but what was the original motivating factor? An application-level requirement?

@KendallWeihe
Copy link
Contributor

Oh so, wait, at what point during the exchange is an orderinstructions valid? In between a quote and an order?

So the happy path would be: RFQ --> Quote --> OrderInstructions --> Order --> OrderStatus --> Close

Co-authored-by: Kendall Weihe <kendallweihe@gmail.com>
@jiyoonie9
Copy link
Contributor Author

Oh so, wait, at what point during the exchange is an orderinstructions valid? In between a quote and an order?

So the happy path would be: RFQ --> Quote --> OrderInstructions --> Order --> OrderStatus --> Close

Happy path is RFQ > Quote > Order > OrderInstructions > {alice uses the instructions to execute payin} > OrderStatus(es) > Close

@jiyoonie9
Copy link
Contributor Author

jiyoonie9 commented Jul 23, 2024

I don't see any issues which will arise out of this, but what was the original motivating factor? An application-level requirement?

The original motivating factor came about while testing with frontend. If Alice had to take action to pay in (i.e. use some link), it wasn't clear whether she should send the Order message and then pay, or pay and then send an Order message.

having order instructions breaks out the steps more, so that she can:

  1. get the quote
  2. if she's satisfied with the quote, she signals her intent to go through with the exchange by sending an Order message
  3. PFI sends instructions on how to execute her order
  4. alice uses the instructions to begin payin (or decides she doesn't like the instructions, sends a cancel instead)

@nitro-neal
Copy link

adding orderinstructions from the pfi after order makes a lot of sense ✅

@nitro-neal
Copy link

@jiyoontbd does there need to be something added to the http client?

get_order_instructions()

or would that come back in a new field in teh response in submit_order()?

@nitro-neal
Copy link

nitro-neal commented Jul 26, 2024

oh no I can't unsee it!
On the naming we have:

OrderInstructions
paymentinstruction

Thoughts on making it OrderInstruction ?

...

But on second thought, it does contains 2 order instructions, payin payout... so maybe thats fine :hmm:

@KendallWeihe
Copy link
Contributor

No strong opinions on the singular-vs-plural naming. Any of the above are fine. In my head the word "instructions" is always plural, but maybe that's just me. In which case, we should actually rename paymentInstruction to paymentInstructions (plural), but I can see why that would be overly disruptive at this time. Totally fine for the names to be inconsistent right now too, or to make OrderInstruction singular. Like I said, any are fine.

"kind": "orderinstructions",
"id": "orderinstructions_01ha83f663e3e88fshb06h6g78",
"createdAt": "2023-09-13T20:24:37.315Z",
"protocol": "1.0"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

will the protocol version be increased? if so, we wouldn't have an OrderInstructions message with 1.0 would we?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hmm, we've certainly made quite a few changes to the protocol, but i wasn't part of the versioning discussion when this field first got added here so i will have @mistermoe / @frankhinek chime in here

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@angiejones
Copy link
Member

is OrderInstructions an optional message in an exchange?

@jiyoonie9
Copy link
Contributor Author

@jiyoontbd does there need to be something added to the http client?

get_order_instructions()

or would that come back in a new field in teh response in submit_order()?

order instructions is a net new message and is not returned as a response to submitting an order. so if you're following a pattern of get_quote() or get_order_status() then yea that makes sense to me

@jiyoonie9
Copy link
Contributor Author

is OrderInstructions an optional message in an exchange?

good question @angiejones - i was thinking it would be required, but payin and payout instructions can be empty if there's no instructions needed (i.e. for an offering that's just a stored_balance > stored_balance transfer)

@angiejones
Copy link
Member

hmm, programmatically, how do you envision a wallet app handling order instructions? Present the text from the message to the user? Then what? How does the Wallet know once those instructions have been executed? Do they wait for the PFI to provide an OrderStatus or Close?

@jiyoonie9
Copy link
Contributor Author

hmm, programmatically, how do you envision a wallet app handling order instructions? Present the text from the message to the user? Then what? How does the Wallet know once those instructions have been executed? Do they wait for the PFI to provide an OrderStatus or Close?

orderinstructions contains payin and payout, with link and/or instruction fields

if there's a payment link that alice needs to navigate to in order to fulfill payin, the payin object of orderinstructions should be populated. once the payment is done, the wallet should detect completion and redirect back to the wallet view where alice is now waiting for PFI to confirm that her payin has been received, and PFI will initiate payout.

cc-ing @ethan-tbd for how didpay accommodated redirect after payin was completed

@jiyoonie9
Copy link
Contributor Author

i'm going to merge this because we're up against an internal team deadline. creating issues for questions not yet resolved now

@jiyoonie9
Copy link
Contributor Author

hmm, programmatically, how do you envision a wallet app handling order instructions? Present the text from the message to the user? Then what? How does the Wallet know once those instructions have been executed? Do they wait for the PFI to provide an OrderStatus or Close?

orderinstructions contains payin and payout, with link and/or instruction fields

if there's a payment link that alice needs to navigate to in order to fulfill payin, the payin object of orderinstructions should be populated. once the payment is done, the wallet should detect completion and redirect back to the wallet view where alice is now waiting for PFI to confirm that her payin has been received, and PFI will initiate payout.

cc-ing @ethan-tbd for how didpay accommodated redirect after payin was completed

#359

@jiyoonie9 jiyoonie9 merged commit 47db162 into main Jul 31, 2024
@jiyoonie9 jiyoonie9 deleted the orderinstructions branch July 31, 2024 18:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Introduce OrderInstructions Message
4 participants