Remove dependency from @openzeppelin remapping

[alephao]

Currently, the contract imports @openzeppelin which is the default way to import openzeppelin when using a hardhat project.

ERC721A/contracts/ERC721A.sol

Lines 6 to 12 in b738004

	import '@openzeppelin/contracts/token/ERC721/IERC721.sol';
	import '@openzeppelin/contracts/token/ERC721/IERC721Receiver.sol';
	import '@openzeppelin/contracts/token/ERC721/extensions/IERC721Metadata.sol';
	import '@openzeppelin/contracts/utils/Address.sol';
	import '@openzeppelin/contracts/utils/Context.sol';
	import '@openzeppelin/contracts/utils/Strings.sol';
	import '@openzeppelin/contracts/utils/introspection/ERC165.sol';

There are two main problems with that approach:

1. The project importing ERC721A might be using a different remapping for openzeppelin so instead of @openzeppelin/contracts might be openzeppelin-contracts or just @openzeppelin or any other thing. So they will have to change their remapping to follow this project's remapping which is not a great experience.
2. The project importing ERC721A might be using a different version of openzeppelin, which might result in a different outcome for the contract

To support any project and to provide a better developer experience when using ERC721A, I suggest either providing a flattened version of the contract or copying the source code of the 3rd party libraries used to this project

Happy to do it myself next week if this is desirable

[Vectorized]

We have OpenZeppelin dependency on both the main repo and upgradable repo. Changes will need to be made not just in the code, but also possibly the workflow files.

Let me ask the team for comments. @cygaar

[Vectorized]

Some reasons I'm for keeping the dependency:

• Most users will use the default OpenZeppelin mapping.
• Most of the imports are interfaces, which won't change. The rest are also very unlikely to change.
• Avoid duplication of tests.
• The Upgradeable repo uses the initializable contract from OpenZeppelin, which is written for OpenZeppelin's transpiler.

If we were to implement this:

• We should inline only the required functions from ERC165 (like Solmate), Context, Strings (used to convert uint256 to string), Address right into ERC721A.sol.
• Tests will be needed to cover the new inlined code.
• We can directly copy and paste IERC721, IERC721Metadata, IERC165 definitions into the tentatively upcoming IERC721A.sol (Feat: Add Interface #243).
• We don't want to introduce any new classes to avoid namespace collision with OpenZeppelin's interfaces (which are frequently used).
  • Instead of including a definition of the IERC721Receiver, we can use the dynamic call method here.
• We can just keep the Upgradeable repo as it is.

You can make a PR, and we can consider from there.

This does not look trivial -- there is a good chance we may end up keeping the dependency for simplicity.

[alephao]

Thanks for checking it out. Seems non trivial, and you're right, it's unlikely to change so the 2nd issue I mentioned is not a real problem. I'll play with it and see if I can come up with something nice. Then we can discuss in the PR!

Most users will use the default OpenZeppelin mapping.

I see how this is true for hardhat, but In my case I use foundry which is becoming very popular very quickly, so my workflow is the following:

• forge init to init a project
• forge install chiru-labs/ERC721A to install this repo
• Then I import it in a contract and run forge build. It crashes because it can't find @openzeppelin
• Then I install openzeppelin forge install OpenZeppelin/openzeppelin-contracts
• I run forge build and still crashes since the default remapping is openzeppelin-contracts
• I update the remapping to @OpenZeppelin, and all is good

Here is an asciinema with the recording. I had those issues before so I know how to solve, but it's definitely frustrating.

[ahbanavi]

@alephao I see where you coming from.
I also use brownie sometimes and remapping is different there.
IMO it's more like an OZ problem, I mean why is their npm package name different from the GitHub repo? all of this (apart from using a different version) could be fixed if their GitHub repo and npm package names were the same.

IMHO in-sourcing an audited and well-tested contract like OZ might add very additional overhead for maintainers, especially in the upgradable repo, keeping the dependencies is ok for now.

We could update the docs and readme for foundry and brownie to how to install erc721a and update the remappings properly.

Might be related:
OpenZeppelin/openzeppelin-contracts#3334

[mpeyfuss]

I also use brownie. My brownie-config.yaml files already contains the remapping so that my projects are compatible with hardhat/truffle developers. Updating docs seems like the easiest solution IMO. Keeping OZ dependencies is a better "trust builder" as well.