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

Make ExampleNFT compatible for contract update #205

Merged
merged 5 commits into from
Mar 26, 2024

Conversation

SupunS
Copy link
Member

@SupunS SupunS commented Mar 21, 2024

Related to onflow/cadence#3098 (comment).

Closes #206

Description

Made the contract compatible for updating by reverting the following changes:

  • Removed the two new fields storagePath and publicPath.
  • Changed back the type of ownedNFTs to @{UInt64: NonFungibleToken.NFT}
  • Brought back the ExampleNFTCollectionPublic interface.
  • Brought back the conformance of Collection to ExampleNFTCollectionPublic.

For contributor use:

  • Targeted PR against master branch
  • Linked to Github issue with discussion and accepted design OR link to spec that describes this work.
  • Code follows the standards mentioned here.
  • Updated relevant documentation
  • Re-reviewed Files changed in the Github PR explorer
  • Added appropriate labels
  • Update the version in package.json when there is a change in the smart contracts

@SupunS SupunS force-pushed the supun/example-nft branch from ad26f3e to d95554d Compare March 21, 2024 18:25
@SupunS SupunS marked this pull request as ready for review March 21, 2024 21:47
@SupunS SupunS requested a review from joshuahannan as a code owner March 21, 2024 21:47

access(all) var storagePath: StoragePath
access(all) var publicPath: PublicPath
access(contract) var ownedNFTs: @{UInt64: {NonFungibleToken.NFT}}
Copy link
Member

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

Copy link
Member Author

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:

  1. 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.
  2. 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 any NonFungibleToken.NFT in the map, and not just ExampleNFT.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 from ExampleNFT.NFT to NonFungibleToken.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.

Copy link
Member

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

contracts/ExampleNFT.cdc Outdated Show resolved Hide resolved
@SupunS SupunS force-pushed the supun/example-nft branch from 6b9ee41 to 3954fbc Compare March 22, 2024 16:01
contracts/ExampleNFT.cdc Outdated Show resolved Hide resolved
@SupunS SupunS force-pushed the supun/example-nft branch from 692b3de to 5160258 Compare March 25, 2024 17:06
@joshuahannan joshuahannan merged commit c01c72c into standard-v2 Mar 26, 2024
2 checks passed
@joshuahannan joshuahannan deleted the supun/example-nft branch March 26, 2024 15:58
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.

3 participants