search

onflow / flow-nft

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

Add new `attributes` field and associated views. #152

Closed cybercent closed 1 year ago

cybercent commented 1 year ago

Instructions

As I understood from @bjartek the usage for traits field is to fit data that does not have a place elsewhere. @bjartek please correct me if I'm wrong.

Issue To Be Solved

I'm proposing an Attributes field that is similar to Traits but the usage is for user facing data, and specifically for attributes that a user can filter on (in a marketplace.)

(Optional): Suggest A Solution

Add a new field called attributes.

(Optional): Context

Metadata example from the recent Doodles mint: https://find.xyz/0x7ed2693f6f85e33d/collection/main/Wearables/873059584

The context_ fields are valid traits but not valid attributes.

austinkline commented 1 year ago

I have a little concern folks might get confused about which to implement here, but totally see the purpose! Is it worth also considering a standardized displayType of "none" on traits to flag to platforms not to show it?

cybercent commented 1 year ago

A display type none could work, unless there is a trait with a display type date that should be hidden.

Another issue with current traits is that trait names are not user friendly, ex "wearable_status", it should be either "Status" or "Wearable status".

Maybe only adding a view and not a new attributes field could work. For example using the traits as is and adding a view to get a subset of the traits in a user friendly display mode.

The developer could choose to return in the Attributes View all traits, if they are user friendly, or implement conditions to clean/reformat them.

bluesign commented 1 year ago

This is interesting idea, I think basically what you are suggesting is equal to adding a field to trait to show what kind of trait it is, maybe another field to group traits and label.

Current trait is: trait(name, value, displayType, rarity)

making it: trait(name, value, displayType, rarity, displayLabel, category/context) and using displayType='hidden' for none user facing traits could work.

I agree with @austinkline, it can be confusing for developer to know, when to use attribute when to use trait otherwise.

https://f.dnz.dev/0x7ed2693f6f85e33d/storage/wearables/873059574

here is the existing example:

Traits
name: ape's helmet
template: ape's helmet
position: head
set: OG Wearables: Custom
set_creator: Doodles
wearable_status: active
condition: mint
context_ethereum_tx: 0xb99cb3bcc62d6fc0896944d87dfbfa3248ec3e01f315380b87100f87279f2ed5
context_doodle_id: 6197
context_dooplicator_id: 6668
context_ethereum_received: 2023-02-01 00:20:23 +0000 UTC
context_ethereum_address: 0x695589d2bc5a63db1e830b1da7a07de73e58c6a8
license: https://doodles.app/terms
cybercent commented 1 year ago

If context is added, it could be used to signal that the data is meant for the NFT owner, or a dapp. Context could be an enum: context: owner | dapp.

In that case adding a new hidden option to displayType is no needed.

bjartek commented 1 year ago

Thank you for bringing this up @cybercent. I think a view that is specific to that context would be very interesting. So a view that said "how where you minted/created" in the first place. It can basically just be a struct with a name that has Trait structs in it. For me the semantics are important there though.

bjartek commented 1 year ago

Another issue with current traits is that trait names are not user friendly, ex "wearable_status", it should be either "Status" or "Wearable status".

I have been meaning to propose that traits could be split on "_" to group them, but I have been so busy that I have not had time.

basically i think we need some way to group traits together. When displaying traits having a way to group them together would be nice. Maybe in a new Attributes view we could have a grouping?

bjartek commented 1 year ago

Should the displayName of a thing also be a trait or shoult it not?

cybercent commented 1 year ago

I spent more time studying the ERC-1155 Metadata standard and the Enjin specifications, and I believe that the current Flow traits specs: trait(name, value, displayType, rarity) are sufficient.

It is not necessary to use underscored or camel-cased strings in the name field, and doing so is a misuse of the field. Developers should ensure that the name they choose is display friendly.

In addition, using Set information, Max editions and other fields that are not traits as traits is a misuse of the traits field.

Custom views can be used to add information that is not a trait.

If everyone here agrees, I'm ready to close this issue.

bjartek commented 1 year ago

I think adding a new Attributes view is a good option. Since i belive as you have illustrated above Traits is not specific enough and it also has some other problems:

So i think adding something like this is a good idea

Attributes:

joshuahannan commented 1 year ago

I'm still kind of confused about the difference between attributes and traits. Can someone please explain that more to me? It also seems confusing for what to implement for users and I think existing traits should be able to handle it if we can be creative. Also, I think I heard that AnyStruct will be able to be emitted in events soon, so that concern might not be warranted. I don't want to make the metadata views too redundant

bjartek commented 1 year ago

Then lets just close this issue, however naming of traits is already done differently all over the place. Some use human readable names that are not url safe, others use camelCase some use snake_case.