onflow / flow-nft

The non-fungible token standard on the Flow blockchain
https://onflow.org
The Unlicense
465 stars 169 forks source link

NFTView #74

Closed austinkline closed 2 years ago

austinkline commented 2 years ago

Issue To Be Solved

A metadata view that encompasses all data of an NFT resource is needed, otherwise we have to rely on hand-made scripts like what's used in Alchemy's NFT endpoint which differs from what folks at .find return. These scripts shouldn't be needed, I should instead be able to ask for a standard view which would contain all information about an NFT that other platforms can then support.

Separately, a complete view is necessary for dapps to support features on their platforms which require on-chain metadata. For instance, suppose there is a marketplace that wants to allow people to make global offers on nfts, allowing any nft to fill the offer so long as it has certain metadata key/value pairs.

(Optional): Suggest A Solution

After some discussion with @bjartek, @bluesign, and @aishairzay, there are a few options for this that I see. First is that we could use the DictView as I have brought up in #73. This would be flexible enough to work, but some concerns were raised about its lack of explanation on the data being shown.

Another option suggested by @aishairzay is to adopt the OpenSea API Metadata Standard as the NftView. Here is what that structure looks like:

{
  "description": "Friendly OpenSea Creature that enjoys long swims in the ocean.", 
  "external_url": "https://openseacreatures.io/3", 
  "image": "https://storage.googleapis.com/opensea-prod.appspot.com/puffs/3.png", 
  "name": "Dave Starbelly",
  "attributes": [ ... ], 
}

And attributes are made up of:

{
  "display_type": "number",  # this one is optional
  "trait_type": "Generation", # this is the key name
  "value": 2
}

Using that as the model, our view would look something like this:

pub struct NftView {
    pub let description: String
    pub let externalUrl: String
    pub let image: String
    pub let name: String
    pub let attributes: [NftViewAttribute]
}

pub struct NftViewAttribute {
    pub let displayType: String?
    pub let traitType: String
    pub let value: AnyStruct
}

I think that we can do away with the things already covered in other views here. I'm just leaving them for reference and consistency with the returned format from OpenSea. You could strip away description, externalUrl, image, and probably name as well. In that case, we are just returning attributes in the format OpenSea returns them. This is a model that folks in the NFT community would be familiar with since that's how much of Ethereum's tokenURI results are formatted. Alignment with the broader NFT ecosystem would be a nice plus there.

Lastly, an option from @bluesign was given to support a generic toDictionary function on structs. In which case, a platform could grab all views an NFT supports and translate them to dictionaries. While this approach would not be a view, I am including it here for the sake of completeness of discussions on the issue at hand.

(Optional): Context

Not having a view like this or something like #73 prevent us from making generic metadata-driven features on flowty and on other platforms. We cannot import all view types that we would like to support on a per-collection basis, so something broader would be a nice value-add.

aishairzay commented 2 years ago

Along with the opensea API metadata standard, I believe it'd be helpful to extend the NftView struct with some additional metadata that is Flow-specific like the following:

pub struct NftView {
    // original
    pub let description: String
    pub let externalUrl: String
    pub let image: String
    pub let name: String
    pub let attributes: [NftViewAttribute]

    // new
    pub let collectionPublicPath: PublicPath
    pub let collectionStoragePath: StoragePath
    pub let type: Type
}

Adding public paths, storage paths, and types will allow third parties to know how to deposit, discover, compare, and retrieve the relevant NFTs.

psiemens commented 2 years ago

I want to start by saying I totally see the pain point here. We designed a metadata system that is modular and allows for many views, but in doing so we also pushed complexity onto consumers and require them to know which views are relevant, sometimes even for a specific application.

My only concern here is that a view like this feels like it moves away from that modular design. My hope is that metadata views will evolve alongside new use cases. A single NFT struct, in my opinion, may result in an ecosystem that is locked into an overly rigid representation of what an NFT can or should be.

I wonder if there's a way to remove complexity for the consumer while still preserving that modularity?

Just a sketch, but maybe something like this:

// Display is already defined
pub struct Display {
  pub let name: String
  pub let description: String
  pub let image: File
}

// Defined here: https://github.com/onflow/flow-nft/pull/70
pub struct ExternalURL {
  pub let externalUrl: String
}

pub struct Collection {
  pub let collectionPublicPath: PublicPath
  pub let collectionStoragePath: StoragePath

  // IMO "type" should always come directly from nft.getType(), as that is not spoof-able.
}

pub struct Attributes {
  pub let attributes: [NftViewAttribute]
}

We can then introduce a utility contract that makes it easier for consumers to fetch the base-level NFT views (i.e. the ones that 90% of applications require). This utility could be versioned to support new views in the future.


pub struct BaseNFTViewsV1 {
  pub let display: Display?
  pub let externalURL: ExternalURL?
  pub let collection: Collection?
  pub let attributes: Attributes?
}

pub fun getBaseNFTViewsV1(nft: MetadatViews.Resolver): BaseNFTViews { ... }
austinkline commented 2 years ago

@psiemens thanks for the thoughts and I absolutely see your point of view here. At the end of the day, I'm more concerned with enabling my use case than I am the structure of any view I'm proposing. I think maintaining modularity while also rolling certain views up into one cohesive one is a compromise I can live with.

The only thing I'm not sure about is whether each of the above options should actually be optional, or if some should be required? External URLs and Display info might not always be relevant, but I think that attributes and collection info are basically the underlying key pieces of data that this issue is meant to address. For that reason, in my opinion they should be required.

psiemens commented 2 years ago

I think that attributes and collection info are basically the underlying key pieces of data that this issue is meant to address. For that reason, in my opinion they should be required.

Yeah that makes sense. I made all the fields optional in that sketch because I wasn't sure what the behaviour should be if one of the view wasn't present.

I guess if attributes is required, the BaseNFTViewsV1 could just return an empty attributes list.

But if collection is required and the Collection view isn't defined by the NFT, should the getBaseNFTViewsV1 function fail? That might be the right behaviour, as it would encourage developers to make sure they meet the minimum requirements for this use case.

austinkline commented 2 years ago

Circling back to this (apologies for the delay). @psiemens I like what you've highlighted above a lot, I think my only exception with it is that Collection should not be optional. To dig into why, I think the best way to frame that is what each of these pieces of the View would be for:

  1. Display - How to show the NFT. There are placeholders and fallbacks all the time. For instance, without a display you would probably do CollectionName #ID. For instance, Flunk #8210. I can supplement a missing display with a pretty straightforward option.
  2. ExternalURL - Backlinks. Missing this isn't the end of the world, it just means that a dapp can't give you a backlink to what site "owns" this collection.
  3. Attributes - Metadata on an NFT. My nitpick to this one is that I'd rather just flatten that view out and not have to do view.attributes.attributes and instead just make it pub let attributes: [NftViewAttribute]? on the top-level view. When I think about the worse case here (there are no attributes), that is a perfectly valid use-case. All it means is I don't show any on my dapp. It's the same as attributes being null. An NFT doesn't need to have any to be valid, it's just uncommon that this would be the case. I realize this one might be contentious, it is mostly a nitpick; definitely open to suggestions/dissent on this one!
  4. Collection - Information about how to access this item. After thinking on this a while, this is the most important piece of the entire view. It tells me how to reach the NFT itself whether that's public or private. Without this information, the NFT is actually not usable (without manually researching what those paths might be). On that basis, I'm curious your thoughts on the following:
pub struct BaseNFTViewsV1 {
  pub let display: Display?
  pub let externalURL: ExternalURL?
  pub let collection: Collection
  pub let attributes: [NftViewAttribute]?
}
bluesign commented 2 years ago

In my understanding, collection is not a property of the NFT, but actually it is property of the contract that defines the NFT. [0] I mean NFT doesn't know about where it is stored.

So naming it something like below can be more beneficial:

  pub let defaultCollectionPublicPath: PublicPath
  pub let defaultCollectionStoragePath: StoragePath

[0] https://github.com/onflow/flow-nft/blob/758836c84ece752b3ccb10e96a3d91e5ceeedad3/contracts/ExampleNFT.cdc#L16

psiemens commented 2 years ago

My nitpick to this one is that I'd rather just flatten that view out and not have to do view.attributes.attributes and instead just make it pub let attributes: [NftViewAttribute]? on the top-level view.

@austinkline Yup this makes sense to me!

I see your point about collection. @aishairzay just put up a PR that accomplishes this use case and more: https://github.com/onflow/flow-nft/pull/82/files

Let's get some code up! Do you want to put ~ExternalURL~ and Attributes into a PR? I can do it if you're busy -- just LMK.

Edit: I forgot that ExternalURL was already added here: https://github.com/onflow/flow-nft/pull/70/files

austinkline commented 2 years ago

Thanks @psiemens! Happy to pick this up, I should have some bandwidth in the next two weeks to get it going

psiemens commented 2 years ago

Hey @austinkline, it looks like @aishairzay may need the Attributes view fairly soon. Do you mind if I put up a draft PR for it?

austinkline commented 2 years ago

@psiemens feel free! I'd much rather this get out there than stall who should actually make it

austinkline commented 2 years ago

@psiemens submitted an initial PR for the Attributes view: https://github.com/onflow/flow-nft/pull/87

bjartek commented 2 years ago

I think this is a data structure that can be derrived from the other views a resource expose. Making the developers implement this in adition to all the else is just a hassle. Create some code to transform a Metadata.ViewResolver with some views into this resource is IMHO a way better option.

psiemens commented 2 years ago

@bjartek Are you referring to the Attributes view or the larger NFTView proposed above?

bjartek commented 2 years ago

In this issue I am refering to the NFTView. In the other one where I just commented I am referring to the Attributes view.

austinkline commented 2 years ago

A factory to make the NftView is a pattern that we could definitely do, I'm not as concerned with that. This is all about the uses cases I'm trying to enable and we definitely don't need people making it themselves if that's the pattern we want to establish

austinkline commented 2 years ago

Closed the attempted PR #87. I will circle back to this later.

psiemens commented 2 years ago

Thanks @austinkline, I'm just catching up on this. Will also circle back once I've thought about it a bit more.

austinkline commented 2 years ago

Picking this back up. Talked with @bluesign and @bjartek about this offline for quite a while. Here are some general asks pertaining to the Attributes view and the larger NftView:

  1. Make the ExampleNft a more verbose showcase of this view and actually use metadata like we are talking about here, it would exclude name/description potentially.
  2. Show what implementing this could mean for TopShot, provide factory methods to assist in less code being needed for collections to implement
  3. Can we enforce that this view (or nftview) must be implemented (sounds like this is up for debate)
  4. How to engage with dapper and ensure they would adopt this, Attributes/NftView are only useful if we can be sure that it would be adopted everywhere.

@psiemens any thoughts on the above? 1-3 I can take on myself but 4 not so much 😄

Separately, I think there is some miscommunication about what is being proposed and why it is useful/needed. This ticket is proposing two new views:

  1. Attributes - A way to represent the underlying metadata or an item. This would replace views like TopShot's TopShotMomentMetadataView so that we can standardize what types are being vended from all these different collections set to come out once permission-less deployment is enabled at our June 15th spork.
  2. NftView - A rollup view containing other views all in one. Think of this as a replacement for the Alchemy response format that many people rely on right now.

To reiterate, we have a few use cases here which I would like to see brought together. Let's dig into Attributes. An Attribute really is a wrapper around a single key-value pair and tells you:

  1. What is the value?
  2. What is the name of the value?
  3. How should I show this value? (Optional)

The last piece, information about how to show a value, is the key part here for why it is different than another {String: AnyStruct} dictionary. Some pieces of metadata are more useful when contextualized with an additional disclaimer.

Take, for example, an attribute that represents the time in which the item was minted. Most likely, that attribute is going to be the block timestamp accessible to us in cadence and will be a number. So, instead of showing a number as the dateMinted attribute, I can instead set its display type to Date and platforms know to take that number and attempt to parse it differently. So now we've gone from returning

{
  "dateMinted": 1653409561
}

to

[
  {
    "traitType": "dateMinted",
    "value": 1653409561,
    "displayType": "Date"
  }
]

Next, it's worth discussing the differentiation between the Attributes view and the NftView and what I think many of us are thinking but we haven't really laid out yet. The NftView is really just a set of "core" views bundled together. At the time of this ticket being made, those core views are what is laid out in the discussion history, although never really explicitly called this. Those are:

  1. Display - What information should I show as short-hand for this item?
  2. ExternalURL - How do I link this item back to its creator(s)?
  3. Collection - Where should the collection this NFT belongs to be located?
  4. Attributes - What metadata exists on this nft and how should I show it?

The above might not be the real set of core views, I'd encourage everyone who is part of this discussion to weigh in on that. What's nice, however, is that Attributes could be used to express additional views that are more accurately expressed as Attributes instead of a core view. For instance, let's say we think SerialView or EditionView should be included but we aren't sure they're core to all NFTs, a collection could elect to contextualize the Serial attribute to not just be a number, but instead be the SerialView itself as the value and mark the DisplayType as Serial. In this way, we can keep our core views consistent while also allowing these additional versions which permit more rich and purposeful expression. This could also help address some concerns expressed by @bjartek and @bluesign about vending data in multiple places. Instead of vending a serial number attribute and vending a Serial view, bring them together and return the Serial view as an attribute when either Attributes or NftView is requested.

To really emphasize an important piece here..

  1. Attributes - What metadata exists on this nft and how should show it?

Other approaches we've discussed for this problem aren't able to solve the contextualized piece of how to show something. A toDictionary primitive has been proposed in #73 but allowing an attribute to tell you how it should be shown is very powerful. It means an NFT collection owner can be the one who decides/customizes how their items will look on a marketplace. A dictionary cannot express this, it would only have the key and value, no display type.

As for the feedback about what would help get @bjartek and @bluesign over the finish line, I will make another pass at it and see about implementing the whole thing. One nice helper function we should do is just have a dictionaryToAttributes function for folks to use who have no special attributes to show. Then there's very little boilerplate code unless you need something special.

Thoughts? Concerns? I'd like to be on the same page here before submitting PRs that way feedback on the PR itself is more about the organization of what we're doing, not the core concept itself.

bshahid331 commented 2 years ago

Hey @austinkline, lets sync up on the NFTView specifically. I'm currently working on a catalog of NFTs on chain to be leveraged by the Alchemy API and am working on something similar to this

austinkline commented 2 years ago

Hey @austinkline, lets sync up on the NFTView specifically. I'm currently working on a catalog of NFTs on chain to be leveraged by the Alchemy API and am working on something similar to this

Sure thing @bshahid331, you can find my contact info in my gh profile bio, happy to sync up in whatever way works best for you

bluesign commented 2 years ago

Instead of vending a serial number attribute and vending a Serial view, bring them together and return the Serial view as an attribute when either Attributes or NftView is requested.

My only concern here was Attributes to cover all core views. (I was ok with duplicate ) This solution is more elegant in my opinion, I support fully.

bjartek commented 2 years ago

@bshahid331 @austinkline did you sync on this? I would love to see progress here :)

bshahid331 commented 2 years ago

Syncing this week, was out of town for the weekend =)

psiemens commented 2 years ago

Thanks for following up on this @austinkline!

@bshahid331 is definitely closer to the NFTView discussion than me. I'm curious to hear the result of your conversation.

Attributes view design / purpose

As for the Attributes view, I think it makes sense to include it if these two things are true:

  1. The Attributes view behaves similarly to a SQL view in the sense that it provides another way to view data that may already be defined directly on the NFT or in another view.
  2. The Attributes view isn't a strict requirement for all Flow NFTs in perpetuity. I think there's a strong use case today with collectible NFT sets (where an array of traits/attributes play a huge role in the NFT's value), but I think we should expect a future where NFTs express themselves with more complex data and hopefully make more use of strongly-typed on-chain data structures.

Both points above address my main concern of a lack of on-chain interoperability. If Attributes were to become the ubiquitous way to define all NFT data, then I agree with @bjartek that it may defeat the purpose of the MetadataViews design.

A simple example is a set of Automobile NFTs that defines a vehicleType attribute. Somebody else may then deploy a racing game contract that allows all cars with vehicleType="compact" to enter a special race. It wouldn't be ideal if that Cadence contract had to iterate through the attributes list to find vehicleType -- it's both inefficient and difficult to program. A better solution, IMO, would be to have a domain-specific Vehicle view that defines a vehicleType enum field.

However I then think it's valid for Automobile to also implement Attributes to provide a more generic representation that can be easily consumed by marketplaces, wallets, etc.

Relationship with rarity

I've been following the rarity discussion in #60 and starting to think that it makes sense for each individual Attribute to include an optional rarity field. @bjartek, I saw your implementation in #92, but I prefer to have the rarities defined directly on each attribute.

At first I thought that Attributes and Rarity should be rolled into one, but now I think that the total rarity of an NFT should be separated from the individual rarities of its traits.

But with that in mind, can there be a common Rarity struct that is shared by both? Something like this:

// An NFT can add MetadataViews.Rarity as a top-level view to define the total rarity.
pub struct Rarity {
  pub let score: UFix64?
  pub let description: String?
}

pub struct Attribute {
  pub let displayType: String?
  pub let traitType: String
  pub let value: AnyStruct

  // Rarity can also be used directly on an attribute.
  // 
  // This is optional because not all attributes need to contribute to the NFT's rarity.
  pub let rarity: Rarity?
}

Traits or attributes?

Lastly, would it be more accurate to call this view Traits? It's a subtle name change, but I feel like it more accurately captures the predominant use case of this view.

austinkline commented 2 years ago

@psiemens Makes sense on the above, appreciate the weigh-in!

The Attributes view behaves similarly to a SQL view in the sense that it provides another way to view data that may already be defined directly on the NFT or in another view.

I would say this is true. We're vending a standardized format for others can consume with some context that otherwise wouldn't have existed on an individual value in a dictionary (like Rarity as you mention later)

The Attributes view isn't a strict requirement for all Flow NFTs in perpetuity. I think there's a strong use case today with collectible NFT sets (where an array of traits/attributes play a huge role in the NFT's value), but I think we should expect a future where NFTs express themselves with more complex data and hopefully make more use of strongly-typed on-chain data structures.

This makes sense. I see the use of this view versus a more strongly typed one as different. If I can use a strongly-types view, I think the implication is that I know the actual type that I'm interacting with. In that scenario, using a strongly-typed option makes sense (and should absolutely be done). Whereas for Attributes (or Traits), it's meant to encapsulate this information without needing to know anything about what type the NFT actually is. In that way, I think these two can co-exist.

In the Automobile example, for instance, a strongly-typed view make sense because I'm making a game for a specific type, so I can tailor my application to that type or its views accordingly. The proposed Attributes/Traits view is more useful for things like marketplaces that support all types of NFTs, not just a specific one. In that way, I agree that this new view should not be seen as a replacement for all views that give metadata, it is to fill a more generic need in a uniform way

Rarity existing on the attribute itself makes sense to me but I also have not been part of the discussion so much. I will need to catch up and circle back to that.

But with that in mind, can there be a common Rarity struct that is shared by both?

If rarity can be specific to individual traits/attributes of an asset, this makes sense to include in my opinion. In that same light, are there other key pieces of optional info that we would want to add or is rarity the only one? @bjartek any thoughts on rarity going here? The main knock against it in my opinion would be that it increases the scope of what this view is used for. At the same time, if rarity on an attribute is part of a complete picture of that piece of data, then it fits the purpose of this view and its component pieces in my opinion.

Lastly, would it be more accurate to call this view Traits? It's a subtle name change, but I feel like it more accurately captures the predominant use case of this view.

Fine by me! No strong feelings between Trait or Attribute from me, happy to accept either

spacepluk commented 2 years ago

New flow user here. I would be happy with just a url that points to an Open Sea style json. That's super easy to implement and adopt and it would solve a lot of problems in the short term.

My interest is in making games and having all the metadata stored on chain makes things very complicated. I agree it makes sense for immutable metadata though.

Just my 2cents.

austinkline commented 2 years ago

@spacepluk thanks for chiming in!

A url option is certainly useful for projects that opt into that model of metadata. I would point out, however, that flow is meant to store data on-chain so you'll get lots of pushback on it being the default. One of the primary value-props of flow is this very ability and it's why the metadata standard isn't just a tokenURI like it is in ethereum.

We could consider adding in an optional tokenURI, but I think I'd rather it be a separate view given how different the implementations would be. Call it OffChainNftView or something?

bjartek commented 2 years ago

I really like this latest adition to Rarities in Attributes/Traits. Very well thought out @psiemens and @austinkline

bjartek commented 2 years ago

My interest is in making games and having all the metadata stored on chain makes things very complicated. I agree it makes sense for immutable metadata though.

Once Traits/Attributes is done it would be very possible to create a proxy that could convert all the onChain data into a opensea style json document and serve that.

We could consider adding in an optional tokenURI, but I think I'd rather it be a separate view given how different the implementations would be. Call it OffChainNftView or something?

Alchemy has a tokenURI field, I know that topshot/rarible/sturdy has json metadata stored elsewhere. I think a seperate TokenURI view in that case makes sense.

spacepluk commented 2 years ago

Syncing from the blockchain to the server could work for some games. But depending on the speed of the changes in the attributes you'll have many cases where that's just not practical or even possible. Very often you need a highly optimized game server to be the source of truth. In these cases the NFT is just used to track ownership of the game assets and having a url that points to the actual data is a much better solution. Not having a url and standardized json as the lowest common denominator makes things more complicated than they need to be imho.

I'm pretty new to flow, so maybe my take on this is completely wrong, but as a newcomer I can tell you that the current state of metadata is very confusing. Looking at the alchemy repo you can see that every project has their own way of doing things, and there aren't a lot of project using MetadataViews.

I'm just afraid that if the new standard doesn't guarantee that the metadata will be understood across different wallets/markets there's little incentive to change it for people that already have their own solution and we'll just end up with another competing standard added to the list.

I think the url approach has potential to fare better in that respect and it also happens to suit my project best, hehe

austinkline commented 2 years ago

@spacepluk

I'm pretty new to flow, so maybe my take on this is completely wrong, but as a newcomer I can tell you that the current state of metadata is very confusing. Looking at the alchemy repo you can see that every project has their own way of doing things, and there aren't a lot of project using MetadataViews.

Yes, this is true. Metadata standards still need to be adopted by folks, and that's going to get easier in a few weeks when people can deploy their own contracts with no assistance needed from the Dapper team. Once that is done, the community as a whole can assist in onboarding collections onto these standards.

I'm just afraid that if the new standard doesn't guarantee that the metadata will be understood across different wallets/markets there's little incentive to change it for people that already have their own solution and we'll just end up with another competing standard added to the list.

The views proposed here on this issue are actually meant to address almost exactly this. It will be composed of other underlying views that answer the questions:

In a non-standard way, I've been calling these "core" views. That is, these views are the necessary pieces to give a full picture of an NFT. Other views can exist for a specific purpose, but these are the ones that dapps and wallets will look for to know what to do with your asset.

I don't follow on "another competing standard" though. Views are composable, there isn't going to be one single Metadata View that is the only one to ever be relevant. You are meant to be able to ask for a specific one if you want, or maybe the NftView is you need all core pieces of data.

I think the url approach has potential to fare better in that respect and it also happens to suit my project best, hehe

But isn't this a whole new standard you are proposing? A view that points to supplemental data is good, but only doing TokenURIs, in my opinion, is not. It would mean there's no incentive to put data on-chain which is part of what "owning" your NFT on flow means. If a server is the one that contains all the information of an NFT, then theoretically it can also be tampered with or invalidated. That kind of concept has actually happened in the ethereum world with collections that alter the metadata of an asset if it goes against some kind of rule of theirs. A TokenURI would also make one of the two main use cases this view was proposed for (on-chain metadata-driven functionality) impossible.

I feel like my comment above (https://github.com/onflow/flow-nft/issues/74#issuecomment-1136208132) does a fairly good job in laying out why this is needed in the way it's been proposed, is there something specific you think isn't covered?

bjartek commented 2 years ago

Come to think of it, beeing able to add a tag to a given Attribute could be useful. But not sure, it is starting to be very complex.

austinkline commented 2 years ago

Come to think of it, beeing able to add a tag to a given Attribute could be useful. But not sure, it is starting to be very complex.

What does a tag look like to you? Is it different from displayType? All these additional pieces of data can be optional. If we make a helper function that takes a dictionary and turns it into the no-frills version of the traits view then maybe it's fine?

bjartek commented 2 years ago

@spacepluk I totally see your usecase, and having a TokenURI for these kinds of metadata would for sure make sense. Though I still think the core metadata that is not so often updated should be stored onChain.

bjartek commented 2 years ago

Come to think of it, beeing able to add a tag to a given Attribute could be useful. But not sure, it is starting to be very complex.

What does a tag look like to you? Is it different from displayType? All these additional pieces of data can be optional. If we make a helper function that takes a dictionary and turns it into the no-frills version of the traits view then maybe it's fine?

One usecase we have at .find is beeing able to do something with a selected set of Attributes/Tags. If we use all Tags/Attributes for this then that could end up beeing too much. So if you could then tag a given attribute to say that it could be used to say Filter something. Then that would be useful. But I am not sure. We are streching it here I think.

austinkline commented 2 years ago

One usecase we have at .find is beeing able to do something with a selected set of Attributes/Tags. If we use all Tags/Attributes for this then that could end up beeing too much. So if you could then tag a given attribute to say that it could be used to say Filter something. Then that would be useful. But I am not sure. We are streching it here I think.

Makes sense on both counts. I'm not sure the cut-off for when it's too much or not. For instance, why does rarity make more sense than tags to be included? Is there a better way we can be flexible with that somehow?

spacepluk commented 2 years ago

@austinkline Thanks for all the context, that really helps me understand the proposal better.

I don't follow on "another competing standard" though. Views are composable, there isn't going to be one single Metadata View that is the only one to ever be relevant. You are meant to be able to ask for a specific one if you want, or maybe the NftView is you need all core pieces of data.

I'm just saying this because most projects I've looked at don't use MetadataViews at all and rely on simple maps or custom structs.

But isn't this a whole new standard you are proposing?

I'm proposing to follow OpenSea's standard which is widely adopted.

A view that points to supplemental data is good, but only doing TokenURIs, in my opinion, is not. It would mean there's no incentive to put data on-chain which is part of what "owning" your NFT on flow means. If a server is the one that contains all the information of an NFT, then theoretically it can also be tampered with or invalidated. That kind of concept has actually happened in the ethereum world with collections that alter the metadata of an asset if it goes against some kind of rule of theirs. A TokenURI would also make one of the two main use cases this view was proposed for (on-chain metadata-driven functionality) impossible.

I get off-chain metadata is controversial. But the use-case I'm trying to present here is a game with a traditional server that can't be implemented fully on-chain at the moment. In this case, even if we put all the metadata on-chain if our company disappears the NFTs are worthless anyway. And in this case people will just buy the NFT if they trust that we'll stick around (just like with any other game). And it's still valuable to have the game assets be NFTs so people can trade whatever value they create in the game.

@bjartek

@spacepluk I totally see your usecase, and having a TokenURI for these kinds of metadata would for sure make sense. Though I still think the core metadata that is not so often updated should be stored onChain.

Thanks! we've been thinking about doing that. But it feels like having the metadata split in two systems adds unnecessary complexity just for the sake of having something on-chain without bringing any practical advantage because the NFT represents an entity in the game's server anyway.

Anyway, I appreciate you hearing me out. I understand it's not the typical NFT use-case and maybe this should be a separate proposal but it would be very helpful to have a standard way of encoding off-chain metadata.

austinkline commented 2 years ago

@spacepluk

Anyway, I appreciate you hearing me out. I understand it's not the typical NFT use-case and maybe this should be a separate proposal but it would be very helpful to have a standard way of encoding off-chain metadata.

A ticket about this is definitely a great place to start!

I'm proposing to follow OpenSea's standard which is widely adopted.

This is meant to emulate those standards while still being true to Flow and the intent of its own patterns for what it's worth. They will differ a little, but should still be familiar in that they share many of the same basic pieces. This intent is called out in the ticket itself, actually. And @bjartek is right that we could definitely build a transformer of some kind that takes our NftView and outputs the OS standard as well

I get off-chain metadata is controversial

The NftView proposed here didn't consider collections which split data on and off-chain. And there are other initiatives out there like Celer who are working on NFT Bridges to allow Flow NFTs on EVMs and vice versa. There is certainly a need for some kind of way to represent off-chain data. Maybe it is a core view we need to include in this proposal. The main pushback from me is about all of the data being off-chain, so apologies if it feels like your need isn't being taken into account. Would something like a supplemental TokenURI view that is taken in addition to on-chain data fit your needs?

Adoption is the next challenge, and your concerns there are shared by lots of us. The metadata standard is predated by quite a few collections, and since deployments require approval from the dapper team, there's a lot of friction when doing upgrades that folks don't want to go through. The deployment friction is set to go away on June 15th, though, when deployments are no longer gated!

austinkline commented 2 years ago

Sync'd with @bshahid331 (feel free to share anything I'm leaving out here)

In general, based on what @bshahid331 has relayed, there are lots of folks talking about the same core deliverable of an NFTView. Because of that, the general thought is that we should get the Traits view merged, and then we circle back in a separate discussion purely about what this complete NFTView would be and how it could be implemented. Lots of different platforms have different things in mind there, and if we don't make sure this end-result can satisfy most needs, adoption may be difficult to achieve.

Next Steps:

Anyone is free to make the new ticket, but I will create it over the weekend otherwise with as much detail as possible so we can get started discussing it. The longer we wait beyond folks having deployment permissions the more difficult it will be to get community-wide adoption of whatever we settle on

for visibility: @psiemens @bjartek @bluesign

spacepluk commented 2 years ago

@austinkline thanks again for the context, the dapper review friction is real!

Just a few more points that I hope you can consider before I move this convo somewhere else.

The main pushback from me is about all of the data being off-chain, so apologies if it feels like your need isn't being taken into account. Would something like a supplemental TokenURI view that is taken in addition to on-chain data fit your needs?

The TokenURI would definitely be an improvement, but in the end it boils down to adoption and support across wallets/marketplaces. So, if in the end if the standard pushes for all on-chain and this is optional we will likely be forced to put it on-chain if we want our NFTs to look like something.

In that scenario, and ignoring the extra complexity of this solution for a game where the actual data is in the game server there are a few extra issues that would arise. For example:

Having a TokenURI that points to a metadata view of the game state doesn't have any of these issues and it's a lot easier to implement, and that's why I think it's a good minimum default. I totally agree that data should be on-chain when possible (aka. immutable assets) but I think there are some use-cases like the one I'm describing where it's just a bad solution.

austinkline commented 2 years ago

Traits are finished! As a reminder, please add discussion to #98 so we can move to the next step which is a single "full picture" view that dapps can use to get key pieces of information they need about an NFT. Once that discussion is somewhat settled, I can start throwing up a PR for it.

As a callout, some of that implementation can be found here which was started by @bshahid331: https://github.com/onflow/nft-catalog/blob/main/cadence/contracts/NFTRetrieval.cdc#L11

joshuahannan commented 2 years ago

Can this issue be closed?

austinkline commented 2 years ago

Can this issue be closed?

Sorry! Missed this, I think we are good to close!