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

Add TopShotTier contract to resolve Tier on-chain #182

Closed
wants to merge 2 commits into from

Conversation

austinkline
Copy link
Contributor

Related to #170

Still work that needs to be done, I'm throwing this up so that folks can take a look and give feedback in case something is completely off and isn't the way they'd want this done.

This PR will add a new contract called TopShotTier which will be used to maintain a mapping to resolve the Tier trait on TopShot Moments onchain.

Assumptions

  • some moments might need a special override just for them
  • all other moments are a combination of setID+playID as a string

Approach

To keep things simple, we are keying on the concatenated string here, but some consideration was also made about whether this could be some sort of nested structure.

There is one resource created and maintained by this contract called TierMapping which has:

  • A public interface which exposes a method to resolve rarity given a moment id, setID, and playID.
  • some admin methods that allow setting tiers so they can later be resolved
  • If a mapping doesn't exist, the tier will return as an empty string (it will not panic)

TODO:

  • Add tests to return Tier in both cases of mappings
  • Add test to return an empty string for no mapping being present
  • Verify if any methods which create plays/sets should be setting this mapping automatically.
  • Any other verification I'm not aware of?

@austinkline austinkline requested a review from a team as a code owner November 21, 2022 20:21
Copy link
Collaborator

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

I think this could be useful. Are we sure that each play and set combination can only have one rarity?

@jrkhan
Copy link
Contributor

jrkhan commented Nov 23, 2022

I think this could be useful. Are we sure that each play and set combination can only have one rarity?

Can confirm that presently each play and set combination has only one rarity, however that might not always be the case in the future, so leaving room for a per moment override makes sense.

@austinkline
Copy link
Contributor Author

I think this could be useful. Are we sure that each play and set combination can only have one rarity?

Can confirm that presently each play and set combination has only one rarity, however that might not always be the case in the future, so leaving room for a per moment override makes sense.

Just to add to this as well, the dictionary here is String -> String so we can add more and more ways for that to be mapped, hopefully that makes this open enough that flexibility is permitted for future use

@austinkline
Copy link
Contributor Author

coming back to this @jrkhan @joshuahannan is there a checklist of things we need to make sure to address to get this accepted? I'm not sure if this is the approach that folks on the TS team want or not, happy to amend it if needed, but it would give flowty (and I'm sure other third-party platforms) a much easier way to deal with TS assets

Certainly there are some other data processing items that the TS team will have to do to backfill this data as well which is a whole other story that might take a while

@joshuahannan
Copy link
Collaborator

I think this would be great to have on-chain and I think it would be a valuable use of the Top Shot team's time, but I'm not on the team, so I don't have much visibility or control of their priorities. @jrkhan will need to be the driver here.

@jrkhan
Copy link
Contributor

jrkhan commented Feb 17, 2023

Apologies about the delay, I think this generally makes sense. We would need to create a few things on our BE to backfill the tier.
Wanted to run a few thoughts by @joshuahannan and @austinkline for the approach -> we also don't have any of our tags/badges available on chain yet.. and those are all dynamic (In the sense that they can be added to a moment/edition post minting)

Thinking that generally we have three kinds of data:

  1. Data that's always been on chain, and is associated with the moment when it's minted
  2. Things that have always been associated with the moment in product, are missing on chain (just moment tier)
  3. Things that can be dynamically added in product (e.g. tags/badges)

I'm wondering if we could, instead of solving specifically for moment tier, try to approach this to handle both 2 and 3.

Perhaps two maps of string to MetadataViews.Trait could live in a TopShotTags contract (either on the contract root, or on a resource):

  • MomentTags (sparsely populated, key value store of data associated with individual moments, falls back to EditionTag if not set)
  • EditionTags (the default value for a moment of that edition)

Presumably, a TagsAdmin capability could be used to add/edit tags, and likely create one for our TopShot minter.

We could then have the moments "rarity" stored as a tag, and potentially add getters to the core TopShot contract for well known fields.

@austinkline
Copy link
Contributor Author

@jrkhan I think it makes a lot of sense to try to use this as an opportunity to pull in all off-chain data that's currently out there. Would it make sense to get a call scheduled or circulate a more formal document for what's missing so we have a complete view of what's truly needed? What you've highlighted is a great start but it would be great if we could ensure that whatever steps are taken now are sufficient for any future needs

@joshuahannan for your thoughts as well

@joshuahannan
Copy link
Collaborator

yeah, I think this would require a bigger product discussion, because it also might apply to other NFT projects that have similar types of metadata. I think it is a god start though, but we should try to make sure it fits it well with the current metadata views and doesn't require too much extra custom code

@austinkline
Copy link
Contributor Author

yeah, I think this would require a bigger product discussion, because it also might apply to other NFT projects that have similar types of metadata. I think it is a god start though, but we should try to make sure it fits it well with the current metadata views and doesn't require too much extra custom code

I wonder if this isn't a good opportunity to propose a standard for how to properly add new metadata? I don't want to block progress on unnecessary process, though. Is a good path forward to get a call scheduled or something? Open to ideas!

@joshuahannan
Copy link
Collaborator

I wonder if this isn't a good opportunity to propose a standard for how to properly add new metadata?

Do you mean like a general standard for any project, or something specific to Top Shot? I assume that we'd probably use the new attachments feature

@tpetrychyn
Copy link
Contributor

@austinkline do you still need this feature? Would you like to resume work on this PR?

@austinkline
Copy link
Contributor Author

@austinkline do you still need this feature? Would you like to resume work on this PR?

I would say lots of folks are looking for this feature, and a similar one to bring other off-chain traits that are currently on TopShot on-chain. That being said, if we want to go back to the drawing board for that, I'm fine with closing this and syncing up on the best path forward.

The cadence side is likely to be much simpler than whatever migration work is needed on your side

@tpetrychyn
Copy link
Contributor

Thanks @austinkline I have moved this to an internal linear ticket. We will need to prio with product/engg team to investigate a solution like Jamil suggested.

@tpetrychyn tpetrychyn closed this Jan 16, 2024
@austinkline
Copy link
Contributor Author

Sounds good, happy to brainstorm on the cadence side if I can be of any help!

@austinkline austinkline deleted the tier branch January 16, 2024 17:20
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.

None yet

4 participants