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

Plan support AEX 141 (NFT) #629

Closed
thepiwo opened this issue Apr 8, 2022 · 26 comments
Closed

Plan support AEX 141 (NFT) #629

thepiwo opened this issue Apr 8, 2022 · 26 comments
Assignees

Comments

@thepiwo
Copy link
Collaborator

thepiwo commented Apr 8, 2022

In Review Standard proposal:
aeternity/AEXs#141

First plan support, how do we do the integration, what endpoints will be needed? First use-case would be the explorer I'd imagine.

@marc0olo can you provide some input from your experience?
@jyeshe do you wish to support that from MDW side?

@jyeshe
Copy link
Member

jyeshe commented Apr 8, 2022

Definitely yes @thepiwo !

@marc0olo
Copy link

marc0olo commented Apr 8, 2022

let me think about that in detail and come back to this. cannot wait to see that coming! =)

maybe @arjanvaneersel also wants to provide some requirements addressing the API

I also invite @the-icarus to provide feedback here :)

@marc0olo
Copy link

what I think we should have from functional perspective is:

  • search for an NFT contract by name included in meta_info
  • get all holders of a specific NFT contract (+ providing the ID's the holder owns)
  • get all NFT contracts
  • all NFTs owned by a specific account
    • for all NFT contracts
    • for a specific NFT contract
  • transfer history
    • all transfers of a specific NFT contract
    • all transfers of a specific account (across all kinds of NFT contracts)
      • ideally also filterable by a specific NFT contract

the NFT contract info should contain meaningful data:

  • meta_info
  • metadata_type
  • extensions (mintable, ...)
  • total amount of NFTs
  • amount of unique holders

will think about this again. but I think with these functionalities I'd be very happy already =)

ideally we wait until @arjanvaneersel comes up with the examples (reference implementation) because we faced some issues in the AEX-141 proposal. there will be some small changes.

@jyeshe
Copy link
Member

jyeshe commented Apr 12, 2022

This gives a north to start implementing the basic structure on what needs to be stored and retrieved. Thanks a lot @marc0olo

@thepiwo
Copy link
Collaborator Author

thepiwo commented May 3, 2022

As discussed in our last call, it will be good not to make a proprietary support for AEX 141, as we did for AEX 9, but as the inner workings are similar on how we can index that data find a shared approach to reuse our AEX 9 infrastructure.

@jyeshe
Copy link
Member

jyeshe commented Jun 3, 2022

@thepiwo this the error shown by AE Studio about the number of indexed values for the Approval event defined on the spec:
| Approval(indexed address, indexed address, indexed int, bool)
image

@jyeshe
Copy link
Member

jyeshe commented Jun 3, 2022

From Sophia [features] (https://aeternity.com/aesophia/v6.1.0/sophia_features/) docs:

The event can have 0-3 indexed fields, and an optional payload field. A field is indexed if it fits in a 32-byte word, i.e. - bool - int - bits - address - oracle(_, _) - oracle_query(_, _) - contract types - bytes(n) for n ≤ 32, in particular hash

The payload field must be either a string or a byte array of more than 32 bytes. The fields can appear in any order.

By the description above and the error message, the bool field is being implicitly considered as indexed.

The name of the event doesn't count because this works:
Approval(indexed address, indexed address, indexed int)

@thepiwo
Copy link
Collaborator Author

thepiwo commented Aug 1, 2022

@jyeshe can you summarize the latest state here?

@marc0olo
Copy link

marc0olo commented Aug 3, 2022

keep in mind the standard has been updated (as well as the examples) -> https://github.com/aeternity/aex141-examples

@jyeshe
Copy link
Member

jyeshe commented Aug 3, 2022

Thank you for the notification @marc0olo .

@thepiwo the fix on the standard regarding the Approval event won't impact the work done so far. Some signature checking gonna be reviewed for the change on metadata type. After adjusting to this, the feature status will be:

  • search for an NFT contract by name included in meta_info
  • get all holders of a specific NFT contract (+ providing the ID's the holder owns)
  • get all NFT contracts
  • all NFTs owned by a specific account
    • for all NFT contracts
    • for a specific NFT contract
  • transfer history
    • all transfers of a specific NFT contract
    • all transfers of a specific account (across all kinds of NFT contracts)
      • ideally also filterable by a specific NFT contract

For the NFT contract data it is:

  • meta_info
  • metadata_type
  • extensions (mintable, ...)
  • total amount of NFTs
  • amount of unique holders

@jyeshe
Copy link
Member

jyeshe commented Aug 8, 2022

Metadata was validated with 3 possibilities (definitions of metadata in 3 different contracts):

a. datatype metadata = Map(string, string)
b. datatype metadata = String(string)
c. datatype metadata = String(string) | Map(string, string)

using the BasicNFT with this change: aeternity/aex141-examples#8

@marc0olo
Copy link

@jyeshe can you make sure the README is up to date? e.g. I am missing the endpoint / usage for getting the NFTs owned by a specific account there right now which is marked as done already

@marc0olo
Copy link

just for the record -> I adapted your proposed changes in the aex-141 examples repo to follow the record type like defined in the standard definition: aeternity/aex141-examples#9

@jyeshe
Copy link
Member

jyeshe commented Aug 15, 2022

just for the record -> I adapted your proposed changes in the aex-141 examples repo to follow the record type like defined in the standard definition: aeternity/aex141-examples#9

Thanks @marc0olo. Regarding the order of meta info fields, unfortunately the Mdw doesn't have the info to do it unless the source code is parsed or a swap on name and symbol indexation is accepted. However accepting any order with this possible swap would bring benefit by listing the contract as an NFT contract (and the user would have the info to adapt to it looking at the meta info). Should it be implemented or should the Mdw continue to accept only one strict order?

@marc0olo
Copy link

introduced mapped_metadata extension proposal, deployed at ct_Fv9d66QTjr4yon9GEuMRc2B5y7Afy4to1ATaoYmpUTbN6DYiP with (hopefully) meaningful metadata ;-)

also there were new changes, the interface now looks as follows:

contract interface IAEX141 =
    datatype metadata_type = URL | IPFS | OBJECT_ID | MAP
    datatype metadata = MetadataIdentifier(string) | MetadataMap(map(string, string))

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

    datatype event 
        = Transfer(address, address, int)
        | Approval(address, address, int, string)
        | ApprovalForAll(address, address, string)

    entrypoint aex141_extensions : () => list(string)
    entrypoint meta_info : () => meta_info
    entrypoint metadata : (int) => option(metadata)
    entrypoint balance : (address) => option(int)
    entrypoint owner : (int) => option(address)  
    stateful entrypoint transfer : (address, address, int, option(string)) => unit
    stateful entrypoint approve : (address, int, bool) => unit
    stateful entrypoint approve_all : (address, bool) => unit
    entrypoint get_approved : (int) => option(address)
    entrypoint is_approved : (int, address) => bool
    entrypoint is_approved_for_all : (address, address) => bool

see full example repository here:

@jyeshe
Copy link
Member

jyeshe commented Sep 14, 2022

@jyeshe can you make sure the README is up to date? e.g. I am missing the endpoint / usage for getting the NFTs owned by a specific account there right now which is marked as done already

Hi @marc0olo the features from this list are implemented and the README updated accordingly.

@thepiwo
Copy link
Collaborator Author

thepiwo commented Oct 17, 2022

@jyeshe can you summarize what is left todo in terms of what you would consider final support?

@marc0olo
Copy link

let's aim to finalize following PRs. it's still kind of a liquid state unfortunately:

I think most important is that we clarify if Mint event needs to be part of the default interface or not. after discussions with @jyeshe I finally decided to take it out again and provide different events based on the respective extension.

also @mradkov came up with a proposal to be able to indicate the max. cap of an NFT collection which I think should be part of the respective mintable extension as it would be different for unique NFTs and template based NFTs:

  • cap for unique NFTs
  • cap for templates
    • each template already has a defined edition size which limits the amount of NFTs that can be minted for an edition size

plus there is the discussion around the on_aex141_received interface which might cause problems. but I don't think this effects the mdw, right?

@jyeshe
Copy link
Member

jyeshe commented Oct 18, 2022

@jyeshe can you summarize what is left todo in terms of what you would consider final support?

@thepiwo the mdw would need to be adapted to eventual changes for aeternity/AEXs#148 in order close the issue for NFTs and aeternity/AEXs#149 extensions for semi-fungible and mutable tokens would be tackled on separate issues. Is that fine for you @marc0olo ?

@marc0olo
Copy link

it's fine for me to tackle aeternity/AEXs#149 in a separate issue. but we still have some open topics and also @mradkov jumped in and provided some valuable comments in regards to introducing a max cap on collection level and I am not sure where to put that in.

there might be changes in the mintable extensions in that regards. I don't think it makes sense to introduce that elsewhere 🤔

@thepiwo
Copy link
Collaborator Author

thepiwo commented Oct 18, 2022

@jyeshe my question was more in line, if we are finish with adaption of the changes, what else is there to do before we can call AEX-141 support "final"?

@jyeshe
Copy link
Member

jyeshe commented Oct 18, 2022

@jyeshe my question was more in line, if we are finish with adaption of the changes, what else is there to do before we can call AEX-141 support "final"?

For current state of aeternity/AEXs#148 these would be missing:

  • handle Burn event
  • in the future for contract validation, check if total_supply matches the amount of minted tokens

Then we would have the aeternity/AEXs#149 complements.

@thepiwo
Copy link
Collaborator Author

thepiwo commented Oct 18, 2022

@jyeshe whats our goal behind all the contract validation? Wouldn't it generally be better to index more lean as long as the information we need to index is correct?

@jyeshe
Copy link
Member

jyeshe commented Oct 18, 2022

Wouldn't it generally be better to index more lean as long as the information we need to index is correct?

That's a good question where would be the boundary between contract validation and contract verification. For the total_supply as it's dynamic could be for verification. So for a lean indexation we could care only about with Burn.

@thepiwo thepiwo added this to the 2022-10-21 Planning milestone Oct 20, 2022
@thepiwo
Copy link
Collaborator Author

thepiwo commented Oct 25, 2022

As long as specification won't change from today to finalisation we can consider it done.

@marc0olo maybe you can check the current endpoints and see if anything is missing for you?

@jyeshe
Copy link
Member

jyeshe commented Jan 11, 2023

Closing as all requirements were implemented. @marc0olo I would be glad to implement other new features you have in mind One that I miss is to expose the metadata as just simply an URL, a map with the properties or the ID according to the metadata type.

@jyeshe jyeshe closed this as completed Jan 11, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants