SEP24 - A simpler SEP6 #407
Merged
Conversation
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
|
What's the compatibility goal with existing "modern" SEP-6 implementations like the SEP-6 demo client or AnchorUSD? |
|
This should be fully compatible. I've made no breaking changes, just limited the feature set to the interactive flow |
tomquisel
approved these changes
Sep 20, 2019
|
|
||
| ## Simple Summary | ||
|
|
||
| This SEP defines the standard way for anchors and wallets to interact on behalf of users. This improves user experience by allowing wallets and other clients to interact with anchors directly without the user needing to leave the wallet to go to the anchor's site. It is based on, and backwards compatible with, [SEP-0006](sep-0006.md), but only supports the interactive flow, and cleans up or removes confusing artifacts. |
There was a problem hiding this comment.
...allowing wallets and other clients to interact with anchors directly without the user needing to leave the wallet to go to the anchor's site...
Because we're standardizing on the interactive flow the user will need to leave the wallet to go to a web page. Ideally that leaving the wallet will occur in an OS embedded browser in the app where the user can see the URL and has confidence about the website they are using, and it isn't an embedded web view. This statement feels a little misleading because the user will be using the anchors interactive flow, which I assume is a website.
There was a problem hiding this comment.
This seems like something that's more of a suggestion to wallet implementors. Interactive websites should have branding that makes it clear who it is, but if a wallet sends you to a malicious webpage it means your wallet is compromised and all bets are off anyway
There was a problem hiding this comment.
I think it is worthwhile to do some security thinking (if not a full audit) on whether there's a way for a malicious entity to impersonate an anchor. Given the two way binding between asset and DNS entry it seems difficult, but definitely something to keep in mind
There was a problem hiding this comment.
if a wallet sends you to a malicious webpage it means your wallet is compromised and all bets are off anyway
This isn't always true. If a user is trying out a wallet for the first time and planning to deposit a small amount from their personal bank account that holds their life savings, there's little risk if the they lose that small amount of money to the wallet, but a huge risk if a maliciously crafted webview usage captures a users bank account credentials.
There was a problem hiding this comment.
I'm not sure what the attack vector you're referring to is, that still sounds like a malicious wallet stealing peoples bank info, and they wouldn't comply with our requirement to open in the system browser anyway
There was a problem hiding this comment.
Agreed, but if we make it part of our recommendations that apps use a system browser it sets expectations and recommendations that hopefully app developers will follow that will increase the trust that others can have in using their apps. I think this is similar to how we recommend HTTPS in this SEP.
There was a problem hiding this comment.
It is, however, a good discussion of whether training people to enter sensitive info into embedded third party webviews inside of another third party app is something that's dangerous in general, even if this particular use case isn't dangerous. I'm not thrilled about the lines blurred between wallet and anchor in this flow
There was a problem hiding this comment.
yeah let me marinate on this a bit. I don't want to block landing this in Draft status on this, but would like to give it some thought before we move it to Active. @tomquisel any thoughts here?
There was a problem hiding this comment.
I agree with @leighmcculloch about it being a best practice to show the user the URL. That said, realistically, I'm not sure it affects the security profile much. In most cases, the user has no idea who the anchor is at the start, or what domain to expect the anchor to be on, so I don't know how much that URL helps.
|
This is compatible with the sep006 tester website?
…Sent from my iPhone
On Sep 20, 2019, at 8:03 PM, Leigh McCulloch ***@***.***> wrote:
@leighmcculloch commented on this pull request.
Some questions that don't need to block the PR at all.
In ecosystem/sep-0024.md:
> +
+## Cross-Origin Headers
+
+Valid CORS headers are necessary to allow web clients from other sites to use the endpoints. The following HTTP header must be set for all transfer server responses, including error responses.
+
+```
+Access-Control-Allow-Origin: *
+```
+
+## HTTPS Only
+
+This protocol involves the transfer of value, and so HTTPS is required for all endpoints for security. Wallets and anchors should refuse to interact with any insecure HTTP endpoints.
+
+## Recommendations
+
+SEP-6 lays out many options for how deposit and withdrawal can work. These are recommendations for getting a wallet or anchor implementation working with minimal effort while providing a great user experience.
Should the start of the first sentence here be SEP-24?
⬇️ Suggested change
-SEP-6 lays out many options for how deposit and withdrawal can work. These are recommendations for getting a wallet or anchor implementation working with minimal effort while providing a great user experience.
+SEP-24 lays out many options for how deposit and withdrawal can work. These are recommendations for getting a wallet or anchor implementation working with minimal effort while providing a great user experience.
In ecosystem/sep-0024.md:
> + * Fetch the asset's deposit & withdawal fee structure: if `fee_fixed` and `fee_percent` are provided, show this to the user early in the process so they're fully informed.
+ * If the `/fee` endpoint is enabled, use it for computing fees when you need to show them to the user.
+* **Authentication**
+ * Perform [authentication](#authentication) via SEP-10 before hitting those endpoints
+* **Make a request to `/deposit` or `/withdraw`.**
+ * This will respond with the interactive url needed to proceed with KYC and deposit/withdraw details.
+* **For `/deposit` and `/withdraw`**
+ * Optionally attach any fields from [SEP-9](sep-0009.md) as query parameters in the interactive URL, in order to let the anchor pre-fill them in the interactive flow UI. This is optional, but can create a much nicer user experience.
+ * If you do attach fields, make sure you attach them using a proper URL builder instead of just appending a string, as the interactive URL may already have query parameters or a hash-fragment value.
+* **For `/deposit`**
+ * Handle the interactive flow, handle it as [described in detail](#3-customer-information-needed-interactive).
+ * Handle the [special cases](#special-cases), they're relatively common.
+* **For `/withdraw`**
+ * Handle the interactive flow, handle it as [described in detail](#3-customer-information-needed-interactive).
+ * Do not handle the non-interactive or status responses to the `/withdraw` request. These may be from the old [SEP6](sep-0006.md) protocol.
+ * Some wallets might exchange currencies only once they're ready to send the withdrawal payment, so exchange rate fluctuations might require withdrawal values to slightly vary from the originally provided `amount`. Anchors are instructed to accept a variation of ±10% between the informed `amount` and the actual value sent to the anchor's Stellar account. The withdrawn amount will be adjusted accordingly.
Some wallets might exchange currencies only once they're ready...
Is this because a user might have TOK, but want to withdraw USD, and the app won't convert the TOK=>USD until the withdrawal is confirmed to be going ahead?
Is the assumption that this is okay because what the anchor will withdraw will be one-to-one with the token the wallet is sending? How will this work in cases where it isn't one-to-one?
In ecosystem/sep-0024.md:
> + * If you do attach fields, make sure you attach them using a proper URL builder instead of just appending a string, as the interactive URL may already have query parameters or a hash-fragment value.
+* **For `/deposit`**
+ * Handle the interactive flow, handle it as [described in detail](#3-customer-information-needed-interactive).
+ * Handle the [special cases](#special-cases), they're relatively common.
+* **For `/withdraw`**
+ * Handle the interactive flow, handle it as [described in detail](#3-customer-information-needed-interactive).
+ * Do not handle the non-interactive or status responses to the `/withdraw` request. These may be from the old [SEP6](sep-0006.md) protocol.
+ * Some wallets might exchange currencies only once they're ready to send the withdrawal payment, so exchange rate fluctuations might require withdrawal values to slightly vary from the originally provided `amount`. Anchors are instructed to accept a variation of ±10% between the informed `amount` and the actual value sent to the anchor's Stellar account. The withdrawn amount will be adjusted accordingly.
+* **Transaction history**
+ * Provide a list of historical and current deposits and withdrawals at the [`/transactions`](#transactions) endpoint for wallets to show a view of all of a single anchors transactions in a list.
+ * Provide status or instructions for a specific deposit or withdraw at the [`/transaction`](#transaction) endpoint
+
+### Basic Anchor Implementation
+
+* Provide a full-featured implementation of [`/info`](#info).
+* Pick your approach to [fees](https://github.com/stellar/stellar-protocol/blob/master/ecosystem/sep-0006.md#fee). We recommend using `/info` to express fees as it provides a better user experience (the user can see the fee structure in the wallet early in the process).
If we recommend /info, should we make that a requirement?
In ecosystem/sep-0024.md:
> @@ -0,0 +1,688 @@
+## Preamble
+
+```
+SEP: 0024
+Title: Anchor/Client interoperability
+Author: SDF
+Status: Active
+Created: 2019-09-18
+Updated: 2019-09-18
+Version 1.0.0
+```
+
+## Simple Summary
+
+This SEP defines the standard way for anchors and wallets to interact on behalf of users. This improves user experience by allowing wallets and other clients to interact with anchors directly without the user needing to leave the wallet to go to the anchor's site. It is based on, and backwards compatible with, [SEP-0006](sep-0006.md), but only supports the interactive flow, and cleans up or removes confusing artifacts.
...allowing wallets and other clients to interact with anchors directly without the user needing to leave the wallet to go to the anchor's site...
Because we're standardizing on the interactive flow the user will need to leave the wallet to go to a web page. Ideally that leaving the wallet will occur in an OS embedded browser in the app where the user can see the URL and has confidence about the website they are using, and it isn't an embedded web view. This statement feels a little misleading because the user will be using the anchors interactive flow, which I assume is a website.
In ecosystem/sep-0024.md:
> + * Handle the interactive flow, handle it as [described in detail](#3-customer-information-needed-interactive).
+ * Do not handle the non-interactive or status responses to the `/withdraw` request. These may be from the old [SEP6](sep-0006.md) protocol.
+ * Some wallets might exchange currencies only once they're ready to send the withdrawal payment, so exchange rate fluctuations might require withdrawal values to slightly vary from the originally provided `amount`. Anchors are instructed to accept a variation of ±10% between the informed `amount` and the actual value sent to the anchor's Stellar account. The withdrawn amount will be adjusted accordingly.
+* **Transaction history**
+ * Provide a list of historical and current deposits and withdrawals at the [`/transactions`](#transactions) endpoint for wallets to show a view of all of a single anchors transactions in a list.
+ * Provide status or instructions for a specific deposit or withdraw at the [`/transaction`](#transaction) endpoint
+
+### Basic Anchor Implementation
+
+* Provide a full-featured implementation of [`/info`](#info).
+* Pick your approach to [fees](https://github.com/stellar/stellar-protocol/blob/master/ecosystem/sep-0006.md#fee). We recommend using `/info` to express fees as it provides a better user experience (the user can see the fee structure in the wallet early in the process).
+* **For both deposit and withdrawal**:
+ * To start an interactive flow, provide the [Customer info needed response](#3-customer-information-needed-interactive).
+ * Some wallets may include other fields indicated in [SEP-9](sep-0009.md) as query parameters added to the interactive URL. These can be used to pre-populate fields in the interactive flow. This is optional for the wallet to provide and optional for anchors to respect, but it does create a much nicer user experience.
+ * Include the `id` field in your response to `/deposit` and `/withdraw` so the wallet can check up on the status of the transaction if it wants.
+ * Also include the `id` field in the popup URL you provide to the wallet. This allows you to keep track of the transaction when the user visits the URL.
⬇️ Suggested change
- * Also include the `id` field in the popup URL you provide to the wallet. This allows you to keep track of the transaction when the user visits the URL.
+ * Also include the `id` field in the popup URL you provide to the wallet. This allows you to keep track of the transaction when the user visits the URL.
In ecosystem/sep-0024.md:
> +* **For `/withdraw`**
+ * Handle the interactive flow, handle it as [described in detail](#3-customer-information-needed-interactive).
+ * Do not handle the non-interactive or status responses to the `/withdraw` request. These may be from the old [SEP6](sep-0006.md) protocol.
+ * Some wallets might exchange currencies only once they're ready to send the withdrawal payment, so exchange rate fluctuations might require withdrawal values to slightly vary from the originally provided `amount`. Anchors are instructed to accept a variation of ±10% between the informed `amount` and the actual value sent to the anchor's Stellar account. The withdrawn amount will be adjusted accordingly.
+* **Transaction history**
+ * Provide a list of historical and current deposits and withdrawals at the [`/transactions`](#transactions) endpoint for wallets to show a view of all of a single anchors transactions in a list.
+ * Provide status or instructions for a specific deposit or withdraw at the [`/transaction`](#transaction) endpoint
+
+### Basic Anchor Implementation
+
+* Provide a full-featured implementation of [`/info`](#info).
+* Pick your approach to [fees](https://github.com/stellar/stellar-protocol/blob/master/ecosystem/sep-0006.md#fee). We recommend using `/info` to express fees as it provides a better user experience (the user can see the fee structure in the wallet early in the process).
+* **For both deposit and withdrawal**:
+ * To start an interactive flow, provide the [Customer info needed response](#3-customer-information-needed-interactive).
+ * Some wallets may include other fields indicated in [SEP-9](sep-0009.md) as query parameters added to the interactive URL. These can be used to pre-populate fields in the interactive flow. This is optional for the wallet to provide and optional for anchors to respect, but it does create a much nicer user experience.
+ * Include the `id` field in your response to `/deposit` and `/withdraw` so the wallet can check up on the status of the transaction if it wants.
Is this the first point that the anchor's id becomes available to the wallet? What happens if there's a breakdown in communication getting the ID back, but the user has already entered enough information to kick something off at the anchors side?
In ecosystem/sep-0024.md:
> +* Provide a full-featured implementation of [`/info`](#info).
+* Pick your approach to [fees](https://github.com/stellar/stellar-protocol/blob/master/ecosystem/sep-0006.md#fee). We recommend using `/info` to express fees as it provides a better user experience (the user can see the fee structure in the wallet early in the process).
+* **For both deposit and withdrawal**:
+ * To start an interactive flow, provide the [Customer info needed response](#3-customer-information-needed-interactive).
+ * Some wallets may include other fields indicated in [SEP-9](sep-0009.md) as query parameters added to the interactive URL. These can be used to pre-populate fields in the interactive flow. This is optional for the wallet to provide and optional for anchors to respect, but it does create a much nicer user experience.
+ * Include the `id` field in your response to `/deposit` and `/withdraw` so the wallet can check up on the status of the transaction if it wants.
+ * Also include the `id` field in the popup URL you provide to the wallet. This allows you to keep track of the transaction when the user visits the URL.
+ * Make sure to pass along the `amount` to the popup URL, especially on the `/withdraw` flow.
+ * We recommend you use [SEP-10](#authentication) for authentication in the interactive flow and do not separately prompt for password to achieve a good user experience (although asking for MFA when confirming a transaction or requiring email confirmation is reasonable). Be sure to accept the [`jwt` parameter](#adding-parameters-to-the-url) so the user does not have to re-login. Support the `callback` parameter as well.
+ * Test your interactive flows on mobile. They should be easy to use on all devices: make them responsive, handle auto-fill well, and do smart keyboard management on mobile devices.
+* **Interactive deposit**
+ * Your interactive deposit popup will do everything needed to initiate the deposit without the user needing to interact with the wallet further. It should either directly initiate the deposit, or handle displaying information (such as reference number or bank account number) that the user will need to complete their deposit.
+* **Interactive withdrawal**
+ * Your withdrawal flow will have to pass control back to the user's wallet, so it can initiate the withdrawal with a Stellar payment to your withdrawal address. You'll need to communicate the withdrawal address, amount and status to the wallet using the [`callback` parameter](#adding-parameters-to-the-url), and also by making it available on your `/transaction` endpoint. See [details](#guidance-for-wallets-completing-an-interactive-withdrawal) for polling by the wallet.
+ * If an `amount` parameter is provided to the interactive URL, make sure to use that value to pre-populate the amount on the interactive form, since this greatly improves the user experience.
+ * In order to fulfill a withdrawal, a wallet must make a payment to the Stellar address that the anchor provides. It is the anchor's job to watch for Stellar payments to the given address and make the external transaction as soon as they're detected. It is recommended for anchors to listen to at least `payment` and `path_payment` [operations](https://www.stellar.org/developers/horizon/reference/endpoints/payments-all.html), although supporting the remaining ones is important for covering all use cases. Most Stellar SDKs already support listening to all payment forms via streaming.
What are the other forms of payment operations other than payment and path_payment? Can we explicitly list them here?
In ecosystem/sep-0024.md:
> +The deposit endpoint allows a wallet to get deposit information from an anchor, so a user has all the information needed to initiate a deposit. It also lets the anchor specify additional information that the user must submit interactively via a popup or embedded browser window to be able to deposit.
+
+If the given account does not exist, or if the account doesn't have a trust line for that specific asset, see the [Special Cases](#special-cases) section below.
+
+
+### Request
+
+```
+GET TRANSFER_SERVER/deposit
+```
+
+Request Parameters:
+
+Name | Type | Description
+-----|------|------------
+`asset_code` | string | The code of the asset the user wants to deposit with the anchor. Ex BTC, ETH, USD, INR, etc. This may be different from the asset code that the anchor issues. Ex if a user deposits BTC and receives MyBTC tokens, `asset_code` must be BTC.
This may be different from the asset code that the anchor issues.
Is this because a path_payment would be used?
—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub, or mute the thread.
|
|
That's the goal. If you find any incompatibilities let me know |
accordeiro
approved these changes
Sep 23, 2019
There was a problem hiding this comment.
LGTM! I've left some comments, but none should block the PR from being merged.
@msfeldstein what are your thoughts about how we'll evolve this SEP and SEP6 moving forward? Should we always make sure we send a PR with updates to both?
I'd rather deprecate SEP6 and create another sep for non-interactive if its useful for the ecosystem. Having the messy documentation of sep6 which is really 2 different approaches mixed together is no good |
Co-Authored-By: Alex Cordeiro <accordeiro@users.noreply.github.com>
Co-Authored-By: Leigh McCulloch <leigh@stellar.org>
Co-Authored-By: Leigh McCulloch <leigh@stellar.org>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
SEP6 has gone through many iterations, and retains many artifacts of previous incarnations which can be confusing to implementors and result in out-of-spec implementations. I've created a fresh SEP that is still compatible with SEP-6 but leaves all the non-interactive stuff behind.
Changes from SEP6 can be seen at this commit (bc1433a)
Changes made to the SEP6 doc:
accountparameter on/transactions. It's not needed since auth is required and you can get the account from there.external_extrasince this should all live in a human readable more_info_urlThere is some additional cleanup i want to do, but this is the cleanest diff from sep6 atm.
Future cleanup: Re-ordering sections so they appear in the order which a UX flow would hit them, add some UX mocks to show what the status should look like when, make missing JWTs an error case, and possibly letting us return 200 instead of 403 to return the interactive_customer_info_needed url (in addition to 403 to not break existing anchors)