-
Notifications
You must be signed in to change notification settings - Fork 2
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
Refactor from nested resource attachments -> native attachments #33
Conversation
…PieceNFT. Needs manual testing.
…ntracts deploy, need txn & test refactor
7c43adc
to
87d8020
Compare
I'm starting my review now, but just from reading the description about potential collisions between NFT IDs, couldn't we still index by UUID? You can get the uuid from any resource, regardless of if it is from the resource object or a reference, so you could still use uuid I think. Just a thought. |
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.
Looks good so far! I like how many lines of code it saves.
Maybe it would be good to use uuid for everything here. I think we'll try to move towards standardizing uuid in the v2 standard anyway, so it could be good to start using it by default in smart contracts now. What do you think?
@joshuahannan I agree, we should just use UUID. I realized this morning that hashing the leaderboard obfuscates identifying information about the NFT such that a meaningful implementation of a leaderboard becomes impossible. So it's a bit of an over-engineered solution to one problem that creates another. I think your suggestion to use NFT uuid is the way to go. |
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 it looks good! Needs another review though
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.
Yeah everything looks great so far! approving and merging asap so we can move to the incoming child account PR
Closes: #29 #30
Description
This PR refactors code from our custom makeshift attachments via the DynamicNFT contract to Cadence's native attachments. Some functionality was necessarily removed as native attachments do not support attachment iteration. The lack of iteration in conjunction with the fact that accessing attachments requires naming the attachment's type statically means that getting metadata views and resolving attachment metadata views via the NFT is not possible.
As a result, resolving attachment metadata is moved to scripts where the base NFT can be referenced, attachment accessed and metadata resolved via the attachment reference.
The role of GamePieceNFT is therefore (at least for the moment) to function as a generic NFT to which attachments can be made for the purposes of an MVP demo. Once attachment iteration is implemented, GamePieceNFT can support retrieving attachment view types (
getAttachmentViews()
) andGamingMetadataViews.GameAttachmentsView
.The two attachment implementations are
RPSWinLossRetriever
andRPSAssignedMoves
. Both are designed to be attached to anyNonFungibleToken.INFT
. The big advantage is that any NFT is compatible with theRockPaperScissorsGame
contract without alterations to their contract (e.g. MonsterMaker). With this advantage comes one primary consideration - maintaining win/loss records in the contract & accepting any NFT could lead to collisions in thewinLossRecords
mapping ofNFT.id
toGamingMetadataViews.BasicWinLoss
. For NFTs that assign ids on the resource's UUID, this shouldn't pose a problem. However, it could be a concern for NFTs that do not. The only solution I've thought of so far is indexing on the hash of an NFT's ID along with its Type, and would appreciate feedback on whether we think this is a good idea/necessary. See the hashing encoding scheme at the bottom for an example of what this might look like (also documented this issue in #34).Another primary feature change required by migrating from DynamicNFT to native attachments is the inability to reset win/loss records from the attachment. Since
RPSWinLossRetriever
is available via reference to the base resource, anyone with a reference to the base NFT has access to all methods defined inRPSWinLossRetriever
. This means that anyone could borrow a reference to an NFT from aCollectionPublic
Capability, access the nft'sRPSWinLossRetriever
attachment and callresetWinLossData()
. Since this method should only be accessible via the NFT holder, I've decided to remove the method for the time being. This is expanded on a bit more in this comment in #27, but the gist is that we need a way in Cadence to define attachment behavior that is/is not accessible via reference to the base resource and there is a Flip out that would enable such definitions. Special thanks to @dsainati1 for the support in figuring out attachments & raising the use case in the linked Flip!Appendix
The encoding scheme would look something like:
Reference
A couple reference issues related to Cadence attachments that arose as a result of implementing the feature here:
For contributor use:
master
branchFiles changed
in the Github PR explorer