Create FungibleTokenMetadataViews contract #89

alilloig · 2022-09-01T11:11:03Z

Closes: #82

Description

Creates the contract for the Fungible Token's Metadata Views.

For contributor use:

Targeted PR against master branch
Linked to Github issue with discussion and accepted design OR link to spec that describes this work.
Code follows the standards mentioned here.
Updated relevant documentation
Re-reviewed Files changed in the Github PR explorer
Added appropriate labels
Update the version in package.json when there is a change in the smart contracts

alilloig · 2022-09-01T11:13:21Z

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

ggpt-labs · 2022-09-01T15:07:02Z

contracts/FungibleTokenMetadataViews.cdc

+
+        /// Function that allows creation of an empty FT vault that is intended
+        /// to store the funds.
+        pub let createEmptyVault: ((): @FungibleToken.Vault)


What is the reasoning for this createEmptyVault function to be inside the FTVaultData struct?

People can get metadata views from an object without importing or knowing what contract the object comes from, so this view allows code that has the object or a reference to it, but not the imported contract, to be able to create a new vault and also know what the storage and public paths are for it without having to find out that information elsewhere.

joshuahannan

Looks good so far! I think the name is good and just copying MetadataViews into the utility contracts directory here is probably fine until we figure out a better way to manage it.

Now we just need to add some tests and we can start sharing this PR around a bit more for reviews

contracts/FungibleTokenMetadataViews.cdc

satyamakgec · 2022-09-01T21:03:41Z

contracts/FungibleTokenMetadataViews.cdc

+            bannerImage: MetadataViews.Media,
+            socials: {String: MetadataViews.ExternalURL}
+        ) {
+            self.name = name


What about the token symbol?

I wanna discuss over the FLIP on the forum about the fields of this FTDisplay, the symbol is a really good catch, and I also would like to discuss which images we really do need for the FTs, I'm not sure at all that we need a square and a banner Image

can you please mention the flip here

boczeratul · 2022-09-08T05:33:18Z

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

alilloig · 2022-09-15T17:21:49Z

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

boczeratul · 2022-09-16T00:48:35Z

Awesome 👍

alilloig · 2022-09-20T18:53:56Z

contracts/FungibleToken.cdc

+        /// @return An array of Types defining the implemented views. This value will be used by
+        ///         developers to know which parameter to pass to the resolveView() method.
+        ///
+        pub fun getViews(): [Type]{


@joshuahannan I've done the default implementations of the Resolver functions on this branch since it seemed to me that it belonged more to the features than to the example, does it look good?

yeah, that looks good. The version of Cadence that you're using to test it supports default implementations and is working, right?

Not at all, I'm on v0.39.3 of the cli, the latest distributed via brew. I thought the version supporting default implementations was not live yet. I haven't merge this changes into the example token branch which is the one that includes the tests.

Working with v0.41.0 🎉

psiemens

The logic looks good! My comments are almost entirely stylistic.

contracts/FungibleToken.cdc

contracts/FungibleTokenMetadataViews.cdc

psiemens · 2022-09-29T22:23:43Z

contracts/utilityContracts/MetadataViews.cdc

@@ -0,0 +1,740 @@
+import FungibleToken from "./../FungibleToken.cdc"


Another nitpick, but I think the flow-nft repo stores utility contracts in contracts/utility. Feels a bit redundant to mention "contracts" again in contracts/utilityContracts

You have a pretty good point there, should we change the folder's name here and on the flow-nft repo @joshuahannan ???

contracts/FungibleTokenMetadataViews.cdc

contracts/FungibleToken.cdc

contracts/FungibleTokenMetadataViews.cdc

alilloig · 2022-09-30T09:38:45Z

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

psiemens

This LGTM!

alilloig · 2022-10-05T13:20:31Z

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

bjartek

LGTM, a small comment about the required restrictions of the balancer cap.

bjartek · 2022-10-10T06:26:32Z

contracts/FungibleTokenMetadataViews.cdc

+        ) {
+            pre {
+                receiverLinkedType.isSubtype(of: Type<&{FungibleToken.Receiver}>()): "Receiver public type must include FungibleToken.Receiver."
+                metadataLinkedType.isSubtype(of: Type<&{FungibleToken.Balance, MetadataViews.Resolver}>()): "Metadata public type must include FungibleToken.Balance and MetadataViews.Resolver interfaces."


should this also ensure that it is a subtype of receiver? personally I never understood why FT capabilities do not link both receiver and balance.

That's a good question. I guess that if we are making sure that a {FungibleToken.Receiver} is provided on the receiverLinkedType we don't need to check for it on the metadataLinkedType.

Also you make me thing if we really want to be checking that the providerLinkedType is also a {MetadataViews.Resolver}

bshahid331 · 2022-10-13T17:49:28Z

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

sisyphusSmiling

LGTM! Just needs Go assets updated for CI to build properly

joshuahannan · 2022-10-14T18:12:14Z

@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

bshahid331 · 2022-10-14T18:28:52Z

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

joshuahannan · 2022-10-24T18:09:16Z

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.

… FTVaultData

… said test

…ct interface

alilloig requested a review from joshuahannan as a code owner September 1, 2022 11:11

alilloig linked an issue Sep 1, 2022 that may be closed by this pull request

Build FT Metadata Views features #82

Closed

alilloig requested review from ggpt-labs, satyamakgec and sisyphusSmiling September 1, 2022 11:11

alilloig added Feature Improvement SC-Eng Issues that we want to see surfaced in SC-Eng ZH Board and removed Improvement labels Sep 1, 2022

ggpt-labs reviewed Sep 1, 2022

View reviewed changes

joshuahannan reviewed Sep 1, 2022

View reviewed changes

satyamakgec reviewed Sep 1, 2022

View reviewed changes

franklywatson removed the SC-Eng Issues that we want to see surfaced in SC-Eng ZH Board label Sep 7, 2022

alilloig commented Sep 20, 2022

View reviewed changes

psiemens approved these changes Sep 29, 2022

View reviewed changes

psiemens approved these changes Oct 3, 2022

View reviewed changes

bjartek reviewed Oct 10, 2022

View reviewed changes

sisyphusSmiling approved these changes Oct 13, 2022

View reviewed changes

This was linked to issues Nov 17, 2022

Normalize flow.json #98

Closed

Refactor utilityContracts folder into utility folder #99

Closed

alilloig and others added 25 commits November 29, 2022 19:48

Finish docs

48167ad

avoid oppressive terminology

cfa52c3

avoid oppressive terminology

3a53138

add metadata views to go tests

c2d30c5

move view methods to the balance interface

41c84af

Remove MetadataViews import from FungibleToken contract

8ca58e6

Normalize flow.json

49fd8e4

Split test suites

ffa52cc

Move timeout to jest config instead of per test suite

9e6e418

Update go test for metadataviews

5549c6e

get go tests working

57791fc

update ci

b78c2ad

Change references to ResolvePublicPath for VaultPublicPath

03177c8

Separate metadata tests from core features

1cb57de

Create folder only for metadata transactions

27bb63b

Rename utilityContracts folder as utility

6536fe9

Remove the need of linking a resolver on the provider for returning a…

5d04b1a

… FTVaultData

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

09930d8

… said test

Fix github ci to get latest Flow CLI version

9f8a857

Fix bug on event from issue #92

5adbf97

Standarize comments on Example Token

c313d16

Standarize comments to docgen tool format on the FungibleToken contra…

7e33650

…ct interface

Include contracts docs

0820c16

Fix broken links

f287fa2

Update package.json version

ee3dd49

alilloig force-pushed the alilloig/ft-metadata-views branch from 247fb07 to ee3dd49 Compare November 29, 2022 19:09

alilloig added 2 commits November 29, 2022 20:50

Fix typos

ff1f5ef

Fix public path after merge conflicts

ffa7f24

alilloig merged commit 339da75 into master Nov 29, 2022

alilloig deleted the alilloig/ft-metadata-views branch November 29, 2022 20:04

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Create FungibleTokenMetadataViews contract #89

Create FungibleTokenMetadataViews contract #89

alilloig commented Sep 1, 2022 •

edited

Loading

alilloig commented Sep 1, 2022

ggpt-labs Sep 1, 2022

joshuahannan Sep 1, 2022

joshuahannan left a comment •

edited

Loading

satyamakgec Sep 1, 2022

alilloig Sep 2, 2022

satyamakgec Sep 5, 2022

boczeratul commented Sep 8, 2022

alilloig commented Sep 15, 2022 •

edited

Loading

boczeratul commented Sep 16, 2022

alilloig Sep 20, 2022

joshuahannan Sep 26, 2022

alilloig Sep 27, 2022

alilloig Oct 6, 2022

psiemens left a comment

psiemens Sep 29, 2022

alilloig Sep 30, 2022

alilloig commented Sep 30, 2022

psiemens left a comment

alilloig commented Oct 5, 2022

bjartek left a comment

bjartek Oct 10, 2022

alilloig Oct 13, 2022

bshahid331 commented Oct 13, 2022

sisyphusSmiling left a comment •

edited

Loading

joshuahannan commented Oct 14, 2022

bshahid331 commented Oct 14, 2022

joshuahannan commented Oct 24, 2022

		@@ -0,0 +1,740 @@
		import FungibleToken from "./../FungibleToken.cdc"

Create FungibleTokenMetadataViews contract #89

Create FungibleTokenMetadataViews contract #89

Conversation

alilloig commented Sep 1, 2022 • edited Loading

Description

alilloig commented Sep 1, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joshuahannan left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

boczeratul commented Sep 8, 2022

alilloig commented Sep 15, 2022 • edited Loading

boczeratul commented Sep 16, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

psiemens left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alilloig commented Sep 30, 2022

psiemens left a comment

Choose a reason for hiding this comment

alilloig commented Oct 5, 2022

bjartek left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bshahid331 commented Oct 13, 2022

sisyphusSmiling left a comment • edited Loading

Choose a reason for hiding this comment

joshuahannan commented Oct 14, 2022

bshahid331 commented Oct 14, 2022

joshuahannan commented Oct 24, 2022

alilloig commented Sep 1, 2022 •

edited

Loading

joshuahannan left a comment •

edited

Loading

alilloig commented Sep 15, 2022 •

edited

Loading

sisyphusSmiling left a comment •

edited

Loading