Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Traits view #97

Merged
merged 9 commits into from
Jun 16, 2022
Merged

Traits view #97

merged 9 commits into from
Jun 16, 2022

Conversation

austinkline
Copy link
Contributor

@austinkline austinkline commented Jun 4, 2022

Add Traits view to return the traits of an NFT.

You can find discussion about this view in #74

Some snippets from the original ticket:

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

An Attribute really is a wrapper around a single key-value pair and tells you:

  • What is the value?
  • What is the name of the value?
  • How should I show this value? (Optional)

The last piece (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 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"
  }
]

@austinkline austinkline changed the title Traits view (#1) Traits view Jun 4, 2022
contracts/MetadataViews.cdc Outdated Show resolved Hide resolved
austinkline and others added 2 commits June 4, 2022 10:30
* Traits view

* remove null check

* tidy
* added rarity

* reworded text

* make test

* Update contracts/MetadataViews.cdc

Co-authored-by: Satyam Agrawal <satyam0499@gmail.com>

* fix assertion in Rarities

* spacing

* tidy

Co-authored-by: Bjarte Stien Karlsen <bjarte@bjartek.org>
Co-authored-by: Satyam Agrawal <satyam0499@gmail.com>
This was referenced Jun 4, 2022
Copy link
Member

@joshuahannan joshuahannan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good so far! Should we also include Rarity in the ExampleNFT contract resolver, or do we think that it will only be used for traits?

Also, how do we feel about having a Traits view that just includes an array of Trait like we do for the other views?

contracts/MetadataViews.cdc Outdated Show resolved Hide resolved
contracts/MetadataViews.cdc Outdated Show resolved Hide resolved
@austinkline
Copy link
Contributor Author

Looks good so far! Should we also include Rarity in the ExampleNFT contract resolver, or do we think that it will only be used for traits?

Rarity I debated about before submitting this. I wanted to showcase the helper function to generate traits that don't need special handling. I could add a third rarity trait as well if we want and it would just be separate from the other two?

Also, how do we feel about having a Traits view that just includes an array of Trait like we do for the other views?

I was thinking about this, I generally don't like having Object.object kind of syntax, like a Traits struct having a traits field in it. At the same time, all our other views are non-array types. Sounds like there are other places where this is up in the air as @bjartek has asked about in #95

Co-authored-by: Joshua Hannan <joshua.hannan@dapperlabs.com>
@bjartek
Copy link
Contributor

bjartek commented Jun 7, 2022

I think adding Traits makes sense since we already have the pattern in all the other views.

@austinkline
Copy link
Contributor Author

I think adding Traits makes sense since we already have the pattern in all the other views.

I can live with that, sure. Not my ideal but I agree it's better we adhere to established precedent, especially if it's just nitpicks like this. Will update tonight

- Add a wrapper struct around an array of traits and assign to resolver type
- Add test for rarity on a trait
- Add test for display on a trait
Copy link
Contributor

@psiemens psiemens left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good overall! Left a couple suggestions and then one question about the rarity score field.

contracts/MetadataViews.cdc Outdated Show resolved Hide resolved
contracts/MetadataViews.cdc Outdated Show resolved Hide resolved
contracts/MetadataViews.cdc Outdated Show resolved Hide resolved
contracts/MetadataViews.cdc Outdated Show resolved Hide resolved
contracts/MetadataViews.cdc Show resolved Hide resolved
Copy link
Contributor

@psiemens psiemens left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 👍

@psiemens
Copy link
Contributor

@aishairzay @joshuahannan This PR looks good to me. Any final feedback?

Thanks for pushing this through @austinkline!

Copy link
Contributor

@aishairzay aishairzay left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM !

@joshuahannan
Copy link
Member

I still think it would be good to have a stringDictToTraits(_ dict: {String: String}): Traits utility function. Does anyone object to adding that? Also have you run all the tests with make test to make sure everything passes? I still haven't figured out how to get CI to run for PRs from forks

@austinkline
Copy link
Contributor Author

austinkline commented Jun 16, 2022

I still think it would be good to have a stringDictToTraits(_ dict: {String: String}): Traits utility function. Does anyone object to adding that? Also have you run all the tests with make test to make sure everything passes? I still haven't figured out how to get CI to run for PRs from forks

So just to make sure I understand the value-add, shouldn't I be able to use a {String: String} as {String: AnyStruct} for a method input?

All tests pass using make ci

Copy link
Member

@joshuahannan joshuahannan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're right, I guess it does work. I didn't know if the type system would be ok with that. I'm fine with it then

@joshuahannan joshuahannan merged commit affd46c into onflow:master Jun 16, 2022
@austinkline austinkline mentioned this pull request Jun 16, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants