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

WIP: V2 FungibleToken Standard #77

Closed
wants to merge 59 commits into from
Closed

WIP: V2 FungibleToken Standard #77

wants to merge 59 commits into from

Conversation

joshuahannan
Copy link
Member

Adds the v2 fungible token standard, as described in this forum post: https://forum.onflow.org/t/streamlined-token-standards-proposal/3075/37

A few highlights:

  • Turns FungibleToken into a contract instead of a contract interface
  • Introduces a new contract interface with a totalSupply field and function to get the vault types that the contract implements. This will help external parties learn about which vaults a contract implements since contracts can implement multiple vault types now
  • Adds a Transferable interface to include a transfer function
  • Adds getAcceptedTypes() to the Receiver interface so a receiver interface can indicate which vault types it accepts.
  • Moves standard paths to inside the vault object definition because they are specific to the actual vault instead of the contract now.
  • Defines the VaultInfo struct to return type and path information for a specific vault type
  • Moves createEmptyVault into the vault resource
  • Adds type parameters to the all the events since contracts can have multiple vault types defined

There are probably more that I am missing, but most of the details are in the forum post, so that is another good resource.

@joshuahannan
Copy link
Member Author

joshuahannan commented Jul 18, 2022

The only test I have written so far is a deployment test, which is currently failing because it says the ExampleToken.Vault does not implement FungibleToken.Vault correctly, which I am confused about. maybe it is a bug.

Once we can align on which features to include, I'll work on adding more tests

@bluesign
Copy link

The only test I have written so far is a deployment test, which is currently failing because it says the ExampleToken.Vault does not implement FungibleToken.Vault correctly, which I am confused about. maybe it is a bug

Transfer?

@joshuahannan
Copy link
Member Author

oh yeah, good catch! The test still fails with the same error though. 😅

@bluesign
Copy link

bluesign commented Jul 18, 2022

oh yeah, good catch! The test still fails with the same error though. 😅

createEmptyVault inside vault interface, it should be on contract interface I guess. tried to do a PR but couldn't manage

Copy link

@austinkline austinkline left a comment

Choose a reason for hiding this comment

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

I hope it isn't too early to give feedback! 🙏

Main thing I think we could explore is whether a Provider can be rethought to include a way to scope them to a specific allowed amount to be withdrawn. Would love to hear your thoughts on that!

contracts/ExampleToken-v2.cdc Outdated Show resolved Hide resolved
let recipient = getAccount(recipient)

// Get a reference to the recipient's Receiver
let receiverRef = recipient.getCapability(self.ReceiverPublicPath)

Choose a reason for hiding this comment

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

Maybe out of scope, but you could make these transfers "safe" by making use of LostAndFound

Copy link
Member Author

Choose a reason for hiding this comment

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

I love what LostAndFound does, but I don't think it belongs in a standard like this unfortunately. I might be able to be convinced otherwise though

Choose a reason for hiding this comment

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

ExampleToken isn't the standard, though, is it? that's where we can add these kinds of utiltities

Choose a reason for hiding this comment

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

It absolutely doesn't belong in the FT interface or the base FT contract though. If ExampleToken should be a minimum implementation, it wouldn't belong here either so I think it just depends on what the purpose of ExampleToken is

Choose a reason for hiding this comment

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

I think there is definitely room for more than one Example, there should be at least a minimal single token example plus a separate multi-token example, a derived balance example etc.

Copy link
Member Author

Choose a reason for hiding this comment

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

Good point. I was getting them mixed up. I'll leave this open and come back to it once we address all the other comments. I like the suggestion of keeping this as a minimal example, then making another one that has multiple tokens, lostandfound usage, etc

Choose a reason for hiding this comment

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

This makes sense to me 👍

}

destroy() {
ExampleToken.totalSupply = ExampleToken.totalSupply - self.balance

Choose a reason for hiding this comment

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

Is there a way the FungibleToken interface itself can enforce that this is correct?

Copy link
Member Author

@joshuahannan joshuahannan Jul 18, 2022

Choose a reason for hiding this comment

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

not that I am aware of. @turbolent do you know if it would be possible to implement post conditions for destructors in a resource interface

Choose a reason for hiding this comment

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

I would put there something like:
if self.balance>0
99% of the cases it is 0.

Copy link
Member Author

Choose a reason for hiding this comment

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

What would that accomplish @bluesign ? Maybe I am misunderstanding.

Copy link

@bluesign bluesign Jul 21, 2022

Choose a reason for hiding this comment

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

flow still thinks registers are updated even update is no-op.

For example, currently FlowToken spams this on every block. ( FlowToken.totalSupply as changed ) Actually it is a bit Flow-go problem. But it is good to not even write something, when it is not changed I guess.

Copy link
Member Author

@joshuahannan joshuahannan Jul 21, 2022

Choose a reason for hiding this comment

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

oh yeah, that is a good point. I'll change that. That could be changed in the current example contract and flowtoken too

contracts/FungibleToken-v2-ContractInterface.cdc Outdated Show resolved Hide resolved
contracts/FungibleToken-v2-ContractInterface.cdc Outdated Show resolved Hide resolved
contracts/FungibleToken-v2.cdc Show resolved Hide resolved
contracts/FungibleToken-v2.cdc Outdated Show resolved Hide resolved
contracts/FungibleToken-v2.cdc Outdated Show resolved Hide resolved
contracts/FungibleToken-v2.cdc Outdated Show resolved Hide resolved
contracts/FungibleToken-v2.cdc Outdated Show resolved Hide resolved
pub resource interface Transferable {
/// Function for a direct transfer instead of having to do a deposit and withdrawal
///
pub fun transfer(amount: UFix64, recipient: Address)
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is this address and not a capability? I just feel that a capability gives the user so much more options here. They can transfer into their own capability with a good event.

Copy link
Member Author

Choose a reason for hiding this comment

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

I figured that people would only ever use this for a basic transfer, and if they want to use a special capability, they can just use the regular withdraw-deposit method. I'm not dedicated to that position though, so I could be convinced to put it as a capability if enough people want that.

Copy link
Contributor

Choose a reason for hiding this comment

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

The problem with withdraw/deposit is that it is impossible to create a good event out of that in the transfer method since it only contains one side. In .find profile we already link ftReceiver using our wallet abstraction and I would love to be able to send in that capability to this to be used for transfer.

Other use cases can be if you want to transfer funds from one wallet to a shared/pooled wallet of some kind.

I would really love though to have a static function in the new standard that can emit a shared event. Right now when you want to crawl for deposit events you have to know all the contract names. If the shared interface contract has a static function (when we get those) that said you could use to transfer with then that function could emit an event and everything would be so much simple. (The same argument is even stronger in NFTs IMHO)

Copy link
Member Author

@joshuahannan joshuahannan Jul 19, 2022

Choose a reason for hiding this comment

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

I'm not understanding the point about the event. We could still have a shared event here whether or not we have a capability or address:

pub event TokensTransferred(amount: UFix64, from: Address?, to: Address?, type: Type)

I do see your point about wanting to be able to use different kinds of capabilities. So are you imagining a transaction like this?

// Can pass in any storage path and receiver path instead of just the default.
// This lets you choose the token you want to send as well the capability you want to send it to.
transaction(amount: UFix64, to: Address, senderPath: StoragePath, receiverPath: PublicPath) {

    prepare(signer: AuthAccount) {

        // Get a reference to the signer's stored vault
        let vaultRef = signer.borrow<&{FungibleToken.Provider}>(from: senderPath)
			?? panic("Could not borrow reference to the owner's Vault!")

        let recipient = getAccount(to)
        let receiverCap = recipient.getCapability(receiverPath)

        // Transfer tokens from the signer's stored vault to the receiver capability
        vaultRef.transfer(amount: amount, recipient: receiverCap)
    }
}

That seems reasonable

contracts/FungibleToken-v2.cdc Outdated Show resolved Hide resolved
Comment on lines +103 to +106
/// Method to get the balance
/// The balance could be a derived field,
/// so there is no need to require an explicit field
pub fun getBalance(): UFix64

Choose a reason for hiding this comment

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

I am still not sure about balance being derived field.

are there some use-cases require this ?

Copy link
Member Author

Choose a reason for hiding this comment

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

@dete Was the one who made the ask for a derived field. Maybe he can provide some insight

Choose a reason for hiding this comment

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

Maybe it is because a derived field will make it easy to add some special mechanisms like rebase

Copy link

Choose a reason for hiding this comment

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

@dete Was the one who made the ask for a derived field. Maybe he can provide some insight
Maybe it is because a derived field will make it easy to add some special mechanisms like rebase

Highly discouraged to use getBalance() on flow just for an rebase token.
Since the contract codes can be updated on flow, malicious project can modify getBalance() to attack contracts that include this function.
It is risk to import an external function only for reading balance. for example, re-entrancy attack could ocuur

Choose a reason for hiding this comment

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

To give another side of the coin for balance being a function which I ran into recently while implementing ScopedFTProviders

Because balance is not a function, I could not actually wrap the FT standard because I need to delegate balance to the underlying capability being stored.

@joshuahannan
Copy link
Member Author

I'm actually going to hold off on including the scoped provider for now. It is something we will definitely include in this upgrade the future, but I don't want that discussion to distract from the discussion of the core proposal yet

@justjoolz
Copy link

justjoolz commented Jul 19, 2022

I'm not sure if I'm not getting something here but does this proposal allow creating new vault types at run time?
ie. using LP Tokens as an example.... how would you create a new token/vault type to match the new swap pool?
#59

@bjartek
Copy link
Contributor

bjartek commented Jul 19, 2022

Can we consider overflow or flow-js-testing for tests here? Both a very much more readable then these tests that contains alot of boilerplate.

contracts/FungibleToken-v2.cdc Outdated Show resolved Hide resolved
contracts/FungibleToken-v2.cdc Show resolved Hide resolved
@joshuahannan
Copy link
Member Author

@justjoolz I don't understand the use case very well, so I'm not totally sure, I don't think it could create new token types at run time that are equivalent to every other fungible token that gets implemented with this standard. You could potentially give your Vaults IDs so that they would be the same type but differentiated by IDs, but I don't know if that solves your use case, because to Cadence, they still would look like the same type. Does that help? Do you have any ideas?

@joshuahannan
Copy link
Member Author

@bjartek Yes, we're also going to refactor the tests here to use a better testing framework, but just haven't gotten to it yet

@justjoolz
Copy link

@joshuahannan use case is just that, creating tokens at run time. For example currently BloctoSwap and Incremental.fi have to deploy a new copy of the LP token contract to a new address for every swap pair that they list, which means a lot of identical code deployed on the network... extra storage space used etc.

A protocol that wants to issue 'social tokens' like Roll would have to do the same. Or an NFT fractionalizing protocol like Fractional.art .. any platform that wanted to programmatically issue unique tokens to creators on their platform.

With the current proposal here you'd need to be able to create a new resource type definitions at run time, I can't see how that could happen easily....

One possible solution following same pattern as #59 would be to use a more generic string identifier (which can still come from getType().identifier and be appended to if there are multiple tokens of the same type defined... something like: A.00000000.ExampleToken.Vault.TypeA
A.00000000.ExampleSwap.Vault.FLOW_FUSD

The identifier could be passed in as an optional parameter to the createNewVault, withdraw and transfer functions.... though it would be ugly passing in nil all the time when you didn't want to use this feature..... it would work great if Optional parameters where totally optional... as in you could choose to not pass in a value at all (instead of passing in nil it would simply default to nil if omitted) That would improve the semantics for interacting with normal single token per contract

What do you think? Is it too much to try and force this into the same standard? Would it be better as a separate complementary standard for these kind of use cases? Or better to just have everyone who wants this functionality to roll their own?

@joshuahannan
Copy link
Member Author

I still don't really see the need to deploy a whole new contract to do this, even with the existing fungible token standard. They could just give their vaults an identifier field and add a pre-condition to the deposit function that the deposited vault must have the same identifier as the vault being deposited into. Then they could also define a separate createEmptyVault with another parameter, like you say, to create a vault for a specific token type. Same with minting.

With this standard, if there is enough support for it, we could even make a getIdentifier(): AnyStruct? that optionally returns an identifier and also make the createEmptyVault() method include that second optional identifier by default.

Does all that make sense?

alilloig and others added 17 commits September 30, 2022 11:33
@joshuahannan joshuahannan changed the base branch from master to alilloig/ft-metadata-views October 5, 2022 15:46
@alilloig alilloig force-pushed the alilloig/ft-metadata-views branch from 247fb07 to ee3dd49 Compare November 29, 2022 19:09
Base automatically changed from alilloig/ft-metadata-views to master November 29, 2022 20:04
* avoid oppressive terminology

* avoid oppressive terminology

* Create FungibleTokenMetadataViews contract, include MetadataViews contract from NFT repo

* Apply suggestions from code review

Co-authored-by: Satyam Agrawal <[email protected]>

* Fix CI

* Remove unnecesary init parameter on ftvaultdata

* Fix CI

* Remove thumbnail and images from FTDisplay, add logo as only image and symbol string field

* Restore files after the testing crisis

* Fix setup account from view test

* Add auxiliary function for returning views and the case for returning the FTView

* Change logo (media) for logos (medias) at FTView

* Add comments and use of getFTView function instead of resolveView

* Add scripts for read metadata

* Delete returnview functions

* Switch to MetadataPublicPath

* Add metadata path

* Change balance for metadata path at transactions. Fix FTVaultData construction at ExampleToken

* Add default implementation for MetadataViews.Resolver methods

* Update contracts/FungibleToken.cdc

Co-authored-by: Peter Siemens <[email protected]>

* Update contracts/FungibleToken.cdc

Co-authored-by: Peter Siemens <[email protected]>

* Update contracts/FungibleTokenMetadataViews.cdc

Co-authored-by: Peter Siemens <[email protected]>

* Update contracts/FungibleTokenMetadataViews.cdc

Co-authored-by: Peter Siemens <[email protected]>

* Update contracts/FungibleToken.cdc

Co-authored-by: Peter Siemens <[email protected]>

* Update contracts/FungibleTokenMetadataViews.cdc

Co-authored-by: Peter Siemens <[email protected]>

* Update contracts/FungibleTokenMetadataViews.cdc

Co-authored-by: Peter Siemens <[email protected]>

* Update contracts/FungibleTokenMetadataViews.cdc

Co-authored-by: Peter Siemens <[email protected]>

* Update contracts/FungibleTokenMetadataViews.cdc

Co-authored-by: Peter Siemens <[email protected]>

* Update contracts/FungibleTokenMetadataViews.cdc

Co-authored-by: Peter Siemens <[email protected]>

* Update contracts/FungibleTokenMetadataViews.cdc

Co-authored-by: Peter Siemens <[email protected]>

* Add consistency about vaultData field name

* Fix spelling typos

* FT metadata docs section

* Finish docs

* add metadata views to go tests

* move view methods to the balance interface

* Remove MetadataViews import from FungibleToken contract

* Normalize flow.json

* Split test suites

* Move timeout to jest config instead of per test suite

* Update go test for metadataviews

* get go tests working

* update ci

* Change references to ResolvePublicPath for VaultPublicPath

* Separate metadata tests from core features

* Create folder only for metadata transactions

* Rename utilityContracts folder as utility

* Remove the need of linking a resolver on the provider for returning a FTVaultData

* Add test case for FTDisplay and kill bug on ExampleToken catched with said test

* Fix github ci to get latest Flow CLI version

* Fix bug on event from issue #92

* Standarize comments on Example Token

* Standarize comments to docgen tool format on the FungibleToken contract interface

* Include contracts docs

* Fix broken links

* Update package.json version

* Fix typos

* Fix public path after merge conflicts

* Bump decode-uri-component from 0.2.0 to 0.2.2 in /lib/js/test (#105)

Bumps [decode-uri-component](https://github.com/SamVerschueren/decode-uri-component) from 0.2.0 to 0.2.2.
- [Release notes](https://github.com/SamVerschueren/decode-uri-component/releases)
- [Commits](SamVerschueren/decode-uri-component@v0.2.0...v0.2.2)

---
updated-dependencies:
- dependency-name: decode-uri-component
  dependency-type: indirect
...

Signed-off-by: dependabot[bot] <[email protected]>

Signed-off-by: dependabot[bot] <[email protected]>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

* Add functionality to switchboard to link receiver capabilities to different receiver types (#107)

* Link the capabilty to the FT.Receiver also

* Add addNewVaultWrapper method

* Move create forwarder tx to its own folder

* Undo folder change to avoid modify tests

* Typo

* Create tx to add vault capability specifying the type to be deposited

* Create test for adding vault wrapper capability

* Make test

* Fix test name

* Fix comment

* Update version

* Update docs

* Update README.md

Co-authored-by: Joshua Hannan <[email protected]>

* Fix ci

* Update flow-cli version in ci workflow to 0.41.2 for flow-js-testing compatibility

Co-authored-by: Joshua Hannan <[email protected]>
Co-authored-by: Giovanni Sanchez <[email protected]>

* add import addresses

* Bump github.com/ethereum/go-ethereum in /lib/go/test (#117)

Bumps [github.com/ethereum/go-ethereum](https://github.com/ethereum/go-ethereum) from 1.9.13 to 1.10.22.
- [Release notes](https://github.com/ethereum/go-ethereum/releases)
- [Commits](ethereum/go-ethereum@v1.9.13...v1.10.22)

---
updated-dependencies:
- dependency-name: github.com/ethereum/go-ethereum
  dependency-type: indirect
...

Signed-off-by: dependabot[bot] <[email protected]>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

* Bump golang.org/x/text from 0.3.7 to 0.3.8 in /lib/go/test (#121)

Bumps [golang.org/x/text](https://github.com/golang/text) from 0.3.7 to 0.3.8.
- [Release notes](https://github.com/golang/text/releases)
- [Commits](golang/text@v0.3.7...v0.3.8)

---
updated-dependencies:
- dependency-name: golang.org/x/text
  dependency-type: indirect
...

Signed-off-by: dependabot[bot] <[email protected]>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

* Bump golang.org/x/net in /lib/go/test (#122)

Bumps [golang.org/x/net](https://github.com/golang/net) from 0.0.0-20220812174116-3211cb980234 to 0.7.0.
- [Release notes](https://github.com/golang/net/releases)
- [Commits](https://github.com/golang/net/commits/v0.7.0)

---
updated-dependencies:
- dependency-name: golang.org/x/net
  dependency-type: indirect
...

Signed-off-by: dependabot[bot] <[email protected]>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

* Bump golang.org/x/text from 0.3.3 to 0.3.8 in /lib/go/templates (#120)

* Bump golang.org/x/text from 0.3.3 to 0.3.8 in /lib/go/templates

Bumps [golang.org/x/text](https://github.com/golang/text) from 0.3.3 to 0.3.8.
- [Release notes](https://github.com/golang/text/releases)
- [Commits](golang/text@v0.3.3...v0.3.8)

---
updated-dependencies:
- dependency-name: golang.org/x/text
  dependency-type: indirect
...

Signed-off-by: dependabot[bot] <[email protected]>

* tidy

* update gomod

---------

Signed-off-by: dependabot[bot] <[email protected]>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Co-authored-by: Josh Hannan <[email protected]>

* Bump golang.org/x/crypto from 0.0.0-20210220033148-5ea612d1eb83 to 0.1.0 in /lib/go/templates (#124)

* Bump golang.org/x/crypto in /lib/go/templates

Bumps [golang.org/x/crypto](https://github.com/golang/crypto) from 0.0.0-20210220033148-5ea612d1eb83 to 0.1.0.
- [Release notes](https://github.com/golang/crypto/releases)
- [Commits](https://github.com/golang/crypto/commits/v0.1.0)

---
updated-dependencies:
- dependency-name: golang.org/x/crypto
  dependency-type: indirect
...

Signed-off-by: dependabot[bot] <[email protected]>

* go mod tidy

---------

Signed-off-by: dependabot[bot] <[email protected]>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Co-authored-by: Josh Hannan <[email protected]>

* update to latest v2 standard

---------

Signed-off-by: dependabot[bot] <[email protected]>
Co-authored-by: Bastian Müller <[email protected]>
Co-authored-by: Álvaro Lillo Igualada <[email protected]>
Co-authored-by: Satyam Agrawal <[email protected]>
Co-authored-by: Peter Siemens <[email protected]>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Co-authored-by: Giovanni Sanchez <[email protected]>
@joshuahannan
Copy link
Member Author

Closing this in favor of #131 because I screwed up Git and I'm not sure how to fix it

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.