Improve safemath signed int #714
Conversation
|
@cedricwalter would you mind checking the CI status? |
|
Yes i am looking at it now |
|
was nothing a temporary error in the build, i've run all tests twice locally with yarn |
contracts/math/SafeMath.sol
Outdated
| } | ||
|
|
||
| function add(int256 a, int256 b) internal pure returns (int256) { | ||
| return a + b; |
There was a problem hiding this comment.
@cedricwalter shouldn't these have overflow checks too?
There was a problem hiding this comment.
@cedricwalter please fix
contracts/math/SafeMath.sol
Outdated
| } | ||
|
|
||
| function add(int256 a, int256 b) internal pure returns (int256) { | ||
| return a + b; |
There was a problem hiding this comment.
@cedricwalter please fix
There was a problem hiding this comment.
Thanks @cedricwalter! Please address the concerns I raised below.
| */ | ||
| function mul(uint256 a, uint256 b) internal pure returns (uint256) { | ||
| if (a == 0) { | ||
| return 0; |
There was a problem hiding this comment.
It's hard to understand the scope of the changes if you change the indentation, please respect the indentation of the rest of the file.
| return 0; | ||
| int256 constant INT256_MIN = int256((uint256(1) << 255)); | ||
|
|
||
| int256 constant INT256_MAX = int256(~((uint256(1) << 255))); |
There was a problem hiding this comment.
Do we need these constants to check for overflow? Shouldn't the sign be enough?
There was a problem hiding this comment.
i dont think so, what do you mean with the sign?
| let b = 2; | ||
| await assertRevert(safeMath.multiply(a, b)); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
Please keep all tests related to the SafeMath contract in the same file.
| result = SafeMath.add(a, b); | ||
| } | ||
|
|
||
| } |
There was a problem hiding this comment.
Why not add these to the same SafeMathMock to avoid confusion?
|
Closing due to inactivity. Superseded by #835. |
Fixes #705
🚀 Description
Improve SafeMath to support signed int operations
npm run lint:all:fix).