two precision improvements (Uniswap#18)

* make sqrt less lossy make muli overflow safe to the extent possible * finish up tests
rkirmann · Oct 22, 2020 · a327b80 · a327b80
1 parent 15920ce
commit a327b80
Show file tree

Hide file tree

Showing 3 changed files with 54 additions and 32 deletions.
diff --git a/contracts/libraries/FixedPoint.sol b/contracts/libraries/FixedPoint.sol
@@ -54,8 +54,9 @@ library FixedPoint {
     // multiply a UQ112x112 by an int and decode, returning an int
     // reverts on overflow
     function muli(uq112x112 memory self, int256 y) internal pure returns (int256) {
-        uint144 z = decode144(mul(self, uint256(y < 0 ? -y : y)));
-        return y < 0 ? -int256(z) : z;
+        uint256 z = FullMath.mulDiv(self._x, uint256(y < 0 ? -y : y), Q112);
+        require(z < 2**255, "FixedPoint: MULI_OVERFLOW");
+        return y < 0 ? -int256(z) : int256(z);
     }
 
     // multiply a UQ112x112 by a UQ112x112, returning a UQ112x112
@@ -120,8 +121,20 @@ library FixedPoint {
     }
 
     // square root of a UQ112x112
-    // lossy to 40 bits
+    // lossy between 0/1 and 40 bits
     function sqrt(uq112x112 memory self) internal pure returns (uq112x112 memory) {
-        return uq112x112(uint224(Babylonian.sqrt(uint256(self._x) << 32) << 40));
+        if (self._x <= uint144(-1)) {
+            return uq112x112(uint224(Babylonian.sqrt(uint256(self._x) << 112)));
+        }
+
+        uint8 safeShiftBits = 32;
+        while (safeShiftBits < 112) {
+            if (self._x < (uint256(1) << (256 - safeShiftBits - 2))) {
+                safeShiftBits += 2;
+            } else {
+                break;
+            }
+        }
+        return uq112x112(uint224(Babylonian.sqrt(uint256(self._x) << safeShiftBits) << ((112 - safeShiftBits) / 2)));
     }
 }
diff --git a/contracts/libraries/FullMath.sol b/contracts/libraries/FullMath.sol
@@ -17,7 +17,7 @@ library FullMath {
         uint256 z
     ) internal pure returns (uint256) {
         (uint256 l, uint256 h) = fullMul(x, y);
-        require(h < z);
+        require(h < z, 'FullMath: MULDIV_OVERFLOW');
         uint256 mm = mulmod(x, y, z);
         if (mm > l) h -= 1;
         l -= mm;

diff --git a/test/FixedPoint.spec.ts b/test/FixedPoint.spec.ts
@@ -110,43 +110,38 @@ describe('FixedPoint', () => {
       expect(await fixedPoint.muli([BigNumber.from(3).mul(Q112)], BigNumber.from(-2))).to.eq(BigNumber.from(-6))
     })
 
-    it('overflow', async () => {
-      await expect(fixedPoint.muli([BigNumber.from(1).mul(Q112)], BigNumber.from(2).pow(144))).to.be.revertedWith(
-        'FixedPoint: MUL_OVERFLOW'
+    it('max without overflow, largest int', async () => {
+      const maxInt = BigNumber.from(2).pow(255).sub(1)
+      expect(await fixedPoint.muli([BigNumber.from(1).mul(Q112)], maxInt)).to.be.eq(maxInt)
+
+      const minInt = BigNumber.from(2).pow(255).mul(-1)
+      await expect(fixedPoint.muli([BigNumber.from(1).mul(Q112)], minInt)).to.be.revertedWith(
+        'FixedPoint: MULI_OVERFLOW'
       )
-      await expect(
-        fixedPoint.muli([BigNumber.from(1).mul(Q112)], BigNumber.from(2).pow(144).mul(-1))
-      ).to.be.revertedWith('FixedPoint: MUL_OVERFLOW')
+
+      expect(await fixedPoint.muli([BigNumber.from(1).mul(Q112).sub(1)], minInt)).to.be.eq(
+        '-57896044618658097711785492504343942776262393067508711251869655679775811829760'
+      )
+      expect(await fixedPoint.muli([BigNumber.from(1).mul(Q112)], minInt.add(1))).to.be.eq(minInt.add(1))
     })
     it('max without overflow, largest fixed point', async () => {
-      const maxMultiplier = BigNumber.from(2).pow(32)
+      const maxMultiplier = BigNumber.from(2)
+        .pow(255 + 112)
+        .div(BigNumber.from(2).pow(224).sub(1))
       expect(await fixedPoint.muli([BigNumber.from(2).pow(224).sub(1)], maxMultiplier)).to.eq(
-        BigNumber.from('22300745198530623141535718272648361505980415')
+        BigNumber.from('57896044618658097711785492504343953926634992332820282019728792003954417336320')
       )
       await expect(fixedPoint.muli([BigNumber.from(2).pow(224).sub(1)], maxMultiplier.add(1))).to.be.revertedWith(
-        'FixedPoint: MUL_OVERFLOW'
+        'FixedPoint: MULI_OVERFLOW'
       )
-      // negative version
+
+      // negative versions
       expect(await fixedPoint.muli([BigNumber.from(2).pow(224).sub(1)], maxMultiplier.mul(-1))).to.eq(
-        BigNumber.from('22300745198530623141535718272648361505980415').mul(-1)
+        BigNumber.from('57896044618658097711785492504343953926634992332820282019728792003954417336320').mul(-1)
       )
       await expect(
         fixedPoint.muli([BigNumber.from(2).pow(224).sub(1)], maxMultiplier.add(1).mul(-1))
-      ).to.be.revertedWith('FixedPoint: MUL_OVERFLOW')
-    })
-
-    it('max without overflow, smallest fixed point', async () => {
-      const maxInt = BigNumber.from(2).pow(255).sub(1)
-      expect(await fixedPoint.muli([BigNumber.from(2)], maxInt)).to.eq(
-        BigNumber.from('22300745198530623141535718272648361505980415')
-      )
-      await expect(fixedPoint.muli([BigNumber.from(3)], maxInt)).to.be.revertedWith('FixedPoint: MUL_OVERFLOW')
-      // negative version
-      const minInt = BigNumber.from(2).pow(255).mul(-1)
-      expect(await fixedPoint.muli([BigNumber.from(1)], minInt)).to.eq(
-        BigNumber.from('11150372599265311570767859136324180752990208').mul(-1)
-      )
-      await expect(fixedPoint.muli([BigNumber.from(2)], minInt)).to.be.revertedWith('FixedPoint: MUL_OVERFLOW')
+      ).to.be.revertedWith('FixedPoint: MULI_OVERFLOW')
     })
   })
 
@@ -360,7 +355,21 @@ describe('FixedPoint', () => {
       expect((await fixedPoint.sqrt([BigNumber.from(25).mul(Q112)]))[0]).to.eq(BigNumber.from(5).mul(Q112))
     })
 
-    it('works for max uint112', async () => {
+    it('works for max uint144', async () => {
+      const input = BigNumber.from(2).pow(144).sub(1)
+      const result = (await fixedPoint.sqrt([input]))[0]
+      const expected = BigNumber.from('340282366920938463463374607431768211455')
+      expect(result).to.eq(expected)
+    })
+
+    it('works for 2**144', async () => {
+      const input = BigNumber.from(2).pow(144)
+      const result = (await fixedPoint.sqrt([input]))[0]
+      const expected = BigNumber.from('340282366920938463463374607431768211456')
+      expect(result).to.eq(expected.shr(2).shl(2))
+    })
+
+    it('works for encoded max uint112', async () => {
       const input = BigNumber.from(2).pow(112).sub(1).mul(Q112)
       const result = (await fixedPoint.sqrt([input]))[0]
       const expected = BigNumber.from('374144419156711147060143317175368417003121712037887')