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

[SEP-38] Add buy/sell delivery method to quote api response #1556

Merged
merged 7 commits into from
Sep 26, 2024

Conversation

JiahuiWho
Copy link
Contributor

@JiahuiWho JiahuiWho commented Sep 25, 2024

Added to both POST /quote and GET /quote response

Context
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

ecosystem/sep-0038.md Show resolved Hide resolved
ecosystem/sep-0038.md Outdated Show resolved Hide resolved
ecosystem/sep-0038.md Outdated Show resolved Hide resolved
ecosystem/sep-0038.md Outdated Show resolved Hide resolved
ecosystem/sep-0038.md Outdated Show resolved Hide resolved
ecosystem/sep-0038.md Outdated Show resolved Hide resolved
lijamie98
lijamie98 previously approved these changes Sep 25, 2024
Copy link
Contributor

@lijamie98 lijamie98 left a 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.

Copy link
Contributor

@JakeUrban JakeUrban left a 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?

@JiahuiWho
Copy link
Contributor Author

JiahuiWho commented Sep 25, 2024

I will update the desc. For your reference @JakeUrban More context can be found here :
https://stellarfoundation.slack.com/archives/C036T11AY84/p1719603232852849

Copy link
Contributor

@JakeUrban JakeUrban left a 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?

ecosystem/sep-0038.md Outdated Show resolved Hide resolved
@JakeUrban
Copy link
Contributor

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.

@JiahuiWho
Copy link
Contributor Author

JiahuiWho commented Sep 25, 2024

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?

@JakeUrban
Copy link
Contributor

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.

ecosystem/sep-0038.md Outdated Show resolved Hide resolved
ecosystem/sep-0038.md Outdated Show resolved Hide resolved
@JiahuiWho
Copy link
Contributor Author

I updated desc to include the rationale according to the slack discussion. Let me know of it looks good to you.

@JakeUrban
Copy link
Contributor

JakeUrban commented Sep 26, 2024

Current SEP-38 quote API response doesn't include the buy/sell delivery method, even though it's stored in the DB

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:

  1. What is the problem?
  2. What is the proposed solution?
  3. Are there any comments / concerns with the proposed solution that you're looking for feedback on?

I would specifically leave out any implementation specifics of the anchor platform. So in this case, the description could look like this:

The POST /quote and GET /quote API responses do not include the buy_delivery_method or sell_delivery_method key-value pair that was provided in the associated POST /quote request, which forces clients to save this information in their own data store if they later need to know how to deliver funds. This doesn't make a lot of sense, because clients can fetch all other quote information using the GET /quote/:id.

This PR adds the sell_delivery_method and buy_delivery_method attributes as optional response parameters to GET /quote/:id and POST /quote endpoints, allowing the client to only store the quote ID on their end. When it is time to transfer funds associated with the quote, the client can then fetch the delivery method using the GET /quote/:id endpoint.

Adding these response parameters as optional attributes ensures any existing implementations are still compatible with the specification.

JakeUrban
JakeUrban previously approved these changes Sep 26, 2024
Copy link
Contributor

@JakeUrban JakeUrban left a 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!

ecosystem/sep-0038.md Outdated Show resolved Hide resolved
ecosystem/sep-0038.md Outdated Show resolved Hide resolved
ecosystem/sep-0038.md Outdated Show resolved Hide resolved
Copy link
Contributor

@lijamie98 lijamie98 left a comment

Choose a reason for hiding this comment

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

LGTM

@JiahuiWho JiahuiWho merged commit 9fe27a1 into master Sep 26, 2024
3 checks passed
@JiahuiWho JiahuiWho deleted the anchor-729 branch September 26, 2024 21:59
JiahuiWho added a commit to stellar/java-stellar-anchor-sdk that referenced this pull request Sep 30, 2024
…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
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.

3 participants