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

What about using ERC721A? #24

Closed
oririechman opened this issue Feb 23, 2022 · 12 comments
Closed

What about using ERC721A? #24

oririechman opened this issue Feb 23, 2022 · 12 comments
Assignees
Labels
enhancement New feature or request

Comments

@oririechman
Copy link

First I want to say thank you!
You are doing amazing job helping the community.

As I know, Azuki as published a better implementation of IERC721 with significant gas savings for minting multiple NFTs in a single transaction.
Why you are not using it?
How much does it cost multiple minting in this contract?

@liarco
Copy link
Member

liarco commented Feb 23, 2022

Hi @oririechman, thank for your feedback.

Why you are not using it?

May I ask you to give me further information about this statement? If you could point the main differences you see between the two contracts that would help me answering.

Thank you very much!

@liarco liarco added the feedback requested To be closed if no feedback is received label Feb 23, 2022
@oririechman
Copy link
Author

My main concern is gas efficiency.
As I haven't compared between the contracts, I shouldn't ask it like this (assumed 721A is more efficient).

From the documentation of Azuki they mention 3 optimizations, I think 2 of them are relevant:

  • Optimization 2 - updating the owner’s balance once per batch mint request, instead of per minted NFT
  • Optimization 3 - updating the owner data once per batch mint request, instead of per minted NFT

Again, I just want to make sure that yours contract is as efficient as 721A

Thanks a lot

@liarco
Copy link
Member

liarco commented Feb 23, 2022

Thank you @oririechman, now I get your point.
Our contract is based on my PR on the original contract by HashLips which was already a "low gas implementation" compared to the "standard" ERC721Enumerable. It was released on Dec 17, while the ERC721A was published on Jan 11 that's why we still didn't consider migrating to it.

I made some rough benchmarks by extending the ERC721A and updating what was causing errors. I then tested both implementations side-by-side:

Minting 1 token:
Screen Shot 2022-02-24 at 00 17 50

Minting 3 tokens:
Screen Shot 2022-02-24 at 00 10 47

So here you can see that our contract is already really efficient compared to an ERC721Enumerable, because minting 1 token is pretty much the same. I admin that minting more tokens costs 31% less with the ERC721A and this is absolutely amazing, but before I plan to migrate I have to do proper research and testing.

Unfortunately I can't give any ETA since there is a lot of stuff going on at the moment. I hope this helps.

@liarco liarco added enhancement New feature or request and removed feedback requested To be closed if no feedback is received labels Feb 23, 2022
@liarco liarco self-assigned this Feb 23, 2022
@gitrevo
Copy link

gitrevo commented Feb 24, 2022

It would be a great enhancement to the contract since cost is the main issue with batch minting.

It would entice users to mint more tokens as it will be nearly the same fee as minting 1 token.

Great explanation Marco and thanks OP for bringing this up.

@liarco
Copy link
Member

liarco commented Feb 24, 2022

It would be a great enhancement to the contract since cost is the main issue with batch minting.

Yes I totally agree, anyway I just want to point out the the current implementation uses 70% less gas for 3 tokens compared to a standard ERC721Enumerable. This is just to say that even the current code, while being less efficient compared to the ERC721A, is still incredibly cheaper compared to anything that has been used by almost all collections up to the end of 2021.

@liarco
Copy link
Member

liarco commented Feb 24, 2022

@oririechman, @gitrevo I opened a new PR (#29) where I'm testing this migration. I would really appreciate some help testing and reviewing the code (if possible). Thank you very much.

@gitrevo
Copy link

gitrevo commented Feb 25, 2022

Hi again Marco,

I've deployed it on rinkeby for our reference: https://rinkeby.etherscan.io/address/0x62d21f60442454a90c119fdc4bb9969fb6b49736

It works like a charm on the minting-dapp 👍

However, I do notice query issue with token ID '0' and '1'. When I tried to query (read contract) token 0 which is the first ID, it produce error message while you can view token holder page which clearly shows token 0 has an owner.

Another is minting via contract (write contract - owner address - mint), it will show that the transaction will likely fail and the gas is extremely high. However, mintForAddress via write contract is working perfectly.

I received another warning while updating whitelist.json stating that the maximum optimize list should be 5000 when I had pasted a 6000 lists.

Please review attached images for your kind reference:
owner-mint-contract
read-token-0
token-holder-0

@liarco
Copy link
Member

liarco commented Feb 25, 2022

Hi @gitrevo, thank you very much for your time testing this and providing with such amazing info. I think I got the problem: the Azuki contract is using token IDs starting from 0 (which is amazing for devs, but quite weird for end-users), so I tried to force some functions to increase the ID by 1 in order to align the result with what users are probably expecting, but it seems like I did a terrible job there... really 😅

I'm gonna work on a fix and I also ask if you can bring any further information directly to the PR (#29) so we can move the discussion there.

Thank you

@gitrevo
Copy link

gitrevo commented Feb 25, 2022

Sure Marco, please merge it there and I shall share new findings (if any) there as well.

Have a great weekend ahead, cheers!

@oririechman
Copy link
Author

Just got to it now and I see that you have progressed a lot!!
Great work!
waiting for the next commit and I will test it as well

@joven1555
Copy link

Is the new commit using 721A? I'm watching this thread closely! Nowhere near as skilled as all of you, but I will help test the 721A on Rinkeby.

@liarco
Copy link
Member

liarco commented Feb 25, 2022

Hi @oririechman and @joven1555,
thank you for your support, as you can see here, my next commit won't work properly until the PR on ERC721A will be merged and published as a package, but I'm gonna let you know as soon as it's ready to be tested again.

In the meanwhile I'm gonna close this issue so we can move the discussion to the PR.

Thank you.

@liarco liarco closed this as completed Feb 25, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

4 participants