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

Add `containsNFT` method to CollectionPublic after default implementation is in place

Open bjartek opened this issue 3 years ago • 6 comments
trafficstars

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

bjartek avatar Sep 08 '22 07:09 bjartek

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 )

bluesign avatar Sep 08 '22 09:09 bluesign

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.

bjartek avatar Sep 08 '22 10:09 bjartek

+1 to this

bshahid331 avatar Sep 08 '22 17:09 bshahid331

I think this is a great idea. We'll make a PR to add this

joshuahannan avatar Sep 14 '22 14:09 joshuahannan

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.

bjartek avatar Sep 16 '22 10:09 bjartek

I think it is going to be added in the next spork, so we'll get them integrated into the standards soon in preparation

joshuahannan avatar Sep 16 '22 14:09 joshuahannan

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

alilloig avatar Oct 13 '22 14:10 alilloig

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 avatar Oct 17 '22 07:10 bjartek

@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

joshuahannan avatar Oct 17 '22 18:10 joshuahannan

I think so too, but after default implementations are here, we can just try and see that it works.

bjartek avatar Oct 18 '22 19:10 bjartek