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.

Sign up for GitHub

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
Merged

Make ExampleNFT compatible for contract update #205

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
d95554d
Make ExampleNFT compatible for contract update
SupunS Mar 21, 2024
3954fbc
Add back the type assertion for the deposited token
SupunS Mar 22, 2024
5160258
Make the collection path backward compatible
SupunS Mar 25, 2024
83d28b5
remove docs, INFT, and update paths
joshuahannan Mar 26, 2024
ffda944
make ci
joshuahannan Mar 26, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
33 changes: 16 additions & 17 deletions contracts/ExampleNFT.cdc
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
/*
/*
*
* This is an example implementation of a Flow Non-Fungible Token
* using the V2 standard.
Expand All @@ -7,7 +7,7 @@
*
* This contract does not implement any sophisticated classification
* system for its NFTs. It defines a simple NFT with minimal metadata.
*
*
*/

import "NonFungibleToken"
Expand Down Expand Up @@ -36,7 +36,7 @@ access(all) contract ExampleNFT: NonFungibleToken {

/// Generic dictionary of traits the NFT has
access(self) let metadata: {String: AnyStruct}

init(
name: String,
description: String,
Expand All @@ -58,7 +58,7 @@ access(all) contract ExampleNFT: NonFungibleToken {
access(all) fun createEmptyCollection(): @{NonFungibleToken.Collection} {
return <-ExampleNFT.createEmptyCollection(nftType: Type<@ExampleNFT.NFT>())
}

access(all) view fun getViews(): [Type] {
return [
Type<MetadataViews.Display>(),
Expand Down Expand Up @@ -117,26 +117,23 @@ access(all) contract ExampleNFT: NonFungibleToken {
let fooTraitRarity = MetadataViews.Rarity(score: 10.0, max: 100.0, description: "Common")
let fooTrait = MetadataViews.Trait(name: "foo", value: self.metadata["foo"], displayType: nil, rarity: fooTraitRarity)
traitsView.addTrait(fooTrait)

return traitsView
}
return nil
}
}

access(all) resource Collection: NonFungibleToken.Collection {
// Deprecatred: Only here for backward compatibility.
access(all) resource interface ExampleNFTCollectionPublic {}

access(all) resource Collection: NonFungibleToken.Collection, ExampleNFTCollectionPublic {
/// dictionary of NFT conforming tokens
/// NFT is a resource type with an `UInt64` ID field
access(contract) var ownedNFTs: @{UInt64: ExampleNFT.NFT}

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

init () {
self.ownedNFTs <- {}
let identifier = "cadenceExampleNFTCollection"
self.storagePath = StoragePath(identifier: identifier)!
self.publicPath = PublicPath(identifier: identifier)!
}

/// getSupportedNFTTypes returns a list of NFT types that this receiver accepts
Expand Down Expand Up @@ -191,7 +188,7 @@ access(all) contract ExampleNFT: NonFungibleToken {

/// Borrow the view resolver for the specified NFT ID
access(all) view fun borrowViewResolver(id: UInt64): &{ViewResolver.Resolver}? {
if let nft = &self.ownedNFTs[id] as &ExampleNFT.NFT? {
if let nft = &self.ownedNFTs[id] as &{NonFungibleToken.NFT}? {
return nft as &{ViewResolver.Resolver}
}
return nil
Expand Down Expand Up @@ -304,8 +301,11 @@ access(all) contract ExampleNFT: NonFungibleToken {

// Create a Collection resource and save it to storage
let collection <- create Collection()
let defaultStoragePath = collection.storagePath
let defaultPublicPath = collection.publicPath

let identifier = "cadenceExampleNFTCollection"
let defaultStoragePath = StoragePath(identifier: identifier)!
SupunS marked this conversation as resolved.
Show resolved Hide resolved
let defaultPublicPath = PublicPath(identifier: identifier)!

self.account.storage.save(<-collection, to: defaultStoragePath)

// create a public capability for the collection
Expand All @@ -317,4 +317,3 @@ access(all) contract ExampleNFT: NonFungibleToken {
self.account.storage.save(<-minter, to: self.MinterStoragePath)
}
}

Loading
Loading