FEATURE: V2 NonFungibleToken Standard

[joshuahannan]

Adds the v2 fungible token standard, as described in this flip: https://github.com/onflow/flips/pull/56

A few highlights:

• Turns NonFungibleToken into a contract instead of a contract interface
• Introduces a new contract interface (NonFungibleTokenInterface) with event specifications, functions to get the NFT and collection types that the contract implements, and functions to get important metadata views for the types in the contract. This will help external parties learn about which types a contract implements since contracts can implement multiple collection and NFT types now.
• Adds a Transferable interface to include a transfer function
• Adds getAcceptedTypes() to the Receiver interface so a receiver interface can indicate which NFT types it accepts.
• Moves standard paths inside the collection object definition because they are specific to the actual collection instead of the contract now.
• Adds type parameters to the all the events since contracts can have multiple nft types defined
• Adds a type parameter to the createEmptyCollection method

The contract currently has a couple errors that should be able to be resolved easily, such as it not working properly with the createEmptyCollectionFunction field of the NFTCollectionData metadata view, and the ownedNFTs field not conforming to the resource interface's field, but there are no other errors. I am looking for feedback on the initial proposal. Tests will come later There are probably more things that I am missing, but most of the details are in the forum post, so that is another good resource.

[bluesign]

Very good and much better than the current standard. I left few minor comments.

[bymi15]

Hi, I was just wondering whether this could also be considered / added: https://github.com/onflow/flow-nft/issues/123 Perhaps an optional medias field would be nice to support a wider variety of NFTs such as videos, music, or multiple images, etc. The current structure is limited in supporting just one thumbnail image, but some platforms/marketplaces may wish to display videos / music players or multiple images in a carousel instead.

[joshuahannan]

@bymi15 That is a separate workstream that is unrelated to this standard update. I'll respond to your issue. Thank you!

[joshuahannan]

What do we think about adding a batch deposit, batch transfer, and batch withdraw? Is that worth adding?

[bjartek]

I saw in mail that austin commented on the transfer discussion that has been marked as resolved. Just a heads up that there might be new comments in resolved discussions.

[joshuahannan]

@bjartek @bluesign @austinkline and anyone else who can give feedback. Quick question about how this should be structured. Right now, I have it so that all the resource interfaces are in a contract (FungibleToken) and I have a separate contract interface (NonFungibleToken-v2) that requires the standard events and some utility functions to return the types defined by the contract and some standard views. @alilloig made the suggestion that those could be just moved into FungibleToken and make it a contract interface like it currently is, then any utility functions with implementations like verifyCollectionTypes() like we have in there right now can just be moved into a NonFungibleTokenUtilities contract along with other useful things like scoped providers and such. Do you think we should go with Alvaro's suggestion, or stick with what we have now and put those utility functions/scoped providers directly in the token standard contract?

[alilloig]

I don't really have any important comments, I love the proposal!! Can't wait to see what people can create using multiple NFTs and FTs on a single contract 🎉

[austinkline]

@bjartek @bluesign @austinkline and anyone else who can give feedback. Quick question about how this should be structured. Right now, I have it so that all the resource interfaces are in a contract (FungibleToken) and I have a separate contract interface (NonFungibleToken-v2) that requires the standard events and some utility functions to return the types defined by the contract and some standard views. @alilloig made the suggestion that those could be just moved into FungibleToken and make it a contract interface like it currently is, then any utility functions with implementations like verifyCollectionTypes() like we have in there right now can just be moved into a NonFungibleTokenUtilities contract along with other useful things like scoped providers and such. Do you think we should go with Alvaro's suggestion, or stick with what we have now and put those utility functions/scoped providers directly in the token standard contract?

Just to make sure I understand the question fully, is this mostly around separating the interface from any implementation? Or is there some other reason to split things up? Is there a compute cost to putting them elsewhere to consider as well?

[joshuahannan]

We originally were going to completely remove type specifications in interfaces from the language, which would mean there would be no contract interface at all here and everything would have been contained in the NonFungibleToken contract, but we realized that it is nice to be able to specify the events and such, but that was after we had already put everything in a contract, so now I want to put everything back in the interface.

We still want to have general utility functions and types for token such as verifyCollectionTypes() and scoped providers though, and those can't be in the standard interface because we can't write implementations of those functions in there, so we'd have to have them in a separate utility contract.

[bjartek]

Since we do not have a universal collection anymore I honestly do not see the value of beeing able to have multiple NFTs in the same contract. It introduces changes that provide very little value IMHO.

If we could disconnect collections from NFTs it would be a totally different thing for me. But I do not see a concrete example of that beeing possible here. Can i create an NFT in a file that does not have a collection and just add that to another collection?

Also having collections with multiple different nft types is not trivial IMHO. So for me the benefit does not justify the cost here.

[hier01]

pub fun getAcceptedTypes(): {Type: Bool} { return { Type<@ExampleNFT.NFT>(): true } } Is there a reason not to hold the accepted types in an array in the Collection, instead of coded into this function? That way an append mechanism could allow new types to be added. Or deleted, but that would depend on whether the collection actually held any of the type being deleted.

[joshuahannan]

@hier01 We're trying to make it so we can upgrade the existing standard to this one, and adding fields to existing types is not supported in upgrades, so we can't require that. If a project is still deploying a completely new NFT or collection, they are welcome to include that field in their collection though, even though it isn't required by the standard

[Peixer]

@joshuahannan I was talking with @bjartek about a FLIP idea I'm working on, I think it will be useful to share it here with all of you. @hichana @gpfurlaneto and me were discussing about how solve the path conflicts, the first idea we had was implementing a path registry like this where all new smart contract would register the path into the init function, example image below. Contract type would be concat with the path name creating an unique path for each new smart contract.

It will create a centralized resource to handle all paths, using it to register new paths and consume paths. It can be used to improve the developer experience by a vscode plugin showing if the path is available even before deployment.

Please let me know your thoughts about it :)

[joshuahannan]

@Peixer I think that standardization for paths is a great idea. I like the idea of your registry and correct me if I am wrong, but it looks like yours would only be for NFT contracts since it has the collection path paradigm built in? I think it would be better to make it more generalized. Can that be part of your FLIP?

[Peixer]

@joshuahannan yeah, initially we thought about having this feature for collections. Not sure if I understood your point, make it more generalized would be a static function where anyone can create new paths using the standard?

In the first draft we added a dictionary using the NFT contract type as a key, we would need to think about how to cover this part to be more generalized. Or maybe a contract registry could be made where the focus is on centralized contract information. How do you feel about this last part?

[bluesign]

Why registry I didn't understand that that part?

Now we say path P is: P = Concat(contractType, path name); contractType we have access from the object, path name object can expose itself (via metadata, or simple standard interface)

As we add contractType already, I don't think we need registry. A central registry would be a single point of failure, meaning that if it is compromised, the entire system could be at risk.

[joshuahannan]

I talked with @Peixer and he is going to make a FLIP for his paths proposal so we can concentrate discussion for that there and keep this focused on the standards

[joshuahannan]

Based on our discussions in calls and in https://github.com/onflow/flips/pull/56 recently, I've pushed some big changes to the contracts:

I added more withdraw options to the standard collection such as withdrawWithUuid(), withdrawWithType(), and more so that both the old id and uuid systems can still be used. I don't think we'll be able to completely migrate to only using uuid, so supporting the old system is probably still important. I thought about having two different collection interfaces, one for the old and one for the new implementations, but that seemed too complex and confusing for users to try to migrate. We have uuid and id emitted in all the standard events so that should hopefully help indexers migrate to uuid. I also created a method on collections for projects to specify if they are using UUID or not to avoid misunderstandings after the upgrade

Still waiting on the FLIP for events emitted from interfaces, so the events are still as they always have been, but will hopefully be in interfaces once we get the design figured out there.

I'm only include name and thumbnail as metadata event arguments. I'm also using type identifier as the event argument so the identifier off-chain is somewhat human readable.

For now, I also split the NFT metadata views out from the core metadata views contract so I could deploy everything and make sure it all works properly and typechecks. Ideally, we could actually have the metadata views split out like this, but idk if that will be possible.

Please take a look and leave feedback if you get the time! Thank you! :)

[bluesign]

For now, I also split the NFT metadata views out from the core metadata views contract so I could deploy everything and make sure it all works properly and typechecks. Ideally, we could actually have the metadata views split out like this, but idk if that will be possible.

I think extracting resolver interfaces are cleaner. With interfaces conforming to interfaces ( if we will have ) it will be backwards compatible (I guess)

something like:

pub contract MetadataResolvers {

    pub resource interface Resolver {
        pub fun getViews(): [Type]
        pub fun resolveView(_ view: Type): AnyStruct?
    }

    pub resource interface ResolverCollection {
        pub fun borrowViewResolver(id: UInt64): &{Resolver}?
        pub fun getIDs(): [UInt64]
    }
}

pub contract MetadataViews {

    pub resource interface Resolver : MetadataResolvers.Resolver {
        pub fun getViews(): [Type]
        pub fun resolveView(_ view: Type): AnyStruct?
    }

    pub resource interface ResolverCollection: MetadataResolvers.ResolverCollection {
        pub fun borrowViewResolver(id: UInt64): &{Resolver}?
        pub fun getIDs(): [UInt64]
    }
}

and NonfungibleToken will import MetadataResolvers

[joshuahannan]

Thanks for the feedback @bluesign! I did move the standard views back into MetadataViews and moved the Resolver and ResolverCollection interfaces into the ViewResolver contract.

I also just pushed some more changes to use events in interfaces and I changed the standard to just be a contract and not an interface. I'm curious to hear if anyone has any feedback! I have a few things I want to discuss at the next breakout, so I'll start planning an agenda and scheduling the next breakout

[bjartek]

Have we spoken about an event when something is Burned? At the moment is basically impossible to distinguish a Withdraw that escrow the resource elsewhere and a burn if you do not know what other event to look for.

[joshuahannan]

@bjartek yep, it is in both proposals already: https://github.com/onflow/flow-nft/pull/126/files#diff-0aa93a6098e86167b39c0a5202f7cbdc953e8bd7ee949b594c2d2695b67372a8R120-R125

[joshuahannan]

Just want to leave a note here that it might not be a good idea to include this default implementation because existing projects who upgrade might forget to include it and if they do and are using a different ID system, they would start returning uuid instead which could cause some problems.

It is nice to have for new projects though because they wouldn't even have to implement it, reducing the number of lines of code they have to write, but that might not be worth the risk.

Anyone else have any thoughts? I'm not fully decided yet.

[austinkline]

Just want to leave a note here that it might not be a good idea to include this default implementation because existing projects who upgrade might forget to include it and if they do and are using a different ID system, they would start returning uuid instead which could cause some problems.

It is nice to have for new projects though because they wouldn't even have to implement it, reducing the number of lines of code they have to write, but that might not be worth the risk.

Anyone else have any thoughts? I'm not fully decided yet.

We should not have this default, existing collections upgrading to this new standard won't see that this is in-place because the default will lead to the contract being able to be deployed. In that case, the NFTs ID could be mismatched against this method which could cause a ton of confusion, especially if events use this method for the id field

[bjartek]

There are two things i would like to discuss to be added to the standard.

1. an event that is emitted like the others when an NFT is updated without a movement event. For doodles we use DoodleUpdated. This is very useful for indexer that wants to make a cache of nfts offchain.

2. A way to get attached/sub-nfts in a standard way for any nft. Personally i think it is overkill to say that an NFT can have an entire new NFTCollection but that is an option. Another options is some simple methods that are defaulty implemented to empty that says what sub-nfts you have (type and ids) and another method to get an sub-nft using type and id.

I know there is uuid but it is currently not emited in any old events and there are conflicts in them. Personally I think just these two methods would be enough.

[crash13override]

I totally agree with @bjartek about the need for those events, so that the real state of an NFT can be tracked properly. A Flovatar wearing an epic accessory for example is much more worth than without it, and it's important that people have a way to keep track of any change on the NFT.

[joshuahannan]

Good suggestions! For the first one, I think that would be nice, but since we don't have nested type requirements any more, we can't enforce that certain events are included and still can't enforce that they are emitted properly. How would you propose we implement that and enforce that people use it properly?

For the second suggestion, that sounds like a metadata view, which we already have an open issue for. I'm not sure that it should be part of the standard.

For both, if you'd like to include them, please make a PR or a comment with your suggestion! Y'all know much better about what you need than I do, so it would save a lot of back and forth if y'all made the proposal first and we can iterate on that. We also are going to discuss the new standards again at the smart contract open house on tuesday next week if we want to discuss it there too, but it would really help if we had your concrete proposal to discuss instead of just an idea.

[bjartek]

Good suggestions! For the first one, I think that would be nice, but since we don't have nested type requirements any more, we can't enforce that certain events are included and still can't enforce that they are emitted properly. How would you propose we implement that and enforce that people use it properly?

I dont think you can enforce it, but if there is an method in the interface like with destroy then atleast you can offer them the chance to emit an update event that is on the same form as the other events.

[bjartek]

For the second suggestion, that sounds like a metadata view, which we already have https://github.com/onflow/flow-nft/issues/146 for. I'm not sure that it should be part of the standard.

Part of this can not be a view, what can be a view is exposing that you have sub-items and what ids you have. However for the borrowing of an sub-nft it would be most flexible if there was a method defaultly implmented as empty that allowed you to borrowSubResource(type:Type, id:UInt64) or something like that. I will make PR.

Or i guess with stable cadence if we have an interface for that we could just say that if you expose that view your NFT should also implement the interface that has the borrowSubResource(type:Type, id:UInt64) method and we can cast it as that. That might work, I will play with it.

[bjartek]

I added a PR targeted to this branch with the update event.