flow-ft
flow-ft copied to clipboard
Create FungibleTokenMetadataViews contract
Closes: #82
Description
Creates the contract for the Fungible Token's Metadata Views.
For contributor use:
- [x] Targeted PR against
master
branch - [x] Linked to Github issue with discussion and accepted design OR link to spec that describes this work.
- [x] Code follows the standards mentioned here.
- [x] Updated relevant documentation
- [x] Re-reviewed
Files changed
in the Github PR explorer - [x] Added appropriate labels
- [ ] Update the version in package.json when there is a change in the smart contracts
There are at least two thing I'd love to have some input, the contract name and how to deal with the MetadataViews.cdc
file
I love where this is heading!
From Blocto's perspective, it would be useful to include 2 different kinds of logos in FTDisplay
:
- One as png/jpg image, which is the colored version of the logo
- One as vector like svg. Since FT logos can appear in multiple places and may need to be changed to different colors depending on the context it's used
Hi @boczeratul! It's such an honour that you like this proposal.
What do you think if we turn pub let logo: MetadataViews.Media
into pub let logos: MetadataViews.Medias
so we can store any amount of different logo formats?
Just for context
pub struct Medias {
/// An arbitrary-sized list for any number of Media items
pub let items: [Media]
init(_ items: [Media]) {
self.items = items
}
}
Awesome 👍
Thanks a lot for your comments @psiemens! I love descriptive names on variables so I love the "maybeView" names, it makes the code way more readable!!
@satyamakgec package.json
now stands...
"name": "@onflow/flow-ft",
"version": "1.0.0",
According to the PR Template I should move the version to 2.0.0
since we are adding the new contract, am I right?
Quick thought, we might want to support the TokenForwarder pattern that both DapperUtilityCoin
and FlowUtilityToken
use. Right now if I was resolving metadata views on these contracts I would not be able to figure out the storage path for the token forwarder is users accounts and I wont be able to use the data to write any transactions for Dapper Wallet. We would still want the core vault path to grab the main dapper vaults for Flow and DUC.
A couple potential solutions...
- Add it as an optional field
- Add a new Metadata View for forwarder tokens
@bshahid331 can you elaborate on what you would want with a code sample? IMO these use cases are not that common and will be phased out eventually, so I don't see much reason to build them into the standard. It might make more sense for those forwarder contracts to upgrade their contracts and relink their paths so that their forwarders expose the metadata though, if that is what you want
@joshuahannan I agree, not a common use case. It can be a custom MetadataView like the TopShot ones that dapper wallet fungible tokens can support. If you look at dapper wallet transactions there is the main dapper vault path that stores all the DUC for example. Then each account has a receiver path that is essentially what you deposit DUC too. Custom use case but still DUC and FUT are the main FT's being used outside of Flow that's why I wanted to bring it up.
I have one main question. Currently in the FungibleToken contract, we have the Vault resource implementing the MetadataViews.Resolver
now. This would be a breaking change to all fungible token contracts. Are we ok with that? I feel like we won't want to do any breaking changes until we do the main upgrade to the token standards as part of stable cadence.
Therefore, if we don't want to introduce a breaking change, then the only other way to include this before then would be to put the metadata views methods into one of the existing interfaces, either Receiver
or Balance
. Balance
makes the most sense to me since it is already kind of a metadata field, but it is unfortunate that the name balance is not accurate to what it fully represents.
Does anyone have any thoughts? I'll probably just add it to Balance
if nobody says anything.
Last (I hope) push before merging!
Some notes:
- The CI was tied to the secure cadence version, I believe we should be running the CI agains the latest version of the CLI so I removed any specific version flag.
- I created one extra test I just realise I didn't have and split the test suites so they are easier to read, @sisyphusSmiling or @satyamakgec when you have a couple of minutes check the
metadata.test.js
file and let me know it the last new test looks good. - Anyone can think of a reason why the provider type linked in the
FTVaultData
should include{MetadataViews.Resolver}
along with{FungibleToken.Provider}
? I removed that requirement and only required the Resolver interface in themetadataLinkedType
field. I believe @bjartek had some thoughts about this. - Finally this will be packing default implementations of the
getViews(): [Type]
andresolveView(_ view: Type): AnyStruct?
functions inside the{FungibleToken.Balance}
if nobody is against it.
Thanks to everyone, cheers!!
I took the opportunity and fixed a small bug on the switchboard and updated the docs to get everything merged at the same time. If nobody is agains it I'll merge it by Friday and hopefully we can have it deployed to testnet along with the Switchboard next week.