diff --git a/src/error/ErrDecimalFloat.sol b/src/error/ErrDecimalFloat.sol index 3d375f27..3980fb9a 100644 --- a/src/error/ErrDecimalFloat.sol +++ b/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); diff --git a/src/lib/LibDecimalFloat.sol b/src/lib/LibDecimalFloat.sol index 2c2d251d..fd7cc773 100644 --- a/src/lib/LibDecimalFloat.sol +++ b/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 diff --git a/test/src/lib/LibDecimalFloat.agree.t.sol b/test/src/lib/LibDecimalFloat.agree.t.sol index 7f8ca039..0eed4a76 100644 --- a/test/src/lib/LibDecimalFloat.agree.t.sol +++ b/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.