OpenZeppelin / openzeppelin-contracts

OpenZeppelin Contracts is a library for secure smart contract development.
https://openzeppelin.com/contracts
MIT License
24.75k stars 11.75k forks source link

Unused class/interface in flattened code #4629

Open vittominacori opened 11 months ago

vittominacori commented 11 months ago

Using flattener (both truffle or hardhat) all the custom errors from draft-IERC6093.sol are imported into the flattened file also if unused. I mean using the ERC20 contract we will find also the IERC721Errors and IERC1155Errors into the code. It should import only the required (and necessary) file in import {IERC20Errors} from "../../interfaces/draft-IERC6093.sol"; instead of importing the whole file.

Is there a way to avoid this behavior? Is this a flattener issue and should we reference it there?

To reproduce simply run:

npx hardhat flatten contracts/token/ERC20/ERC20.sol > ERC20.flat.sol

Amxx commented 11 months ago

Hello @vittominacori

That is a flattener issue. Basically the flattener is not smart. Whenever any symbol is imported from a file, the flattener will get the entier file (and not just the symbols needed).

Here, I see 3 options:

vittominacori commented 11 months ago

Thanks @Amxx

Basing on your list, right now I can accept having unused errors :)

As you are working on ERC6093, do you think OZ will keep all the custom errors into the same file or will we split them into distinct files? I'm also interested as I'm porting ERC1363 to v5 and I like your EIP approach. I'm writing an IERC1363Errors interface following that rationale. IMHO it could be better to have the IERC20Errors, IERC721Errors, etc, next to their standard interface instead of having a single file for all.

Regarding the other point, it could be good to write a smart flattener to avoid other unwanted code in flattened files.

Amxx commented 11 months ago

Where to put custom errors in something we discussed a lot.

I guess we will want to make ERC6093 final at some point, which will prevent us from adding more into it. As a consequence, we should plan for error interfaces outside ERC6093

vittominacori commented 11 months ago

Thank you for your answer.

I didn't mean to add IERC1363Errors in ERC6093 to expand it, but that I'm following that logic (naming, parameters...) to be consistent. You can check the PR #4631.

Also, when I said to keep IERC*Errors next to their interface, I was referring to the same folder. So we could put IERC20Errors in the token/ERC20 folder (or somewhere inside like token/ERC20/errors).