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

Add native asset support to SEP-24 #1363

Merged
merged 6 commits into from
Jun 16, 2023

Conversation

philipliu
Copy link
Contributor

@philipliu philipliu commented Jun 14, 2023

Abstract

Today, only issued assets are supported in SEP-24 transactions. The native asset XLM cannot be specified using the current asset_code, asset_issuer pair as XLM does not have an issuer.

Proposal

This proposes native be treated as a special asset_code, that when specified in combination with a null asset_issuer, allows wallets to deposit and withdraw XLM. Although this differs from the SEP-38 asset identification format, this reduces the friction for existing anchors to onboard to support this new feature.

Backwards compatibility

This change is fully compatible.

@philipliu philipliu changed the title [DRAFT] Add XLM support to SEP-24 Add XLM support to SEP-24 Jun 14, 2023
@philipliu philipliu marked this pull request as ready for review June 14, 2023 19:58
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.

This LGTM, I think its just missing a note in the section for GET /info on being able to use native as the asset code for XLM.

@philipliu philipliu requested a review from JakeUrban June 14, 2023 21:01
Ifropc
Ifropc previously approved these changes Jun 14, 2023
ecosystem/sep-0024.md Outdated Show resolved Hide resolved
Ifropc
Ifropc previously approved these changes Jun 15, 2023
Copy link
Member

@leighmcculloch leighmcculloch left a comment

Choose a reason for hiding this comment

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

Thoughts inline.

| `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

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.

ecosystem/sep-0024.md Outdated Show resolved Hide resolved
Wording changes

Co-authored-by: Leigh McCulloch <[email protected]>
@leighmcculloch leighmcculloch changed the title Add XLM support to SEP-24 Add native asset support to SEP-24 Jun 15, 2023
@philipliu philipliu merged commit 910fdc9 into stellar:master Jun 16, 2023
@philipliu philipliu deleted the feature/xlm-transactions branch June 16, 2023 19:23
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.

4 participants