-
Notifications
You must be signed in to change notification settings - Fork 170
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
Make ExampleNFT compatible for contract update #205
Conversation
ad26f3e
to
d95554d
Compare
|
||
access(all) var storagePath: StoragePath | ||
access(all) var publicPath: PublicPath | ||
access(contract) var ownedNFTs: @{UInt64: {NonFungibleToken.NFT}} |
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 know this is being done to make the migrations work, but I strongly prefer that the type of this field stays as ExampleNFT.NFT
. It makes the collection implementation much safer because it enforces that only ExampleNFT
NFTs can be stored here.
It would be great to find another way around this
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 I understand the concerns.
The reason for not allowing changing the field is, if the type was changed in the contract, then all of a sudden the existing data stored no longer match the type of this field. So in order make it compatible, we would need to also migrate the values to the changed type, by running a migration. However, this is going to be very complex because:
- We would have to keep track of all the field types changes, and then during the migration, we would need to backtrack and find the type of the field, and then change the type of value accordingly.
- Some changes to types may not be compatible, but we can only know that for sure during the migration of the value. For e.g: given the type of the map's values were previously
NonFungibleToken.NFT
, it is theoretically possible to have anyNonFungibleToken.NFT
in the map, and not justExampleNFT.NFT
(maybe not in this particular example, but possible in other similar cases). i.e: This is "narrowing" the type, and narrowing isn't always guaranteed to be succeed. Where as broadening the type (changing fromExampleNFT.NFT
toNonFungibleToken.NFT
) would. So given that the contract is already staged by the time we run the migrations, if we run into such incompatibilities, those data would be forever corrupted. So that's why the contract update validator only allows changes that are 100% guaranteed to be valid.
Again, I get this may not be ideal, maybe we could do some brainstorming and try to find a middle ground.
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.
okay that is fine for now. I mostly want it there as an example for people who are creating new collections, but I guess we can just change it after the cadence 1.0 upgrade is complete
6b9ee41
to
3954fbc
Compare
a993d23
to
692b3de
Compare
Co-authored-by: Navid TehraniFar <[email protected]>
692b3de
to
5160258
Compare
Related to onflow/cadence#3098 (comment).
Closes #206
Description
Made the contract compatible for updating by reverting the following changes:
storagePath
andpublicPath
.ownedNFTs
to@{UInt64: NonFungibleToken.NFT}
ExampleNFTCollectionPublic
interface.Collection
toExampleNFTCollectionPublic
.For contributor use:
master
branchFiles changed
in the Github PR explorer