fix: agree rejects nonsense tolerances itself

src/error/ErrDecimalFloat.sol

@@ -79,3 +79,23 @@ error ScientificMinNotLessThanMax(Float scientificMin, Float scientificMax);
 /// @param actualCodehash The codehash currently at `tablesAddress` (zero
 /// if no contract is deployed there).
 error LogTablesNotDeployed(address tablesAddress, bytes32 expectedCodehash, bytes32 actualCodehash);
+
+/// @dev Thrown when `agree` is given a negative tolerance. A spread is a
+/// distance and so is never negative, which leaves nothing a negative
+/// tolerance could express. Without this revert it would be silently dominated
+/// by the other term, because the limit is the LARGER of the two, so a
+/// closeness check built on a malformed tolerance would pass as though it were
+/// well formed.
+/// @param absolute The absolute tolerance as given.
+/// @param proportional The proportional tolerance as given.
+error AgreeToleranceNegative(Float absolute, Float proportional);
+
+/// @dev Thrown when neither tolerance given to `agree` is positive. The limit
+/// is then zero and `agree` degenerates into exact equality, which `eq`
+/// already answers more cheaply and more clearly. Without this revert a caller
+/// that meant to set a tolerance and set none would get a check that only ever
+/// accepts identical values. Either tolerance ALONE may be zero; that is how a
+/// caller asks for only the other one.
+/// @param absolute The absolute tolerance as given.
+/// @param proportional The proportional tolerance as given.
+error AgreeNoPositiveTolerance(Float absolute, Float proportional);

src/lib/LibDecimalFloat.sol

@@ -11,7 +11,9 @@ import {
     LossyConversionFromFloat,
     LossyConversionToFloat,
     ZeroNegativePower,
-    PowNegativeBase
+    PowNegativeBase,
+    AgreeToleranceNegative,
+    AgreeNoPositiveTolerance
 } from "../error/ErrDecimalFloat.sol";
 import {LibDecimalFloatImplementation} from "./implementation/LibDecimalFloatImplementation.sol";
 
@@ -912,20 +914,57 @@ library LibDecimalFloat {
     /// the last place of the aligned coefficient, so a spread accepted at the
     /// boundary exceeds the limit by less than `1e-76` of its own magnitude.
     ///
-    /// The caller is responsible for the tolerances being sensible. A negative
-    /// tolerance is simply dominated by the other term, and two zero
-    /// tolerances make this an exact equality test.
+    /// NEITHER TOLERANCE MAY BE NEGATIVE, and AT LEAST ONE MUST BE POSITIVE.
+    /// Both are rejected here rather than given a meaning, and rejected here
+    /// rather than left to callers, because a guard a caller can skip is not a
+    /// guard. See `AgreeToleranceNegative` and `AgreeNoPositiveTolerance` for
+    /// what each would otherwise silently do. Either tolerance ALONE may be
+    /// zero, which is how a caller asks for only the other one.
     /// @param absolute The absolute tolerance, in the same units as the values.
     /// @param proportional The proportional tolerance, as a fraction.
     /// @param lowest The lowest value in the set.
     /// @param highest The highest value in the set.
     /// @return Whether the spread is within the limit.
     function agree(Float absolute, Float proportional, Float lowest, Float highest) internal pure returns (bool) {
+        agreeValidateTolerances(absolute, proportional);
         (int256 spreadCoefficient, int256 spreadExponent) = agreeSpread(lowest, highest);
         (int256 limitCoefficient, int256 limitExponent) = agreeLimit(absolute, proportional, lowest, highest);
         return LibDecimalFloatImplementation.lte(spreadCoefficient, spreadExponent, limitCoefficient, limitExponent);
     }
 
+    /// Rejects tolerances that do not describe a tolerance.
+    ///
+    /// A NEGATIVE tolerance cannot mean anything. The spread is a distance, so
+    /// it is non-negative, which makes `spread <= negative` unsatisfiable on its
+    /// own and makes the negative term inert under the `max` — it cannot cancel
+    /// the other term, only fail to be it. It is representable solely because
+    /// floats are signed.
+    ///
+    /// NEITHER POSITIVE is a tolerance of nothing: the limit is zero and this
+    /// degenerates into exact equality, which `eq` answers directly. A caller
+    /// reaching for a closeness test and getting exact equality has been
+    /// misunderstood rather than served.
+    ///
+    /// Rejected here rather than in each caller, because a guard a caller can
+    /// skip is not a guard.
+    ///
+    /// Both tests compare against a zero `Float` rather than unpacking, so every
+    /// representation of zero is treated alike. The positive test is stated as
+    /// `neither is greater than zero` rather than `both are zero`, so it names
+    /// the invariant rather than one case that violates it, and stays correct if
+    /// the negative check is ever changed.
+    /// @param absolute The absolute tolerance.
+    /// @param proportional The proportional tolerance.
+    function agreeValidateTolerances(Float absolute, Float proportional) private pure {
+        Float zero = packLossless(0, 0);
+        if (lt(absolute, zero) || lt(proportional, zero)) {
+            revert AgreeToleranceNegative(absolute, proportional);
+        }
+        if (!gt(absolute, zero) && !gt(proportional, zero)) {
+            revert AgreeNoPositiveTolerance(absolute, proportional);
+        }
+    }
+
     /// The distance between the two extremes, unpacked.
     ///
     /// Split out of `agree` because holding the four unpacked values and the

test/src/lib/LibDecimalFloat.agree.t.sol

@@ -8,6 +8,7 @@ import {
     LibDecimalFloatImplementation,
     ADD_MAX_EXPONENT_DIFF
 } from "src/lib/implementation/LibDecimalFloatImplementation.sol";
+import {AgreeToleranceNegative, AgreeNoPositiveTolerance} from "src/error/ErrDecimalFloat.sol";
 
 // The exponent gap at which the spread subtraction stops seeing the smaller
 // operand at all, so the spread reads as exactly the larger one. `add` aligns
@@ -34,6 +35,18 @@ contract LibDecimalFloatAgreeTest is Test {
         return LibDecimalFloat.packLossless(type(int224).max, type(int32).max);
     }
 
+    /// `agree` is an internal library call, so it inlines and its reverts land
+    /// at the test's own call depth where `vm.expectRevert` cannot see them.
+    /// The revert tests go through here, matching how the rest of this suite
+    /// asserts library reverts.
+    function agreeExternal(Float absolute, Float proportional, Float lowest, Float highest)
+        external
+        pure
+        returns (bool)
+    {
+        return LibDecimalFloat.agree(absolute, proportional, lowest, highest);
+    }
+
     /// The proportional tolerance is of the LARGER MAGNITUDE of the two
     /// extremes, so with positive values it is a proportion of the highest.
     ///
@@ -113,11 +126,14 @@ contract LibDecimalFloatAgreeTest is Test {
         assertTrue(LibDecimalFloat.agree(f(0, 0), f(1, -2), maxPositive(), maxPositive()));
     }
 
-    /// Identical values agree under any non negative tolerance, and a zero
-    /// spread is the only thing two zero tolerances accept.
+    /// Identical values agree under any valid tolerance, however small.
     function testAgreeZeroSpread() external pure {
-        assertTrue(LibDecimalFloat.agree(f(0, 0), f(0, 0), f(100, 0), f(100, 0)));
-        assertFalse(LibDecimalFloat.agree(f(0, 0), f(0, 0), f(100, 0), f(101, 0)));
+        // The smallest positive tolerance still accepts a zero spread, and
+        // still refuses a spread of 1.
+        assertTrue(LibDecimalFloat.agree(f(1, -20), f(0, 0), f(100, 0), f(100, 0)));
+        assertFalse(LibDecimalFloat.agree(f(1, -20), f(0, 0), f(100, 0), f(101, 0)));
+        // And via the proportional term alone.
+        assertTrue(LibDecimalFloat.agree(f(0, 0), f(1, -20), f(100, 0), f(100, 0)));
     }
 
     /// The comparison is numerical, so the representation of the tolerances
@@ -187,6 +203,11 @@ contract LibDecimalFloatAgreeTest is Test {
         // with an anchor up to 1e12 stays exact.
         int256 absoluteValue = int256(bound(absoluteSeed, 0, 1e12));
         int256 proportionalHundredths = int256(bound(proportionalSeed, 0, 100000));
+        // `agree` rejects a pair with no positive tolerance, so nudge rather
+        // than discard the run: bounding spends every run on a valid call.
+        if (absoluteValue == 0 && proportionalHundredths == 0) {
+            absoluteValue = 1;
+        }
 
         int256 spread = highestValue - lowestValue;
         int256 anchor = highestValue < 0
@@ -387,17 +408,144 @@ contract LibDecimalFloatAgreeTest is Test {
     /// result. `agree` never packs back, so it cannot inherit it — this asserts
     /// that rather than relying on it.
     /// forge-config: default.fuzz.runs = 20000
-    function testAgreeNeverReverts(bytes32 absolute, bytes32 proportional, bytes32 lowest, bytes32 highest)
-        external
-        pure
-    {
-        bool result = LibDecimalFloat.agree(
-            Float.wrap(absolute), Float.wrap(proportional), Float.wrap(lowest), Float.wrap(highest)
-        );
+    function testAgreeNeverRevertsOnValues(bytes32 lowest, bytes32 highest, uint256 toleranceSeed) external pure {
+        // A valid tolerance, so the only thing under test is the VALUES. The
+        // tolerance guard has its own tests; this one asserts that no pair of
+        // representable values can make the arithmetic revert.
+        Float absolute = f(int256(bound(toleranceSeed, 1, 1e12)), -3);
+        bool result = LibDecimalFloat.agree(absolute, f(0, 0), Float.wrap(lowest), Float.wrap(highest));
         // Only that it returned. The value is whatever the operands imply.
         assertTrue(result || !result);
     }
 
+    /// A NEGATIVE TOLERANCE IS REJECTED, either side.
+    ///
+    /// It cannot mean anything: the spread is a distance and so non-negative,
+    /// which makes `spread <= negative` unsatisfiable, and under the `max` the
+    /// negative term is inert — it cannot cancel the other term, only fail to
+    /// be it. Left unrejected it would be silently dominated and the check
+    /// would pass on a malformed tolerance.
+    function testAgreeRejectsNegativeTolerance() external {
+        vm.expectRevert(abi.encodeWithSelector(AgreeToleranceNegative.selector, f(-1, 0), f(1, -2)));
+        this.agreeExternal(f(-1, 0), f(1, -2), f(99, 0), f(100, 0));
+
+        vm.expectRevert(abi.encodeWithSelector(AgreeToleranceNegative.selector, f(1, 0), f(-1, -2)));
+        this.agreeExternal(f(1, 0), f(-1, -2), f(99, 0), f(100, 0));
+
+        // Rejected even where the other term would have accepted the spread on
+        // its own, which is the case that would otherwise pass silently.
+        vm.expectRevert(abi.encodeWithSelector(AgreeToleranceNegative.selector, f(-1, 0), f(1, 0)));
+        this.agreeExternal(f(-1, 0), f(1, 0), f(99, 0), f(100, 0));
+
+        // And where both are negative.
+        vm.expectRevert(abi.encodeWithSelector(AgreeToleranceNegative.selector, f(-1, 0), f(-1, 0)));
+        this.agreeExternal(f(-1, 0), f(-1, 0), f(100, 0), f(100, 0));
+    }
+
+    /// NEITHER TOLERANCE POSITIVE IS REJECTED. The limit would be zero and
+    /// `agree` would degenerate into exact equality, which `eq` answers
+    /// directly, so a caller that meant to set a tolerance and set none is
+    /// misunderstood rather than served.
+    function testAgreeRejectsNoPositiveTolerance() external {
+        vm.expectRevert(abi.encodeWithSelector(AgreeNoPositiveTolerance.selector, f(0, 0), f(0, 0)));
+        this.agreeExternal(f(0, 0), f(0, 0), f(100, 0), f(100, 0));
+    }
+
+    /// A packed zero with a NON-ZERO EXPONENT is rejected the same way, which is
+    /// what makes the guard numeric rather than bytewise.
+    ///
+    /// `packLossless` cannot construct one — it canonicalises every zero to
+    /// `FLOAT_ZERO`, which the first two assertions pin, so a test written with
+    /// `f(0, 5)` silently tests the canonical case twice. These are built with
+    /// `Float.wrap` instead: the exponent occupies the high 32 bits, so a zero
+    /// coefficient with exponent 5 and with exponent -5 are distinct words that
+    /// are both numerically zero.
+    function testAgreeRejectsNonCanonicalZeroTolerances() external {
+        Float zeroExponentFive = Float.wrap(bytes32(uint256(5) << 224));
+        Float zeroExponentMinusFive = Float.wrap(bytes32(uint256(0xfffffffb) << 224));
+
+        // The premise: these are not what `packLossless` would give, and they
+        // are numerically zero.
+        assertTrue(Float.unwrap(f(0, 5)) == Float.unwrap(LibDecimalFloat.FLOAT_ZERO), "f(0,5) is not canonical zero");
+        assertTrue(
+            Float.unwrap(zeroExponentFive) != Float.unwrap(LibDecimalFloat.FLOAT_ZERO), "wrapped zero is canonical"
+        );
+        assertTrue(zeroExponentFive.isZero(), "exponent 5 zero is not zero");
+        assertTrue(zeroExponentMinusFive.isZero(), "exponent -5 zero is not zero");
+
+        vm.expectRevert(
+            abi.encodeWithSelector(AgreeNoPositiveTolerance.selector, zeroExponentFive, zeroExponentMinusFive)
+        );
+        this.agreeExternal(zeroExponentFive, zeroExponentMinusFive, f(100, 0), f(100, 0));
+
+        // Mixed with a canonical zero, either way round.
+        vm.expectRevert(abi.encodeWithSelector(AgreeNoPositiveTolerance.selector, zeroExponentFive, f(0, 0)));
+        this.agreeExternal(zeroExponentFive, f(0, 0), f(100, 0), f(100, 0));
+
+        vm.expectRevert(abi.encodeWithSelector(AgreeNoPositiveTolerance.selector, f(0, 0), zeroExponentMinusFive));
+        this.agreeExternal(f(0, 0), zeroExponentMinusFive, f(100, 0), f(100, 0));
+    }
+
+    /// A non-canonical zero is not itself the problem: paired with a positive
+    /// term it is accepted, exactly as a canonical zero is. Without this the
+    /// test above would also pass for a guard that rejected any non-canonical
+    /// tolerance.
+    function testAgreeAcceptsNonCanonicalZeroWithPositive() external pure {
+        Float zeroExponentFive = Float.wrap(bytes32(uint256(5) << 224));
+        assertTrue(LibDecimalFloat.agree(zeroExponentFive, f(1, -2), f(99, 0), f(100, 0)));
+        assertTrue(LibDecimalFloat.agree(f(1, 0), zeroExponentFive, f(100, 0), f(101, 0)));
+    }
+
+    /// Either tolerance ALONE may be zero. This is the boundary between the two
+    /// guards: one positive term is enough, and it is what makes the rejection
+    /// above about having no tolerance rather than about zero appearing at all.
+    function testAgreeAcceptsOneZeroTolerance() external pure {
+        assertTrue(LibDecimalFloat.agree(f(1, 0), f(0, 0), f(100, 0), f(100, 0)));
+        assertTrue(LibDecimalFloat.agree(f(0, 0), f(1, -2), f(100, 0), f(100, 0)));
+    }
+
+    /// THE GUARD, STATED AS A LAW over arbitrary packed tolerances: `agree`
+    /// reverts exactly when a tolerance is negative, or when neither is
+    /// positive, and returns otherwise.
+    ///
+    /// The expectation is derived from the tolerances' own signs via `lt`/`gt`
+    /// against zero, not from calling `agree`, so this pins the guard's domain
+    /// rather than restating its implementation. Values are held fixed and
+    /// valid, so the only variable is the tolerance pair.
+    /// forge-config: default.fuzz.runs = 5000
+    function testAgreeGuardHoldsForArbitraryTolerances(bytes32 absoluteRaw, bytes32 proportionalRaw) external {
+        Float absolute = Float.wrap(absoluteRaw);
+        Float proportional = Float.wrap(proportionalRaw);
+        Float zero = f(0, 0);
+
+        bool anyNegative = absolute.lt(zero) || proportional.lt(zero);
+        bool nonePositive = !absolute.gt(zero) && !proportional.gt(zero);
+
+        if (anyNegative) {
+            vm.expectRevert(abi.encodeWithSelector(AgreeToleranceNegative.selector, absolute, proportional));
+            this.agreeExternal(absolute, proportional, f(100, 0), f(100, 0));
+        } else if (nonePositive) {
+            vm.expectRevert(abi.encodeWithSelector(AgreeNoPositiveTolerance.selector, absolute, proportional));
+            this.agreeExternal(absolute, proportional, f(100, 0), f(100, 0));
+        } else {
+            // A zero spread, so a valid tolerance of any size accepts it.
+            assertTrue(this.agreeExternal(absolute, proportional, f(100, 0), f(100, 0)));
+        }
+    }
+
+    /// The guard runs BEFORE the arithmetic, so a bad tolerance is rejected even
+    /// where the spread is the widest representable. A guard placed after the
+    /// spread and limit were computed would still reject these, so this is about
+    /// ordering being irrelevant to the outcome rather than about the ordering
+    /// itself.
+    function testAgreeRejectsBadToleranceAtTheRangeExtremes() external {
+        vm.expectRevert(abi.encodeWithSelector(AgreeToleranceNegative.selector, f(-1, 0), f(1, -2)));
+        this.agreeExternal(f(-1, 0), f(1, -2), minNegative(), maxPositive());
+
+        vm.expectRevert(abi.encodeWithSelector(AgreeNoPositiveTolerance.selector, f(0, 0), f(0, 0)));
+        this.agreeExternal(f(0, 0), f(0, 0), minNegative(), maxPositive());
+    }
+
     /// The exact counterexample `testSubPacked` fails on, driven through
     /// `agree`. Pinned as a case in its own right so a future change to the
     /// packing path cannot quietly make `agree` revert.