Update MetadataViews Collection views to include more flexible public & private types

[sisyphusSmiling]

Issue To Be Solved

Currently, MetadataViews.NFTCollectionData has pre-conditions which prescriptively define that public and private types an NonFungibleToken implementing Collection can define. While these conditions are logical, these are not necessarily part of the NonFungibleToken standard and reduce the flexibility of the standard set of interfaces and how they can be combined.

    pre {
        publicLinkedType.isSubtype(of: Type<&{NonFungibleToken.CollectionPublic, NonFungibleToken.Receiver, MetadataViews.ResolverCollection}>()): 
            "Public type must include NonFungibleToken.CollectionPublic, NonFungibleToken.Receiver, and MetadataViews.ResolverCollection interfaces."
        providerLinkedType.isSubtype(of: Type<&{NonFungibleToken.Provider, NonFungibleToken.CollectionPublic, MetadataViews.ResolverCollection}>()):
            "Provider type must include NonFungibleToken.Provider, NonFungibleToken.CollectionPublic, and MetadataViews.ResolverCollection interface."
    }

A counter-example to Collections of these sets of restricted composite public & private types is an NFT Collection that only allows private deposits. Another might be one that allows public withdrawals. In both cases, the set of public and private types would differ from those defined in the pre-condition above - are we to say that they're not NFT compliant if the contract successfully implements the NFT standard?

Suggest A Solution

Remove the pre-condition from MetadataViews.NFTCollectionData.init(). Alternatively, define a minimum viable set of interfaces that are not as prescriptive as those currently defined.

Context

This came up as a result of implementing the NonFungibleToken, MetadataViews, and Resolver standards in a new FLIP#72 iteration.

[highskore]

I support this. I'm not a fan of preconditions that limit creativity and possible use cases.

However, I suggested a similar case for the FT standard and got denied. 🥲

[joshuahannan]

@lukaracki Can you link to your proposal for the FT standard? I don't remember it. maybe we need to reconsider. I'm fine with this change

[highskore]

@joshuahannan onflow/flow-ft#77 (comment)

[joshuahannan]

@lukaracki Thats a complete different issue. Here we're talking about removing type restrictions from a metadata view. You're were referring to removing an important security precondition from the Vault resource deposit method

[highskore]

@joshuahannan well it's a condition that limits implementation creativity, that's what I meant 😁

[bluesign]

if we have valid non hypothetical case, we can discuss I guess. But this as is prevents a lot of wrong linking on contract side. @bjartek and @austinkline had experience on this.

[joshuahannan]

@sisyphusSmiling Want to share your example?

[austinkline]

So Flowty actually uses neither of these, but I know that other platforms definitely do. My main callout here is about who is using this metadata view? You don't have to be compliant with all metadata views to follow the NFT standard, and metadata views like this are generally not for the collection itself but for general-purpose products that need to use it. To touch on each of these hypotheticals:

A counter-example to Collections of these sets of restricted composite public & private types is an NFT Collection that only allows private deposits.

If you only want to allow private deposits, you should not return this metadata view at all, because you can't know if another platform will use that publicLinkedType (flowty won't) or not. We can't use that type definition as T in a call to link something on an account, so we set things up that the NFT standard demands and nothing else, we'd break them and I think it's reasonable to assume others would as well.

let cd = nft.resolveView(Type<MetadataViews.NFTCollectionData>()) ...

// can't do this
acct.link<cd.publicLinkedType>(...)

// so we do this
acct.link<&{NonFungibleToken.CollectionPublic, NonFungibleToken.Receiver, MetadataViews.Resolver}>(...)

Another might be one that allows public withdrawals.

Similarly, if you want to have a public provider, why not specify it in another path so that it doesn't get overridden? If it's core to your collection's intended functionality, then putting them in places that other platforms churn often is high-risk because you don't know how much effort others can reasonably put into following what you want.

are we to say that they're not NFT compliant if the contract successfully implements the NFT standard?

To reframe this question, would dapper let someone into the nft catalog that didn't permit public deposits? It would mean consumers of the catalog cannot trust what's there, or that collections can't trust that being on it won't break things. publicLinkedType and providerLinkedType are not actual requirements to be an NFT, and we shouldn't give the false sense of security that might come from removing this restriction. Having it here makes it very clear to those returning this metadata view what it is for (a place to send the nfts, and a place to take them from). If we take that away, there will be unintended consequences that collections might not foresee which could end up costing them greatly.

[bjartek]

Find market used CollectionPublic and MetadataViews.ResolverCollection

[bluesign]

Another might be one that allows public withdrawals.

I think this is covered in the current pre condition. ( with subtype ) NonFungibleToken.Receiver seems to be the only friction here I guess. Btw this is a hint if someone creates the collection ( such as wallet ) it is not a restriction for dapp to create their collection. ( as @austinkline said dapp can even ignore this view, if they don't want someone else to create )

[sisyphusSmiling]

This is all really useful feedback and context!

If it's core to your collection's intended functionality, then putting them in places that other platforms churn often is high-risk because you don't know how much effort others can reasonably put into following what you want

Yeah, this is a good point.

if we have valid non hypothetical case, we can discuss I guess.

The concrete context is the implementation of NFT & MetadataViews standards in the FLIP#72 implementation, with the idea being that implementing MDV standards would facilitate easier implementation of the new linked account standard.

In the proposed standard, deposit() is restricted to prevent phishing attempts and require explicit user acceptance of a linked account for it to be added to a Collection.

That said, it looks like the solution in that specific concrete case is to simply not resolve the NFTCollectionData view given that ecosystem-wide expectations around accessibility of deposit() is that it's public.

With regard to this issue in the context of NFTCollectionData, it looks like the general consensus so far is that is that the pre-conditions should remain. I can see why these expectations are useful, especially given the static type declarations needed to link Capabilities in accounts as @austinkline mentioned.

Of course, if others have opinions on how this is handled in the linked account standard, please feel free to provide feedback on the FLIP or PR,

[austinkline]

@sisyphusSmiling thanks for the context! I'll check out the PR

[austinkline]

The concrete context is the implementation of NFT & MetadataViews standards in the FLIP#72 implementation, with the idea being that implementing MDV standards would facilitate easier implementation of the new linked account standard.

In the proposed standard, deposit() is restricted to prevent phishing attempts and require explicit user acceptance of a linked account for it to be added to a Collection.

I think what you could do is add some kind of pre-approval step that cannot be set via any public methods. The receiver itself doesn't have to be a pass-through method, it can do whatever checks it needs to like looking for a duplicate AccountInfo object or vetting that the account being shared is in some approved list that only the parent account can set (and must set) before the deposit can work. As @bluesign mentioned, you can't trust anyone setting this collection up to make Capabilities private, anyone can change that. The contract itself must handle those core requirements.

That said, it looks like the solution in that specific concrete case is to simply not resolve the NFTCollectionData view given that ecosystem-wide expectations around accessibility of deposit() is that it's public.

With regard to this issue in the context of NFTCollectionData, it looks like the general consensus so far is that is that the pre-conditions should remain. I can see why these expectations are useful, especially given the static type declarations needed to link Capabilities in accounts as @austinkline mentioned.

I think this is accurate. What you are proposing might adhere to an NFT collection's requirements, but it is only really useful when interacting directly with concrete types apart from some metadata resolution which I can still do by using getAuthAccount in a script. Changing all other standards for that when we really aren't likely to borrow this collection as a generic NFT.Collection resource feels like a trade-off that isn't worth it.

[bluesign]

In the proposed standard, deposit() is restricted to prevent phishing attempts and require explicit user acceptance of a linked account for it to be added to a Collection.

This is a really valid use case; but on the other hand, I am not sure how I feel about those stuff ( child accounts ) presented as NFT. ( In one way it is great idea, in another little confusing )

Technically it allows ( considering deposit is public ) to link my account to my address ( without using FCL login ) Technically dapp can never touch FCL.

[sisyphusSmiling]

Changing all other standards for that when we really aren't likely to borrow this collection as a generic NFT.Collection resource feels like a trade-off that isn't worth it.

Originally, I thought it would be nice to increase flexibility of interface usage but agree with your point after our discussion here @austinkline.

( In one way it is great idea, in another little confusing )

My hope after @dete suggested it was that it would be easier for wallets & dapps to build on or at least resolve metadata about a user's linked accounts. If we can get the public/private linked types to match existing expectations around NFT collections, do you think that would resolve concerns about potential confusion @bluesign?

Technically it allows ( considering deposit is public ) to link my account to my address ( without using FCL login ) Technically dapp can never touch FCL.

Yeah the idea is to require a "handshake" of sorts between delegator & delegatee. So the linking account must sign to provide a linked account and the receiving account must add the account link to their collection themselves or, as @austinkline suggested in this PR, authorize the linking account to be added via public Capability.

[joshuahannan]

So are we not gonna do this any more @sisyphusSmiling ? Should we close this issue?

[sisyphusSmiling]

Consensus looks to be against the change, so I'll close the issue