-
Notifications
You must be signed in to change notification settings - Fork 22
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
Custom errors #566
Custom errors #566
Conversation
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.
Changes LGTM. I know @geoff-vball prefers separating out too low / too high error messages and I do see the benefit but I don't consider it a blocker for this change right now.
We could go back to them once we have more headroom with respect to size.
Additionally separating the cases out even with the same error message would improve the accuracy of coverage reports since it would make sure that we are testing both cases.
error InvalidDelegatorStatus(); | ||
error InvalidNonce(); | ||
error InvalidDelegationID(); | ||
error ValidatorNotPoS(); | ||
error MaxWeightExceeded(); | ||
error InvalidDelegationFee(); | ||
error InvalidStakeDuration(); | ||
error InvalidStakeAmount(); | ||
error InvalidStakeMultiplier(); |
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.
Thoughts on adding parameters to a some of these custom errors? For instance:
error InvalidDelegatorStatus(ValidatorStatus status);
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.
oh that could perhaps also help distinguish between too low / too high for cases like InvalidDelegationFee
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.
yup we should try to use parameters here
Co-authored-by: Ian Suvak <[email protected]> Signed-off-by: cam-schultz <[email protected]>
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.
Should we also replace the require
statements in ValidatorMessage.sol
with custom errors? Or is that not as relevant here?
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.
LGTM. I think we should use params, but not going to block this for the audit since we don't currently have them.
Why this should be merged
Reduces the staking contract sizes to get us further away from the 24kb limit.
ERC20TokenStakingManager
:24,459
->20,931
NativeTokenStakingManager
:23,637
->20,177
PoAValidatorManager
:14,311
->12,775
How this works
Replaces all string errors with custom errors. I tried to strike a balance between error reuse and clarity, since the real size savings come from reuse. My goal was to have every error condition be distinct within a given context (with some exceptions - for example I don't distinguish between stake amount too high vs stake amount too low - I call both invalid), but happy to take into consideration any differing opinions.
How this was tested
CI
How is this documented