Discussion: Should we remove the SafeMath library for Solidity 0.8?

[frangio]

There has been some discussion in #2442 on whether to keep or remove SafeMath from the Solidity 0.8 release of the library. We don't seem to have consensus yet so I would like to continue the discussion and explore the arguments that are being made.

The origin of the discussion is that Solidity 0.8 includes checked arithmetic operations by default, and this renders SafeMath unnecessary. What is not under discussion is that internally the library will use the built in checked arithmetic, rather than the current manual overflow checks in SafeMath. The question is whether we want to keep SafeMath for other reasons.

Backwards compatibility

It may be better to keep SafeMath to ease the transition from Contracts 3.x (Solidity 0.6-0.7) to Contracts 4.x (Solidity 0.8). If we remove it, users who update will be flooded with error messages that they will have to fix before they do anything else. They may want to try other interesting things in Solidity 0.8, and/or remove SafeMath progressively (for example making sure their tests continue to pass during the migration).

@axic's counterargument:

Perhaps it is better biting the bullet by removing it, and that could trigger users to review the new semantics and use them properly (checked vs. unchecked). And also be aware of the gas implications.

It's true that using SafeMath function calls will be more expensive than just built in arithmetic. The idea would be that users should remove SafeMath once they're using Solidity 0.8. But should we force people to do this migration as soon as they jump on Solidity 0.8? If there was a way to nudge them, such as a deprecation warning, I would agree that we should do it, but removing SafeMath entirely is a lot stronger and causes what seems like unnecessary breakage.

Use inside unchecked blocks

@Amxx mentions that SafeMath would be a way to insert checked operations inside unchecked blocks. This is an interesting idea. If we think this is a genuine use case it would not make sense to deprecate SafeMath.

Several people seem to have reacted against this proposal and it would be great to hear more from them. (@mlntr @cruzdanilo @hrkrshnn @stvndf)

---

Personally I think we should keep the wrapper for backwards compatibility only, deprecated and planned to be removed in the next major release. This is motivated by an intention to keep breaking changes to a minimum. Even if we do not remove it, we can communicate this deprecation clearly and recommend users to refactor their code to remove SafeMath once they migrate to 0.8.

[aaronovz1]

I think as long as all internal contracts do not use SafeMath, then keeping it around to ease the transition is probably OK. My biggest concerns was keeping it around and losing the gas savings if it was still embedded in the internal contracts. Another option is it could be broken out into a separate project - that way users have to explicitly add the package to remove errors, or the errors will encourage them to migrate fully to 0.8.

[Amxx]

An additional reason to keep it:

Have custom revert message for safemath operation

Solidity 0.8.0 will revert in case of overflow, but will not let you customize the revert reason. In some cases people may want to pass a reason message such as "ERC20: transfer amount exceeds balance". SafeMath provide these functions.

[Amxx]

For the SafeMath inside unchecked block, here is the reason.

"Checked" math, which is default in 0.8.0 is not free. The compiler will add some overflow checks, somehow similar to those implemented by SafeMath. While it is reasonable to expect these checks to be less expensive then the current SafeMath, one should keep in mind that this checks will increase the cost of "basic math operation" that were not previously covered. This is the case of variable increments in for loops.

Lets consider this piece of code

function sum(uint[] memory array) internal pure returns (uint result) {
    result = 0;
    for (uint i = 0; i < array.length; ++i) { result += array[i]; }
}

For each element in the array, there are two math operations, one is result += array[i], the other one is ++i. Some people have raised the concern that the second operation does not need the check. An option is to do

function sum(uint[] memory array) internal pure returns (uint result) {
    result = 0;
    unchecked { for (uint i = 0; i < array.length; ++i) { result += array[i]; } }
}

but then you lose the overflow protection on array, so its not good. This is why you may want to do

function add(uint x, uint y) internal pure returns(uint) { return a + b; }

function sum(uint[] memory array) internal pure returns (uint result) {
    result = 0;
    unchecked { for (uint i = 0; i < array.length; ++i) { result = add(result, array[i]); } }
} 

In that particular example, the extra cost of the function might make the whole uncheck thing pointless, but if the core of the loop was to contain a significant number of math operation, only a few of which needs protection, then the SafeMath would be usefull.

---

Thus my 0.8.0 SafeMath library would look like

library SafeMath {
   ...
    function sub(uint256 a, uint256 b) internal pure returns (uint256) {
        return a - b;
    }
    
    function sub(uint256 a, uint256 b, string memory errorMessage) internal pure returns (uint256) {
        unchecked {
            require(b <= a, errorMessage);
            return a - b;
        }
    }
    ....
}

[aaronovz1]

@Amxx that is actually a pretty interesting example. I think you've convinced me that it should be included. The user gets the most flexibility for optimizing this way. If a function has loops and various things, they could put the whole function contents inside unchecked and then pick and choose what to use SafeMath on.

[hrkrshnn]

One reason against it that it simply would lead to more confusion. On announcement, several people were not sure whether checked arithmetic in solidity can replace SafeMath libraries. Having it available in OpenZeppelin will likely exacerbate this feeling.

If a wrapper is made part of it, please make it clear that using the library is not recommended.

[aaronovz1]

Agree. I think the documentation could provide similar examples to what @Amxx posted, where it shows the cases that it can be useful to still be used.

[leonardoalt]

Regarding gas, I'm curious whether anyone has actual data on how much more gas is spent?

[Amxx]

Regarding gas, I'm curious whether anyone has actual data on how much more gas is spent?

// SPDX-License-Identifier: CC0-1.0
pragma solidity ^0.8.0;

contract OverflowCost {
    event Log(uint256);
    function add(uint256 x, uint256 y) external { emit Log(x + y); }
    function sub(uint256 x, uint256 y) external { emit Log(x - y); }
    function mul(uint256 x, uint256 y) external { emit Log(x * y); }
    function div(uint256 x, uint256 y) external { emit Log(x / y); }
    function mod(uint256 x, uint256 y) external { emit Log(x % y); }
}

contract OverflowCostUnchecked {
    event Log(uint256);
    function add(uint256 x, uint256 y) external { unchecked { emit Log(x + y); } }
    function sub(uint256 x, uint256 y) external { unchecked { emit Log(x - y); } }
    function mul(uint256 x, uint256 y) external { unchecked { emit Log(x * y); } }
    function div(uint256 x, uint256 y) external { unchecked { emit Log(x / y); } }
    function mod(uint256 x, uint256 y) external { unchecked { emit Log(x % y); } }
}

• for add(17,42), unchecked saves 61 gas
• for sub(42,17), unchecked saves 58 gas
• for mul(17,42), unchecked saves 81 gas
• for div(42,17), unchecked saves 32 gas
• for mod(42,17), unchecked saves 32 gas
  using 0.8.0 with optimizations.

[axic]

@Amxx can you share the total cost too (not just the savings)?

[axic]

I did not mean transaction total, rather the total cost of the operation (including stack manipulation).

[zemse]

Perhaps it is better biting the bullet by removing it, and that could trigger users to review the new semantics and use them properly (checked vs. unchecked). And also be aware of the gas implications.

I'm totally with this sentiment. But the only concerns I had while reviewing the new semantics about the inbuilt checks in 0.8 were that I could no longer specify revert reasons. If I imagine SafeMath is removed, then whenever I need revert reasons, I would need to maintain a library file with this logic in the project (and probably need to copy-paste the logic around next projects). And that's why I think it would be beneficial if this logic is available in OpenZeppelin, so I could just import it from there instead of extra copy-paste code in multiple projects. Also doing a good looking unchecked i++ as in #2465 (comment).

SafeMath has been an essential piece of code in Solidity, it makes the math safe. But since 0.8, a SafeMath.sol file name seems ironic since it doesn't technically make math safe since it is already safe now inbuilt. If I would maintain a file for the above use cases, Math.sol sounds fine and compact.

Personally I think we should keep the wrapper for backwards compatibility only, deprecated and planned to be removed in the next major release. This is motivated by an intention to keep breaking changes to a minimum.

I understand how frustrating it would be to see 100s or 1000s of errors in a project, I don't think there is a point partially upgrading to Solidity 0.8. But I think dedicating some time for the effort to completely upgrade to 0.8 should be worth it. And one can always stay on older version of Solidity until they block some time to work on upgrade to 0.8 in their project. If SafeMath.sol file exists in OpenZeppelin (for backwards compatibility) then people would procastinate changing SafeMath stuff for later. I'd like to suggest to remove this file and have another file with name like Math.sol or BasicMath.sol that contains just enough functions (like in last of #2465 (comment)) that helps developer to "write better 0.8 code" (and not keep old code as it is).

[nventuro]

I fully agree with @Amxx points and comments. Migrating to 0.8 doesn't mean just removing usage of SafeMath and replacing it with the native operators, but also adding new revert reasons. So something like

balance = balance.sub(amount, "Insufficient balance");

would turn into

require(amount >= balance, "Insufficient balance");
balance -= amount;

but since checked arithmetic is no longer needed,

require(amount >= balance, "Insufficient balance");
unchecked {
  balance -= amount;
}

I think the net effect is good: it allows for overall lower gas costs and highlights the actual error conditions. However, forcing this sort of migration on a user is too much of a burden.

Keep in mind most codebases are not even on 0.7: if checked arithmetic is what convinced them to pull the trigger, then the migration is already non-trivial no begin with. And if people only upgrade once 0.9 comes with some other long-awaited feature (like the mythical Yul IR), this will only make moving forward harder.

[crazyrabbitLTC]

Taking more of the developer point of view, we should also considered there are years worth of documentation that scare developers away from doing math themselves without using SafeMath. The absence of SafeMath will be disorienting to those developers and might require a bit of an educational push to properly inform users as to why it's now safe to not use safe math.

Perhaps it could be moved to a deprecated folder which will be self-explanatory to users.

As a side note, I love the explicitness of safeMath. X.sub(y) is just a more pleasurable experience. Maybe we can keep it as syntactic sugar and build a transpiler...... (joking!)

[abcoathup]

Twitter poll for some community sentiment:

https://twitter.com/OpenZeppelin/status/1348768595350745088?s=20

[Amxx]

Following this discussion, SafeMath has been slightly modified to benefit from solidity ^0.8.0 new features. This new version is visible in the master branch (which is now ^0.8.0 based) and soon in 4.0 preview. If you have any concern with the "new" version of SafeMath, fell free to raise an issue.

[frangio]

Thanks everyone for participating in the discussion! It was great to get everyone's perspectives and we're looking forward to more discussions like this one in the future.