Safe Math #27
Safe Math #27
Conversation
| // We cap the supply to avoid overflows in computations. | ||
| // Due to the signed math in rebase(), MAX_RATE x MAX_SUPPLY must fit into an int256. | ||
| // MAX_SUPPLY = UInt256Lib.getMaxInt256() / MAX_RATE | ||
| uint256 private constant MAX_SUPPLY = 578960446186580977117854925043439539266349923328202820197; |
There was a problem hiding this comment.
~(uint256(1) << 255) - MAX_RATE instead? I expect that the compiler precomputes it on compilation. So no gas cost.
There was a problem hiding this comment.
The docs are actually not super clear about this.
See here:
The compiler does not reserve a storage slot for these variables, and every occurrence is replaced by the respective constant expression (which might be computed to a single value by the optimizer).
| // We cap the rate to avoid overflows in computations. | ||
| // 18 decimal fixed point format | ||
| // MAX_RATE = 100 * 10**18 | ||
| uint256 private constant MAX_RATE = 100000000000000000000; |
There was a problem hiding this comment.
100 * 10**18 makes it clear that the max rate is 100x, so I went with that.
There was a problem hiding this comment.
I meant to change 100000000000000000000 to 100 * 10**18? I think we can trust the optimizer to get it right.
There was a problem hiding this comment.
From all that I've read, I'm still not sure.
For example, this old issue: ethereum/solidity#976
The new issue that addresses it still uses the fuzzy language in the docs: ethereum/solidity#992
the keyword constant on any variable means it cannot be modified (and could be placed into memory or bytecode by the optimiser)
I'm inclined to save this until we can accurately measure gas usage.
| function div(int256 a, int256 b) internal pure returns (int256) { | ||
| // Prevent overflow when dividing INT256_MIN by -1 | ||
| // https://github.com/RequestNetwork/requestNetwork/issues/43 | ||
| assert(!(a == - 2**255 && b == -1)); |
There was a problem hiding this comment.
Cheaper and simpler:
assert(!(a == (int256(1)<<255) && b == -1));
|
|
||
|
|
||
| /** | ||
| * @title SafeMathInt |
There was a problem hiding this comment.
Add a TODO to add tests and potentially optimize the methods below. Adding comments below for optimizations.
| function mul(int256 a, int256 b) internal pure returns (int256) { | ||
| // Prevent overflow when multiplying INT256_MIN with -1 | ||
| // https://github.com/RequestNetwork/requestNetwork/issues/43 | ||
| assert(!(a == - 2**255 && b == -1) && !(b == - 2**255 && a == -1)); |
There was a problem hiding this comment.
The line below is more efficient, to be added after the multiplication.
assert(c != 1<<255 || ((a & 1<<255) != (b & 1<<255)))
| return c; | ||
| } | ||
|
|
||
| function sub(int256 a, int256 b) internal pure returns (int256) { |
There was a problem hiding this comment.
sub and add are using two different styles. We should finf if either is cheaper and use it for both.
sub can be:
int256 c = a - b; assert((b >= 0 && c <= a) || (b < 0 && c > a));
Make all the math safe (I think!)
Also makes the types match the Oracle after this PR