stacksgov / sips

Community-submitted Stacks Improvement Proposals (SIPs)
132 stars 80 forks source link

feat: proposed fungible token trait #5

Closed hstove closed 3 years ago

hstove commented 3 years ago

Included is a proposed standard and trait for fungible tokens on the Stacks blockchain.

This addresses #1 and https://github.com/stacksgov/Stacks-Grants/issues/44

cc @psq - almost all of this code comes from your work, and you deserve all the credit for this. Please request any changes you'd like to see, and I will include them.

psq commented 3 years ago

there were some discussions on adding an icon for the token. One of them was to add a function such as:


;; Icon URL, limited to 2048 chars
    (icon-url () (response (string-ascii 2048) (tuple (kind (string-ascii 32)))))

The problem, though is that urls come and go, so this may not be ideal. However, we now have attachments, so that would be one way to do this. Or we can use IPFS/Filecoin/Skynet urls which should be more permanent.

Furthermore, we don't have optional functions like Solidity has, so my proposal would be to have optional traits that may not always be implemented (whether a contract support these optional traits can be done by parsing, and would be enforced at runtime by failing to meet the trait requirement.

So the icon function would be in such an optional trait:

(define-trait ft-trait-icon
  ;; Icon URL, limited to 2048 chars
    (icon-url () (response (string-ascii 2048) (tuple (kind (string-ascii 32)))))
)

One suggestion from @jcnelson was to return a (on-chain) BNS name instead of a url. Or make the url scheme be more like a more general uri that supports all possible ways to address the content.

hstove commented 3 years ago

Yeah, I think the icon problem is important but perhaps best to not be in this trait to keep things simple. There are also problems around icon URLs - potential for abuse (both via content and privacy), etc. If I was going to use this in a wallet or app like Swapr, I'd pull the icon from a registry, with whitelisted icons. See also https://tokenlists.org , used by Uniswap and others. Worth noting that name/symbol could be abused, too.

jasperjansz commented 3 years ago

Can't tell from the code if this is included, but: Should the name or symbol be globally unique? Imagine two different FT's with the same symbol "Foo". If none of the human-readable strings are globally unique we'd be back to using the fully-qualified name to differentiate between tokens.

On the other hand, this could also lead to squatting, so I'm not too sure about it.

hstove commented 3 years ago

@jasperjansz , this is not in the spec, nor is it really possible to address in this spec. I linked up above, but there is almost no way around having some kind of registry for tokens, even if on-chain. For example, a token registry contract could be fully permissionless, and enforce unique names within that registry.

Squatting is definitely something you wouldn't want. If you want human-readable names and global uniqueness, which is obviously good, then BNS is probably a good option.

stackatron commented 3 years ago

Same thing was on my mind as @jasperjansz. The scenario where an end user really needs to verify which token is which is also probably the scenario where that token is not whitelisted yet. If so, does that mean contract address will become the de facto canonical identifier? Pretty easy, not super intuitive, and I wonder if we can do better.

hstove commented 3 years ago

If so, does that mean contract address will become the de facto canonical identifier?

Yes, however, we can and should improve on that. I just think it's out of scope for this SIP. This is what registries are for. Uniswap uses https://tokenlists.org, and we can utilize on-and-off chain registries. Such registries could use BNS, too. Either way, you need some de-facto trusted source for "this contract identifier is the true $JEFF token".

jcnelson commented 3 years ago

Lots of interesting things happening here. Keep up the great work everyone!

hstove commented 3 years ago

One thing that isn't clear to me, yet. Should all of these functions be define-public, but all of them except transfer should be noted to not alter state? It sounds like we should make that change.

jcnelson commented 3 years ago

One thing that isn't clear to me, yet. Should all of these functions be define-public, but all of them except transfer should be noted to not alter state? It sounds like we should make that change.

I think it's generally a good idea to define things as read-only unless you can think of a non-trivial use-case that absolutely needs read/write. Think of it as an application of the principle of least privilege -- code that doesn't need permission to write should be denied it at deploy-time.

But, I don't think it can be enforced by the trait itself. Downstream consumers of the contract, like exchanges and registries, would need to inspect the analysis metadata and determine that the right methods are read-only (but note that analysis metadata is not consensus-critical -- it is allowed to change from release to release). If you decide that these methods should be read-only, you should say so in the SIP and advise implementers to confirm this via the contract ABI.

pooleja commented 3 years ago

Was the allowance/approval features of ERC20 left out of this on purpose (it looks like transfer and transferFrom were collapsed to a single transfer function)? This feature is pretty integral to contracts coordinating flow of funds between each other on Ethereum. Just curious if this should be documented on the rationale of leaving it out.

hstove commented 3 years ago

Was the allowance/approval features of ERC20 left out of this on purpose

They were left out, because in Clarity / Stacks you don't need approval. We have built-in fungible token primitives, and things like post conditions make it safer and easier. If you wanted to let a contract manage your tokens for you, then those contracts could have their own custom-built logic to ensure that they're not doing anything that a user doesn't want.

If you're thinking about other use cases that I'm missing, definitely let me know.

pooleja commented 3 years ago

Was the allowance/approval features of ERC20 left out of this on purpose

They were left out, because in Clarity / Stacks you don't need approval. We have built-in fungible token primitives, and things like post conditions make it safer and easier. If you wanted to let a contract manage your tokens for you, then those contracts could have their own custom-built logic to ensure that they're not doing anything that a user doesn't want.

If you're thinking about other use cases that I'm missing, definitely let me know.

If I am executing logic in contract A and I want to trigger logic in contract B which includes sending tokens to contract B, how would this be accomplished?

In Ethereum it would follow this pattern:

  1. Contract A approves Contract B to move X tokens on its behalf
  2. Contract A calls Function Y on Contract B
  3. In Function Y, Contract B moves tokens and executes other logic

The main question is "How do I prove to Contract B that I sent X tokens"? In Ethereum it is solved by contract B moving the tokens itself, therefore it knows the tokens were sent. Can this be done another way using the built-in primitives?

hstove commented 3 years ago

Does contract A own the tokens (i.e. a liquidity pool), or does the user own the tokens, and is simply initiating with contract A?

If the user is initiating the tx, and the token amount is 50 $XYZ:

This flow would only be possible with contracts that conform to this trait (or whatever FT trait). Well, maybe not exactly true, if contract B had a hard-coded ft-token-contract reference.

It would be awesome to flesh out this with some real code.

Edit: this is also why Post Conditions are so important. If any contract can simply invoke (contract-call? whatever-token 50 tx-sender recipient), then you could see that it'd be dangerous. But with post conditions, you're saying "The only asset transfer this tx can do is X", and if that fails, then the whole tx is rejected.

pooleja commented 3 years ago

Does contract A own the tokens (i.e. a liquidity pool), or does the user own the tokens, and is simply initiating with contract A?

If the user is initiating the tx, and the token amount is 50 $XYZ:

  • User makes a contract call tx on contract A

    • The tx payload includes a post condition saying "Only allow this tx to move 50 $XYZ from my account"
  • contract A has the Clarity code (contract-call? SPXX.contract-b contract-b-method) (aka calling contract B)
  • Contract B has the clarity code (contract-call? ft-token-contract transfer 50 tx-sender recipient)

    • When doing a contract-call?, tx-sender remains the original user
  • Contract B does whatever, based on the success/failure of the transfer.

    • Contract B can also check the balance of the recipient if needed, i.e. (contract-call? ft-token-contract recipient)

This flow would only be possible with contracts that conform to this trait (or whatever FT trait). Well, maybe not exactly true, if contract B had a hard-coded ft-token-contract reference.

It would be awesome to flesh out this with some real code.

Edit: this is also why Post Conditions are so important. If any contract can simply invoke (contract-call? whatever-token 50 tx-sender recipient), then you could see that it'd be dangerous. But with post conditions, you're saying "The only asset transfer this tx can do is X", and if that fails, then the whole tx is rejected.

OK cool. I will look into post conditions.

How would the other scenario be handled? E.g. a contract is holding the tokens? Something like a contract is trying to interact with a DEX to swap tokens?

Also, using tx-sender is considered very unsafe in the Ethereum world. If any malicious contract gets control of the logic flow then it can take actions the original user may not expect. For this reason, I would expect that the token contract would be looking at contract-caller instead of tx-sender.

hstove commented 3 years ago

How would the other scenario be handled? E.g. a contract is holding the tokens? Something like a contract is trying to interact with a DEX to swap tokens?

Pretty much the same way - although contract A will use as-contract, which modifies the tx-sender keyword.

If any malicious contract gets control of the logic flow then it can take actions the original user may not expect. For this reason, I would expect that the token contract would be looking at contract-caller instead of tx-sender.

Yeah, I definitely get that. However, aren't post conditions basically the same thing as allow in ETH, but with more strictness and less complexity, like having to do two transactions to swap on Uniswap? Also in Clarity, there is a lot more transparency into the exact execution of a contract call. You can deterministically know the exact set of code paths that a single contract call will take (because it's not Turing complete). Obviously you can never just make any contract call to any contract and have a guarantee that it's not malicious, but you can guarantee it won't send any unapproved assets.

jcnelson commented 3 years ago

On top of that, there's no reentrancy in Clarity, so if contract A calls into contract B during a transaction, contract B cannot call any functions in contract A. This would preclude, for example, contract B being able to manipulate state in contract A.

jcnelson commented 3 years ago

So, I'm trying to help you get this over the finish line. The normal sequence of steps would be for this SIP to pass through Draft, Accepted, Recommended, Activation-in-Progress, and Ratified. Once my editing comments are addressed, let's move this to Accepted status -- this is a well-formed SIP that addresses a novel problem, and has a chance of being ratified.

What happens next is that the folks who sit on the Technical consideration board (which right now is just me) take a look at the SIP and vote to move it to Recommended status. I went ahead and created the boilerplate built-in consideration advisory board records in this repo. I'm happy to just unilaterally advance the SIP to Recommended status, but I'd like to invite @psq, @friedger, and anyone else who'd like to join the technical consideration advisory board to please do so, meaning they'll have to sign off on advancing this SIP to start its activation. If I don't hear back from anyone in a week, I'll go ahead and advance it myself.

hstove commented 3 years ago

Thanks Jude, that's great to hear! I'll update with you requested changes - they all sound good.

jcnelson commented 3 years ago

It is about writing into the standard that it must use a native asset, and therefore transfer must fail if no post-conditions are mentioned.

I don't think either of these can be programmatically enforced. The Clarity VM can't read a transaction's post-conditions, and there's no way for a trait to require that the implementation use a native token type (even if it could, there's no way to ensure that the native token type is used correctly). Would it be okay if the SIP text instead provided a strong recommendation that all downstream consumers of contracts implementing this trait should use post-conditions when interacting with it, and should avoid trait implementations that don't use a native token type?

lgalabru commented 3 years ago

Yeah, I definitely get that. However, aren't post conditions basically the same thing as allow in ETH, but with more strictness and less complexity, like having to do two transactions to swap on Uniswap?

I don’t think that post-condition are a way to get around the approve/sender scheme, specially when we look at “asynchronous” token transfers (perform a token transfer when another future transaction is realized). The DEX use case is great for illustrating this.
With the current trait, If Alice wants to sell 1 TOKA for 100 TOKB, we would have the following flow:

The approve/spender design is a way to avoid the Exchange contract to take custody of Alice’s tokens by introducing the concept of allowance. So back to our use case:

It’s true that when dealing with a contract like Uniswap, we wouldn’t need this scheme, but Uniswap is not an exchange, it’s a liquidity provider - as in when you want to swap TOKA for TOKB, the supply for TOKB is already there.

I'm a bit confused by the shape of the current trait, in particular, this method:

(transfer ((amount uint) (sender principal) (recipient principal)) (response bool uint))

If I'm trying to do a parallel with the ERC20 interface, this would be the equivalent of

function transferFrom(address _from, address _to, uint256 _value) public returns (bool success)

This function make sense in an ERC20 context, with an approve/sender scheme logic available, but in our case, I think that would be a footgun, because we're suggesting to contract developers that sender can be a free field, when it should only be tx-sender.

hstove commented 3 years ago

Sure, I follow this logic. I think there are some strong pros/cons around the allow/permit pattern:

Pros:

Cons:

If approve is really desired, then a meta-contract for approval (that takes custody) kind of gives you the best of both worlds. You can revoke the token later on, and only have to trust this one meta-contract to respect the revoke command. It can provide a simple interface that wallets can query to show "approved" wallet balances - technically not in your custody, but known and trusted with the ability to revoke.

Footnote: a nice improvement on approve is permit, which accepts a signature. This allows for gas-less token approvals. If we were going to go down this road, then it would probably best to skip approve and the crappy two-tx process.

hstove commented 3 years ago

Hey @friedger, I recognize that I've kind of avoided your suggestions around naming standards. I personally would prefer the shorter naming pattern (that also closely matches erc20), but I agree that we should have clear guidance for naming patterns. How about this - 👍🏼 this comment if you want get-balance (and get- for read-only intended methods), or 👎🏼 if you prefer the current naming scheme. I am more than happy to go with group consensus on this. Would appreciate comments from @kantai here.

It is about writing into the standard that it must use a native asset, and therefore transfer must fail if no post-conditions are mentioned.

I also am not sure we want to do this. Consider Flexr, which has a re-balancing token. This is pretty common in ETH these days. Can you always implement such a token with native assets?

jcnelson commented 3 years ago

I also am not sure we want to do this. Consider Flexr, which has a re-balancing token. This is pretty common in ETH these days. Can you always implement such a token with native assets?

If the token isn't implemented via one or more native token types, then post-conditions can't protect you. But, I think this should be possible with the fungible token type -- you can ft-burn? tokens to decrease the supply, just as you can ft-mint? tokens to increase it.

jcnelson commented 3 years ago

A meta-contract for implementing what is basically a custodial feature of tokens (you're letting someone else take possession of them for a time) almost sounds like a separate SIP to me?

kantai commented 3 years ago

How about this - 👍🏼 this comment if you want get-balance (and get- for read-only intended methods), or 👎🏼 if you prefer the current naming scheme

I vote for get-... for the read-only methods because it matches stx-get-balance, ft-get-balance, etc.

dantrevino commented 3 years ago

Yeah, I think the icon problem is important but perhaps best to not be in this trait to keep things simple. There are also problems around icon URLs - potential for abuse (both via content and privacy), etc. If I was going to use this in a wallet or app like Swapr, I'd pull the icon from a registry, with whitelisted icons. See also https://tokenlists.org , used by Uniswap and others. Worth noting that name/symbol could be abused, too.

A principal that mints a token could have a gaia bucket associated with it. Is there a way to utilize native functionality to enable icon urls? Maybe even Atlas attachments?

pooleja commented 3 years ago

Yeah, I think the icon problem is important but perhaps best to not be in this trait to keep things simple. There are also problems around icon URLs - potential for abuse (both via content and privacy), etc. If I was going to use this in a wallet or app like Swapr, I'd pull the icon from a registry, with whitelisted icons. See also https://tokenlists.org , used by Uniswap and others. Worth noting that name/symbol could be abused, too.

A principal that mints a token could have a gaia bucket associated with it. Is there a way to utilize native functionality to enable icon urls? Maybe even Atlas attachments?

In a token I am building, I took the Ethereum approach to add a "meta" URI that can contain links to things like icons, documentation, etc...

https://github.com/tokensoft/tokensoft_token_stacks/blob/main/contracts/metadata-uri-token-trait.clar

dantrevino commented 3 years ago

Yeah, I think the icon problem is important but perhaps best to not be in this trait to keep things simple. There are also problems around icon URLs - potential for abuse (both via content and privacy), etc. If I was going to use this in a wallet or app like Swapr, I'd pull the icon from a registry, with whitelisted icons. See also https://tokenlists.org , used by Uniswap and others. Worth noting that name/symbol could be abused, too.

A principal that mints a token could have a gaia bucket associated with it. Is there a way to utilize native functionality to enable icon urls? Maybe even Atlas attachments?

In a token I am building, I took the Ethereum approach to add a "meta" URI that can contain links to things like icons, documentation, etc...

https://github.com/tokensoft/tokensoft_token_stacks/blob/main/contracts/metadata-uri-token-trait.clar

This is the issue, imho. Without a standard some will opt to add metadata called "token-uri", which is perfectly valid. Others will opt to use "icon" or "image" or "img" or whatever. Supporting those is not scalable.

hstove commented 3 years ago

Thank you all for the prompt feedback! Two changes have been added to this SIP:

@friedger I've also added a note about using the native asset primitive functions. We can't mandate it in the trait, but I've included language that indicates they should always be used.

hstove commented 3 years ago

If the token isn't implemented via one or more native token types, then post-conditions can't protect you. But, I think this should be possible with the fungible token type -- you can ft-bun? tokens to decrease the supply, just as you can ft-mint? tokens to increase it.

I agree that rebalancing tokens should use the primitives and workaround any limitations provided by them. I would note that there are Ethereum rebalancing tokens where the result of balanceOf returns a different value every block, without actually changing chain state. It's interesting, but I would prefer using the native primitives despite not being able to have such behavior.

jcnelson commented 3 years ago

I think all the outstanding issues have been addressed, so I'm going to go ahead and move this to Activation-in-Progress. I've already seen at least one token contract implement this SIP on-chain. I modified the Status field in this branch: hstove-feat/sip-10-ft

hstove commented 3 years ago

@jcnelson I've added two commits - adding Pascal as an author, and adding the mainnet deployed trait. Just FYI in case you're planning on using your branch eventually when merging.

friedger commented 3 years ago

@hstove Is it too late to use the same type of errors in SIP 9 and SIP 10 for the transfer function?

dantrevino commented 3 years ago

As a developer and user, aligning error types across these standards would be immensely helpful. SIP9 error messages make more sense.

jcnelson commented 3 years ago

Nope, not too late at all! If you want to update the SIP and the trait deployment, that's fine with me. In general, it's only "too late" if the SIP gets activated.

friedger commented 3 years ago

get-balance-of vs get-balance

I am in favour of get-balance to match ft-get-balance

friedger commented 3 years ago

get-token-supply: What should I put if the supply is unlimited? (ok u0) ? What should I put if the supply is 0?

hstove commented 3 years ago

What is an "unlimited supply"? Even if there is no hard-cap, there has to be a fixed number of tokens at a specific point in time. Even for a rebalancing, algorithmic token.

In this sense, get-token-supply only refers to the current liquid supply. I don't think it should be in this trait, but there could be a trait for hard-cap tokens if the need arises.

psq commented 3 years ago

as part of writing a ui for swapr, I discovered that it is not possible (at least easily) to derive what native token may be used by a sip-010 token in order to set relevant post conditions. In some cases, you can correlate the contract principal of a token with the values returned by https://blockstack.github.io/stacks-blockchain-api/#operation/get_account_balance, but it is not always possible, for example in the case where the sip-010 contract is calling an other contract where the contract resides.

So, there are 4 options here I can see, but maybe others can propose other ways to fix this.

  1. add a new function to the trait to get the token name (get-native-token)
  2. not use post conditions if the token can not be inferred
  3. add an entry to the metadata file with the native-token-name as a string
  4. add an entry to the metadata file that provides hints on how to build the post conditions (which could support more complicated cases where a token uses multiple ft or nft for its implementation, but this probably requires more research)

My current thinking favors #3, and use this as the only option of retrieving the native token to use, but open to hearing what others thoughts before going to deep into this. Reverting to #2 if this information is not in the metadata, or metadata-uri is none

friedger commented 3 years ago

@psq The long term goal is to run a function call in read-only mode, the resulting events should provide detailed information to create post conditions.

jcnelson commented 3 years ago

Looks like we got our first taker! https://explorer.stacks.co/txid/0x022bed728d648ff1a68036c40f3aff8136ee22fee18380731df0ab9d76d3c4a9?chain=mainnet

Zk2u commented 3 years ago

Looks like we got our first taker! https://explorer.stacks.co/txid/0x022bed728d648ff1a68036c40f3aff8136ee22fee18380731df0ab9d76d3c4a9?chain=mainnet

have you only just discovered Nothing!? wow...

you can go get some for free here :)

jcnelson commented 3 years ago

I know Nothing ;) This is just the first time I've seen Nothing officially implement the SIP 010 trait.

lgalabru commented 3 years ago

I was thinking about SIP10, and was wondering if we should add an optional buff 20/32 memo, in the transfer function. Any thought?

Zk2u commented 3 years ago

I was thinking about SIP10, and was wondering if we should add an optional buff 20/32 memo, in the transfer function. Any thought?

Definitely. That's essential.

hstove commented 3 years ago

wondering if we should add an optional buff 20/32 memo, in the transfer function

I certainly see the reasoning for this.

In Stacks 2.1, we're adding a new function to transfer STX with a memo. Should we do the same for fungible token transfers?

The memo is mainly for exchange deposits, and so even if we add a transfer-memo type function to the SIP, we still need to add a bunch of guidance (to print the memo) and API wrappers to get transfer events with memos. Does it makes sense to wait to do this in 2.1? I can see why we shouldn't wait.

The other question is - should this SIP be updated, or should there be an extension trait?

hstove commented 3 years ago

Regarding @psq 's point about getting the token name for post conditions (sorry for the late response):

I totally see the issue here. However, I don't think adding to the trait is the best idea. Generally, we shouldn't have to add on-chain data for things that only are needed off-chain. Now, I can see why this is contrary to the name, symbol, etc methods.

I also agree with Friedger that we should just fetch this from an API. One thing you can do is fetch the interface, and the interface includes the token name, which can be used for post conditions. This could include more than one token, technically, but that could be considered an "invalid token implementation" IMO.

Fetching from the interface is also more "safe". There is no guarantee that the developer-provided token name is the actual token name. However, the token name in the interface is guaranteed to be "correct"

jcnelson commented 3 years ago

The other question is - should this SIP be updated, or should there be an extension trait?

Either one is fine with me. A contract could simply implement both SIP 010 and the extension trait, and services that need the memo field can use the Stacks node's RESTful API to verify that the SIP 010 implementation also implements this hypothetical "send-with-memo" trait.

lgalabru commented 3 years ago

The memo is mainly for exchange deposits

Agreed, however for protocol designers, having their token listed on exchanges (centralized or decentralized) is a key step, and, pushing developers to design their tokens after a standard incompatible with exchange requirements could compromise success. Moving from (transfer ((amount uint) (sender principal) (recipient principal)) (response bool uint)) to (transfer ((amount uint) (sender principal) (recipient principal) (optional (buff 32))) (response bool uint)) is a migration that sounds pretty straightforward today with a nascent ecosystem.

jcnelson commented 3 years ago

If you choose to do that, and update the SIP, then the number of compatible tokens will reset to 0.