Remove tokenOfOwnerByIndex

[fulldecent]

Fixes #69 (ish)

tokenOfOwnerByIndex is incompatible with ERC721a. And tokenWithIndex is incompatible if there will be burning. This PR removes it.

Therefore we lose ERC721Enumerable extension. That's fine.

But we also keep tokenbyIndex and totalSupply. It is fine to implement these functions even though we are not implementing ERC721Enumerable. Nobody says we can't do this.

[Vectorized]

I think tokenByIndex should be removed too for this PR, it's O(totalSupply) with burn function.

The line containing IERC721Enumerable in the supportsInterface function can be removed too.

ERC721A/contracts/ERC721A.sol

Line 143 in 34788d8

interfaceId == type(IERC721Enumerable).interfaceId ||

[fulldecent]

Yes, removed tokenByIndex.

Kept the inheritance in order to qualify this reference in totalSupply: See {IERC721Enumerable-totalSupply}.

[syffs]

@fulldecent I did this #81 a week ago. Waiting for signal to rebase.

[fulldecent]

Right but #81 adds a new extension. I don't think that new extension should exist.

[syffs]

Right but #81 adds a new extension.

It moves IERC721Enumerable to an extension, so that ERC721A is free of it.

I don't think that new extension should exist.

Why ? There're very selected cases in which ERC721Enumerable functions may be useful on-chain.

Always been the same deal with OZ: they moved IERCEnumerable because they couldn't assume that it would never be make sense...
Maybe it should include very clear warnings though: so many devs who don't know better still use OZ ERC721Enumerable even though OZ themselves recommand not to use it.

[fulldecent]

OpenZeppelin contracts includes ERC721Enumerable because it is a feasible extension. It is implemented as O(1).

ERC721Enumerable is not O(1), therefore ERC721Enumerable is not feasible here.

I think the threshold for this repository should be/is: "things that people probably want with ERC721A". Not "dangerous things that we have to warn people not to use and that nobody specifically wants and which are basically not compatible with ERC721A". Therefore the extension should not be included here.

But again, this repo is missing a mission scope, so I cannot argue further on this topic without any source of authority.

[cygaar]

I'm in agreement with @fulldecent here - these functions were originally implemented to market gas improvements, but ERC721A has become primarily a means of minting tokens in batch efficiently. At the current point in time we want to make this contract as safe as possible and prevent someone from doing something dangerous.

We will update the README with a better mission statement shortly

[cygaar]

Approving this, but will wait a day or so before merging so our community members can add their thoughts

[syffs]

OpenZeppelin contracts includes ERC721Enumerable because it is a feasible extension. It is implemented as O(1).

this "feasibility" you mention is highly debatable. Only that the issue isn't security related, but gas related: devs who don't know better keep implementing this extension that takes ~2M gas for minting 20 tokens for no functional reason in most cases. Now this is not a subject for this repo.

Anyway, this is a bit disappointing that someone put the work into changing this as requested here, and that you would choose to submit a new PR instead of commenting an existing one...

FYI this line should be removed:

ERC721A/contracts/ERC721A.sol

Line 9 in 584d073

import '@openzeppelin/contracts/token/ERC721/extensions/IERC721Enumerable.sol';

[fulldecent]

Developers /do/ know better and they have /chosen/ to implement ERC721Enumerable. Very many projects do this. There is nothing wrong this this extension. The tradeoffs have been discussed thoroughly in the standardization process and this is implemented in the first ERC-721 token.

Arguing this in either direction probably won't go anywhere. Especially against me.

---

For OpenZeppelin Contracts, a good implementation of a well-known standard, following the specifications, and achieving O(1) complexity is in scope for that project. Their implementation meets those requirements, that's why it's published in that project.

---

If you want to feel better, watch me embarrass myself by developing a huge PR with the team and then the /real/ decision maker decides they don't have time to review PRs any more.

google/open-location-code#463

I have piles of examples like this.

[syffs]

Arguing this in either direction probably won't go anywhere. Especially against me.

I'll agree with you on this: it is completely pointless !

There is nothing wrong this this extension

You don't know better is all: have a chat with OZ devs before you mention it. You're talking about EIP when I talk specifically about OZ ERC721Enumerable impl being unoptimized gas-wise, nothing related there.

If you want to feel better, watch me embarrass myself by developing a huge PR with the team and then the /real/ decision maker decides they don't have time to review PRs any more.

Nothing closely related to this situation ! This is not about you: open source is about collaboration, disrespecting other's work to flatter your ego certainly isn't the spirit.

Wasted enough of my time already, I'm done here.

[fulldecent]

If there are unresolved criticisms here of the OpenZeppelin Contracts ERC721Enumeration implementation, please open the issue there and feel free to mention me. I'm happy to learn more on this topic.

Also I just want you to know I appreciate this discussion here and that PR that's being discussed. Arguing about a PR's acceptability doesn't change that.

[Vectorized]

Just leaving this here for those interested.

https://www.twitch.tv/videos/1299266697 <- (around 48 minute mark)
^ The steam will only be available for 2 weeks after recording.

I've asked William about his thoughts on ERC721A. Specifically on the enumerable parts, and whether we should include a convenience tokensOfOwner function for small collections (he recommends against it).

Designing standards / libraries to be efficient, future proof, and generalizable requires a lot of thought.

[ahbanavi]

As @Vectorized and @syffs mentioned earlier, Shouldn't these lines be deleted?

ERC721A/contracts/ERC721A.sol

Line 9 in f3205a7

import '@openzeppelin/contracts/token/ERC721/extensions/IERC721Enumerable.sol';

ERC721A/contracts/ERC721A.sol

Line 175 in f3205a7

interfaceId == type(IERC721Enumerable).interfaceId ||

Edit:
I see now it's for the reference, but still are these lines, especially L175 in supportsInterface required?

[fulldecent]

@ahbanavi Thank you, corrected at 4513ef5