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

feat: proposed fungible token trait #5

Merged
merged 17 commits into from
Sep 7, 2021

Conversation

hstove
Copy link
Contributor

@hstove hstove commented Jan 26, 2021

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

This addresses #1 and stacksgov/grants-program#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
Copy link

psq commented Jan 26, 2021

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
Copy link
Contributor Author

hstove commented Jan 26, 2021

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.

Copy link
Contributor

@friedger friedger left a comment

Choose a reason for hiding this comment

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

What about the error codes, should we use uint or a more useful description like proposed in #3


### Transfer

`(transfer ((recipient principal) (amount uint)) (response bool uint))`
Copy link
Contributor

Choose a reason for hiding this comment

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

The signature should have a sender because the sender can be the tx-sender or the contract-caller.

Copy link
Contributor

Choose a reason for hiding this comment

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

The order in the native function is amount sender recipient. To make it easier for clarity developers, I suggest to use that ordering of the native function.

Copy link

Choose a reason for hiding this comment

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

I had not needed to set a sender, but I suppose this is a valid point

Copy link
Contributor Author

@hstove hstove Jan 26, 2021

Choose a reason for hiding this comment

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

Thanks Friedger, I will change this based on that suggestion. I am actually not sure how authorization works in ft-transfer - is there any? The docs don't mention it as an error code, so likely not. We should add a note about authorization, and add it as boilerplate in the example repo. I'll also add tests around authorization.

Error codes will be fleshed out, as well.

Copy link
Contributor

Choose a reason for hiding this comment

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

There isn't any built-in authorization in ft-transfer? (or nft-transfer? for that matter). The contract that declares the token is solely responsible for ensuring that the caller is permitted to use the given sender argument. Note that this is different from stx-transfer?, which does require that the sender argument is equal to the calling context's tx-sender.

@jasperjansz
Copy link

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
Copy link
Contributor Author

hstove commented Jan 26, 2021

@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
Copy link

stackatron commented Jan 26, 2021

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
Copy link
Contributor Author

hstove commented Jan 26, 2021

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".


I have provided a simple implementation along with a Javascript client and tests. https://github.com/hstove/stacks-fungible-token

## Credit
Copy link
Contributor

Choose a reason for hiding this comment

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

Would you consider adding @psq as a co-author of this SIP?

Also, this paragraph would belong in the Related Work section, since it's describing a similar system.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Of course I would be more than happy to add @psq! Per SIP-000, I need to add an email address - do you have one you're willing to share, Pascal?

@jcnelson
Copy link
Contributor

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

@hstove
Copy link
Contributor Author

hstove commented May 28, 2021

I've also just rebased my branch on top of the hstove-feat/sip-10-ft branch in this repo

@friedger
Copy link
Contributor

@hstove Can we change get-balance-of to get-balance ?

@hstove
Copy link
Contributor Author

hstove commented May 28, 2021

We have contracts that are aiming to go on mainnet within a week, that have already dealt with lots of thrash due to change in the SIP. I personally would rather keep the SIP as-is than go through another round of changes.

@friedger
Copy link
Contributor

What is a week compared to eternity?

The request is months old.

@hstove
Copy link
Contributor Author

hstove commented May 29, 2021

Personally, I'm fine with the change, and I see the reasoning. I honestly am not sure how best to move forward on this. Am I supposed to be a dictator or is this a group decision? I certainly want it to be a group decision based on folks who have contributed in any way on this thread.

My only concern are the folks who are seriously close to shipping. Devs working on tokens: @whoabuddy @psq @philipdesmedt @pooleja. Also @aulneau who has a PR for supporting SIP 10 in the wallet

@Zk2u
Copy link

Zk2u commented May 29, 2021

Devs working on tokens: @whoabuddy @psq @philipdesmedt @pooleja. Also, @aulneau who has a PR for supporting SIP 10 in the wallet

Nah we should be okay. to be honest, it's a very minor change, and we can change it in a few cmd+h's and a single commit.

@hstove
Copy link
Contributor Author

hstove commented May 29, 2021

Ok, in the interest of time, let's just make the call now. I'm deploying the new trait at the moment and will push the changes soon

@hstove
Copy link
Contributor Author

hstove commented May 29, 2021

New traits deployed: SP3FBR2AGK5H9QBDH3EEN6DF8EK8JY7RX8QJ5SVTE.sip-010-trait-ft-standard
testnet: STR8P3RD1EHA8AA37ERSSSZSWKS9T2GYQFGXNA4C.sip-010-trait-ft-standard

@philiphacks
Copy link

philiphacks commented May 29, 2021

my 2 cents: this change was useless and nitpicking.. I can give the same argument for get-balance-of.. I like the -of since it is more expressive...

let's just not change the SIP anymore now, since this is becoming annoying :) let's ship

@jcnelson
Copy link
Contributor

jcnelson commented May 29, 2021

I honestly am not sure how best to move forward on this. Am I supposed to be a dictator or is this a group decision?

In general, it's best to resolve all points of contention within the same SIP. If we later regret the decisions made for the fungible token trait, we can always submit a new SIP that supersedes this SIP after this SIP activates (or gets rejected).

In this specific case, it looks like the point of contention is in the name of a function. I recommend that in such cases, the SIP author just pick a reasonable name and ship it if no resolution can be had.

@whoabuddy
Copy link
Member

Makes sense to me, and I'm less picky about the name, will update things on our side to match the current standard as it sounds like this is the final decision.

314159265359879 added a commit to 314159265359879/sips that referenced this pull request May 30, 2021
Update SIP-010 marktdown with latest from stacksgov#5
get balance change of May 29th
@314159265359879
Copy link
Contributor

314159265359879 commented Jul 31, 2021

New traits deployed: SP3FBR2AGK5H9QBDH3EEN6DF8EK8JY7RX8QJ5SVTE.sip-010-trait-ft-standard
testnet: STR8P3RD1EHA8AA37ERSSSZSWKS9T2GYQFGXNA4C.sip-010-trait-ft-standard

@hstove do you want to redeploy this to testnet now that it is restarted?

image

On the main page it still shows the link to the outdated trait with get-balance-of that may lead to mistakes. Want to update that? Or has the SIP since been activated when I search for "SP3FBR2AGK5H9QBDH3EEN6DF8EK8JY7RX8QJ5SVTE.sip-010-trait-ft-standard" on clarity-search I find two mentions (a) wrapped-nothing-v8) and (b) btc-ft-swap, I do not understand that second one so I don't know if that is implemented correctly, @jcnelson you help to analyze if (b) is correct?

I do see recent contracts that look like they try to implement the SIP010 trait but still use get-balance-of for example this one 11 days ago: https://search-clarity.dev/contracts/SP23DAB333A5CPFXNK13E5YMX1DZJ07112QNZEBCF.tokensoft-token-v3a this one did not show in the search because they deployed their own copy of the trait which is actually an outdated one (with get-balance-of) @psq is probably using it for testing.

Looks like 1 max 2 down, and at least 1 to go for activation.

@whoabuddy
Copy link
Member

Noting that MiamiCoin will support the mainnet trait tomorrow, and we have a version of SIP-010 deployed to testnet since we couldn't find the original (linked below):

https://explorer.stacks.co/txid/0x08cb05b2ecc3ffc67d5391d338314a89d53818917c6ba42090fe1a02681d3620?chain=testnet

@friedger also deployed one, but it is named slightly different and doesn't support the optional memo. Didn't notice that until just now.

https://explorer.stacks.co/txid/ST2PABAF9FTAJYNFZH93XENAJ8FVY99RRM4DF2YCW.ft-trait?chain=testnet

It's definitely hard to find these if you don't know where they are! There should be a process to redeploy these from the same address or at least the same/correct version upon chain resets for testnet.

@jcnelson
Copy link
Contributor

jcnelson commented Aug 31, 2021

Something interesting happened. A lot more token contracts seem to be implementing SP23DAB333A5CPFXNK13E5YMX1DZJ07112QNZEBCF.sip-010-v0a.ft-trait instead of this SIP's trait. @hstove what if this trait became the standard instead, since it's getting traction? Would it be reasonable for people who have implemented this SIP's trait to implement a proxy contract that implements this other trait?

@lgalabru
Copy link
Contributor

Unless I'm missing something and assuming clarity-search is working correctly, all the contracts implementing the trait SP23DAB333A5CPFXNK13E5YMX1DZJ07112QNZEBCF.sip-010-v0a.ft-trait are contracts deployed by the same owner.

@jcnelson
Copy link
Contributor

Ah, you're right @lgalabru. Should have noticed that. @hstove please disregard the above.

@314159265359879
Copy link
Contributor

314159265359879 commented Aug 31, 2021 via email

@whoabuddy
Copy link
Member

whoabuddy commented Sep 2, 2021

Seconding that question - @jcnelson can we consider SIP-010 activated?

Also noting that when I search for the trait via clarity-search, the miamicoin-token contract isn't coming up, but you can see we impl-trait on line 20 of the source.

https://search-clarity.dev/?declare=&use=&search=SP3FBR2AGK5H9QBDH3EEN6DF8EK8JY7RX8QJ5SVTE.sip-010-trait-ft-standard

https://explorer.stacks.co/txid/0xc513b769c261233865c43f101438bd6359636ecacfe34576e6424d6c2629174e?chain=mainnet

@jcnelson
Copy link
Contributor

jcnelson commented Sep 7, 2021

Yes! Looks like the activation threshold has been reached! Merging.

@jcnelson jcnelson merged commit 4b61c47 into stacksgov:main Sep 7, 2021
jcnelson pushed a commit that referenced this pull request Dec 7, 2021
Adding an estimated block production time
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.