-
Notifications
You must be signed in to change notification settings - Fork 11.9k
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
Replace revert strings with custom errors #4261
Replace revert strings with custom errors #4261
Conversation
🦋 Changeset detectedLatest commit: 1cd28fe The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
No dependency changes detected. Learn more about Socket for GitHub ↗︎ 👍 No dependency changes detected in pull request |
371a4c0
to
9f58996
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left comments about the "contracts" part. Haven't touched the "tests" part yet
Co-authored-by: Hadrien Croubois <[email protected]>
05a195e
to
c29e041
Compare
I want to touch again on the "where errors are declared", because I fell we never took the proper time to discuss the options. We discuss the syntax of the error at length, but I think there is more to it than just how they are named. Particularly since our codebase is designed to be reused. AFAIK, there are 4 options to declaring errors
I don't really like defining them globally, because it pollutes the namespace, and can cause to identifier conflicts very easily (particularly if there are many errors) For contracts vs interfaces vs library, there is the following:
In our codebase, we use (custom) errors in different cases:
IMO there is real value to having clear "error" libraries, as it would improve reusing the errors in helper libraries (SafeERC20), or between contracts if/when that makes sens. All the libraries errors are already declared within the libraries, which IMO is good! Now its just a matter of moving the errors declared in contracts to dedicated libraries. Example n°1:I would propose that in the Example n°2:For ERC20 (and the other tokens), I would add a
What do you think? EDIT: my point includes a direct consequence, the pragma should be no lower than 0.8.20 so the custom errors get in the ABI |
Fixes #2839
Fixes LIB-839
PR Checklist
Files to replace errors
Tests
General
npx changeset add
)