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-6: Make account_id optional in withdraw response #1417

Merged
merged 2 commits into from
Nov 1, 2023

Conversation

philipliu
Copy link
Contributor

This makes the account_id field in the withdraw and withdraw-exchange response optional. This is to support Anchors integrating with a Custodian, in which a Stellar address other than the asset's distribution account is used as the withdraw_anchor_account. This is to prevent users from accidentally depositing to the wrong address in case the withdraw_anchor_account is changed during the SEP transaction's lifecycle.

@philipliu philipliu marked this pull request as ready for review October 31, 2023 20:47
@philipliu philipliu marked this pull request as draft October 31, 2023 20:48
@philipliu philipliu marked this pull request as ready for review October 31, 2023 20:49
@JakeUrban
Copy link
Contributor

JakeUrban commented Nov 1, 2023

I'm not sure we want to move forward this with this.

Ideally, withdraw_anchor_account should never change once its originally set in order to avoid confusion / mistakes. We only allow withdraw_anchor_account to be changed in the Anchor Platform because the field is set to the asset's configured distribution account when the transaction is created.

This is a bug, since withdraw_anchor_account should only be available once the business is ready to receive funds and the transaction is in pending_user_transfer_start. But because businesses already depend on that behavior, we can't change that until 3.0.

All this is to say that account_id should probably remain a required field for the synchronous success response. The Anchor Platform will never return this response type so we don't need to be concerned about providing the wrong account_id.

@JakeUrban
Copy link
Contributor

Actually, I misspoke when I said the Anchor Platform will never return the success response (it will).

So now I see the problem.. the Anchor Platform's implementation has a bug but we can't fix it without potentially breaking business's application logic. At the same time, allowing businesses to update withdraw_anchor_account means the wallet will get an incorrect account address in the synchronous response.

So I've come full circle now 😅 and think making account_id optional is really the only viable path forward. I think this makes sense from an API design perspective as well. We don't want to provide deposit instructions in the synchronous response, so we shouldn't do that in the withdraw response either.

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 we update the description of the responses too?

This is the correct response if the anchor is able to execute the withdrawal and needs no additional information about the user.

This isn't actually true now. This probably shouldn't be considered a "success" response since its possible for the business to request KYC info after returning 200 and the transaction's ID.

Lets update the deposit response description too if it has the same issue.

@philipliu
Copy link
Contributor Author

@JakeUrban Thanks for catching that, I made the changes 👍

@philipliu philipliu merged commit c39c84f into stellar:master Nov 1, 2023
2 checks passed
@philipliu philipliu deleted the sep-6-custody branch November 1, 2023 17:00
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.

None yet

2 participants