revert native rounding up

rkirmann · Oct 29, 2020 · 075f305 · 075f305
1 parent 3a8c67e
commit 075f305
Show file tree

Hide file tree

Showing 4 changed files with 8 additions and 70 deletions.
diff --git a/contracts/libraries/FullMath.sol b/contracts/libraries/FullMath.sol
@@ -4,7 +4,7 @@ pragma solidity >=0.4.0;
 // taken from https://medium.com/coinmonks/math-in-solidity-part-3-percents-and-proportions-4db014e080b1
 // license is CC-BY-4.0
 library FullMath {
-    function fullMul(uint256 x, uint256 y) internal pure returns (uint256 l, uint256 h) {
+    function fullMul(uint256 x, uint256 y) private pure returns (uint256 l, uint256 h) {
         uint256 mm = mulmod(x, y, uint256(-1));
         l = x * y;
         h = mm - l;
@@ -28,20 +28,12 @@ library FullMath {
         return l * r;
     }
 
-    function mulDiv(uint256 x, uint256 y, uint256 d, bool roundUp) internal pure returns (uint256) {
+    function mulDiv(uint256 x, uint256 y, uint256 d) internal pure returns (uint256) {
         (uint256 l, uint256 h) = fullMul(x, y);
         uint256 mm = mulmod(x, y, d);
         if (mm > l) h -= 1;
         l -= mm;
-        if (roundUp && mm > 0 && h < uint256(-1)) {
-            if (l > uint256(-1) - d) h += 1;
-            l += d;
-        }
         require(h < d, 'FullMath: FULLDIV_OVERFLOW');
         return fullDiv(l, h, d);
     }
-
-    function mulDiv(uint256 x, uint256 y, uint256 d) internal pure returns (uint256) {
-        return mulDiv(x, y, d, false);
-    }
 }
diff --git a/contracts/test/FullMathTest.sol b/contracts/test/FullMathTest.sol
@@ -5,23 +5,12 @@ pragma solidity >=0.4.0;
 import '../libraries/FullMath.sol';
 
 contract FullMathTest {
-    function fullMul(uint256 x, uint256 y) external pure returns (uint256 l, uint256 h) {
-        return FullMath.fullMul(x, y);
-    }
-
-    function mulDiv(
-        uint256 x,
-        uint256 y,
-        uint256 z
-    ) external pure returns (uint256) {
+    function mulDiv(uint256 x, uint256 y, uint256 z) external pure returns (uint256) {
         return FullMath.mulDiv(x, y, z);
     }
 
-    function mulDivRoundingUp(
-        uint256 x,
-        uint256 y,
-        uint256 z
-    ) external pure returns (uint256) {
-        return FullMath.mulDiv(x, y, z, true);
+    function mulDivRoundingUp(uint256 x, uint256 y, uint256 z) external pure returns (uint256) {
+        bool roundUp = mulmod(x, y, z) > 0;
+        return FullMath.mulDiv(x, y, z) + (roundUp ? 1 : 0);
     }
 }
diff --git a/test/FixedPoint.spec.ts b/test/FixedPoint.spec.ts
@@ -291,7 +291,7 @@ describe('FixedPoint', () => {
       // long division but makes fewer iterations
       expect(
         await fixedPoint.getGasCostOfDivuq([BigNumber.from(10).pow(10).mul(Q112)], [BigNumber.from(25).mul(Q112)])
-      ).to.eq(1583)
+      ).to.eq(1480)
     })
 
     it('gas cost of long division with all iterations', async () => {
@@ -301,7 +301,7 @@ describe('FixedPoint', () => {
           [BigNumber.from(10).pow(10).mul(Q112)],
           [BigNumber.from(3).mul(BigNumber.from(10).pow(10)).mul(Q112)]
         )
-      ).to.eq(1583)
+      ).to.eq(1480)
     })
   })
 

diff --git a/test/FullMath.spec.ts b/test/FullMath.spec.ts
@@ -25,49 +25,6 @@ describe('FullMath', () => {
     fm = await deployContract(wallet, FullMathTest, [], overrides)
   })
 
-  describe('#fullMul', () => {
-    it('less than max uint', async () => {
-      const [l, h] = await fm.fullMul(5, 8)
-      expect(l).to.eq(40)
-      expect(h).to.eq(0)
-    })
-
-    it('result just 1 more than max uint', async () => {
-      const [l, h] = await fm.fullMul(BigNumber.from(2).pow(128), BigNumber.from(2).pow(128))
-      expect(l).to.eq(0)
-      expect(h).to.eq(1)
-    })
-
-    it('combination of both', async () => {
-      // try multiplying two numbers, 3.5 * 8.45 both 128.128, resulting in a 256x256
-      // l should contain fractional part, h should contain whole part
-      const [l, h] = await fm.fullMul(
-        BigNumber.from(35).mul(BigNumber.from(2).pow(128)).div(10),
-        BigNumber.from(845).mul(BigNumber.from(2).pow(128)).div(100)
-      )
-
-      // 3.5 * 8.45 = 29.575
-      // fractional part
-      // to get 0.575, divided by (2^256 - 1)
-      // https://www.wolframalpha.com/input/?i=66580451311456812368553316379995547015392043525898667398263748579347811794944+%2F+%282%5E256-1%29
-      expect(l).to.eq('66580451311456812368553316379995547015392043525898667398263748579347811794944')
-      // whole part
-      expect(h).to.eq('29')
-    })
-
-    it('max inputs', async () => {
-      const [l, h] = await fm.fullMul(constants.MaxUint256, constants.MaxUint256)
-      expect(l).to.eq(1)
-      expect(h).to.eq(constants.MaxUint256.sub(1))
-    })
-
-    it('min inputs', async () => {
-      const [l, h] = await fm.fullMul(0, 0)
-      expect(l).to.eq(0)
-      expect(h).to.eq(0)
-    })
-  })
-
   describe('#mulDiv', () => {
     const Q128 = BigNumber.from(2).pow(128)
     it('accurate without phantom overflow', async () => {