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

Improve & clarify interactive SEP-6 flow #342

Merged
merged 9 commits into from Jul 22, 2019
Merged

Conversation

tomquisel
Copy link
Contributor

@tomquisel tomquisel commented Jul 5, 2019

This is a breaking protocol change (the new version is 3.0.0), but it's a change in parts of the protocol that very few implementors have covered so far, so I think the negative impact will be small.

Recommendations

We now recommend that anchors implement the interactive flow for deposits and withdrawals. It is the most flexible while being the least complex protocol option for wallets and anchors to implement. We recommend use of SEP-10 web authentication to avoid users having to sign in repeatedly. Implementing SEP-12 is not necessary for a working interactive flow, reducing implementation complexity.

The breaking changes (all for the interactive flow):

  • In the interactive flow, if a wallet uses the callback parameter to receive notifications of status changes to the transaction from the anchor, the format of the message from the anchor has changed. The format is now identical to the transaction object returned by the /transaction endpoint.
  • Wallets should now poll the /transaction endpoint if they take the polling approach to see the status of an interactive deposit / withdrawal, instead of polling the /deposit or /withdrawal endpoints.

Some clarifications:

  • An anchor should support one of:
    • the callback parameter on the interactive popup URL
    • the /transaction endpoint with new withdraw_ fields
      otherwise successful interactive withdrawal is not possible.
  • An anchor should return an id for the transaction in the interactive_customer_info_needed response if it wants to support wallets polling for transaction status.

This change handles the case where anchors use the interactive flow to collect per-request info (like SMS verification, or which connected bank account the user wants to use for this request).

Other changes:

  • interactive_customer_info_needed response
    • Add an id response field for the interactive_customer_info_needed response. This allows wallets to check the status of a particular ongoing interactive deposit or withdrawal request via the /transaction endpoint.
    • Remove unnecessary interactive_deposit flag.
  • Clarify that the callback URL passed into an interactive flow popup's URL may also respond in failure cases. This allows wallets to more gracefully handle failed interactive requests.
  • Add the optional jwt parameter to the interactive flow popup's URL. This allows for users to go through an anchor's interactive flow without having to re-authenticate.
  • Add fields to the /transaction response that the wallet needs to handle ongoing transactions
  • Add advice on how a wallet should use the responses it receives from the server
  • Clarify that customer_info_status is used to see status of uploaded customer info (happens one time per customer generally), as in the non-interactive flow, and that the transaction object is used to see the status of a transaction, both historical and ongoing. The transaction object is used when the user must submit interactive information for every transaction.

@tomerweller
Copy link
Contributor

tomerweller commented Jul 8, 2019

Thanks @tomquisel. It's a messy job but someone has to do it :)

I'm not a huge fan of adding the callback argument for the interactive flow url. Do we really need it? Sounds like an overhead both for the anchor and for the wallet to implement. While in fact just polling the /transaction endpoint should suffice.

In fact, if by convention the wallet queries that as soon as the webview closes, there shouldn't be any lag. No?

* Add advice on how a wallet should use the responses it receives from the server
* Add fields to the /transaction response that the wallet needs to handle ongoing transactions
* Specify that the server should tell the wallet about the result of the interactive flow using the transaction object format, except in the case of an asynchronous deposit.
@tomquisel
Copy link
Contributor Author

@tomerweller I made several more changes, see the Additional changes section. I decided to keep the callback because I think it's good to allow implementors to avoid polling if they want for responsiveness and better performance. I don't think that closing the window is a good enough trigger, because the user may just leave the window open on success and background it. So you'd get some lag.

Copy link
Member

@accordeiro accordeiro left a comment

Choose a reason for hiding this comment

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

This looks really good, thanks for addressing this @tomquisel! I've left some suggestions/comments, but nothing that should block this from being merged :)


Name | Type | Description
-----|------|------------
`type` | string | Always set to `interactive_customer_info_needed`
`url` | string | URL hosted by the anchor. The wallet should show this URL to the user either as a popup or an iframe.
`interactive_deposit` | boolean | (optional) Flag indicating that depositing is also handled in the anchor's interactive customer info flow. The wallet need not make additional requests to `/deposit` to complete the deposit. Defaults to `false`. Only relevant for responses to `/deposit` requests.
`id` | string | (optional) The anchor's internal ID for this deposit / withdrawal request. Can be passed to the [`/transaction`](#single-historical-transaction) endpoint to check status of the request.
Copy link
Member

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 an id be generated every time the endpoint is hit?

Here's a suggestion, which might make the wallet-anchor integration even simpler:

  1. We generate an id for each /deposit or /withdraw request, regardless of it being interactive or not
  2. On future calls to /deposit or /withdraw, the wallet also provides an id 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
  3. As soon as a 200 is received, the status of that deposit or withdrawal can be checked via /transaction by passing along that id

I think my reasoning is that we're basically doing a REST API around the /transactions resource, but using GET /withdraw or GET /deposit instead of POST /transactions, so it might make sense for them to share the same id.

Copy link
Contributor Author

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:

  1. Add an optional id parameter to /deposit and /withdraw success responses. These are useful for calling GET /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.
  2. After receiving a interactive_customer_info_needed response, the interactive_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 an id, then the client doesn't need to re-include any parameters on its second call to /deposit, it just needs to supply the id.
  3. Add an optional id parameter to the non_interactive_customer_info_needed response, and specify that the client doesn't need to re-include any parameters if it supplies the id to future requests to /deposit or /withdraw.

Backwards compatibility analysis:

  • old client and new server: may not work, client will need to be updated to check for id if server depends on statefulness
  • new client and old server: will work since new clients check for id

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?

Copy link
Member

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 :)

ecosystem/sep-0006.md Outdated Show resolved Hide resolved
ecosystem/sep-0006.md Outdated Show resolved Hide resolved
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

3 participants