Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Unused class/interface in flattened code #4629

Open
vittominacori opened this issue Sep 27, 2023 · 4 comments
Open

Unused class/interface in flattened code #4629

vittominacori opened this issue Sep 27, 2023 · 4 comments
Labels
Milestone

Comments

@vittominacori
Copy link
Contributor

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
Copy link
Collaborator

Amxx commented Sep 27, 2023

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:

  • write a smart flattener
  • split the draft-IERC6093.sol in 3 distinct files
  • accept to have unecessary errors declared in your flattened file.

@vittominacori
Copy link
Contributor Author

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
Copy link
Collaborator

Amxx commented Oct 4, 2023

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

  • For our own (non standard) contracts, we put them in the interface (for example IAccessControl)
  • For standard interfaces, like ERC20 or ERC721, we don't want to put them in the interface, because the errors are not part of the standard. That is why we came up with ERC6093.
  • I'm not sure we will want to expand ERC6093. Maybe we will, or maybe we will use a different naming scheme. We may put IERC20Errors and IERC721Errors in dedicated files, and have IERC6093 just be an import (some of) these files. That way, we could have an IERC1363Errors somewhere, and stay consistent with the other "error files" without necessarily including it in ERC6093.

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
Copy link
Contributor Author

vittominacori commented Oct 4, 2023

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).

@Amxx Amxx added the idea label Oct 5, 2023
@Amxx Amxx added this to the 5.1 milestone Oct 5, 2023
@Amxx Amxx modified the milestones: 5.1, 5.x Jul 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants