-
Notifications
You must be signed in to change notification settings - Fork 312
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
SEP-1: Add contract
field to CURRENCIES
list
#1437
Conversation
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.
Thoughts inline.
ecosystem/sep-0001.md
Outdated
code | string (<= 12 char) | Token code | ||
code | string (<= 12 char) | Asset code. Required for assets that are issued using a Stellar account as opposed to a contract. | ||
issuer | `G...` string | Asset issuer Stellar public key. Required for assets that are issued using a Stellar account as opposed to a contract. | ||
contract | `C...` string | Contract ID of the asset contract. The asset must be compatible with the [SEP-41 Token Interface](sep-0041.md) to be defined here. |
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.
Should this be limited to assets, which are classic Stellar, or should it support custom tokens?
We're using language here like assets but I think we're using it to refer to also include custom tokens.
As much as possible we should only use the term asset for stellar assets, and use the term token for the superset that includes stellar assets and custom tokens implementing the interface.
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.
As much as possible we should only use the term asset for stellar assets, and use the term token for the superset that includes stellar assets and custom tokens implementing the interface
Is that definition consensus among the ecosystem? I always thought of tokens to be "not Stellar assets" meaning non-SAC tokens, whether they're compatible with the SEP-41 token interface or not.
should it support custom tokens?
I don't think it is useful to list custom tokens that don't adhere to a standardized interface, since the client won't know how to interface with the contract. Thats why I said it must be compatible with the SEP-41 token interface.
Maybe we should add another field like contract_interface
with its supported values being standard token interfaces like SEP-41 or a future NFT token interface, for example.
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.
When communicating with the Stellar Asset contract developers use the token client, so I think tokens definitely include assets. We have pretty uniformly used the term "custom token" with the word "custom" when referring to implementations of tokens that are exclusively not an asset.
For the most part contract devs should dev for tokens, that will support both.
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.
cc @tomerweller @namankumar regarding confirming this nomenclature isn't just me.
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.
custom tokens that don't adhere to a standardized interface, since the client won't know how to interface with the contract. Thats why I said it must be compatible with the SEP-41 token interface.
To be clear, when we say custom tokens, we're referring to any contract that implements the SEP-41 token interface that isn't the asset contract.
token = any contract that implements SEP-41
custom token = any contract that implements SEP-41 excluding the stellar asset contract
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.
What is our term for non-SEP41 tokens like NFTs?
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 don't think a term has arisen for them yet.
ecosystem/sep-0001.md
Outdated
code | string (<= 12 char) | Token code | ||
code | string (<= 12 char) | Asset code. Required for assets that are issued using a Stellar account as opposed to a contract. | ||
issuer | `G...` string | Asset issuer Stellar public key. Required for assets that are issued using a Stellar account as opposed to a contract. | ||
contract | `C...` string | Contract ID of the asset contract. The asset must be compatible with the [SEP-41 Token Interface](sep-0041.md) to be defined here. |
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.
Is the contract field optional? We should specify that.
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.
Well, contracts for Stellar assets are technically optional, since it requires someone to deploy the SAC for it. At the same time, it should be listed if a contract for the asset exists. I'm not sure how to phrase this.
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.
Anyone can create the contract for a stellar asset, it doesn't have to be the issuer, so I'm not sure there is a natural dependency here that if one exists the other should exist.
The contract is always derivable from the code and issuer and network passphrase, so maybe we should never specify the contract for stellar assets and make issuer and contract mutually exclusive. That way applications don't make the assumption the field will be present and applications don't depend on issuers adopting Soroban.
I think it's preferred to assume a contract always exists, and applications that interact with the contract need to be resilient to it not being created yet and creating it if it doesn't exist, or restoring it if it has been archived due to a lack of use.
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.
While it makes things harder for clients, I think making (code, issuer) and (contract) mutually exclusive makes sense. This makes it clear whether there is a Stellar asset (and presumably a contract) or just a contract.
It also could be more secure or less error prone. What are the consequences of a client matching a currency listed in a stellar.toml by its (code, issuer) and then interacting with a malicious contract listed alongside the other fields?
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.
Agreed it could be more secure. If the contract is provided for stellar assets then it will be possible for applications to depend on it being correct, and a compromised .toml file could change the contract ID while keeping the issuer and code the same, causing applications to interact with a contract for a different issuer. Leaving contract out forces everyone to act the same way with a single set of inputs.
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.
cc @orbitlens Thoughts?
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'm opposed to automatically deriving contract
from the code
+issuer
combination for Classic assets.
Reasons:
- The client (e.g. wallet) will always has to do additional unnecessary computations to derive the contract address, which might be expensive in terms of performance for cases when a client needs to process metadata for large number of assets at a time (for example, render account balances or show transactions history). Just to understand, sometimes we have to pull metadata for hundreds of assets from different issuers just to render a single page.
- Automatic contract address derivation doesn't necessarily mean that the contract actually exists on the ledger. If nobody deployed an SAC contract, then wallet needs to verify the SAC contract existence before executing some operations (this won't affect most contract interactions that should be covered by the preflight simulation request, but might pose unnecessary problems for pulling SAC contract statistics and balances).
- Some issuers might want to implement custom logic for their Classic assets on the Soroban side, providing custom token implementation instead of the built-in SAC.
@leighmcculloch 's argument regarding additional risks posed by a compromised TOML file is totally valid, but I don't think we should prevent issuers from creating custom contracts for their Classic assets. We don't discard SEP-24 and SEP-31 only because they both are potentially vulnerable to the same attack.
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 for the insights. I had been thinking that if a contract address was different to the contract for the code and issuer, that that'd be either a mistake, or malicious event. I hadn't considered that it could be valid for an issuer to want to represent their one conceptual token with two tokens on Stellar, the classic one and the Soroban one in a custom contract.
It's worth noting that in that case where an issuer has a stellar asset and a custom contract, the stellar asset still has a stellar asset contract that anyone can deploy, and I anticipate there will be tooling that automatically derives the contract ID, especially for decentralized tokens that have no toml file, but also in tools that don't wish to talk to the outside world to determine what the contract ID is.
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.
Some issuers might want to implement custom logic for their Classic assets on the Soroban side, providing custom token implementation instead of the built-in SAC.
I hadn't considered that it could be valid for an issuer to want to represent their one conceptual token with two tokens on Stellar, the classic one and the Soroban one in a custom contract.
I do not think it is valid to represent two completely distinct tokens as one, and I don't see why anyone would want to do this. If you want custom logic for your token, in what scenario does it still make sense to have a Stellar asset that is presented to the user as the same token? These two tokens would have different functionality and they would be held in two separate balance entries (one trustline & one contract data). Unifying the two under one UX would almost certainly result is weird user flows, since only a portion of the total unified balance would support whatever custom functionality was implemented in the contract.
If you want custom logic for your token, you are definitely better off not having an associated Stellar asset at all.
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 updated the PR to have issuer
and contract
fields be mutually exclusive. I figured code
should be present for all entries.
code_template | string (<= 12 char) | A pattern with `?` as a single character wildcard. Allows a `[[CURRENCIES]]` entry to apply to multiple assets that share the same info. An example is futures, where the only difference between issues is the date of the contract. E.g. `CORN????????` to match codes such as `CORN20180604`. | ||
issuer | `G...` string | Token issuer Stellar public key |
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.
Good idea to group the fields. 👍
ecosystem/sep-0001.md
Outdated
code | string (<= 12 char) | Asset code. Required for assets that are issued using a Stellar account as opposed to a contract. | ||
issuer | `G...` string | Asset issuer Stellar public key. Required for assets that are issued using a Stellar account as opposed to a contract. | ||
contract | `C...` string | Contract ID of the asset contract. The asset must be compatible with the [SEP-41 Token Interface](sep-0041.md) to be defined here. |
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.
code | string (<= 12 char) | Asset code. Required for assets that are issued using a Stellar account as opposed to a contract. | |
issuer | `G...` string | Asset issuer Stellar public key. Required for assets that are issued using a Stellar account as opposed to a contract. | |
contract | `C...` string | Contract ID of the asset contract. The asset must be compatible with the [SEP-41 Token Interface](sep-0041.md) to be defined here. | |
code | string (<= 12 char) | Asset code. Omitted for custom contract tokens not associated with Classic assets. | |
issuer | `G...` string | Asset issuer Stellar public key. Omitted for custom contract tokens not associated with Classic assets. | |
contract | `C...` string | Contract ID of the [SEP-41](sep-0041.md)-compatible asset contract. Omitted for Classic assets not associated with contract tokens. |
Most assets (probably > 90%) will be circulating in both Classic and Soroban environments. Also, the issuer that has an existing Classic asset may advertise a custom contract for Soroban instead of the SAC for this particular asset. To my mind, the wording I proposed better reflects this general idea.
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.
Worth noting that every Stellar asset has a contract, even if it hasn't yet been deployed, anyone can deploy it without any special authorisation. So an issuer can deploy a custom contract that they wish folks to use for their token, but if their token is also a Stellar asset they can't prevent someone else deploying the Stellar asset contract for their asset.
For example, if you're using the latest soroban-cli, you should be able to run the following command for any code and issuer, where the contract hasn't been deployed yet:
soroban contract asset deploy --asset [code]:[issuer] --network ... --source ...
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.
@leighmcculloch Yes, I know, we discussed this specific case here: https://discord.com/channels/897514728459468821/1201751319451553853/1202058781320826940
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.
Ah yes, thanks for the link!
ecosystem/sep-0001.md
Outdated
code | string (<= 12 char) | Token code | ||
code | string (<= 12 char) | Token code. Required. | ||
issuer | `G...` string | Stellar public key of the issuing account. Required for tokens that are issued using a Stellar account. Omitted if the token is not issued using a Stellar account. | ||
contract | `C...` string | Contract ID of the token contract. The token must be compatible with the [SEP-41 Token Interface](sep-0041.md) to be defined here. Required for tokens that are not issued using a Stellar account. Omitted if the token is issued using a Stellar account. |
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.
One thing I'm not sure about is how we distinguish between custom tokens vs stellar assets.
The way we distinguish here is that they are "not issued using a Stellar account," but it's not really who issued them that distinguishes them, it's what capabilities and behaviours they have, or whether they are or aren't Stellar assets specifically.
Maybe we could do with a brief blurb at the top of this section about the two types of "currencies" that this section supports. Something like:
Currencies can be Stellar Assets that have been issued by a Stellar account and share the same common behaviors, or custom token contracts that satisfy the SEP-41 interface but implementation and behavior is customizable.
contract | `C...` string | Contract ID of the token contract. The token must be compatible with the [SEP-41 Token Interface](sep-0041.md) to be defined here. Required for tokens that are not issued using a Stellar account. Omitted if the token is issued using a Stellar account. | |
contract | `C...` string | Contract ID of the token contract. The token must be compatible with the [SEP-41 Token Interface](sep-0041.md) to be defined here. Required for tokens that are not Stellar Assets. Omitted if the currency is a Stellar Asset. |
Discussed in detail on discord, this change adds the
contract
field to theCURRENCIES
list. This enables businesses to list the contract IDs for Stellar assets or Soroban Tokens they support.cc @orbitlens