flow-ft
flow-ft copied to clipboard
FEATURE: V2 FungibleToken Standard
This is a different PR from the original one because the git organization had gotten out of sync
Adds the v2 fungible token standard, as described in this flip: https://github.com/onflow/flips/pull/55
A few highlights:
- Turns
FungibleTokeninto 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
Transferableinterface to include a transfer function - Moves standard paths to inside the vault object definition because they are specific to the actual vault instead of the contract now.
- Moves
createEmptyVaultinto 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.
Would it be possible at all for a Vault in FT to optionally contain the address it was withdrawn from? Just beeing able to look at that to know who is paying for things would make some things way easier.
Would it be possible at all for a Vault in FT to optionally contain the address it was withdrawn from? Just beeing able to look at that to know who is paying for things would make some things way easier.
I think this can be very good case for attachments. Extra metadata like this can be added as an attachment maybe.
I just pushed changes to the standard based on the discussions we've been having in the standard breakout sessions, including events emitted from interfaces.
Would it be possible at all for a Vault in FT to optionally contain the address it was withdrawn from? Just being able to look at that to know who is paying for things would make some things way easier.
I agree this could be good for attachments. It would be possible to do this in a way with the standard, but idk if it is safe
I agree this could be good for attachments. It would be possible to do this in a way with the standard, but idk if it is safe
Can we get some eyes on this to ensure if it is safe or not?
It is kind of similar to the msg.sender problem. If we had a field on every vault that was the previous owner, it would be easy to spoof that and just give it a different previous owner by depositing it into a different vault. Also, we still can't add fields to resources in upgrades, so I'm not sure how we would add that in an upgrade. Does that make sense?
Will FT-V2 be finalized and released before Cadence1.0 testnet (Mid-Feb)?
A lot of contracts are depending on this, it'd be better to leave enough time for ppl to be familiar with a finalized version and prepare for the upgrade.
@dryruner yes that is the plan. It is live in the emulator for stable cadence already.
@bjartek @joshuahannan The emulator version is not enough, a finalized merged version should be released early on github for dev to fetch from.
Also not sure what's wrong with the emulator (Version: v1.9.2-stable-cadence.1), but the deployed contracts there looks weird. For example: FlowToken deployed on emulator still uses PrivatePath, which should be deprecated / disallowed in Cadence 1.0.
Hello @joshuahannan,
We've encountered a problem with the current proposed FT-v2 contract.
Is there any specific reason that's not making FungibleToken a contract interface and enforcing any other token contracts to inherit from it? (so FT becomes the "most base" class of any token in Cadence environment). E.g. change it to:
access(all) contract interface FungibleToken {
access(all) resource interface Balance { ... }
access(all) resource interface Provider { ... }
access(all) resource interface Receiver { ... }
access(all) resource interface Vault { ... }
access(all) fun createEmptyVault(): @{Vault} { ... }
}
Otherwise (with the current proposed FT.cdc), it looks like impossible for us to dynamically borrow a token contract from its Address, creating an empty Vault of its type. So this will severely impact us in swap pair creation, dex-aggregation tx, may be other places.
Please let me know your thoughts on this, thx!
If i remeber correctly @dryruner i belive the pattern that was intended here was to implement FungibleTokenMetadataViews.FTVaultData and have your FT implement ViewResolver and then query for the function that way.
However today we have a meeting in 4 hours or so where we will talk about this. I will make sure to bring this up.
@bjartek @joshuahannan The emulator version is not enough, a finalized merged version should be released early on github for dev to fetch from.
Also not sure what's wrong with the emulator (Version: v1.9.2-stable-cadence.1), but the deployed contracts there looks weird. For example: FlowToken deployed on emulator still uses PrivatePath, which should be deprecated / disallowed in Cadence 1.0.
This privatePath and provider type are gone in the next version that will be deployed. We agreed on this in the last meeting.
@bjartek @joshuahannan The emulator version is not enough, a finalized merged version should be released early on github for dev to fetch from.
Totally agreed
in the next version
Waiting for the next stable-cadence emulator release!
implement FungibleTokenMetadataViews.FTVaultData and have your FT implement ViewResolver and then query for the function that way.
Hey @bjartek, don't quite get it, without a generic most-base interface that all tokens inheriting from, how can we dynamically create an empty vault of arbitrary token just with its address?
The token contract may be implemented in an arbitrary way and completely omitting the metadata part, imo the only place that can be enforced is the most base FT interface.
@dryruner like bjarte said, you should be able to dynamically borrow a contract with ViewResolver because most token contracts will implement ViewResolver, and you can use that to get the FungibleTokenMetadataViews.FTVaultData view which contains the function to create an empty vault of its type. So the patter you describe is still possible.
We want to make it a contract instead of an interface because we want to give people the option to either define tokens in contracts that aren't necessarily token smart contracts or to be able to define multiple fungible tokens in a single contract. We also want to be able to include the global burn method that will become the standard for burning fungible tokens.
This is all still up for discussion though, and we are discussing it at the SC engineering open house today in about 90 minutes if you'd like to join! The information is on the public builders calendar.
This may be the last time we discuss changes though because we are running out of time and have to finalize the standards very soon. Hope to see you there!
most token contracts will implement
ViewResolver
@joshuahannan imo there's no way to enforce it, the major existing tokens including celer-bridged ones and USDC do not have this. So it break existing things and we cannot make aggregation even pair creation work with current proposal.
There is also no way to enforce that token contracts implement FungibleToken, so the same problem still exists even if it is an interface. And this change won't be live until we actually do the cadence 1.0 upgrade, and we're working with community members and partners to ensure that all the major projects update their contracts properly, so your second point shouldn't be much of an issue after the upgrade.
But like I said, this isn't a dealbreaker for us and hasn't been fully committed to yet. it is all still up for debate, so please join the standards discussion today if you'd like to be a part of the discussion. We'll be discussing this exact topic.
Sure, will join the meeting.
The contract-level burn is fine but since custom-destructor is removed there's still a chance that totalSupply cannot self-reflect the truth onchain, as the Vault can be destroyed directly. (This is a minor issue but we're ok with it)
We want to make it a contract instead of an interface because we want to give people the option to either define tokens in contracts that aren't necessarily token smart contracts or to be able to define multiple fungible tokens in a single contract.
Don't quite understand how the interface blocking above, but why would such scenarios be encouraged? Splitting the definition of multiple tokens into multiple files make things more clearer and less error-prone.
Theoretically you're right that even with an interface it cannot be enforced, not sure if it's for everyone but to me some ground-level standards like FT, NFT, etc. are better to be made into the most-base interfaces and ppl will naturally derive from them by convension. - See that IERC20 is also made into interfaces
To follow up for anyone watching, we decided to change both standards back to interfaces. Summary of the discussion can be found in the FLIP
We finalized the design of the standards at our last working group meeting and this PR has been updated with the intended final implementations of FungibleToken, FungibleTokenMetadataViews, and Burner.
Anyone who is interested is welcome to take a look and leave any feedback or questions they want, but any fundamental changes to the standards will likely not be actioned unless the need is very strong because developers need to have a stable set of standards to work on their Cadence 1.0 upgrades with.
The other contracts are all either examples or utility contracts, so those designs aren't finalized yet. The tests and transactions also have not fully been updated yet, but will be in the coming weeks.
Thank you to everyone who contributed to the designs! We couldn't have done it without you. 😃
I just marked this PR as ready for review! There are still some tests that need to be added, but I consider most of the code to be ready! I'm welcome to hear any feedback about any of the choices in the example contracts and transactions so we can make sure we can set a good example for Cadence 1.0!
@joshuahannan Awesome! Maybe the PR title can be updated now? Is flow-go already updated to this?
@turbolent yeah, these latest versions are what was included in https://github.com/onflow/flow-go/pull/5340. so flow-go should be ready to go for another emulator and CLI release.
Looking for reviews on the standard code changes here if anyone has time! I'd like to get this merged soon! 😃
Thanks for the comments @sisyphusSmiling! I've basically made all the changes you suggested. I guess I need to double check all the tests to make sure we are testing all the transactions we have here so we know they are all upgraded and working. Some slipped through the cracks I guess. As for the Switchboard, yes I'll work on adding that entitlement and testing it.
@sisyphusSmiling I finished making the switchboard changes and adding tests in my latest commit. I also addressed all your comments too
@bjartek proposed that we add a balanceAfter field to the FungibleToken.Deposited event to show the balance of the Vault after the deposit happens. I think this is a no-brainer, so unless anyone has any objections, I am going to include it in the standard so that it is in the next emulator/CLI release for Cadence 1.0
Unfortunately, we can't do this for the Withdrawn event because it is emitted from the Provider interface, so it doesn't have access to the balance field
Does anybody remember why we emit Withdrawn events from the Provider and Deposited from the Vault? Because it would be very very nice to have this for withdraw events too.
After thinking about it a little bit, I am pretty sure that the reason that the Withdrawn emission is in the Provider is because we were just copying what we did for NFTs. Since NFTs can be stored anywhere, it made sense to put the event in the provider because they might not necessarily be stored in a collection. It is different with FTs though since they will always end up in a Vault, so I think it makes sense to move the Withdrawn event emission to Vault and include a balanceAfter field.
I opened a PR for this in the FT repo: https://github.com/onflow/flow-ft/pull/158
Looking for reviews! :)
