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

FLIP 56: Non-Fungible Token Standard Version 2 #56

Merged
merged 5 commits into from Mar 11, 2024
Merged

FLIP 56: Non-Fungible Token Standard Version 2 #56

merged 5 commits into from Mar 11, 2024

Conversation

joshuahannan
Copy link
Member

Proposes a fundamental upgrade to the Flow Fungible Token standard.

Original Proposal here: https://forum.onflow.org/t/streamlined-token-standards-proposal/3075/1
Code PR: onflow/flow-nft#126

@bjartek
Copy link
Contributor

bjartek commented Dec 21, 2022

I have been thinking about this quite a lot recently and I have some comments around this. I do not think this NFTv2 suggestion takes the changes far enough. I belive the community could have more benefit if we do even more.

  1. Having multiple NFT types in the same storage pathis not a good idea IMHO. . Large dictionaries eat up gas.

  2. Use the same Collection implementation to store multiple NFT types, but not in the same path. You do not have to rewrite the code to create a collection you only have to know what limitations this NFT type has on the collection it can be put into.

  3. When I add a new NFT type that can be stored with a given Collection i do not have to touch that collection code, only the NFT code. This composes better.

What I think we can do to fix this:

  • make it possible to create a NFT Collection code that can store multiple NFT types. The path that stores a single type has information about what that type is. We have a method that returns what type is stored here in the collection
  • have a contract that holds a NFT (yes only one) that does not also need to have a collection. It needs to expose some information in a standard on way on what it needs to be put into a collection and how to link it.

An example NFT contract could then be as simple as

//imports removed for brevity
pub contract ExampleNFT: NonFungibleToken {
  
    pub resource NFT: NonFungibleToken.INFT, MetadataViews.Resolver {
    
        pub let id:UInt64
        /// Metadata fields
        pub let name: String
        pub let description: String
        pub let thumbnail: String
        access(self) let royalties: [MetadataViews.Royalty]
        access(self) let metadata: {String: AnyStruct}
    
        init(
            id: UInt64,
            name: String,
            description: String,
            thumbnail: String,
            royalties: [MetadataViews.Royalty],
            metadata: {String: AnyStruct},
        ) {
            self.id = id
            self.name = name
            self.description = description
            self.thumbnail = thumbnail
            self.royalties = royalties
            self.metadata = metadata
        }


        pub fun getViews(): [Type] {
            return [
                Type<MetadataViews.Display>(),
                Type<MetadataViews.Royalties>(),
                Type<MetadataViews.Editions>(),
                Type<MetadataViews.Serial>(),
                Type<MetadataViews.Traits>()
            ]
        }

        /// Function that resolves a metadata view for this token.
        ///
        /// @param view: The Type of the desired view.
        /// @return A structure representing the requested view.
        ///
        pub fun resolveView(_ view: Type): AnyStruct? {
            switch view {
                case Type<MetadataViews.Display>():
                    return MetadataViews.Display(
                        name: self.name,
                        description: self.description,
                        thumbnail: MetadataViews.HTTPFile(
                            url: self.thumbnail
                        )
                    )
                case Type<MetadataViews.Editions>():
                    // There is no max number of NFTs that can be minted from this contract
                    // so the max edition field value is set to nil
                    let editionInfo = MetadataViews.Edition(name: "Example NFT Edition", number: self.id, max: nil)
                    let editionList: [MetadataViews.Edition] = [editionInfo]
                    return MetadataViews.Editions(
                        editionList
                    )
                case Type<MetadataViews.Serial>():
                    return MetadataViews.Serial(
                        self.id
                    )
                case Type<MetadataViews.Royalties>():
                    return MetadataViews.Royalties(
                        self.royalties
                    )
                case Type<MetadataViews.ExternalURL>():
                    return MetadataViews.ExternalURL("https://example-nft.onflow.org/".concat(self.id.toString()))
               
                case Type<MetadataViews.Traits>():
                    // exclude mintedTime and foo to show other uses of Traits
                    let excludedTraits = ["mintedTime", "foo"]
                    let traitsView = MetadataViews.dictToTraits(dict: self.metadata, excludedNames: excludedTraits)

                    // mintedTime is a unix timestamp, we should mark it with a displayType so platforms know how to show it.
                    let mintedTimeTrait = MetadataViews.Trait(name: "mintedTime", value: self.metadata["mintedTime"]!, displayType: "Date", rarity: nil)
                    traitsView.addTrait(mintedTimeTrait)

                    // foo is a trait with its own rarity
                    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
        }
    }


   //expose out data on what is needed to store this. Like if you need a certain interface to be present in the collection.
 

    //code that allows you to mint

}

@austinkline
Copy link
Contributor

austinkline commented Dec 21, 2022

Nice to see this make its way into a flip! A few initial questions/comments to get my bearings:

Events

  1. Are we settled on what the "rich" events might be for Deposit/Withdraws? Display makes total sense, I'm curious what Serial gives us if we aren't moving towards storing based on UUID (whole other tangled mess of complexity there). Ideally You're using display anyway to show what the event was, and the id of the nft tells you how to get to it.

  2. Do we plan to emit rich details in both events (Deposit and Withdraw)? For the most part both deposit and withdraw go together, but not always. Flowty, for example, wouldn't emit a deposit when a loan is funded, it gets put into another resource until that loan is expired or it is repaid which would be outside the scope of a collection resource.

Transfers

  1. I'm not sure how I feel about transfer returning a bool, would it not just panic if the receiver can't take the item? Otherwise we might have cases where a user thinks something worked (the transaction executed successfully) but the outcome wasn't what they expected. Has there been discussion about that?

Helper methods

  1. Full agreement on getIDs and its challenges, I'll have to think of a way that we could maybe make that work for pagination and come back later with something
  2. We'd had some discussion about a safeBorrow but it sounds like this is being handled by making borrowNFT return an optional, I think that's great
  3. Would it make sense to add a method on the receiver for whether a type is able to be deposited? It might go nicely with getAcceptedTypes to have an isAcceptedType(type: Type)

@m-Peter
Copy link

m-Peter commented Jan 4, 2023

After reading the comments in the PR and the original proposal, I would like to share a few thoughts also. Mostly they elaborate on comments from @bjartek.

There is however one part I do not like. And that is identification. I believe all NFTs should use uuid as the ID that is used to store it in the collection. If not there will be conflicts and problems.

I totally agree that this is a must-have prerequisite. Since it is something provided by the language, it should be used in all possible places, such as event payloads for example. It will provide a more solid tracing ability, than having to deal with custom NFT IDs from each possible implementation.

Having multiple NFT types in the same storage path is not a good idea IMHO. Large dictionaries eat up gas.

Having paths store a single NFT type, will allow for a more fine-grained control to cover various use cases. For example, a "heavy" NFT type, will eat up gas for transactions dealing with "light" NFT types, because they all share the same storage path.

Use the same Collection implementation to store multiple NFT types, but not in the same path. You do not have to rewrite the code to create a collection you only have to know what limitations this NFT type has on the collection it can be put into.

The Collection in the current NonFungibleToken contract is a first-class citizen, with more responsibility/authority than it should have. The changes should be towards making Collections just arrange NFTs and add capabilities to the NFT that it needs to function, like a primitive data structure but little more supercharged. I think limitations should be a more defining factor, for what is possible in joining a given collection. Basically the end-user should be able to mix-and-match NFTs, to desired collections, or product lists as they would be called in traditional marketplaces.

When I add a new NFT type that can be stored with a given Collection I do not have to touch that collection code, only the NFT code. This composes better.

This sounds like the Open-Closed Principle put into practice!

Make it possible to create a NFT Collection code that can store multiple NFT types. The path that stores a single type has information about what that type is. We have a method that returns what type is stored here in the collection.

That's a pretty good use of leveraging the type system of the language.

Have a contract that holds a NFT (yes only one) that does not also need to have a collection. It needs to expose some information in a standard on way on what it needs to be put into a collection and how to link it.

With the addition of static functions, this contract can act as a factory blueprint, on how to add a NFT type in a given Collection. Using interfaces to restrict what a NFT needs to be stored correctly in a Collection, is another SOLID thing to to.

I hope these remarks are not very confusing, and they do make some sense! 🙏

@chriswayoub
Copy link

A few thoughts:

NFT IDs
I agree wholeheartedly that the UUID of the resource should be used as the identifier for the NFT everywhere. I do not think this should be something that is left up to the developer to define, and should be able to be trusted to refer to the specific NFT in question at all times. I've seen contracts that inadvertently minted two NFTs with the same ID, and it causes havoc with emitted events since there is no way to tell them apart. I'm sure there are also scenarios where this could be used maliciously by the owner of a contract.

Collections
I know this is a hot topic (shared vs separate collections), and personally I'm on the side of a single collection. But I will go a step further and say that collections implemented in a contract should not exist in the first place for a normal use-case. There should be a language-level way to store a resource in an account (without a storage path) and there should be a way to query that storage and retrieve a resource by UUID.

I think that this interface can be very simple:

  • Store any resource
  • Query storage for number of stored resources (perhaps filtering by Type)
  • Retrieve resource from storage by UUID

If I have stored a resource in my account, then I believe that I should not have to rely on a contract implementation to access that resource. As it stands right now, I could prevent someone from withdrawing their own NFT that they are supposed to have ownership of. This can also happen if there is a problem with the contract itself (as we've seen with the secure cadence update), although that might become a non-issue with Stable Cadence.

I do think there are some things that need to be thought through with this approach, like how to give capabilities to withdraw/deposit, etc. I think if the core resource storage interface can be as lightweight as possible, and mainly just facilitate saving/loading resources to a built-in simple collection, then hopefully everything else can be implemented at the contract-level to allow for exposing custom functionality/capabilities to that storage.

@joshuahannan
Copy link
Member Author

A lot to respond to here, so I'll try to respond to groups of comments

@bjarte, I think your suggestion is good, but I think that a proper way of storing multiple NFT types in the same collection is out of the scope of this proposal. Is there anything in this proposal that makes your idea not possible?

Additionally, what you suggest with implementing an NFT without implementing a collection is already possible. We’re getting rid of static type requirements, so if you want to implement an NFT, all you have to do is implement the NFT resource interface on your NFT resource. The contract interface is just an optional thing you can implement if you want more of the convenience methods on your contract.

@joshuahannan
Copy link
Member Author

@austinkline

We are definitely not settled on what rich events can include. I’m not an app developer, so I don’t know what apps are expecting. If you have suggestions, please post them and we can discuss. I would like to include metadata in all events if possible though

Transfer:

That is just something I was playing around with, but I will probably remove it. Basically, since transactions can have any arbitrary code, I’d like for cadence to one day move to a paradigm where functions don’t usually panic, but return error codes and messages and the transaction code can handle the error and decide to panic if it wants or continue execution if the error isn’t a critical issue. This is already how many modern languages work, so it would be great for Cadence to be able to do that to. I don’t really think it works here though because we don’t have all the language support for it yet.

isAcceptedType(type: Type) is a good idea, I’ll add that

@joshuahannan
Copy link
Member Author

Using uuid

I 100% agree that using uuid is the ideal way to mange this, but I can’t think of a way to enforce that contracts rely on uuid from now on. Many projects and smart contract are already using and will continue to use their contract specific IDs and migration to a completely uuid-focused paradigm seems extremely difficult operationally, but if you have ideas, please share them.

@m-Peter, I’m a little confused about most of your comments about bjartek’s comments. Can you share a proposal about what the standard would need to do to address your concerns?

@joshuahannan
Copy link
Member Author

@chriswayoub Your proposal about managing collections and resources in storage sounds like something that is a core language feature, not a token standard feature. Have you tried making a FLIP for it? That would be the best approach to get discussion started on it

@m-Peter
Copy link

m-Peter commented Jan 5, 2023

Hi @joshuahannan, my apologies for the confusion 😅. I will try to clarify things below 🙏

My comments were basically affirmations on bjartek's comments, with some extra details on how they resonate with me.

Ideally, in the final/stable form, I see Collections as a "primitive data type" (maybe not included in the language standard library, but rather as a contract/contract interface possibly). They should be decoupled from a standard storage path, as they can very likely reside inside Dictionary/Array fields, in the contract-level (just like we have done in the NFTPawnshop). However, most of the times they will reside in an account's storage, but this should be up to the developer to decide where.

Just like dictionaries have some limitations on what can be used as the key (any object/data type that can generate a numeric hash value, for fast lookup, for example), Collections should leverage the type-system of the Cadence language, and take this a step further.
The "values" that a specific Collection will hold (in analogy to a Dictionary), doesn't have to be a concrete NFT type, but rather an interface, or list of interfaces. It could be all NFTs which have a Display Metadata View, or that have Editions, or Royalties or anything that the developer finds reasonable.

In closing, NFTs most likely evolve quite fast that's why Collections should be closed for modification, but open for extension, to seamlessly accommodate the need for integrating new NFT contracts/types.

That being said, this is just what I see as the end goal. The changes proposed for the V2 are clearly an improvement step, considering the current version of the NonFungibleToken contract interface.

I hope this does remove some/all of the confusion 🙏

@bjartek
Copy link
Contributor

bjartek commented Jan 5, 2023

We at .find can take a stab at creating a view in MetadataViews that is appropriate to put into events. Currently you cannot have functions and AnyStructs in there so some of the views will need to be transformed a little for this to work.

It will be based on FindMarket.NFTInfo and will support both &{NonFungibleToken.INFT} and &{MetadataViews.Resolver}

@joshuahannan I will take a closer read in a bit to see if any of the current suggestions are prohibiting my suggestions but I do not think so.

@joshuahannan
Copy link
Member Author

Here are the notes from the call last week with the open questions that we still have! I tried to organize them a little more so that are easier to follow. We might be talking about it again next tuesday at the SC eng open house.

  • How to provide a standard for collections that store many NFTs
    • A single dictionary is not scalable when loading elements because you have to load the whole dictionary. When accounts have millions of NFTs, this can not scale
    • maybe a collection that stores sub collections in different storage paths when they get too large. Could this be a standard?
      • Would use AuthAccount capabilities or the proposed restricted auth account feature
    • Can we paginate getIDs()
  • Interface Inheritance
    • https://forum.onflow.org/t/flip-interface-inheritance-in-cadence/3750
    • This FLIP needs to be approved in order to enable enforcing that Vaults and Collections implement all the correct interfaces
    • Needs to have the default implementations conflicts resolved
    • Not required for what we’re trying to do, but if we don’t have it, then we won’t be able to enforce that types that implement Vault and Collection also implement the other standard iterfaces
    • If nested type requirements are not removed and we still have only one token per contract, then this is not required
  • Nested Type Requirements Removal
    • We want to remove nested type requirements from the language, which would mean we need to change how we do events and interfaces.
    • This makes multiple tokens per contract possible. Is multiple NFTs per contract useful?
      • On the surface it seems like a nice flexible option to have, but it isn’t clear how many projects will use it.
      • It also allows us to encapsulate more of the functionality into the resource objects instead of at the contract level
        • this is nice since it is all in one place
        • also allows projects that have multiple NFTs to store them in the same collection instead of in separate collections
      • It makes some of the typical functions different and a little more awkward
        • Getting an empty vault or collection from the contract level now requires a type argument
        • Getting the standard paths now requires querying the metadata view from the contract, then looking at the paths there
        • getting total supply directly from the contract requires a type argument
      • We need more input from the community on if we want this feature or not
        • some might not see the value of having multiple token types and removing nested type requirements. might need some examples to illustrate
      • Universal collections might not be used so it might not be as important to remove type requirements
  • Using UUID
    • Everyone agrees that if would have been perfect could have started using UUID for all NFTs from the beginning, but it wasn’t available then so we haven’t
    • Using uuid universally would be useful, but with existing collections who don’t use them this is hard to be compatible because most current collections, marketplaces, etc are keyed with the regular IDs, not uuid, so migrating would be extremely difficult
    • projects with old collections might have to define a new collection type using uuid that lives alongside the old collection to allow them to migrate over time
    • some apps like Flowty can’t change because they have extended loans that use the old system. migration is really hard and error prone and might permanently break existing state
    • Everyone agrees that sticking with the old ID system probably isn’t sustainable though. currently, NFTs can have the same ID, which is potentially a big problem, and it would be hard to tell which projects are using uuid and which aren’t
    • Events would also be changed with uuid changes
    • We could add more withdraw options to the standard collection such as withdrawWithUuid() or withdrawWithType() so that both systems can still be used
    • Could also have two different collection interfaces, one for the old and one for the new implementations, to allow projects to migrate and use the workarounds in the meantime
    • Need more input and suggestions on this issue
  • Events emitted from Interfaces
  • Universal Collection
    • not very popular
    • if storage fees were different, it might not be an issue because it can be an attack vector
  • Standard events?
    • Use Type or Type identifier as an event argument
    • convenient to be able to see views directly from events instead of having to query afterwards
    • might be too high of a cost to get these views for every withdrawal and deposit and it might make the events too large
    • some views might not be safe to display because some projects don’t allow display legally
    • may need better events that can give us more information for cases like in the staking contract where they are meaningless
  • Upgrade issues
    • Circular dependency with metadata views
      • We would prefer to have the NFT interface directly implement the MetadataViews.Resolver interface, but can’t because that would create a circular dependency because MetadataViews imports NonFungibleToken
      • Create empty collection method is the only one that uses the circular dependency
      • could move the definition to a different contract to avoid the circular dependency
      • maybe move all the NFT specific views to a different contract
      • might break implementations because the types would be incompatible
      • Need to find a resolution for this
    • Upgrade issues that should hopefully be able to be resolved with a special exception in Cadence
      • Can’t give the interfaces the same name as the types in the original standards
      • Can’t remove the original type definitions
      • Can’t include structs in the event emissions
  • Need to decide if we want to define new views for contract view resolver for the NFT standard, especially if we have multiple NFTs per contract

@bjartek
Copy link
Contributor

bjartek commented Mar 31, 2023

A good list. I would really love richer deposit/withdraw events. Just adding things like thumbnail and name would make it so much more useful. However I can see the issue as well since there are so many of these events so I am willing to compromise here unless somebody else has a really strong opinion about it. It was me that wanted it in the first place i think.

@joshuahannan
Copy link
Member Author

Here are the notes from the working group on Apr 4. The action items are bolded. I'll be adding these updates to the FT and NFT PRs and I'll share some of my findings in the next working group. Anyone who is interested in contributing and/or helping with trying out some of our ideas, please let me know!

  • Nested Type Requirements Removal
    • Decided to move forward with removing nested type requirements and encapsulating token functionality within resources
  • Using UUID
    • Josh is going to play around with the below suggestions to see what might work for this and make a writeup for the pros and cons of each one
    • We could add more withdraw options to the standard collection such as withdrawWithUuid() or withdrawWithType() so that both systems can still be used
    • Could also have two different collection interfaces, one for the old and one for the new implementations, to allow projects to migrate and use the workarounds in the meantime
    • maybe we can provide fun getUDID(type:Type, id:UInt64): UInt64 ? so all contracts can use uuid to migrate before we do the spork for stable cadence
      • borrow resource by uuid - talk to Dete
    • What if we emit an event in the getUUID() method that includes both the id and uuid so indexer can map both to each other? We could probably add that as a default implementation in NFT interface
      • already in the deposit and withdraw events
      • need to include get ID methods for old and and uuid
    • Could provide a method on NFTs for projects to specify if they are using UUID or not to avoid misunderstandings after the upgrade
  • Events emitted from Interfaces
  • Standard events?
    • Will only include name and thumbnail as metadata event arguments for now
    • Use Type or Type identifier as an event argument?
      • Type is serialized as the identifier in the event. need the identifier off-chain to be human readable. need to know what the type is actually emitted as
      • Josh will do some research on this
    • Even if events don’t have all the metadata, a third party could offer a service
      • access node could monitor events, then could query the metadata views with a script so that the full “event” can be script enhanced
  • How to provide a standard for collections that store many NFTs
    • Josh is going to research some more for this issue
    • Very important that an NFT can tell where it is stored. You know the type of the NFT from an event, but you don’t know the path. An NFT should own where it can be stored. This is already in the collection view, but could be more general
    • NFTs can be stored anywhere, so how would it indicate a specific location
    • Could use the Collection interface to be the composable NFT standard interface
      • This could also be a bare minimum interface that doesn’t necessarily include all the methods in the collection interface
    • maybe a collection that stores sub collections in different storage paths when they get too large. Could this be a standard? Would use AuthAccount capabilities or the proposed restricted auth account feature
    • Same with attachments, it is hard to know where they are. What if an NFT is part of an attachment. How do you know where it is?
    • Rich events would help to indicate this, like for loading/saving resources, linking types, etc

@joshuahannan
Copy link
Member Author

I made some changes to the PR: onflow/flow-nft#126
I described the changes in a comment . They are basically the action items that I bolded in the comment above this one

@bjartek
Copy link
Contributor

bjartek commented Apr 14, 2023

What if Deposit/Withdraw events contains the uuid of the collection. If they do it is possible to distinguish these from each other if a user happends to have two of them linked.

@joshuahannan
Copy link
Member Author

We're having another working group meeting on Nov 7th to discuss the token standard improvements! We're hoping to finalize most of the changes so we can more confidently say what will be part of the release. You can see the event on the Flow public event calendar.

@j1010001 j1010001 modified the milestones: OKR-23-Q4, C1.0-Launch-M2 Nov 14, 2023
@joshuahannan
Copy link
Member Author

In the most recent smart contract design calls, we made some modifications to the standard proposal:

  • Removed transfer interface, function, and events
  • Removed uses UUID method
  • Removed uuid and type withdraw methods
  • Removed default implementations for getID, get supported types

We also have some things we still need to discuss:

If you have thoughts about wanting to keep some of the things that were removed or want to discuss the proposed changes or anything else about the standard, please join the next token standards discussion on Tuesday Dec 5th to share your feedback! The event is on the flow public events calendar

@joshuahannan
Copy link
Member Author

We had another discussion today about the token standards. We made a few decisions:

  • We will include a default destruction event for NFT:
ResourceDestroyed(id: UInt64 = self.id, uuid: UInt64 = self.uuid)
  • We will add the id field back to NFT to make it accessible in the default destruction event and for better enforcement.

  • We will likely also remove getID because this is now superfluous.

  • We decided to include methods in the default events to say the UUID of all the NFTs and collections involved. For example, withdraw will look like this now:

Withdraw(type: String, id: UInt64, uuid: UInt64, from: Address?, collectionUUID: UInt64)
  • We had a discussion about how we should manage the balance field for Vault, but this doesn't affect the NFT standard.

We'll discuss this topics and more at the next meeting at 9am Pacific on Thurs Jan 11.

Proposed Agenda:

  • addition of id to NFT and removal of getID()
  • Should we have methods to getWithdrawableBalance() or isAvailableToWithdraw(): Bool in the vault, provider, and balance interfaces?
  • Should the token standards be contract interfaces to enforce ViewResolver and createEmptyVault()?
  • How to name entitlements
  • should ViewResolver.resolveView be renamed to resolveContractView?

@joshuahannan
Copy link
Member Author

joshuahannan commented Jan 11, 2024

We had another discussion about the token standards today. here is a summary of what we talked about:

  • Should we include id as a field in {NFT}?

    • People can lie about the ID in the method, which is a risk
    • DECISION: Keep the id field, remove the getID method
  • removing getter functions for standard paths and only including them in metadata views

    • They’re accessible as metadata views so it might make sense to have that be the only way to get them in a standardized way, simplifies the standard
    • Counterpoint: If it isn’t in the standard, people might forget to implement it
    • Probably should remove it since it likely won’t be used much
      *** DECISION:
      • Remove getter functions for standard paths.
      • Add function to get the NFTCollectionData metadata view.
      • remove provider linked path and provider type from NFTCollectionData**
  • Should the token standards be contract interfaces

    • If they are interfaces, we can enforce conformance to ViewResolver and createEmptyVault
    • BUT, we would lose the ability to have standard events and utility functions like burn
    • DECISION: Keep as a contract and find other ways to enforce that people are conforming to ViewResolver
  • should ViewResolver.resolveView be renamed to resolveContractView?

    • Makes it clear that it is for contract metadata instead of resource metadata
    • DECISION: Rename them to getContractViews and resolveContractViews
  • Incompatible upgrades that are part of the new standards

    • Let people know that we are going to have a conversation with the cadence team soon to figure out how to migrate all these weird changes
    • Might be able to include new interfaces that are for legacy NFTs that implement the new ones to make migration easier
    • DECISION: Have a conversation with the Cadence team next week during the next one of these meetings about what the migration difficulties are that need to be reverted or addressed

Topics for next week:

  • Should we keep the default implementations for ViewResolvers as nil?
    • These are really important, so we should make it as difficult as possible to not implement them properly
  • Should we have balance as a field in Vault?
    • Should we have methods to getWithdrawableBalance() or isAvailableToWithdraw(): Bool in the vault, provider, and balance interfaces?
  • How to name entitlements
    • Adjectives, Verbs?
    • Introduce three entitlement with Owner and have a mapping that maps to both withdraw and update?

@joshuahannan
Copy link
Member Author

We continued our token standards discussion today and focused on migration difficulties with the Cadence team.
Most of the discussion involved flagging the parts of the standard that probably needed a special migration.
Here are some notes with decisions bolded:

  • Changing fungible token from an interface to a contract
    • Old interface has to be removed and new contract has to be added
    • Contract has no state, so that won’t be an issue
    • Types defined in the old interface will still work with the new contract
    • Need to check if a contract that implements a contract interface can emit an event defined in the interface
    • Decision: Pending the investigation about events
  • Removing type definitions from standards and contracts
    • Example, removing event definitions, removing an interface definition
    • Should be possible with relaxed upgrade rules in the migration
  • Changing vault, NFT, and Collection, from a resource type requirement to an interface definition
    • Lots of resources are stored as FungibleToken.Vault , but after this upgrade. that resource type will not exist any more and will be {FungibleToken.Vault} instead.
    • Will require a custom migration and there is no way around this because nested type requirements are being removed from the language.
    • Also involves removing the INFT resource interface in favor of the just NFT resource interface
      • Decision: Still include the INFT interface and have it inherit from the NFT interface
  • Changing interface conformance for types in implementing contracts
    • For example, any token contract will have to update their Collection resource from Collection: NonFungibleToken.Receiver, NonFungibleToken.Provider to Collection: NonFungibleToken.Collection
    • Normal upgrade rules don’t allow this
    • Should be possible with relaxed upgrade rules in the migration
  • Updating capabilities like to their correct types
    • Specific examples like Provider capabilities need to have the correct entitlement
    • More generally, capabilities that use restricted types will need to have their types updated to just be the concrete type
    • Cadence team is already aware of this. will require a custom migration
  • Moving MetadataViews.Resolver to ViewResolver.Resolver
    • Updating any capabilities that are linked as MetadataViews to ViewResolver
    • Any stored types that have this type would also need to be updated
    • Also includes ResolverCollection
    • Should be fairly easy to fix in the migration

Next time, we will discuss entitlement naming and default implementations

@joshuahannan
Copy link
Member Author

We had another discussion today that, pending final approval of the code, finalizes the design of the standards!
Here is a summary of the decisions we made:

  • Contract vs interface

    • We will make both standards interfaces now
    • Events that are emitted from implementations do not have the same type, so they are safe
    • Implementations will have to implement a getTypes method and a createEmpty method that takes a type as an argument
    • We will also propose new views for contracts that return information about token types in contracts
  • How to name entitlements

    • nouns are used for "coarse-grained" entitlements (e.g. Storage to access all of the account storage functionality), and verbs are used for "fine-grained" entitlements, like specific functions (e.g. SaveValue to be able to store a value into account storage)
    • Rename entitlements to Withdraw, rename events to Withdrawn and Deposited
    • Add Owner entitlement to NFT standard to map to withdraw and update
  • Default implementations

    • Remove default implementations for all view resolver methods
    • Remove them from every other method that makes sense

    We'll have another chat next Thursday to finish everything up

@joshuahannan
Copy link
Member Author

Looking for some approvals on this FLIP so we can get it merged since it all has been resolved and implemented for a while now!

@joshuahannan joshuahannan changed the title Non-Fungible Token Standard Version 2 FLIP 56: Non-Fungible Token Standard Version 2 Mar 7, 2024
Copy link
Contributor

@bthaile bthaile left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

a few nits

Copy link
Contributor

@sisyphusSmiling sisyphusSmiling left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great!

application/20221219-nft-v2.md Outdated Show resolved Hide resolved
application/20221219-nft-v2.md Outdated Show resolved Hide resolved
application/20221219-nft-v2.md Outdated Show resolved Hide resolved
application/20221219-nft-v2.md Outdated Show resolved Hide resolved
application/20221219-nft-v2.md Outdated Show resolved Hide resolved
application/20221219-nft-v2.md Outdated Show resolved Hide resolved
application/20221219-nft-v2.md Outdated Show resolved Hide resolved
@joshuahannan
Copy link
Member Author

Thanks for the reviews and approvals everybody! I've addressed all your comments and I think we have enough approvals now that I'm okay to merge. 🥳 ❤️

@joshuahannan joshuahannan merged commit d63de52 into main Mar 11, 2024
@joshuahannan joshuahannan deleted the nft-v2 branch March 11, 2024 16:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

9 participants