Comments on ERC721A

[willisk]

I'd like to start off by thanking you for working on this and allowing contributions from the community.

Here are some of my comments on the current implementation and what I personally would change.

1. Remove maxBatchSize_ in constructor

• keeps implementation as close as possible to the original
• allows max batch logic to be implemented in inheriting class, just like before
• as I see it, there is no need for ERC721A to know the max batch size

...

2. Add functionality for starting index to be either 0 or 1 (this has been discussed).

• even though I believe they should simply start at 0, some libraries like hashlips require them to start at 1 by default. That's why it would be nice to have an easy way of switching this in the contract

3. Add _mint alongside _safeMint (and perhaps advocate this as the standard).

• currently _safeMint checks that caller is a ERC721Receiver n times in a loop
• this adds unnecessary checks, as the standard seems to be to disallow minting from contracts

• also added a require for quantity to be > 0, as otherwise someone could technically be added as an owner (at least temporarily), even though the counter doesn't increase

Other than that, I'm not sure how I feel about supporting the IEC721Enumerable interface, if the functionality is added by inefficient loops that should never be called on-chain. I don't see much point in tokenOfOwnerByIndex at all (it has it's use-case in ERC721Enumerable). Why not replace that with something actually useful, such as listing all tokenIds of an owner (though this could also be considered for an extension).

A last note on 1:
I did actually see a case being made for adding the maximum collection size into ERC721A, because otherwise the inheriting contract will need to read from the chain (get totalSupply and make sure it's below max collections size), as opposed to the mint function handling this for no extra read cost (totalSupply is already present).

I haven't submitted a pull request yet, because I would like to see some reactions to this first.

[cygaar]

1. I'll need to think about this some more, because like you said we want to reduce reads to the chain
2. I'm not a huge fan of this because there are parts of the existing contract that rely on starting index being 0, and adding the option to start at 0 means we have to add more checks at these parts to validate the right index
3. I like this idea, feel free to open up a PR for it

[2pmflow]

Thanks for submitting a detailed writeup of initial thoughts @willisk. Random thoughts below

Re: maxBatchSize- ERC721A in its current form still currently relies on maxBatchSize in its current form in ownershipOf to know how far down in indices to traverse before giving up when finding an owner. Although I suppose that given the assumed mint pattern of serial mints, we could just traverse down until a non-null value and still maintain correctness so I think you're right here

Re: custom first serial- Can definitely see the value of not starting from 0, but as @cygaar mentioned, it would mean some additional code in other places like _exists etc. Feel free to open up a PR though if you're interested and we can review any gas deltas there

Re: _mint vs _safeMint- ERC721A currently isn't opinionated about disallowing minting from contracts (for Azuki launch, those checks were in the inheriting class instead), so _mint shouldn't be advocated as a default, but rather an additional option that callers who know what they're doing can use

Re: maxCollectionSize- I see your point about the gas saving, we chose to remove it because we chose the option of supporting uncapped collections over that saving. May revisit this later if the larger community doesn't agree with that decision, not super tied to it. For now, projects should feel free to fork their own copy if they want to push the collectionSize check to the ERC721A layer

[chiru-labs]

Thanks for submitting a detailed writeup of initial thoughts @willisk. Random thoughts below

Re: maxBatchSize- ERC721A in its current form still currently relies on maxBatchSize in its current form in ownershipOf to know how far down in indices to traverse before giving up when finding an owner. Although I suppose that given the assumed mint pattern of serial mints, we could just traverse down until a non-null value and still maintain correctness so I think you're right here

Re: custom first serial- Can definitely see the value of not starting from 0, but as @cygaar mentioned, it would mean some additional code in other places like _exists etc. Feel free to open up a PR though if you're interested and we can review any gas deltas there

Re: _mint vs _safeMint- ERC721A currently isn't opinionated about disallowing minting from contracts (for Azuki launch, those checks were in the inheriting class instead), so _mint shouldn't be advocated as a default, but rather an additional option that callers who know what they're doing can use

Re: maxCollectionSize- I see your point about the gas saving, we chose to remove it because we chose the option of supporting uncapped collections over that saving. May revisit this later if the larger community doesn't agree with that decision, not super tied to it. For now, projects should feel free to fork their own copy if they want to push the collectionSize check to the ERC721A layer

2pm

[willisk]

I agree regarding the starting index. As I mentioned before, I'm personally not a fan of it starting at 1. Also, for implementing it, ideally it should be an extension (because of the additional logic), however it currently can't be implemented with overrides (unless some functions are made internal virtual).

The reason I mentioned the starting index though is because, I've seen collections face issues, due to not being aware of the hashlips minting engine enforcing a start at 1. This was relatively easy to change in the original ERC721. Now in this version it becomes a bit more involved (though also not too hard). I believe that it would be nice to have a vetted implementation, so that people who want this don't have to struggle on their own to figure this out.

I'd like to open up a PR, but not sure if I should mix in all mentioned changes, or have them one at a time.
The changes I mentioned were:

• add _mint()

• add require quantity > 0

• remove maxBatchSize

• add functionality for setting starting index at 1 (add as alternative implementation?)

• add collectionSize for gas saving (undecided about this, though I've done it in my personal implementation)

• removing ERC721Enumerable's interface (tokenByIndex and tokenOfOwnerByIndex seem useless and shouldn't be called on-chain anyway)

• add tokenIdsOf (can be considered for an extension)

Maybe I'll start with the first 3 and see from there.

[chiru-labs]

Please open a separate PR for each change, regardless of how small it is.

[shintalha]

Willisk is right about this issues, i changed manually the mentioned ones and i recognized the gas fee of deploying contract was lower than the one of original one. I think minting gas fee will be lower too if those changes is applied. Waiting for this, thanks to the team and willisk.