flow-ft icon indicating copy to clipboard operation
flow-ft copied to clipboard

Create FungibleTokenMetadataViews contract

Open alilloig opened this issue 2 years ago • 9 comments

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

alilloig avatar Sep 01 '22 11:09 alilloig

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

alilloig avatar Sep 01 '22 11:09 alilloig

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

boczeratul avatar Sep 08 '22 05:09 boczeratul

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
        }
    }

alilloig avatar Sep 15 '22 17:09 alilloig

Awesome 👍

boczeratul avatar Sep 16 '22 00:09 boczeratul

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!!

alilloig avatar Sep 30 '22 09:09 alilloig

@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?

alilloig avatar Oct 05 '22 13:10 alilloig

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

  1. Add it as an optional field
  2. Add a new Metadata View for forwarder tokens

bshahid331 avatar Oct 13 '22 17:10 bshahid331

@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 avatar Oct 14 '22 18:10 joshuahannan

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

bshahid331 avatar Oct 14 '22 18:10 bshahid331

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.

joshuahannan avatar Oct 24 '22 18:10 joshuahannan

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 the metadataLinkedType field. I believe @bjartek had some thoughts about this.
  • Finally this will be packing default implementations of the getViews(): [Type] and resolveView(_ view: Type): AnyStruct? functions inside the {FungibleToken.Balance} if nobody is against it.

Thanks to everyone, cheers!!

alilloig avatar Nov 17 '22 20:11 alilloig

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.

alilloig avatar Nov 23 '22 20:11 alilloig