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

Remove unnecessary SafeMath calls #1552

Closed
nventuro opened this issue Dec 12, 2018 · 5 comments
Closed

Remove unnecessary SafeMath calls #1552

nventuro opened this issue Dec 12, 2018 · 5 comments
Labels
contracts Smart contract code.
Milestone

Comments

@nventuro
Copy link
Contributor

Its usage has become ubiquitous, but there are multiple instances where it is not needed. E.g. in ERC721:

function _mint(address to, uint256 tokenId) internal {
  ..
  _ownedTokensCount[to] = _ownedTokensCount[to].add(1);
  ..
}

That is the only place where _ownedTokensCount[to] is incremented: 2^256 calls to _mint are needed to make it overflow, making these calls more expensive by adding the check makes no sense.

@nventuro nventuro added kind:improvement contracts Smart contract code. labels Dec 12, 2018
@nventuro nventuro added this to the v2.2 milestone Dec 12, 2018
@frangio
Copy link
Contributor

frangio commented Dec 12, 2018

Hm, we've usually been more of the "use safemath everywhere to be sure" camp. I think we should tackle this by using something like the Counter type that we have, although it doesn't have a decrement function that is needed for transfers and burning. Thoughts?

@nventuro
Copy link
Contributor Author

That sounds like a great idea! I run some preliminary tests and the concept seems to work.

@Aniket-Engg
Copy link
Contributor

I think we can add increment() and decrement() in Math.sol to increase of decrease a number by 1.

@frangio
Copy link
Contributor

frangio commented Dec 21, 2018

But SafeMath.increment would still need to check for overflow to be safe, because UINT256_MAX.increment() overflows.

My proposal is that since the Counter type can only be incremented, it will never overflow. This is not entirely true though because Solidity struct members can be written so a Counter instance could be made to overflow if used incorrectly.

@nventuro
Copy link
Contributor Author

We haven't yet found a use case for Counter though, have we? If it had methods to both increment and decrement by one, we could remove the require from inc, since it will never overflow. If there is indeed something that only ever increments, then we should create this new thing (IncDec).

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

No branches or pull requests

3 participants