search

aeternity / AEXs

Aeternity expansions repository — application layer standards
10 stars 25 forks source link

introduce explicit mint and burn events for aex-141 #148

Closed marc0olo closed 2 years ago

marc0olo commented 2 years ago
marc0olo commented 2 years ago

Looks good to me adding the Burn datatype. Maybe only simplify the Burn a bit more by removing the from to prevent inconsistencies when the from doesn't match the owner (or approved account).

that's an interesting topic. depends on if we allow the approved operator to burn a specific token. my suggestion is to adapt burn entrypoint and add from param which we also have in the transfer entrypoint already. then from always has to be the owner of the NFT.

I found some issues with the document in general. will make the PR a draft again and include more updates here. want to polish it first.

marc0olo commented 2 years ago

@jyeshe this doesn't include the mapped_metadata extension yet. will be a separate PR if implementation is finished and accepted.

I summarized all the changes in the description of the PR

marc0olo commented 2 years ago

would also be cool to get feedback from @arjanvaneersel on this one 😅

marc0olo commented 2 years ago

A rename is needed for 'aex141_extensions' and in the swap() instead of 'amount' would say token ids.

ahh saw the mistake with aex9_extensions - fixed :)

regarding the swap() do you think it should return a list/set of token ids, right? do you mean check_swap() specifically here? because swap() doesn't return anything right now

jyeshe commented 2 years ago

A rename is needed for 'aex141_extensions' and in the swap() instead of 'amount' would say token ids.

ahh saw the mistake with aex9_extensions - fixed :)

regarding the swap() do you think it should return a list/set of token ids, right? do you mean check_swap() specifically here? because swap() doesn't return anything right now

swap description would be changed to: Burns the whole balance of Call.caller and stores the same token ids in the swapped map.

marc0olo commented 2 years ago

swap description would be changed to: Burns the whole balance of Call.caller and stores the same token ids in the swapped map.

amount of tokens for check_swap is fine from your perspective? set of token_ids could be too much given the amount of NFTs swapped đŸ€”

marc0olo commented 2 years ago

One final renaming and would add the token_id besides data to the on_nft_received

ok I missed that one, sorry. actually this was anyway sth. I wanted to ask again. can you provide an example where this is useful? actually I removed everything mainly because from now doesn't make sense anymore due to explicit Mint event and avoiding the zero-address.

but I am curious if token_id and to could be relevant in some cases. I might add both again if you think it makes sense

jyeshe commented 2 years ago

One final renaming and would add the token_id besides data to the on_nft_received

ok I missed that one, sorry. actually this was anyway sth. I wanted to ask again. can you provide an example where this is useful? actually I removed everything mainly because from now doesn't make sense anymore due to explicit Mint event and avoiding the zero-address.

but I am curious if token_id and to could be relevant in some cases. I might add both again if you think it makes sense

It's possible that when it subscribes and is notified it wants to do sth with the token using the id. On gaming a receiving contract might manage a character development.

marc0olo commented 2 years ago

@jyeshe made a small change to check_swap() which now returns a list of tokens instead of the swapped amount

marc0olo commented 2 years ago

does anything speak against merging this? @jyeshe @thepiwo

example NFT contracts reflecting this PR are updated here:

jyeshe commented 2 years ago

does anything speak against merging this? @jyeshe @thepiwo

example NFT contracts reflecting this PR are updated here:

As you've mentioned the current swapable adds complexity to the examples and to the standard. I would remove it for now as most collection creators don't need to read and understand it.

marc0olo commented 2 years ago

As you've mentioned the current swapable adds complexity to the examples and to the standard. I would remove it for now as most collection creators don't need to read and understand it.

you are referring to the default collection example, right? I can and will remove it from there if we think this is always being copied and we want to avoid unnecessary usage of it.

however, do we agree now on the definition of swappable extension based on that example?

I would then make an explicit swappable in the https://github.com/aeternity/aex141-examples repository (which also has to be updated because it will be outdated when we merge this)

can we agree on that? @jyeshe @thepiwo ? =)

jyeshe commented 2 years ago

As you've mentioned the current swapable adds complexity to the examples and to the standard. I would remove it for now as most collection creators don't need to read and understand it.

you are referring to the default collection example, right? I can and will remove it from there if we think this is always being copied and we want to avoid unnecessary usage of it.

however, do we agree now on the definition of swappable extension based on that example?

I would then make an explicit swappable in the https://github.com/aeternity/aex141-examples repository (which also has to be updated because it will be outdated when we merge this)

can we agree on that? @jyeshe @thepiwo ? =)

Default collection and since it's not possible to have a straightforward implementation, I would remove it from the standard as well. But leaving in the standard as is it is ok for the indexer.

marc0olo commented 2 years ago

it is an extension to the standard. it's not there by default.

if we remove it now the whole discussion was completely useless 🙈

let's keep the extension. if I personally would launch an NFT collection now I would definitely use swappable as we're very early here and things might change (again)

jyeshe commented 2 years ago

After having followed some messages indicating that the discussion hasn't reached an end, it appeared it could be removed and added later or not (specially due to the continuation/stateful behavior complexity shown by the example).

But as I have mentioned before, even though I don't agree with the extension as is, I don't see it as a blocker for the Middleware (like also said before, it will require more rules for contract validation, including to follow multiple call states but not impossible to track and signalize when the contract doesn't follow the standard).

marc0olo commented 2 years ago

@jyeshe @thepiwo after removing the swappable extension can we approve and merge the proposed changes? :)

marc0olo commented 2 years ago

I updated the example accordingly and removed swappable from recommended contracts:

marc0olo commented 2 years ago

I think we have to include the from again to on_nft_received to be able to store who is the previous owner to send the tokens back to in case needed (e.g. claim the token back from a marketplace contract).

it might not be relevant in many cases and with data attribute there would be another way to handle it and provide the address of the original owner.

but providing the info explicitly can make life of devs easier 😅

the respective contract which the token belongs to Call.caller. this is important to know which contract to use to handle token related actions

jyeshe commented 2 years ago

True, might be useful for some liquidity market based on NFT lending.

marc0olo commented 2 years ago

@jyeshe @thepiwo updated the example again. should reflect the state of the nft-collection example again:

marc0olo commented 2 years ago

https://github.com/aeternity/AEXs/pull/149 can also be checked and is based on the PR here. will be merged afterwards into master then. needs a change of the base

jyeshe commented 2 years ago

@jyeshe @thepiwo updated the example again. should reflect the state of the nft-collection example again:

* https://github.com/aeternity/aex141-nft-collection-example/tree/master/contracts

  * also added tests for:

    * approvals
    * operators
    * aex141receiver

The protected parameter is not an argument on the on_aex141_received entrypoint. Would it be a generic one for ae contract methods? https://github.com/aeternity/aex141-nft-collection-example/blob/a959d2dc4ceb5a8e1d125214d1bdd6c66d8889d4/contracts/CollectionUniqueNFTs.aes#L174 https://github.com/aeternity/aex141-nft-collection-example/blob/52bac76ff23510f3e1ddbc497fe9f5d7081a258e/contracts/recommended/CollectionTemplateEditionNFTs.aes#L215

marc0olo commented 2 years ago

The protected parameter is not an argument on the on_aex141_received entrypoint. Would it be a generic one for ae contract methods?

the protected param is a Sophia feature and an implementation detail that should not be part of the standard IMO. it prevents the contract call from reverting if there is an error at the remote contract, see https://docs.aeternity.com/aesophia/v7.0.1/sophia_features/?h=protected#protected-contract-calls

marc0olo commented 2 years ago

Typos:

  • L18 "Therefore, " comma missing
  • L97 "extention"
  • L241 "wether"

fixed: https://github.com/aeternity/AEXs/commit/07261c056b5b6003749283b21a378a0a7ba7e194

marc0olo commented 2 years ago

@jyeshe @thepiwo please one final check for following changes:

marc0olo commented 2 years ago

@jyeshe now up to date. please check :)

marc0olo commented 2 years ago

@thepiwo can we merge this one already? did you check?