-
Notifications
You must be signed in to change notification settings - Fork 306
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
[SEP-38] Add buy/sell delivery method to quote api response #1556
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please feel free to merge after the comments are taken care of.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can you provide a detailed description of why you're proposing this change?
I will update the desc. For your reference @JakeUrban More context can be found here : |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you update the version number and updated at field at the top of the document?
When we make changes to the protocols, we need to be mindful that these APIs are implemented by various businesses in the ecosystem, not just the anchor platform. We ideally shouldn't even make changes to the protocol's without initiating discussion with the relevant ecosystem businesses, but at the very least we need to publicly document our rationale for making changes. |
Sorry I am confused. I do understand how protocol change will impact out partners. In my opinion there's not a solid reason that we have to make this change or we must not, but I thought an agreement to proceed has been reached according to the slack discussion. Do you wish to hold off the change and reach to partners first? |
Sorry for the confusion, I do think we can proceed here without engagement from the ecosystem since we're adding optional response attributes, so all existing implementations would still be compliant. I just wanted to avoid us merging changes to the protocols without publicly documenting the rationale. |
I updated desc to include the rationale according to the slack discussion. Let me know of it looks good to you. |
This description doesn't quite give enough information for someone that isn't a part of the anchor engineering team. I would try to structure descriptions like this:
I would specifically leave out any implementation specifics of the anchor platform. So in this case, the description could look like this:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approved with some minor edits!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
…ponse (#1520) ### Description Included in `Sep38QuoteResponse` which is returned by POST /quote and GET /quote Also in `GetQuoteResponse` which is used when publish QUOTE_CREATED event ### Testing - `./gradlew test` ### Documentation stellar/stellar-protocol#1556
Added to both
POST /quote
andGET /quote
responseContext
Current SEP-38 quote API response doesn't include the buy/sell delivery method, even though it's stored in the DB
https://stellarfoundation.slack.com/archives/C036T11AY84/p1719603232852849