SafeMath - failed assert reason.

[robertmagier]

I would like to propose changing SafeMath library to use require instead of assert and also describe the reason why require failed. It will make testing in truffle much easier.

For example current sub function is using assert.

    function sub(uint256 _a, uint256 _b) internal pure returns (uint256) {
        assert(_b <= _a);
        return _a - _b;
    }

We can change it to require and provide error description which will be then displayed truffle test.

    function sub(uint256 _a, uint256 _b) internal pure returns (uint256) {
        require(_b <= _a,"SafeMath library subtraction error");
        return _a - _b;
    }

[maxsam4]

A) internal functions of libraries are inlined in the implementing smart contract.
B) Strings cannot be compressed and hence take a good chunk of allowed bytecode size for deployment.

As a result, Adding error strings to SafeMath will considerably increase the size of contracts implementing it. I would avoid making such basic functions so heavy.

I won't mind a separate SafeMathWithReasonStrings library though. Just don't add reason strings to the default one.

[robertmagier]

I think libraries address is inlined in the contract using it. Implementing reason will of course increase size of library and cost of its deployment, but will not increase size of contract using this library. Please correct me if I am wrong. Nvertheless I think size argument is pretty important and it might be a good reason no to implement this change. It would however help me in debugging.

…

On Tue, Nov 20, 2018, 00:00 Mudit Gupta ***@***.*** wrote: A) internal functions of libraries are inlined in the implementing smart contract. B) Strings cannot be compressed and hence take a good chunk of allowed bytecode size for deployment. As a result, Adding error strings to SafeMath will considerably increase the size of contracts implementing it. I would avoid making such basic functions so heavy. I won't mind a separate SafeMathWithReasonStrings library though. Just don't add reason strings to the default one. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#1506 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AhG5LSVIilGICw8vEXcXlPf2fMaF6GOnks5uw4xTgaJpZM4YqNVU> .

[maxsam4]

I think libraries address is inlined in the contract using it.

Library address is used for public functions.
internal functions are directly inlined.

As address cannot be compressed either, making SafeMath functions public will increase deployment cost as well as (more importantly) execution cost as everytime the contract will have to make an external call.

As I said earlier, I won't mind a separate SafeMathWithReasonStrings library. Just don't add reason strings to the default one.

[nventuro]

@maxsam4 is right in that SafeMath is intended to be used as an internal library, and will be inlined in your source code (it'd be way too expensive to use otherwise). We are considering adding revert reasons, but we're not yet sure on how they should look like (there was some discussion in #888), and lack tooling to make sure they've been correctly implemented.

Regarding require, that's already part of OpenZeppelin! It was added in #1187.