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
Improve & clarify interactive SEP-6 flow #342
Merged
Merged
Changes from 3 commits
Commits
Show all changes
9 commits
Select commit
Hold shift + click to select a range
00ca42b
Improve & clarify interactive SEP-6 flow
32eccb2
Add more advice, better /transaction response
2122876
Feedback
5c33449
Address some feedback from @jimdanz
f9979f8
remove unnecessary interactive_deposit flag
1885209
Allow missing JWTs in URL
66b0fbd
Allow a signup form for interactive flow auth
fe8dd44
Explain interactive deposit
87d48fb
Convert funds -> XLM for clarity
File filter
Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
I love this
id
field – it's definitely something that will enable us to track transactions more easily once they have moved from the/deposit
or/withdraw
flows.My question is: should we be more explicit about when this
id
needs to be generated by the anchor? Is it only when the interactive KYC is completed? Or should anid
be generated every time the endpoint is hit?Here's a suggestion, which might make the wallet-anchor integration even simpler:
id
for each/deposit
or/withdraw
request, regardless of it being interactive or not/deposit
or/withdraw
, the wallet also provides anid
query param – this will remove the need for the Wallet to always replicate all request params (+ newly required data) until it gets a success response, since/deposit
is not stateless anymore/transaction
by passing along thatid
I think my reasoning is that we're basically doing a REST API around the
/transactions
resource, but usingGET /withdraw
orGET /deposit
instead ofPOST /transactions
, so it might make sense for them to share the same id.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.
Thanks Alex, this makes sense. It's definitely a bummer that we need to be as backwards-compatible as possible, there are some strange choices in the existing SEP.
Here's what I'm thinking we can do for your suggestion:
id
parameter to/deposit
and/withdraw
success responses. These are useful for callingGET /transaction
. They don't help with repeated calls to/deposit
or/withdraw
, because clients won't make any more of those requests once they get a success response.interactive_customer_info_needed
response, theinteractive_deposit: false
case is the only one where the client needs to make another request to/deposit
. We can specify that if the server has included anid
, then the client doesn't need to re-include any parameters on its second call to/deposit
, it just needs to supply theid
.id
parameter to thenon_interactive_customer_info_needed
response, and specify that the client doesn't need to re-include any parameters if it supplies theid
to future requests to/deposit
or/withdraw
.Backwards compatibility analysis:
id
if server depends on statefulnessid
However, I'm not too worried about this, because I don't think any existing flows are using the repeated requests to
/deposit
or/withdraw
yet.What do you think?
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.
This approach makes sense to me! Thanks :)