Non-Fungible Token standard

[arjanvaneersel]

Discussions-To:

https://forum.aeternity.com/t/aeternity-nft-token-standard/9781/16

Content URL

https://github.com/aeternity/AEXs/blob/master/AEXS/aex-141.md

[mradkov]

Can you update the specification doc to also have a full interface code block for easier readability?

[marc0olo]

thanks for your proposal, @arjanvaneersel!

I agree with @mradkov it's currently a bit hard to read.

suggestions / thoughts from my side for the moment:

• use wording extensions instead of secondary interface
• provide a getter to know what extensions are implemented, see how it is done for the AEX-9 token:
  • https://github.com/aeternity/AEXs/blob/master/AEXS/aex-9.md#aex9_extensions
• I think mintable should also be an extension as there might be cases where minting is not required.
• don't rephrase "Event" in all the events, e.g. use Transfer instead of TransferEvent
• I think the MetaInfo extension should somehow be combined with the TokenData extension
  • I like that you thought about a distinction for the data type but I am not sure if this just complicates things here in the basic NFT interface
• I would like to have an entrypoint that allows to return all tokens of a specific address
• maybe we should also think about a "swappable" extension here like we did for the AEX-9

I think it makes sense to be compliant to ERC-721 for the most common and basic functions. if some things don't make sense we could drop them or if we have other ideas we could add them.

opinions about that?

[arjanvaneersel]

@mradkov @marc0olo Included a full code block of the interfaces in the doc, as well as in this issue.

Thanks for the suggestions.

@marc0olo The idea of MetaInfo is to be similar to the Fungible Token. TokenData is typically for the NFT standard. For that reason I didn't combine them. Also TokenData is an option, not every NFT implementation might need it. Therefore I'm also having some doubts whether it really belongs in MetaInfo if MetaInfo is to be considered a standard by itself and to be similar to the FT. @mradkov, What is your opinion? I implemented an FT compatible MetaInfo based on your suggestion.

I agree with all other suggestions of @marc0olo. I will meanwhile dive into AEX-9 to see how things were done there.

[marc0olo]

personally I am not sure if a NFT (contract/collection) needs a symbol. IMO a name of the collection is sufficient. also saw some discussions about that for ERC-721 back then.

I think it's way more important to think about how we want to deal with the "real" metainfo (properties of the specific NFT ID)

[marc0olo]

we should keep the discussion alive here. will hopefully find some time again to have a deeper look.

@arjanvaneersel are you planning to provide an update or are you waiting for further responses?

@thepiwo maybe you can also share some thoughts here? would appreciate it :)

[marc0olo]

@arjanvaneersel btw I wouldn't recommend to duplicate the proposed interface on various places (issue description, separate file, ...)

we should discuss a proposal which is written in one specific place IMO. the request of @mradkov, when I understood correctly, was about improving the readability of https://github.com/appinesshq/ae-nft/blob/main/AEX-xx.md (@mradkov correct me if I am wrong 😅 )

[arjanvaneersel]

My planning was to start implementing the first batch of ideas from next week. Think everybody then had a reasonable amount of time to share ideas.

On Thu, Sep 23, 2021, 16:26 Marco Walz @.***> wrote:

we should keep the discussion alive here. will hopefully find some time again to have a deeper look.

@arjanvaneersel https://github.com/arjanvaneersel are you planning to provide an update or are you waiting for further responses?

@thepiwo https://github.com/thepiwo maybe you can also share some thoughts here? would appreciate it :)

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/aeternity/AEXs/issues/141#issuecomment-925815654, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABKAWCZLKCWA5FDIRSQAERDUDMTJPANCNFSM5D2RT6XQ .

[thepiwo]

I'd say to make the code and readme style more inline with other ae contracts and AEXs:

• remove the @param comments
• use // comments instead of ///
• make parameters without leading _
• compare readme formatting with AEX-9 for endpoints
• use MUST/SHOULD descriptions in text (MoSCoW method)
• document events where they are emitted
• alias default types for better readability
• unify duplicate requires in single functions
• minters would be more performant as map(address, bool)
• safe_mint and safe_mint_with_token_data do duplicate require checks
• remove ...Event from event names
• in get_approved either remove the require or force lookup

Whats the reason to have the safe_ methods? Please include an example for onNFTReceived. Why forbid owner to be removed as minter? Why default to 0 for balance_of? Why does burn make the NFT contract owner of the NFT, not actually burn? Why consider 0 address for approvals? Why does minting need a id passed? Can you explain how approvals work and what they are used for? Tests should cover all functions, requires and edge cases known (for final implementation).

Thats the things I found for now, haven't checked all function calls and checks in detail.

[marc0olo]

thanks @thepiwo! currently you shouldn't care about the actual implementation. we should agree upon the interface before digging deep into the implementation details.

but anyway there are interesting questions.

what's your opinion about having mintable as extension? IMO it shouldn't be part of the default standard

[marc0olo]

@arjanvaneersel any update here from your side? we decided in our workshop that @thepiwo and I want to actively support you here and get this standard and the implementation ready to be reviewed and tested as soon as possible. do you have some time next week to have a call to discuss this? we can do that on discord ;-)

[marc0olo]

Summary (Call between @marc0olo and @thepiwo)

• don't copy Ethereum/Solidity code-style, @thepiwo can help here
• transfer / mint vs. safe_transfer / safe_mint
  • we should probably only define and allow safe transfers to avoid sending tokens to a contract that doesn't support the token standard
  • is there a reason why we need "non-safe" transfers or why we should keep them?
• harmonize / consolidate the naming
  • transfer_single / transfer_all
  • approve_single / approve_all
  • revoke_single / revoke_all
• we could also introduce a batch transfer in our NFT standard
• metadata
  • on collection level:
    • name
    • abbreviation
  • on item:
    • see discussion below, introduce ADT
• define extensions similar to AEX-9:
  • mintable (only owner, ControlledMinting not required from our perspective)
    • do we need to provide an id for minting a new token? what use cases exist here? couldn't we just increment the id for each mint automatically?
  • burnable
    • burn should burn and set the contract as owner like in current state of implementation

Metadata

We definitely need to discuss how to deal with metadata on item level:

• should we use an IPFS hash for the metadata?
• should we point to an URL?
• should we store metadata in the contract directly?
• which format should we use? json like ERC-721?
  • what attributes should the standard define? => can be defined later on, not crucial for the NFT standard implementation

Maybe use an algebraic data type (ADT) for metadata? Simple example:

@compiler >= 4

contract NFT =

  datatype metadata = Url(string) | Ipfs(string) | Internal(map(string,string))
  type state = map(int, metadata)

  entrypoint init(): state = {}

  stateful entrypoint mint(meta : metadata) = put(state{[Map.size(state)] = meta})
  entrypoint get_metadata(id : int): option(metadata) = Map.lookup(id, state)

[marc0olo]

just had some discussion with @omar-saadoun about that. the metadata topic is really interesting.

in the end we both came to the conclusion that it's definitely desired to have following possibilities:

• base_url for the whole collection where the id points to the specific metadata of the token (like known already for the ERC-721)
  • pro:
    • "trust" for people in a common scheme
    • easier to handle ?!
  • cons:
    • the URL can be obsolete at some point of time (maybe not an issue at all as the NFT would probably also obsolete then)
    • data behind the URL can be changed and is thus mutable
• ipfs_hash for each unique token
  • pro:
    • immutable data
    • services consuming the data can use their own (or preferred) IPFS gateway
  • cons:
    • ???

the question is how to ideally reflect this in the contract. maybe we even want to have the possibility to add metadata "internal" in the contract (e.g. stored in a map) like mentioned above. @thepiwo any suggestions here? especially as we want to avoid having a custom URL for each NFT. it totally makes sense for the URI to introduce a baseUrl instead of having a unique URL for each NFT.

@omar-saadoun please edit / comment with additional feedback

[thepiwo]

@marc0olo I agree, using base_url makes sense as long as it is changeable by an administrator, each unique token id will stay the same. Putting additional data in ipfs per unique token makes sense to have optionally I think.

[marc0olo]

yeah. but when assuming we use a base_url my question is how would we best reflect this e.g. in the ADT (if we aim to use it like e.g. mentioned above).

should we use Url(bool) instead of Url(string) then? but if the type is Url why would I need the bool here then? I am just thinking out loud at the moment 😅 - wondering how it makes sense for the services/interfaces to consume this kind of data best from the contract

[marc0olo]

Summary Call between @arjanvaneersel and @marc0olo

• we agreed that the transfer and mint method will always be a safe one (fails when trying to send to a contract that doesn't support it)
• we agreed that for minting we always increment the id's and don't need to pass the id as param
• we agreed to introduce mintable as extension
• we agreed to use burnable as extension
• we agreed to provide batch_transfer as extension
  • should we allow to transfer different ids to different recipients? (still open question)
• ControlledMinting extension was proposed to @arjanvaneersel from somebody else in the community
  • e.g. somebody wanted another contract to be able to mint tokens
  • we agreed also in this call that we don't really need this and can just set another contract as owner where you can clearly define who can manage certain activities for the contract
• we shortly mentioned the swappable extension
  • haven't really discussed, but we agreed it can make sense to introduce it
• metadata
  • ADT is a good solution, following types should be defined
    • Url
    • Ipfs
    • ObjectId (e.g. for real world objects that have a specific identifier)
    • Internal map (e.g. for certificates where only a few data fields are needed - NO PERSONAL DATA)
  • the type should be defined on contract level, not on item level

How to deal with metadata

// on contract level define what metadata_type the contract uses
datatype metadata_type = URL | IPFS | OBJECT_ID | MAP

// on item level set the metadata for the metadata_type (only allow the correct one)
// for URL we don't need to provide anything 
datatype metadata = Url(string) | Ipfs(string) | ObjectId(string) | Internal(map(string,string))

Init function (deployment)

stateful entrypoint init(type: metadata_type, base_url: option(string)) = {
        metadata_type = type
        // check for and set base_url if metadata_type is URL
        ...                                  }

Mint function

stateful entrypoint mint(meta: option(metadata)) = {
       // check for correct type
       // for metadata_type URL we don't need to pass anything as the `base_url` + `id` forms the url
       //    => thus optional
       // for Ipfs, ObjectId and Internal metadata needs to be provided
       // increment id automatically
       ...
                                            }

Get metadata

entrypoint get_metadata(id : int): option(metadata) = Map.lookup(id, state)

• in this case we would need a check for metadata_type URL and in case of URL we need to return the base_url concatenated with the token id
  • for every other type (Ipfs, ObjectId and Internal) we could just return it as it needs to be stored for each token
• we were discussing if this is conventient or if we should store the complete URL for each token id (where we would have unnecessary storage costs)

[omar-saadoun]

Could you elaborate more about swappable @marc0olo @arjanvaneersel ?

[marc0olo]

Could you elaborate more about swappable @marc0olo @arjanvaneersel ?

Kind of like it's defined in the AEX-9 fungible token standard already, see https://github.com/aeternity/AEXs/blob/master/AEXS/aex-9.md#extension-swappable-swappable.

We could maybe also define different swap target chains and respective contract addresses like e.g. AETERNITY with ct_..., ETHEREUM with 0x..., POLYGON with 0x... (and others) so that consumers know the new chain and respective address to be used for the contract.

It's basically an extension that could be used if the developers want to consider a migration to a new chain and/or contract. At least from my understanding.

Maybe @thepiwo and @mradkov can elaborate a bit more on the thoughts about this for AEX-9 back then.

But we haven't put much thoughts into this right now. Not sure if it's that important. But definitely an interesting topic that might be useful for some people.

[thepiwo]

the goal of swappable is to provide a mechanism to swap AEX-9 to a different token standard in the future

[arjanvaneersel]

Proposal is updated. Please share your thoughts.

[marc0olo]

Hi @arjanvaneersel,

thanks for the update! I just had a very quick look and will dig in deeper during the day.

Following comments right now:

• I think we should keep the map with the token holders separate from the meta_info and provide a separate entrypoint to get that information
• I am wondering if we could/should provide 2 different methods for the batch_transfer extension:
  1. like you proposed to send different tokens to a specific address
  2. a more flexible solution to send different tokens to different addresses
     • here we could even provide the possibility to define different from addresses (which could have approved the Caller)

What do you guys think? cc @thepiwo @mradkov

[arjanvaneersel]

@marc0olo It's not a map with token holders, but a map with token data in meta_info. I remember from the discussion that most people, including you according your comment of the 14th of September, found it more logical to have this data in meta_info than outside of it, as it was before.

[marc0olo]

ahh okay, got it. I remember

I would propose to change the naming here then as follows and I still think the token_data is specific to each token and shouldn't be included in the general metainfo.

Currently

record meta_info = 
        { name: string
        , symbol: string 
        , base_url: option(string)
        , metadata : metadata_type
        , token_data: map(int, metadata)}

Proposal

record meta_info = 
        { name: string
        , symbol: string 
        , base_url: option(string)
        , type : metadata_type}

Note:

• removed token_data and renamed metadata to type

[arjanvaneersel]

@marc0olo I agree with you and what you describe is how I implemented it initially. Amended the proposal and removed token from meta_info. I don't know why I called type metadata in the first place. Must be a mistake. Changed that as well in the proposal.

[marc0olo]

one thing more I just observed when having a quick loog again. I think we currently miss the bool to indicate approval/revocation in the single Approval event:

• Approval(indexed address, indexed address, indexed int)

[thepiwo]

@marc0olo can you elaborate why a metadata_type is needed if we use an ADT for metadata anyway?

[thepiwo]

@arjanvaneersel thanks for your work, I think the overall proposal looks good now. If we all agree that the current state of the proposal (excl. implementation) is good now I would like to make an in-depth review where I again check every type, throw and so on.

[arjanvaneersel]

@thepiwo The idea of metadata_type is that the type is determined at contract level instead of having different tokens with different metadata. The ADT on metadata level is mostly there due to the different kind of types that the metadata types require. Perhaps it's better to abstract metadata down to simply string or map, because I can imagine that how it's formulated now might be confusing. Any thoughts?

[thepiwo]

@arjanvaneersel yeah I understand, I think the ADT as metadata to hold content is good and can be extended by third parties. I am unsure if we want to introduce a restriction.

Pro (for restriction):

• less need to think for NFT-AEX deployments
• uniform metadata across one NFT

Con (against restriction):

• maybe there is a use-case for different metadata types in same NFT
• checking restriction costs gas

[marc0olo]

@marc0olo can you elaborate why a metadata_type is needed if we use an ADT for metadata anyway?

as @arjanvaneersel already pointed out it is mainly for indicating to consumers (marketplaces, ...) what kind of metadata is being used for the NFT contract.

personally I think it's good this way but yeah, open for suggestions

• maybe there is a use-case for different metadata types in same NFT

@arjanvaneersel and I discussed this and we couldn't think of a usecase where people would want different types of metadata. in general this would unnecessary complicate the logic on consumer side IMO.

• checking restriction costs gas

not sure about the implications, but if we wanna support multiple metadata types via ADT I think we need this. also we are already enforcing the NFTReceiver check with this standard which is also sth that costs gas and might be unnecessary for some. though I also talked to @arjanvaneersel regarding this. both of us never used the "unsafe" transfer in Solidity NFT contracts. so I guess this is also fine here.

If we all agree that the current state of the proposal (excl. implementation) is good now I would like to make an in-depth review where I again check every type, throw and so on.

from my side the only thing that is open and got no answer is if we wanna provide another method for the batch_transfer extension that could be more flexible, see https://github.com/aeternity/AEXs/issues/141#issuecomment-964037787

[marc0olo]

to second that:

• personally I am pro restriction

please react with 👍 (pro) or 👎 (against) to this comment :-)

[thepiwo]

I am indifferent to the restriction, either way can result in issues in the future.

[marc0olo]

question is how we can collect as many opinions as possible about this. IMO we are already very flexible with the ADT in general. this is way better than what ERC-721 provides.

I think if we cannot think of a specific usecase where I would want to mix up metadata types in one single contract we should go for the restriction.

so far I wasn't able to think of one.

[thepiwo]

For the batch_transfer topic we should also consider how implementation regarding that works, iteration over lists will result in out of gas issues. I would propose to keep the standard as clean as possible.

Maybe we can just make one function for batch_transfer that just takes a map(int, address) / list(int * address) to make any token to any address the default, which can still be used to send all tokens to the same address. Otherwise I fail to think of a use-case to batch-send multiple tokens to different receivers at once (when still having gas limitations in mind, so it can't be the default for automatic transfers, meaning remote-calling single transfers in a batch is effectively the same). So I don't think its worth to implement it and have it in the standard, over a simple batch send to single address, even for which I don't know the current use-cases in NFT world.

[marc0olo]

right. I think we could keep it like it is in the current proposal then for batch_transfer. though I also like the proposal to just take a map(int, address) / list(int * address) to have more flexibility. @thepiwo what do you think about gas costs implications if we would use this instead of the current proposal?

generally I can only think of airdrops or sth. like that right now if you want to send multiple NFTs to different addresses. in case of airdrops you'd need to consider out of gas issues anyway and then it's probably better to have a service triggering the distribution step by step. for smaller amounts such entrypoint could be useful anyway.

[omar-saadoun]

I understand we are supporting or willing to be as erc 721 similar as possible but what about considering to be half way with collections?

Probably it won't sense for a while to have another standar because of gas fees so low being already quite efficient but if we are considering a batch_transfer which would be great we should also consider batch_mint taking map(int, address) / list(int * address).

ie. If I'm doing some airdrop or launching a complete collection I would first use batch_mint instead of mint and then doing batch_transfer.

Side note for collections or multi-token support: We could say it can be supported through contract factory + NFT-AEX . Correct?

[marc0olo]

I understand we are supporting or willing to be as erc 721 similar as possible but what about considering to be half way with collections?

multiple collections in a contract are not considered in this proposal. we don't want and need to be compliant to ERC-721 here, but we want to keep it as simple as possible while providing flexibility e.g. in terms of metadata.

Side note for collections or multi-token support: We could say it can be supported through contract factory + NFT-AEX . Correct?

you could use a contract factory to achieve that, yes.

we should also consider batch_mint taking map(int, address) / list(int * address).

agree

ie. If I'm doing some airdrop or launching a complete collection I would first use batch_mint instead of mint and then doing batch_transfer.

yes, I think then in general map(int, address) / list(int * address) makes most sense to use then and be consistent there. and I agree that batch_mint should also be part of an extension then. maybe also batch_burn then.

I am wondering how to place that in the extensions. in theory we would need another 2 extensions then or 1 combined one:

• a) batch_mint and batch_burn extension
• b) batch_mint_burn as single extension

---

we are coming in an area now where it starts to become more complex. but as these are all extensions I think this could be fine. question is if we should reconsider these batch_mint and batch_burn extensions at a later point of time or now directly.

[thepiwo]

@marc0olo if introduce list iteration in the standard, users will definitely run into out of gas issues at some point and will thus have to implement an alternative/special-case for this use-case anyway

[marc0olo]

@marc0olo if introduce list iteration in the standard, users will definitely run into out of gas issues at some point and will thus have to implement an alternative/special-case for this use-case anyway

so you propose to avoid it and not even including it as extension?

right now we would include it in an extension of the standard, yes. and yes, it would cause out of gas issues. but experienced devs could analyze and limit the amount of NFTs used in a batch action.

I mean we can also ditch the batch actions completely. also from the extensions. or we decide to introduce the extensions at a later stage if there is demand for it

[ErkanKanden]

Not an expert here, but if we have to decide between user friendliness/smoother experience or advanced functionalities like batch actions, I'd go for ease of use. This way we can get new developers to try out the new NFT standard more easily and not have them frustrate too much over out of gas issues

[thepiwo]

right now we would include it in an extension of the standard, yes. and yes, it would cause out of gas issues. but experienced devs could analyze and limit the amount of NFTs used in a batch action.

@marc0olo yes, but experiences devs can also build this using remote calls and a parent contract. Inexperienced devs will build some product that bricks itself.

[thepiwo]

same with the metadata, I am indifferent here, just analyzing and explaining the risks

[marc0olo]

@marc0olo yes, but experiences devs can also build this using remote calls and a parent contract. Inexperienced devs will build some product that bricks itself.

that's true, agree. so I would be fine with excluding batch transfers. and as already said I think we could introduce a respective extension at a later stage if there really is demand for it

same with the metadata, I am indifferent here, just analyzing and explaining the risks

here I kind of disagree. I don't see any risk and it's just much more convenient if it is restricted. still not sure if there exists a usecase where you would like to mix different metadata types in the same NFT contract

[thepiwo]

@ErkanKanden the problem is with this kind of ease-of-use it nice as long as it works, but when it doesn't any more shits on fire. E.g. I am an inexperienced dev, build an NFT marketplace contract, make use of batch transfers from my marketplace contract using the nft contract. I gain 1000 users, suddenly the marketplace is not working anymore. The inexperienced dev find out its because of too many transfers in batch, but then is to late to patch the marketplace contract as its already deployed without thinking of the possible risks using that external nft contract before, nfts stay locked in there forever. Of course a security review would have prevented that, but in any case, no one can use the feature of batch transfers from a contract without building a fallback, if you build a fallback anyway into your contract you have no benefit of using batch transfer in the first place. Only backend/wallet/trusted services can gain from this, others have to treat with lots of care.

[thepiwo]

Similar issues cost ethereum projects 100s millions in losses, this is not just something that might happen but probably never will. If someone knows about an issue in a third party contract like this it can be actively exploited by an attacker to provoke that bug.

[thepiwo]

With all of that I'm not saying we shouldn't have such feature, just treat anything that includes iteration with utmost care when proposing a standard, for people to rely on.

[marc0olo]

yes, the more I think about the batch thing the more I am on your page. we should avoid it in the standard.

[thepiwo]

When we avoid it we should make sure that batch actions will still be possible from a third party contract, and e.g. provide an example for that.

[marc0olo]

@arjanvaneersel du you agree to avoid iterations in the standard and providing examples for batch actions in third party contracts?

• minting
• transfers
• burning

[arjanvaneersel]

@marc0olo I agree, let's not make batch transfers a part of the standard, but give some examples how one can do it in 3rd party contracts. I already incorporated your earlier suggestion. So I'll update the proposal by removing the batch actions and then @thepiwo should be good to go with his final review.

[marc0olo]

@arjanvaneersel also consider this comment please: https://github.com/aeternity/AEXs/issues/141#issuecomment-964907539

what's your take on the restriction discussion with defining the metadata type on contract level? do you see any usecase that requires not restricting it?

currently I personally am pro restriction, still