flow-nft
flow-nft copied to clipboard
Add `containsNFT` method to CollectionPublic after default implementation is in place
Right now if you want to check if an NFT is in a collection and not panic if it fails you have to get all ids and then check if your id is in that array. With the way that storage is currently implemented this is expensive and inefficient.
This proposal suggests adding a containsNFT method that will return a boolean to signal if an NFT with a given id is here or not.
Note that simply adding this change to the chain might not be enough. Because if the computation of said method will increase as the collection grow it is not a sustainable solution. This issue also touches on the subject https://github.com/onflow/cadence/issues/1938
Very good idea, also we can add something like borrowNFTSafe(id: UInt64): &NFT? for convenience.
I think computation there will be not much problem, checking should be O(logN)
Maybe in Cadence we can have something like panicWrapper, though we need tuple for that probably. ( though I think it can be bad idea, footgun )
I almost think that when default implementation is in we should make nft 1.1 that is bc with NFT with just default impl. It is going to make life so much easier.
+1 to this
I think this is a great idea. We'll make a PR to add this
Do we have an ETA for when default implementations will be added to cadence on testnet/mainnet. Default adding of getViews/resolveViews to NFT resource would also be lovely.
I think it is going to be added in the next spork, so we'll get them integrated into the standards soon in preparation
@bjartek do you think there is a way of introducing default implementations for getViews/resolveViews to collection (and to borrowNFTSafe) without breaking the existing implementations of Collection? I believe that adding an additional conformance to MetadataViews.Resolver to the resource will break those.
getViews/resolveViews are not on the collection, they are on the resource are they not? if we specify that the nft interface needs it and default implement it as empty it should work should it not? So add it as methods on INFT and default implement it there?
@bjartek I see, so you're saying that we don't add the MetadataViews.Resolver conformance to NFT or the ResolverCollection conformance to Collection, we just add it to INFT and add a default implementation there. I think that should work
I think so too, but after default implementations are here, we can just try and see that it works.