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

Consider custom errors #295

Closed
albert-llimos opened this issue Mar 8, 2023 · 4 comments
Closed

Consider custom errors #295

albert-llimos opened this issue Mar 8, 2023 · 4 comments
Assignees

Comments

@albert-llimos
Copy link
Collaborator

albert-llimos commented Mar 8, 2023

If we upgrade the solidity version, consider changing the requires with a revert string for Error functions. It should save both gas and deployment costs. Especially interesting as it will probably decrease bytecodeSize so we can crank up the optimizer more.

The only problem is that Brownie doesn't support errors with the Error catching with "with reverts(..)". I will need to try out some custom made function to catch them instead. Try out:
https://github.com/EdNoepel/brownie-test-issues/blob/master/tests/test_errors.py

@albert-llimos albert-llimos self-assigned this Mar 8, 2023
@albert-llimos
Copy link
Collaborator Author

albert-llimos commented Mar 8, 2023

For example, in the Vault each Error replacing a revert string reduces the bytcodesize by 0.1% (~32 bytes). Also, the reverts will be cheaper.
However, normal execution cost will be exactly the same, so this might not be a priority.

@albert-llimos
Copy link
Collaborator Author

albert-llimos commented Mar 15, 2023

Brownie not supporting it is really painful. I have described the workaround when we expect a reversion and it does work (even though it needs to add support for parameters). See this example too:

eth-brownie/brownie#1108 (comment)

But to debug when we don't know what has reverted it's extremely painful as we can't reverse engineer what is the Error that reverted from the bytes (it's a Hash). We basically need to hash every possible error and figure out which one is it. Might not be worth it.

One thing to try is, when we get an unexpected error, loop over all the errors in the abi ( like in the issue,) and compare them to the obtained error to figure out which one reverted. So basically we would need to create an own revert function like this:

def expect_revert(contract_, err_name, params):
        > Look for the err_name in the abi and encode the error and parameters (https://github.com/eth-brownie/brownie/issues/1108#issuecomment-1478542846)
        > make the function call via try-catch
        > check if the error function selector matches.
        > If it doesn't, loop over all the errors in the abi and try to figure out which one reverted.
        > If the error function selector matches, assert that the parameters match.

@albert-llimos
Copy link
Collaborator Author

albert-llimos commented Mar 31, 2023

This PR is what originally "added support" for custom errors, which only meant preventing crashes. Look into it.
https://github.com/eth-brownie/brownie/pull/1110/files
It should loop over the potential errors as described in the previous comment.
Also, check this out => This seems to be exactly what I am looking for!
eth-brownie/brownie#1675

@albert-llimos
Copy link
Collaborator Author

Moved to Linear

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant