Optimize ERC721 (depends on #1030)

[abandeali1]

This PR depends on #1030 (happy to drop that commit if it's an issue).

🚀 Description

There are a ton of redundant checks and state reads in the current ERC721 implementations. I'm assuming that a lot of this is because everything is broken up into multiple function calls. Even though they read from a lot of the same state, the optimizer has trouble combining the SLOADs because they are occurring in different scopes.

By flattening all of the function calls in transferFrom, quick tests show that we save a whopping ~8000 gas per transfer (this is in addition to the ~2000 gas saved by dropping the modifiers in #1030).

IMO, flattening the functions also makes actual token contracts more readable, as we are able to drop a lot of unused functions. The downside is that there is now a lot of redundant code in the mock contracts, but I think the tradeoff is massively worth it.

• 📘 I've reviewed the OpenZeppelin Contributor Guidelines
• ✅ I've added tests where applicable to test my new functionality.
• 📖 I've made sure that my contracts are well-documented.
• 🎨 I've run the JS/Solidity linters and fixed any issues (npm run lint:all:fix).

[johnhforrest]

As per #977 this line should be safe to remove and save a bit more gas.

[johnhforrest]

This line should get removed too now that #1039 has been merged.

[johnhforrest]

Huge fan of this PR. Really helps from a readability perspective and reduces the coupling on the burn/mint logic. Great work.

[abandeali1]

@johnhforrest I addressed your comments and rebased. Thanks for reviewing!

[johnhforrest]

LGTM!

[mg6maciej]

Why did you remove _mint (and _burn)? It's such a useful function that is exposed in derived contracts after applying some verification logic.

[abandeali1]

@mg6maciej I didn't want to leave in any unused functions just for the sake of using them in derived contracts.

If you're asking why they were removed in the first place, see the PR description. The gist of it is that they are horribly inefficient.

[mg6maciej]

I wouldn't want to force users to understand the internals of ERC721 implementation to use it. If you try to derive from this class, you will quickly see, why it's better to have _mint there.
Other implementations here, here provide it as well. If you search github for OpenZeppelin imports, you will see people expect it to be there: here, here, here, here, here, here, here, here...

[abandeali1]

Why not just split these functions out into a separate contract? I think they are useful, but I just don't think they should be used internally within transferFrom. And it feels wasteful to increase the cost of deployment for anyone not using these functions.

Happy to add this if it is a blocker.

[nventuro]

Hello @abandeali1, thank you for your contribution!

I dug a bit deeper into what was causing the increased gas cost (since it seemed too high for it to come from just a couple extra SLOADs), and found that the reason lies behind the _removeTokenFrom and _addTokenTo calls: the compiler isn't smart enough to realize there's no need to delete the token from the original user because it can simply be re-assigned.

This can be more easily fixed by adding a function to handle the special case of transferring (since add and remove need to continue existing to support _mint and _burn): I've tested this and it achieves similar gas improvements (there is a total gas cost difference of less than 1% between this and your approach).

Because that's a less disruptive change (we do want _mint and _burn), and this PR got outdated after the 2.0 release (sorry about the delay!), I'm closing this in favor of #1539. Once again, thanks to @abandeali1 for pushing for this, and to everyone who participated in the discussion and validated this idea!