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.

Sign up for GitHub

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

Jump to bottom

Add native asset support to SEP-24 #1363

Merged
merged 6 commits into from
Jun 16, 2023
Merged

Add native asset support to SEP-24 #1363

Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
886f7c8
Mention XLM in the SEP-24 docs
philipliu Jun 14, 2023
d84df67
Fix changelog and date
philipliu Jun 14, 2023
6d5084f
Fix withdrawal account
philipliu Jun 14, 2023
944e8c5
Add note about XLM
philipliu Jun 14, 2023
bf219b8
Prettier
philipliu Jun 14, 2023
f5563ac
Apply suggestions from code review
philipliu Jun 15, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
31 changes: 23 additions & 8 deletions ecosystem/sep-0024.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,8 +6,8 @@ Title: Hosted Deposit and Withdrawal
Author: SDF
Status: Active
Created: 2019-09-18
Updated: 2023-02-06
Version 3.0.0
Updated: 2023-06-14
Version 3.1.0
```

## Simple Summary
Expand Down Expand Up @@ -324,8 +324,8 @@ Request Parameters:

| Name | Type | Description |
| ----------------------------- | ----------------------- | -------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- |
| `asset_code` | string | The code of the stellar asset the user wants to receive for their deposit with the anchor. The value passed must match one of the codes listed in the [/info](#info) response's deposit object. |
| `asset_issuer` | string | (optional) The issuer of the stellar asset the user wants to receive for their deposit with the anchor. If asset_issuer is not provided, the anchor should use the asset issued by themselves as described in their TOML file. |
| `asset_code` | string | The code of the stellar asset the user wants to receive for their deposit with the anchor. The value passed must match one of the codes listed in the [/info](#info) response's deposit object. `native` is a special `asset_code` that represents the native XLM token. |
| `asset_issuer` | string | (optional) The issuer of the stellar asset the user wants to receive for their deposit with the anchor. If asset_issuer is not provided, the anchor should use the asset issued by themselves as described in their TOML file. If `native` is specified as the `asset_code`, `asset_issuer` must be not be set. |
Copy link
Member

Choose a reason for hiding this comment

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

If native is specified as the asset_code, asset_issuer must be not be set.

native is a valid asset code on the network, so I don't think SEP-24 should prevent an issued asset with the name native from being used.

I think we would need to allow the case where asset_code is "native" and asset_issuer has a value, and in that case treat the asset as credit / issued. That would make this change truly backwards compatible.

To make sure we're talking about all options, there are two ways we've preferred to represent assets as strings:

  1. Three elements:

    • asset_type with valid values being native,credit_alphanum4,credit_alphanum12
    • asset_code
    • asset_issuer

  2. One string element, based on SEP-11, where:

    • The native asset is rendered as "native"
    • An issued asset is rendered as "ABC:GJKJASD..."

I understand the latter format is unrealistic given the existing API.

The first form may be reasonable with some subset of values such as native and credit given the SEP-24 API already places code and issuer into separate fields.

cc @JakeUrban

Copy link
Contributor

Choose a reason for hiding this comment

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

If there is a "native" asset, that is issued by someone, and it's configured by the anchor, we will have a undefined behavior for the request where only asset_code is specified. Anchor wouldn't know if it's XLM or native:<issuer> asset.
This is really a corner-case, and I doubt it'll happen in real production environment, but worth considering.

Copy link
Member

Choose a reason for hiding this comment

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

Ah, this edge case exists because the asset_issuer field is optional?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, it's because asset_issuer is optional.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I see two ways of supporting an issued native token.

  1. Always require asset_issuer to be specified. I can't comment on if any wallets are leaving asset_issuer as null today, but as far as breaking changes go, I don't think it's too difficult to migrate over.
  2. Use SEP-11/SEP-38 asset identification format by default, but allow clients to use asset_code and asset_issuer with the goal of eventually deprecating the asset_code and asset_issuer pair.

Of these two options, I prefer 2. It lets us move to using SEP-38/11 format everywhere.

Copy link
Member

Choose a reason for hiding this comment

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

Both options assume that it is a good thing to make the issuer required in the API. I'm wondering what the context is on how it came to be optional. @JakeUrban do you have any context?

Copy link
Contributor

Choose a reason for hiding this comment

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

Unfortunately the asset_issuer field was optional from the start (at least of my time at the SDF).

The good news is that while there may be an asset with code native that exists on the network, the only time this becomes problematic is if an anchor wants to support both the native non-XLM asset and XLM itself. In this corner case, the anchor would have to require the asset_issuer field in order to differentiate.

I think this situation is acceptable in order to avoid adding something like asset_type to the list parameters.

Longer-term, I think we do probably need to introduce an asset parameter that can be used as a unique identifier for all on-chain assets and deprecate the existing set of parameters (cc @philipliu), but I wouldn't want to make that parameter required in order to anchor XLM or other assets (such as AMM shares?), so we should find a backwards-compatible solution in the meantime.

Copy link
Member

@leighmcculloch leighmcculloch Jun 16, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

@JakeUrban For true native (rather than credit native) should the SEP require asset_issuer: null? This would be one way to do as you say, but make it deterministic.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yea I think thats appropriate, although I wouldn't be surprised if custom anchor implementations skip. Still good to include it in the specification though.

| `amount` | number | (optional) Amount of asset requested to deposit. If this is not provided it will be collected in the interactive flow. |
| `account` | `G...` or `M...` string | (optional) The Stellar or muxed account the client wants to use as the destination of the payment sent by the anchor. Defaults to the account authenticated via SEP-10 if not specified. |
| `memo_type` | string | (optional) Type of memo that anchor should attach to the Stellar transaction, one of `text`, `id` or `hash`. |
Expand Down Expand Up @@ -518,8 +518,8 @@ Request parameters:

| Name | Type | Description |
| ------------------ | ---------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | ------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------ |
| `asset_code` | string | Code of the asset the user wants to withdraw. The value passed must match one of the codes listed in the [/info](#info) response's withdraw object. |
| `asset_issuer` | string | (optional) The issuer of the stellar asset the user wants to withdraw with the anchor. If asset_issuer is not provided, the anchor should use the asset issued by themselves as described in their TOML file. |
| `asset_code` | string | Code of the asset the user wants to withdraw. The value passed must match one of the codes listed in the [/info](#info) response's withdraw object. `native` is a special `asset_code` that represents the native XLM token. |
| `asset_issuer` | string | (optional) The issuer of the stellar asset the user wants to withdraw with the anchor. If asset_issuer is not provided, the anchor should use the asset issued by themselves as described in their TOML file. If `native` is specified as the `asset_code`, `asset_issuer` must be not be set. |
| `amount` | number | (optional) Amount of asset requested to withdraw. If this is not provided it will be collected in the interactive flow. |
| `account` | `G...` or `M...` string | (optional) The Stellar or muxed account the client will use as the source of the withdrawal payment to the anchor. Defaults to the account authenticated via SEP-10 if not specified. |
| `memo` | string | (**deprecated**, optional) This field was originally intended to differentiate users of the same Stellar account. However, the anchor should use the `sub` value included in the decoded SEP-10 JWT instead. Anchors should still support this parameter to maintain support for outdated clients. See the [Shared Account Authentication](#shared-omnibus-or-pooled-accounts) section for more information. |
Expand Down Expand Up @@ -768,6 +768,12 @@ wallet. For example:
## Info

Allows an anchor to communicate basic info about what their `TRANSFER_SERVER_SEP0024` supports to wallets and clients.
If an anchor supports the native asset, the `native` asset code with a `null` asset issuer should be included in the response.

The `native` asset on the [Stellar Public Network][pubnet] is the [lumen (XLM)][lumen].

[pubnet]: https://developers.stellar.org/docs/fundamentals-and-concepts/testnet-and-pubnet
[lumen]: https://stellar.org/lumens

### Request

Expand Down Expand Up @@ -799,6 +805,11 @@ The response should be a JSON object like:
"enabled": true,
"fee_fixed": 0.002,
"fee_percent": 0
},
"native": {
"enabled": true,
"fee_fixed": 0.00001,
"fee_percent": 0
}
},
"withdraw": {
Expand All @@ -811,6 +822,9 @@ The response should be a JSON object like:
},
"ETH": {
"enabled": false
},
"native": {
"enabled": true
}
},
"fee": {
Expand Down Expand Up @@ -942,7 +956,7 @@ Request parameters:

| Name | Type | Description |
| --------------- | ------------------- | ------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- |
| `asset_code` | string | The code of the asset of interest. E.g. BTC, ETH, USD, INR, etc. |
| `asset_code` | string | The code of the asset of interest. E.g. BTC, ETH, USD, INR, native, etc. |
| `no_older_than` | UTC ISO 8601 string | (optional) The response should contain transactions starting on or after this date & time. |
| `limit` | int | (optional) The response should contain at most `limit` transactions. |
| `kind` | string | (optional) The kind of transaction that is desired. Should be either `deposit` or `withdrawal`. |
Expand Down Expand Up @@ -996,7 +1010,7 @@ Each object in the `transactions` array should have the following fields:

| Name | Type | Description |
| ------------------------- | ------ | --------------------------------------------------------------------------------------------------------------------------------------------------------------------- |
| `withdraw_anchor_account` | string | If this is a withdrawal, this is the anchor's Stellar account that the user transferred (or will transfer) their issued asset to. |
| `withdraw_anchor_account` | string | If this is a withdrawal, this is the anchor's Stellar account that the user transferred (or will transfer) their asset to. |
| `withdraw_memo` | string | Memo used when the user transferred to `withdraw_anchor_account`. Assigned null if the withdraw is not ready to receive payment, for example if KYC is not completed. |
| `withdraw_memo_type` | string | Memo type for `withdraw_memo`. |
| `from` | string | Stellar address the assets were withdrawn from |
Expand Down Expand Up @@ -1276,6 +1290,7 @@ There is a small set of changes when upgrading from SEP-6 to SEP-24.

## Changelog

- `v3.1.0`: Add native asset support to SEP-24 ([#1363](https://github.com/stellar/stellar-protocol/pull/1363))
- `v3.0.0`: Make the `account` parameter for `POST /transactions/deposit/interactive` request optional
([#1343](https://github.com/stellar/stellar-protocol/pull/1343))
- `v2.9.2`: Remove confusing statement on `updated_at` matching `completed_at` when `status` is `refunded`
Expand Down