-
Notifications
You must be signed in to change notification settings - Fork 23
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
Conversation
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.
What I think we can do to fix this:
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
} |
Nice to see this make its way into a flip! A few initial questions/comments to get my bearings: Events
Transfers
Helper methods
|
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.
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 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.
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.
This sounds like the Open-Closed Principle put into practice!
That's a pretty good use of leveraging the type system of the language.
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! 🙏 |
A few thoughts: NFT IDs Collections I think that this interface can be very simple:
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. |
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. |
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.
|
Using uuidI 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? |
@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 |
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 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 I hope this does remove some/all of the confusion 🙏 |
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. |
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.
|
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. |
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!
|
I made some changes to the PR: onflow/flow-nft#126 |
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. |
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. |
In the most recent smart contract design calls, we made some modifications to the standard proposal:
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 |
We had another discussion today about the token standards. We made a few decisions:
We'll discuss this topics and more at the next meeting at 9am Pacific on Thurs Jan 11. Proposed Agenda:
|
We had another discussion about the token standards today. here is a summary of what we talked about:
Topics for next week:
|
We continued our token standards discussion today and focused on migration difficulties with the Cadence team.
Next time, we will discuss entitlement naming and default implementations |
We had another discussion today that, pending final approval of the code, finalizes the design of the standards!
|
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! |
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.
a few nits
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 great!
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. 🥳 ❤️ |
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