From 1a9d61a42b1eaa0f441e55c258bb2a3730a0cb85 Mon Sep 17 00:00:00 2001 From: David Meister Date: Sat, 26 Sep 2026 19:14:01 +0000 Subject: [PATCH] fix: testEqXEqY asserted a false contract about eq, and passed by luck MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `testEqXEqY` claims that for two equal floats with different coefficients, dividing one coefficient by the other gives the power of ten between their exponents. It branched on the SIGNED comparison of the coefficients, then asserted a relation that only holds for MAGNITUDE ordering, and divided the wrong way round in both branches. Both branches were wrong, so the test failed for every equal pair whose coefficients differ, either sign: - `-30e-4` and `-3e-3` are both `-0.003`. `-3 > -30`, so it took the `y > x` branch and asserted `exponentY < exponentX` — but `-3` is the SMALLER magnitude and correctly carries the LARGER exponent. - `30e-4` and `3e-3` are both `0.003`. It took the other branch and asserted `y / x == 10^(Y - X)`, which is `3 / 30 == 0`, against an expected `10`. The division is inverted: it is the larger magnitude divided by the smaller. It passed because random `int256` fuzz inputs are almost never two numerically equal floats whose coefficients differ by an exact power of ten, so the broken branches were almost never reached. CI found it on rain.math.float#278 only because adding tests there changed the test count and ordering, which changed the per-test fuzz sequence. Confirmed seed-independently by driving main's own test body with the counterexample. `eq` itself is correct throughout — `eq(-30, -4, -3, -3)` returns true, which is right. The defect was entirely in the assertion, which is worse than a code bug for anyone reading the suite to learn the contract. Now keyed on magnitude via `absUnsignedSignedCoefficient`, with the invariant extracted to an internal helper so the fuzz form and the concrete cases assert the same thing rather than two similar things that can drift. `testEqXEqYKnownUnequalCoefficients` pins six pairs — both signs, both argument orders, single and multi decade gaps — so the invariant no longer depends on the fuzzer stumbling onto them. The `10 **` cast is hoisted to its own named statement. A `disable-next-line` above a multi-argument call is fragile, because `forge fmt` reflows the call and moves the cast off the next line, silently detaching the suppression — which happened once while writing this. 48 suites, 446 tests, 0 failed. Lint and fmt clean. Co-Authored-By: Claude Opus 5 (1M context) --- .../LibDecimalFloatImplementation.eq.t.sol | 73 +++++++++++++++---- 1 file changed, 59 insertions(+), 14 deletions(-) diff --git a/test/src/lib/implementation/LibDecimalFloatImplementation.eq.t.sol b/test/src/lib/implementation/LibDecimalFloatImplementation.eq.t.sol index e34de05..8977308 100644 --- a/test/src/lib/implementation/LibDecimalFloatImplementation.eq.t.sol +++ b/test/src/lib/implementation/LibDecimalFloatImplementation.eq.t.sol @@ -27,28 +27,54 @@ contract LibDecimalFloatImplementationEqTest is Test { LibDecimalFloatImplementation.eq(1, type(int256).max, 1, type(int256).min); } - /// if xeX == yeY, then x / y == 10^(X - Y) || y / x == 10^(Y - X) + /// If `xeX == yeY` with different coefficients, then the coefficient of + /// LARGER MAGNITUDE carries the smaller exponent, and dividing the larger + /// magnitude by the smaller gives exactly the power of ten between their + /// exponents. + /// + /// MAGNITUDE, not signed order. For negative coefficients the signed-larger + /// value is the smaller magnitude: `-30e-4` and `-3e-3` are both `-0.003`, + /// and `-3 > -30` while `-3` is the smaller magnitude and carries the larger + /// exponent. Keying the branch on `y > x` therefore asserted the exponent + /// relation backwards for negatives, and divided the wrong way round for + /// both signs. function testEqXEqY(int256 x, int256 exponentX, int256 y, int256 exponentY) external pure { + assertEqXEqYInvariant(x, exponentX, y, exponentY); + } + + /// The invariant itself, so the fuzz form and the concrete cases below + /// assert the same thing rather than two similar things that can drift. + function assertEqXEqYInvariant(int256 x, int256 exponentX, int256 y, int256 exponentY) internal pure { bool eq = LibDecimalFloatImplementation.eq(x, exponentX, y, exponentY); if (eq) { if (x == y) { assertTrue(exponentX == exponentY || x == 0); - } else if (y > x) { - assertTrue(exponentY < exponentX, "y > x but exponentY >= exponentX"); - assertTrue(exponentX - exponentY < 77, "y > x but exponentX - exponentY >= 77"); - // we assert that exponentY < exponentX and the diff is < 77. - // forge-lint: disable-next-line(unsafe-typecast) - assertEq(x / y, int256(10 ** uint256(exponentX - exponentY)), "y > x but x / y != 10^(X - Y)"); - assertEq(x % y, 0, "y > x but x % y != 0"); } else { - assertTrue(exponentX < exponentY, "x < y but exponentX >= exponentY"); - assertTrue(exponentY - exponentX < 77, "x < y but exponentY - exponentX >= 77"); - // x < y and they are eq so exponentY - exponentX will always be - // positive. + // Equal and different coefficients means both are non-zero and + // share a sign, so the division below is exact and positive. + int256 large; + int256 largeExponent; + int256 small; + int256 smallExponent; + if ( + LibDecimalFloatImplementation.absUnsignedSignedCoefficient(x) + > LibDecimalFloatImplementation.absUnsignedSignedCoefficient(y) + ) { + (large, largeExponent, small, smallExponent) = (x, exponentX, y, exponentY); + } else { + (large, largeExponent, small, smallExponent) = (y, exponentY, x, exponentX); + } + + assertTrue(largeExponent < smallExponent, "the larger magnitude does not carry the smaller exponent"); + assertTrue(smallExponent - largeExponent < 77, "the exponent gap is >= 77"); + // Hoisted to its own statement so the suppression cannot be + // detached from the cast by a reflow: the gap is asserted to be + // in [1, 76] on the lines above, so this is exact. // forge-lint: disable-next-line(unsafe-typecast) - assertEq(y / x, int256(10 ** uint256(exponentY - exponentX)), "x < y but y / x != 10^(Y - X)"); - assertEq(y % x, 0, "x < y but y % x != 0"); + int256 expectedRatio = int256(10 ** uint256(smallExponent - largeExponent)); + assertEq(large / small, expectedRatio, "large / small != 10^(gap)"); + assertEq(large % small, 0, "large % small != 0"); } } else { if (x == y) { @@ -57,6 +83,25 @@ contract LibDecimalFloatImplementationEqTest is Test { } } + /// The concrete pairs the fuzz form was wrong about, asserted directly so + /// the invariant no longer depends on the fuzzer generating an exact + /// power-of-ten ratio between two equal values — which is why the defect + /// survived: random `int256` pairs are almost never equal-but-differently- + /// represented, so the broken branches were almost never reached. + /// + /// `-30e-4` and `-3e-3` are both `-0.003`. `30e-4` and `3e-3` are both + /// `0.003`. Both directions of argument order, so neither relies on the + /// larger magnitude arriving first. + function testEqXEqYKnownUnequalCoefficients() external pure { + assertEqXEqYInvariant(-30, -4, -3, -3); + assertEqXEqYInvariant(-3, -3, -30, -4); + assertEqXEqYInvariant(30, -4, 3, -3); + assertEqXEqYInvariant(3, -3, 30, -4); + // A wider gap, to exercise more than a single power of ten. + assertEqXEqYInvariant(-3000, -6, -3, -3); + assertEqXEqYInvariant(3000, -6, 3, -3); + } + /// xeX != yeY if x != y (assuming maximized representation) function testEqXNotY(int256 x, int256 exponentX, int256 y, int256 exponentY) external pure { (x, exponentX,) = LibDecimalFloatImplementation.maximize(x, exponentX);