Custom errors #566
Custom errors #566
Conversation
cam-schultz
requested review from
iansuvak,
geoff-vball,
minghinmatthewlam,
bernard-avalabs and
michaelkaplan13
There was a problem hiding this comment.
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.
Thoughts on adding parameters to a some of these custom errors? For instance:
error InvalidDelegatorStatus(ValidatorStatus status);
There was a problem hiding this comment.
oh that could perhaps also help distinguish between too low / too high for cases like InvalidDelegationFee
There was a problem hiding this comment.
yup we should try to use parameters here
Co-authored-by: Ian Suvak <[email protected]> Signed-off-by: cam-schultz <[email protected]>
Why this should be merged
Reduces the staking contract sizes to get us further away from the 24kb limit.
ERC20TokenStakingManager:24,459->20,931NativeTokenStakingManager:23,637->20,177PoAValidatorManager:14,311->12,775How 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