Add Modulo operation for getting the quotient #915
Conversation
|
Hi @JeanoLee! Thank you for contributing. Is this function any different from the built-in operator |
nventuro
added
feature
test/math/SafeMath.test.js
Outdated
| @@ -88,4 +88,21 @@ contract('SafeMath', () => { | |||
| await assertJump(this.safeMath.div(a, b)); | |||
| }); | |||
| }); | |||
|
|
|||
| describe('mod', function () { | |||
| it('modulo correctly', async function () { | |||
There was a problem hiding this comment.
Could you add a couple more test cases? Since modulo is a bit of a tricky operation, I think it'd be nice to test when a < b, a == b, a > b, and a == b * k (for some integer k).
contracts/math/SafeMath.sol
Outdated
| */ | ||
| function mod(uint256 a, uint256 b) internal pure returns (uint256 c) { | ||
| // d = a / b | ||
| // a = d * b + c |
There was a problem hiding this comment.
Consider adding a small comment here noting that truncation happens when d is calculated. Otherwise it may be confusing, since for a real number d, a = d * b + c = (a / b) * b + c = a + c, which obviously doesn't hold.
contracts/math/SafeMath.sol
Outdated
| // c = a - d * b | ||
| uint256 d = div(a,b); | ||
| c = sub(a, mul(d,b)); | ||
| assert(c < b); |
There was a problem hiding this comment.
👍, this is in line with what's being discussed at #1120 (unless I'm mistaken @leonardoalt?)
There was a problem hiding this comment.
@nventuro Yes! (if the other operations add the necessary requires)
There was a problem hiding this comment.
It's more fundamental than that: by definition the result of a modulo operation will always be strictly less than the divisor (it's the remainder of dividing by it), which is why I thought this was an interesting example of 'correct' assert usage.
There was a problem hiding this comment.
Sure, but that's exactly the point. Since here mod is implemented using the other operators, if they overflow etc the assertion might break. By having the other operations ensure their correctness, the corrected of mod follows.
There was a problem hiding this comment.
And you are totally right in saying this is a nice example of the usage of assert: you have the pre-conditions that are true from the other operations, which imply the mod property.
|
@frangio this is indeed how |
|
I went ahead and implemented the changes myself, due to inactivity. |
contracts/math/SafeMath.sol
Outdated
| require(b != 0); | ||
|
|
||
| uint256 c = a % b; | ||
| assert(c < b); |
There was a problem hiding this comment.
I'd consider removing the assert here.
There was a problem hiding this comment.
Removed, since we don't yet have a criteria for this.
Fixes #
🚀 Description
Added modulo operation.
npm run lint:all:fix).