From ba28c8b0f5c46ef45125906f8d5ca6f68e306e42 Mon Sep 17 00:00:00 2001 From: David Meister Date: Sun, 27 Sep 2026 13:34:15 +0000 Subject: [PATCH 1/2] fix: agree rejects nonsense tolerances itself MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `agree` accepted a negative tolerance and accepted a pair with no positive tolerance, leaving both to callers. That was wrong, and the natspec claiming tolerance validity is the caller's business was wrong with it. A negative tolerance cannot mean anything. The spread is a distance, so it is non-negative: `spread <= negative` is unsatisfiable on its own, and under the `max` the negative term is inert — it cannot cancel the other term, only fail to be it. It is representable solely because floats are signed. Accepting it meant a closeness check built on a malformed tolerance silently passed on the other term. Neither tolerance positive is a tolerance of nothing: the limit is zero and `agree` degenerates into exact equality, which `eq` answers directly and more cheaply. A caller that meant to set a tolerance and set none was being misunderstood rather than served. Both are now rejected in `agree`, with `AgreeToleranceNegative` and `AgreeNoPositiveTolerance` carrying the offending tolerances. Either tolerance ALONE may still be zero; that is how a caller asks for only the other one. Rejected HERE, not in callers. rainlang's `agree` word carried these checks while the library did not, which meant any other consumer calling `LibDecimalFloat.agree` directly — st0x.attest, or the next one — got no guard at all. A guard a caller can skip is not a guard, and moving the arithmetic into this library without the guard left the guard behind by accident. Three existing tests changed because the behaviour changed: - `testAgreeZeroSpread` passed 0/0 and now uses the smallest positive tolerance, so it still tests what it meant to. - `testAgreeAgainstIntegerOracle` can fuzz 0/0, so it nudges to a valid pair rather than discarding the run. - `testAgreeNeverReverts` asserted the opposite of the guard. Rescoped to `testAgreeNeverRevertsOnValues`: a valid tolerance, fuzzed values, which is the claim that was actually worth making — no pair of representable values can make the arithmetic revert. Added `testAgreeRejectsNegativeTolerance` (either side, both sides, and the case where the other term would have accepted the spread on its own, which is the one that used to pass silently), `testAgreeRejectsNoPositiveTolerance` (including zeros with non-zero exponents, since every representation of zero is treated alike), and `testAgreeAcceptsOneZeroTolerance` for the boundary between the two. The revert tests go through an external wrapper, because `agree` is an internal library call and its reverts otherwise land at the test's own call depth where `vm.expectRevert` cannot see them. 53 suites, 499 of 500. The one failure is `testSubPacked` reverting `ExponentUnderflow`, which is #271 and reproduces on main. Lint and fmt clean. Co-Authored-By: Claude Opus 5 (1M context) --- src/error/ErrDecimalFloat.sol | 20 ++++++ src/lib/LibDecimalFloat.sol | 47 +++++++++++-- test/src/lib/LibDecimalFloat.agree.t.sol | 88 +++++++++++++++++++++--- 3 files changed, 140 insertions(+), 15 deletions(-) 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..66e66462 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,62 @@ 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)); + + // Every representation of zero is treated alike, so a zero with a + // non-zero exponent is rejected the same way. + vm.expectRevert(abi.encodeWithSelector(AgreeNoPositiveTolerance.selector, f(0, 5), f(0, -5))); + this.agreeExternal(f(0, 5), f(0, -5), f(100, 0), f(100, 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 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. From a296f2eb8ef63de39ae41739e541f9ce394b7600 Mon Sep 17 00:00:00 2001 From: David Meister Date: Sun, 27 Sep 2026 14:55:06 +0000 Subject: [PATCH 2/2] test: the zero-representation case tested the canonical zero twice MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `packLossless` canonicalises every zero to `FLOAT_ZERO`, so `f(0, 5)` and `f(0, -5)` are the same word as `f(0, 0)`. The assertion claiming "every representation of zero is treated alike" therefore tested the canonical case a second time and said nothing about non-canonical zeros. Raised by CodeRabbit on #281 and confirmed: the new test asserts that premise explicitly, so it cannot rot back. Non-canonical zeros are now built with `Float.wrap` — the exponent is the high 32 bits, so a zero coefficient at exponent 5 and at exponent -5 are distinct words that are both numerically zero — and checked alone, and mixed with a canonical zero either way round. Four tests added around the guard: - `testAgreeRejectsNonCanonicalZeroTolerances`, with the premise assertions above, which is what makes the guard demonstrably numeric rather than bytewise. - `testAgreeAcceptsNonCanonicalZeroWithPositive`. Without it the test above also passes for a guard that wrongly rejected ANY non-canonical tolerance, so this is what makes the rejection specifically about having no tolerance. - `testAgreeGuardHoldsForArbitraryTolerances`: the guard as a law over arbitrary packed pairs, 5000 runs. The expectation comes from the tolerances' own signs via `lt`/`gt` against zero rather than from calling `agree`, so it pins the guard's domain instead of restating its implementation. Values are held fixed and valid so the tolerance pair is the only variable. - `testAgreeRejectsBadToleranceAtTheRangeExtremes`: rejection holds where the spread is the widest representable. 33 in the agree suite, up from 29. 53 suites, 503 of 504; the failure is #271 and reproduces on main. Lint and fmt clean. Co-Authored-By: Claude Opus 5 (1M context) --- test/src/lib/LibDecimalFloat.agree.t.sol | 90 ++++++++++++++++++++++-- 1 file changed, 86 insertions(+), 4 deletions(-) diff --git a/test/src/lib/LibDecimalFloat.agree.t.sol b/test/src/lib/LibDecimalFloat.agree.t.sol index 66e66462..0eed4a76 100644 --- a/test/src/lib/LibDecimalFloat.agree.t.sol +++ b/test/src/lib/LibDecimalFloat.agree.t.sol @@ -449,11 +449,51 @@ contract LibDecimalFloatAgreeTest is Test { 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)); + } - // Every representation of zero is treated alike, so a zero with a - // non-zero exponent is rejected the same way. - vm.expectRevert(abi.encodeWithSelector(AgreeNoPositiveTolerance.selector, f(0, 5), f(0, -5))); - this.agreeExternal(f(0, 5), f(0, -5), 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 @@ -464,6 +504,48 @@ contract LibDecimalFloatAgreeTest is Test { 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.