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 <#141 (comment)>, 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?

[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: #141 (comment)

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

[arjanvaneersel]

@marc0olo I tend to say that developers should be free to decide implementations. It's their responsibility to communicate that with users. However, since not of us can come up with a use case where a contract would serve different types of tokens, I think that it's cleaner to define this in the standard. From that perspective it might make sense to reconsider how the metadata ADT is structured and limit it to a string or map type instead of having the different options.

I believe that this will lead to cleaner and less ambiguous contracts, thus avoid confusion about where things should be.

[marc0olo]

From that perspective it might make sense to reconsider how the metadata ADT is structured and limit it to a string or map type instead of having the different options.

mhh, I personally still think this is a quite elegant and flexible solution that might be useful for many projects.

we need to decide how to proceed with his. otherwise we are blocked.

@omar-saadoun can you share your opinion about this again? maybe find other NFT experienced people to comment?

[marc0olo]

to make my standpoint about metadata clear again. my proposal is:

• keep flexibility with ADT to support different metadata types in general for contracts
• keep and restrict metadata_type on contract level

note

• got question about ADT => it means algebraic data type (see https://aeternity.com/aesophia/v6.1.0/sophia_features/#algebraic-data-types)

[omar-saadoun]

I think we can offer this elegance through ADT and we will keep the implementation with a reasonable amount of freedom for developers as they can still decide on map structure or JSON if they opt for URL.

We want to offer good options but also keep marketplace's development as simple and anticipatable as possible in this matters.

I think the best way to understand why this is option can be better is ObjectId option for real world things. I would definitely want to see that number/serial/id on contract.

So, bottom line:

• Keep ADT for multiples metadata types
• Keep and restrict metadata_type at contract level.

[marc0olo]

@arjanvaneersel when can we expect the final update so that @thepiwo can proceed with the final review? =)

[arjanvaneersel]

@marc0olo Updated the proposal. I still have one thing on my mind.

I really dislike this:

datatype metadata_type = URL | IPFS | OBJECT_ID | MAP
datatype metadata = Url(string) | Ipfs(string) | ObjectID(string) | Internal(map(string,string))

I dislike it, because someone could select URL on the contract level and then IPFS on the token level or even worse: Internal.

But I also wonder: Do we really need the difference between URL and IPFS? IPFS also generates a URL. URL expects base_url + token_id as outcome. I don't think it's good to force backends to follow a pattern that we impose in a standard. There might be very legitimate technical or business reasons to follow a different URL model

So I would suggest to remove IPFS and have only a URL. The schema of the URL will show whether this is an ipfs, http or even different URL. We can't know what the future will bring. Maybe one day there will be a better alternative for IPFS, should we really update this standard every time when a new technology comes out? I think that's not a good practice.

So what I would suggest to solve both issues is:

datatype metadata_type = URL | OBJECT_ID | MAP
datatype metadata = string | map(string,string)

Of course someone can still select a wrong type, but at least it's clear what the intention is, because only on contract level is clear what the metadata is supposed to be. In the current proposal it could be a wrongly selected type at contract level, as well as on token level, so the intention of the developer never gets clear. So in my opinion storing the type on contract level and only value on token level is much more cleaner than what we had before, because it takes a potential ambiguity away.

[thepiwo]

The goal of the suggestion was to give a hint to a frontend or consumer of that information whats inside. Matching type and data can be verified by the contract (and defined as MUST in the standard). From not thinking about the endpoint consumer side I am also fine with just using map(string, string) then leave the remaining decision on what to put in there to users of the standard.

[marc0olo]

thanks @arjanvaneersel!

the problem with this discussion is that I don't think we get many opinions about that from the community and have to take assumptions.

if you ask me personally I really like what we have. by doing it the way we originally planned every consumer (marketplace or other app) would be able to render the content by knowing what the metadata type stands for (which should also still be possible with your new proposal). I would absolutely distinguish between IPFS and URL for the metadata type. that's just completely different in terms of interpreting it from my view.

@thepiwo can you maybe recall why datatype metadata = Url(string) | Ipfs(string) | ObjectID(string) | Internal(map(string,string)) is needed if we have datatype metadata_type = URL | IPFS | OBJECT_ID | MAP ?

I think every consumer could also interprete this correctly if we define it like Arjan proposed:

datatype metadata = string | map(string,string)

as consumer I would check the metadata type on contract level and then interprete like this (should be defined in the standard of course):

• URL => string
• IPFS => string
• OBJECT_ID => string
• MAP => map(string, string)

is this your point @arjanvaneersel ? I don't see a problem with changing that but need to have confirmed by @thepiwo. but I would favor to keep IPFS as type if possible. for me this makes absolutely sense as explained above

[thepiwo]

@marc0olo I think your ideation was to force the consistent same metadata across the contract.

[marc0olo]

ok this seems historical to me as we first started without having a concrete metadata_type on contract level where we needed the types explicitely in the metadata datatype.

@arjanvaneersel I am fine with changing it to:

datatype metadata = string | map(string,string)

[marc0olo]

seems like we are close to getting the "final" proposal state reviewed 😎 thanks to everybody involved!

[marc0olo]

thanks for updating the proposal @arjanvaneersel ! - I think we are ready to review now 😎

@arjanvaneersel please fork the AEX repository and provide a pull request according to the defined process ;-) ... the AEX number will be define during the review :-)

[marc0olo]

@omar-saadoun @arjanvaneersel @thepiwo we should think about an additional extension for royalties IMO. not sure why we haven't discussed so far.

for sure it would just be an indicator and we couldn't and shouldn't enforce it. but potential marketplaces could read from royalties extension entrypoint and embrace it accordingly.

what do you think?

[marc0olo]

I just consulted with @jyeshe regarding metadata and I think we need to update the proposal and use (int, (string, string)) instead of (string, string) to handle the metadata map for nfts so that we can add the key-values on asset-level rather than on contract level.

opinions? @arjanvaneersel @thepiwo @omar-saadoun

[jyeshe]

I just consulted with @jyeshe regarding metadata and I think we need to update the proposal and use (int, (string, string)) instead of (string, string) to handle the metadata map for nfts so that we can add the key-values on asset-level rather than on contract level.

opinions? @arjanvaneersel @thepiwo @omar-saadoun

Sorry, revisiting the standard it might be not necessary as the mapping to the token_id (int) would be made on the state and the metadata(token_id: int) would return the Map(string, string). Talking about this Map(string, string), is there a Sophia way to convert a map(string, string) to it? Or if the types are compatible might be only an AEStudio issue.

[marc0olo]

yeah the question was rushed. actually we already have what we want I think

[marc0olo]

we had to fix the definition of datatype metadata as it was wrong. we didn't use a map as we expected to use.

see #146
followed by #147 to further improve the clarity and explain the usage

I will shortly adapt the examples but I think I already identified a problem with SDK / calldata-lib in that regards 😒 - need to double-check

here the updated PR and indeed there is a bug we identified. we aim to resolve the problem as soon as possible:

• fix: metadata datatype aex141-examples#10

[marc0olo]

we recently published the first NFT collection example here:

• https://github.com/aeternity/aex141-nft-collection-example

we will shortly come up with a proposal for a new extension called mapped_metadata where we have a first implementation provided here:

• https://github.com/aeternity/aex141-nft-collection-example/pull/9/files

am example of this is deployed on testnet: ct_Fv9d66QTjr4yon9GEuMRc2B5y7Afy4to1ATaoYmpUTbN6DYiP

[marc0olo]

just had a discussion with @thepiwo and I think we should clarify as soon as possible how we wanna deal with events when it comes to minting:

• in AEX-9 the decision was taken to avoid using Transfer event where from-address is the zero address on minting and instead introduce a Mint event

unfortunately we didn't consider this in AEX-141 (yet) and currently have the problem that we defined and use Transfer event for minting like used in ERC-721.

should we change the standard and go the same route as AEX-9 by introducing a special Mint event? also currently it is not defined what the from address in the Transfer event in case of minting should be. in the first examples we used the address of the contract itself (which makes it hard to distinguish between mints and regular transfers) and in the latest nft collection example I switched now to use the zero address like it's done in ERC-721:

• aeternity/aex141-nft-collection-example@9c74494

any opinions on that? @arjanvaneersel @thepiwo @omar-saadoun @jyeshe @mradkov @ghallak @radrow

background:

• the discussion started when I asked to provide a function in stdlib to return the zero-address ak_11111111111111111111111111111111273Yts 😅

[radrow]

Since minting is clearly a different type of action than transferring, I would make it a different type of event.

[marc0olo]

I guess @thepiwo is happy to hear that. rumors say he'd boykott AEX-141 if we go the zero-address route 😱

[jyeshe]

When the indexing service has a single websocket subscription it is easier to track the status of an NFT. With multiple subscriptions it shall sort the events before processing them in order to know the last owner for example (one owner coming from a Transfer and another from the Mint). If we have the Mdw providing the topic subscription, it can work well with different chain events (Mint, Transfer and Burn) as they would already be received from the Node in a sorted way and publish them all aggregated for each connection.

[marc0olo]

alright I think we should propose to go the AEX-9 route and avoid implicit minting using zero-address. I will provide a PR with the proposed changes which introduce Mint and Burn events as soon as possible

[marc0olo]

I have another topic to discuss. I am comparing AEX-9 and AEX-141 a bit more and we also have some difference in regards to handling transfers of tokens.

AEX-9 explicitly differentiates between entrypoints called by the owner itself vs. calls of 3rd parties on behalf of the owner:

stateful entrypoint transfer             : (address, int)          => unit
stateful entrypoint transfer_allowance   : (address, address, int) => unit

should we also adapt this and be more explicit? I'd really love to also get some feedback of you @arjanvaneersel as you were the main contributor to the standard.

AEX-9 introduced a separate extension allowances to handle transfer_allowance and doesn't allow transfers on behalf of other accounts in the default standard

[marc0olo]

I decided to keep the approval stuff like is. I think it is ok and it makes it better understandable for users switching from ERC-721 to AEX-141.

here is the PR for the explicit Mint and Burn events: #148 @thepiwo @jyeshe @arjanvaneersel