SafeMath can no longer be used as a library for uints smaller than 256 bits

[BrendanChou]

Can no longer use SafeMath as an in-line library for unsigned integers smaller than 256 bits.

💻 Environment

Truffle 5 and OpenZeppelin 2.1

📝 Details

This is because there are new functions for signed integers in SafeMath. Recommend splitting into two different files so that both unsigned ints and signed ints of less than 256 bits can be used without first casting to uint256 or int256. Casting like this can result in hard-to-detect bugs.

🔢 Code to reproduce bug

using SafeMath for uint128;
function failMath() public pure {
    uint128 a = 1;
    a.add(1);
}

results in the error:
Member "add" not unique after argument-dependent lookup in type(library SafeMath).

[frangio]

Good catch @BrendanChou. +1 to splitting into two different libraries (SafeMath + SafeSignedMath?). We have to fix this for 2.1.

[nventuro]

Hey there @BrendanChou!

I'm curious, what are you using the result of the SafeMath call for? I'm asking because the result of any operation will be an uint256, which needs casting down if working in uin128, but that casting is in itself an unsafe operation, since the original value may be out of the representable range.

That said, I do agree with your statement that we should provide mechanisms so that explicit casts can be avoided.

I see a couple ways to move forward with this:

1. As suggested, split SafeMath into two contracts, one with unsigned and one with signed operations. I'm not a fan of this approach mostly because it feels like limitations of the language shouldn't affect how we structure our code, but on a more pragmatic level, I worry we may run into a similar issue in the future (e.g. when providing support for uint128), and we won't be able to repeat this then.
2. Provide support for all types that make sense (e.g. uint128), so that casts are never needed. This would probably mean some sort of template meta-programming.
3. Go with the explicit casts, and provide safe downcasting functions so that the result is usable. Note that option 1 also requires downcasting.

Regarding the immediate future, I'd get the signed operations into a SignedSafeMathcontract in the drafts directory and release 2.1 that way, until we settle on a solution.

[frangio]

Fixed by #1588. Not sure why GitHub didn't close this issue.

[BrendanChou]

@nventuro Afterwards, using my own simple library to do a safe down-cast to uint128