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

Functions in SafeMath contract overloaded to accept custom error messages #1828

Conversation

elch-yan
Copy link
Contributor

Fixes #1827
Overloaded counterparts of the existing functions created, and original functions calls redirected to them.

@nventuro
Copy link
Contributor

Great work @elch-yan, thanks! Do you think you could also tackle the following?

  1. Add an entry to the CHANGELOG file with this new feature
  2. Add some of these custom checks to relevant contracts, e.g. ERC20._transfer, with a custom error message.

Regarding scope, I'm not sure how useful the overloads for add, mul div and mod are, since those are often used to check for actual arithmetic errors (overflows or division by zero) as opposed to e.g. sufficient balance. Perhaps we should limit this to sub? @frangio

@elch-yan
Copy link
Contributor Author

Thank you @nventuro. Sure I'll update CHANGELOG and will add relevant checks to ERC20._transfer.
I was also been thinking about usefulness of this for the remaining 4 methods, and I guess that it might be somewhat useful for div and mod for 0 checks (spliting funds for example), but guess you're right about mul and add.

@nventuro
Copy link
Contributor

and I guess that it might be somewhat useful for div and mod for 0 checks (spliting funds for example)

Ohh, interesting! There are some examples of this in PaymentSplitter, TokenVesting and IncreasingTimeCrowdsale.

@elch-yan elch-yan force-pushed the improvement/safemath-custom-error-messages branch from 3eec22d to ca40fe5 Compare July 24, 2019 20:47
@elch-yan
Copy link
Contributor Author

@nventuro I've made an entry in CHANGELOG, did some modifications in token contracts and related tests, regarding custom errors.
I looked at PaymentSplitter, TokenVesting and IncreasingTimeCrowdsale and I guess that in their case divisor can never become a 0.
I'd be happy to hear your opinion regarding my changes and about whether or not you'd like me to keep overloads for SafeMath functions other than sub.

@frangio
Copy link
Contributor

frangio commented Jul 25, 2019

I think we should only add those that we currently need, which seem to be sub, div and mod, from what @nventuro said.

@elch-yan
Copy link
Contributor Author

All done!

@frangio frangio changed the title Imporvement: functions in SafeMath contract overloaded to accept custom error messages. Functions in SafeMath contract overloaded to accept custom error messages Jul 26, 2019
@frangio
Copy link
Contributor

frangio commented Jul 26, 2019

Thanks @elch-yan!

One last thing: we would like to change the new error messages slightly to make them more aligned with the rest of the library. Could you apply the following changes?

  • "transfer of tokens exceeding allowance" → "transfer amount exceeds allowance"
  • "transfer of tokens exceeding balance" → "transfer amount exceeds balance"/"transfer amount exceeds supply" (depending whether the variable in the operation is the total supply)
  • "decrease of higher allowance than granted" → "decreased allowance below zero"

Those which say "transfer" also apply to "burn".

frangio
frangio previously approved these changes Jul 26, 2019
Copy link
Contributor

@frangio frangio left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome @elch-yan! Thanks!

Copy link
Contributor

@nventuro nventuro left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great work @elch-yan, thanks a lot!

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

Successfully merging this pull request may close these issues.

Custom error messages as a third parameter to SafeMath functions
3 participants