-
Notifications
You must be signed in to change notification settings - Fork 105
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
Conversation
There was a problem hiding this 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?
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 |
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 |
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. |
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. Thinking that generally we have three kinds of data:
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):
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. |
@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 |
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! |
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 |
@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 |
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. |
Sounds good, happy to brainstorm on the cadence side if I can be of any help! |
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 theTier
trait on TopShot Moments onchain.Assumptions
setID+playID
as a stringApproach
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:TODO: