-
Notifications
You must be signed in to change notification settings - Fork 81
Add support for custom errors #502
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
Conversation
The failing tests make no sense to me. I know I can fix them by running Plus, if something is really missing in those files, the "real" tests will fail anyway. @Janther @mattiaerre do you mind if I remove those tests? |
Ok, I preemptively removed them. I also excluded the |
tests/CustomErrors/CustomErrors.sol
Outdated
|
||
function throwCustomError() { | ||
revert | ||
ContractCustomError(); |
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.
This should be named FunctionCustomError
The problem with snapshots in testing is that the snapshots need to be understandable and auditable. A extremely big list or a massive array are not something that snapshots were made for. |
Do you mind if we do/discuss that in another PR so we can get this merged? (FWIW, I'm OK with not testing code-generating scripts if the generated code is tested, and we already have a 100% threshold in the directory where the generated code is added) |
Feedback applied, thank you @Janther! |
Closes #485. Way overdue 😅