FEATURE: V2 NonFungibleToken Standard

[joshuahannan]

Adds the v2 fungible token standard, as described in this flip: onflow/flips#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: #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?

[austinkline]

Didn't realize this was out! Left some comments, and I'd also like to discuss how we can make use of scoped providers to encourage protecting capabilities that can withdraw. Or maybe we can bake it into both V2 standards in some way? Here's my first attempt at scoped providers as a starting point, I'd love for us to try and incorporate it as closely as we can into the standard:

https://github.com/green-goo-dao/flow-utils/blob/main/cadence/contracts/ScopedNFTProviders.cdc

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

[psiemens]

Great job drafting this up @joshuahannan!

I left some more general comments on the forum post, but LMK if it's better to discuss on this PR going forward: https://forum.onflow.org/t/streamlined-token-standards-proposal/3075/45?u=pete

[joshuahannan]

@nvdtf I addressed all your review comments and made almost all of the changes you suggested. Let me know if you have any other comments! Thank you so much for the review! :)

[cody-evaluate]

With collection.ownedNFTs no longer being standard (or really even accessible outside of the contract), there needs to be an efficient and standard way to loop through all owned NFT IDs in a collection. Calling the standard collection.getIDs() has increasingly common compute limit issues for large collections.

We currently rely on collection.ownedNFTs.forEachKey heavily in our production scripts to iterate through NFT IDs in a collection in an efficient way.

My suggestion is to add the following signature to the NonFungibleToken.CollectionPublic standard interface:

access(all) fun forEachID(_ f: fun (UInt64): Bool): Void

implementation would look like this for legacy contracts (or really however the developer sees fit)

access(all) resource Collection: NonFungibleToken.Collection {

      access(contract) var ownedNFTs: @{UInt64: {NonFungibleToken.NFT}}

      access(all) fun forEachID(_ f: fun (UInt64): Bool): Void {
      
          self.ownedNFTs.forEachKey(f)
      }

      ...
}

Another note: collection.getLength() should be updated in ExampleNFT.cdc as follows:

the current collection.getLength() implementation calls self.ownedNFTs.keys which will result in compute limits for large collections due to arrays being value types in cadence.

access(all) resource Collection: NonFungibleToken.Collection {

      access(contract) var ownedNFTs: @{UInt64: {NonFungibleToken.NFT}}

      // bad
      access(all) view fun getLength(): Int {

            return self.ownedNFTs.keys.length
      }

      // good
      access(all) view fun getLength(): Int {

            return self.ownedNFTs.length
      }

      ...
}

[joshuahannan]

Thanks for the comment @cody-evaluate! I'll update the getLength implementation in ExampleNFT.

I agree that what you are asking for would be very nice to have and if it wasn't so late in the process, I would put it in with no hesitation so that every project would have to implement it. As I said in Discord, we've promised to the community that we won't introduce any more breaking changes to the standard, which this would be because it would require that people implement it. We could put it in there with a default implementation though, which would not break any contracts that have already been updated. The downside though is that since there is a default implementation, contracts don't have to implement the function, meaning that there is no guarantee that every NFT has a proper implementation of this method. A lot of projects have already updated their contracts, so I can try to ping the ones I'm connected with, like at Dapper, but there is no guarantee that they actually add it. Is that okay?

[cody-evaluate]

@joshuahannan its better than nothing i guess but this should really be required and even though its late, we are still in the staging period so theres really no other time to do something like this. The biggest issue is this removes a necessary piece of functionality that has been standard in production since the beginning with no replacement currently making some operations with large collections literally impossible. I would have assumed all public signatures/fields of the current standard interfaces would be supported in the new standard either directly or with a replacement otherwise you risk breaking things with no way to rectify.

I dont think you could even have a "default implementation" because theres no standard way for how to store NFTs in a collection anymore and ownedNFTs will either only exist in the more derived resource OR not exist at all so the implementation could ONLY exist in the derived resource.

[joshuahannan]

I understand, but we had no idea that anyone was actually using ownedNFTs for anything, so removing it did not seem like any problem to us and we didn't hear any feedback about this in the year+ that we were working on the new standard until now. I'll see what I can do

[cody-evaluate]

this would be an example use case in a script for current mainnet

import NonFungibleToken from 0x1d7e57aa55817448

pub fun main(ownerAddress: Address, limit: Int): [&{NonFungibleToken.INFT}] {

    let response: [&{NonFungibleToken.INFT}] = []

    let account = getAuthAccount(ownerAddress)

    account.forEachStored(fun (path: StoragePath, type: Type): Bool {

        if (!type.isSubtype(of: Type<@NonFungibleToken.Collection>())) {

            return true
        }

        let storageCollection = account.borrow<&NonFungibleToken.Collection>(from: path)!

        storageCollection.ownedNFTs.forEachKey(fun (nftId: UInt64): Bool {

            let nft = storageCollection.borrowNFT(id: nftId)

            response.append(nft)

            return response.length < limit
        })

        return response.length < limit
    })

    return response
}

[cody-evaluate]

here is a working example of getting the NFT ID at index 150000 on a massive collection

you cant even get index 0 in this example because arrays are value types in cadence

import NonFungibleToken from 0x1d7e57aa55817448

pub fun main(): UInt64? {

	let index = 150000

	let account = getAuthAccount(0x1e3c78c6d580273b)

	let storageCollection = account.borrow<&NonFungibleToken.Collection>(from: /storage/LNVCTCollection)!

	// compute limit
	return storageCollection.getIDs()[index]
}

this works for the same collection

import NonFungibleToken from 0x1d7e57aa55817448

pub fun main(): UInt64? {

	let limit = 150000

	let account = getAuthAccount(0x1e3c78c6d580273b)

	var index = 0

	var response: UInt64? = nil

	let storageCollection = account.borrow<&NonFungibleToken.Collection>(from: /storage/LNVCTCollection)!

	storageCollection.ownedNFTs.forEachKey(fun (nftId: UInt64): Bool {

		response = nftId

		index = index + 1

		return index < limit
	})

	return response
}

[joshuahannan]

I think that it is time to merge this to master! If nobody has any issues with me merging this, I'll merge it on Monday once we've tested it with the new CLI version next week. I'm going to create a branch that is based off current master so that we still have a record of the cadence 0.42 versions of the contracts.

[turbolent]

Amazing work everyone! 👏