From a2b03a93a80ac536f66469e32e7a3b1c1cf267ec Mon Sep 17 00:00:00 2001 From: dan Date: Mon, 7 Sep 2026 18:56:11 +1200 Subject: [PATCH] test(pool-cl): testSwap's InvalidFeeForExactOut expectation was dead code MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit CI went red on `infinity-core#2` - a PR that touched a workflow file and a comment and ZERO Solidity - with `testSwap` reporting `InvalidFeeForExactOut()` after 8,194 runs. The same suite had passed two days earlier on a different seed. That is the signature of a hole in a test's own expectations rather than a flake, and it is. The expectation was written as: if (zeroForOne) { ...price limit... } else if (!zeroForOne) { ...price limit... } else if (amountSpecified <= 0 && swapFee == ONE_HUNDRED_PERCENT_FEE) { vm.expectRevert(InvalidFeeForExactOut) } The first two branches are exhaustive, so the third is UNREACHABLE for every input. The test could therefore never arm that expectation, and failed - rather than passing - whenever the fuzzer drew a 100% fee alongside a valid price limit and a non-negative amountSpecified. Two further bugs were hiding behind the dead branch, and both are fixed: * the guard read `amountSpecified <= 0`, but the pool reverts when the swap is NOT exact input and `exactInput` is `amountSpecified < 0`. The case is `>= 0`. Since the test bounds amountSpecified to [0, int128.max], the old condition could only ever have matched exactly zero; * it compared the LP fee alone against 100%, where the pool compares `state.swapFee` - the LP fee COMPOSITED with this direction's protocol fee. Rewritten to mirror `CLPool.swap`'s own order: the price limit is checked first and the fee case second, and only one of the two can be the revert the pool produces. Proof, rather than a re-run on a kinder seed: `test_testSwapCoversTheHundredPercentFeeExactOutputCase` replays the exact CI counterexample with no fuzzer. Its five `Bound result` logs match the CI trace exactly (1000000, 1019, 256, 6651, 1). It FAILS with `InvalidFeeForExactOut()` against the unfixed testSwap and passes against the fixed one. `src/` is untouched, so `upstream-guard` is unaffected. Full suite at CI fuzz depth: 844 passed, 0 failed. ⚠️ Upstream carries the same defect; it is not ours. Worth reporting. Co-Authored-By: Claude Opus 5 (1M context) --- test/pool-cl/libraries/CLPool.t.sol | 102 ++++++++++++++++++++-------- 1 file changed, 73 insertions(+), 29 deletions(-) diff --git a/test/pool-cl/libraries/CLPool.t.sol b/test/pool-cl/libraries/CLPool.t.sol index 0ad1765..1b56b87 100644 --- a/test/pool-cl/libraries/CLPool.t.sol +++ b/test/pool-cl/libraries/CLPool.t.sol @@ -148,35 +148,43 @@ contract PoolTest is Test { ? swapParams.lpFeeOverride.removeOverrideAndValidate(LPFeeLibrary.ONE_HUNDRED_PERCENT_FEE) : lpFee; - if (swapParams.zeroForOne) { - if (swapParams.sqrtPriceLimitX96 >= slot0.sqrtPriceX96()) { - vm.expectRevert( - abi.encodeWithSelector( - CLPool.InvalidSqrtPriceLimit.selector, slot0.sqrtPriceX96(), swapParams.sqrtPriceLimitX96 - ) - ); - } else if (swapParams.sqrtPriceLimitX96 <= TickMath.MIN_SQRT_RATIO) { - vm.expectRevert( - abi.encodeWithSelector( - CLPool.InvalidSqrtPriceLimit.selector, slot0.sqrtPriceX96(), swapParams.sqrtPriceLimitX96 - ) - ); - } - } else if (!swapParams.zeroForOne) { - if (swapParams.sqrtPriceLimitX96 <= slot0.sqrtPriceX96()) { - vm.expectRevert( - abi.encodeWithSelector( - CLPool.InvalidSqrtPriceLimit.selector, slot0.sqrtPriceX96(), swapParams.sqrtPriceLimitX96 - ) - ); - } else if (swapParams.sqrtPriceLimitX96 >= TickMath.MAX_SQRT_RATIO) { - vm.expectRevert( - abi.encodeWithSelector( - CLPool.InvalidSqrtPriceLimit.selector, slot0.sqrtPriceX96(), swapParams.sqrtPriceLimitX96 - ) - ); - } - } else if (swapParams.amountSpecified <= 0 && swapFee == LPFeeLibrary.ONE_HUNDRED_PERCENT_FEE) { + // `CLPool.swap` rejects two things before it swaps, in this order: an out-of-range + // price limit, and then a swap fee of 100% on an exact-OUTPUT swap (the input would be + // entirely consumed by the fee). Mirror that order and that precedence exactly - only + // one of the two can ever be the revert the pool actually produces. + // + // This used to be written as `if (zeroForOne) ... else if (!zeroForOne) ... else if + // ()`. Those first two branches are exhaustive, so the fee expectation + // was DEAD CODE and could never arm, and the test therefore failed - rather than + // passing - whenever the fuzzer happened to draw a 100% fee together with a valid + // price limit and a non-negative amountSpecified. Seed-dependent and rare, which is + // the worst shape for a CI failure: it looks like flake and it is not. Two further + // bugs were hiding behind the dead branch, and both are corrected here: + // + // * the guard read `amountSpecified <= 0`, but the pool reverts when the swap is + // NOT exact input, and `exactInput` is `amountSpecified < 0`. The case is + // `>= 0`. The test bounds amountSpecified to [0, int128.max], so the old + // condition could only ever have matched exactly zero; + // * it compared the LP fee alone against 100%, where the pool compares + // `state.swapFee` - the LP fee COMPOSITED with this direction's protocol fee. + bool priceLimitReverts = swapParams.zeroForOne + ? (swapParams.sqrtPriceLimitX96 >= slot0.sqrtPriceX96() + || swapParams.sqrtPriceLimitX96 <= TickMath.MIN_SQRT_RATIO) + : (swapParams.sqrtPriceLimitX96 <= slot0.sqrtPriceX96() + || swapParams.sqrtPriceLimitX96 >= TickMath.MAX_SQRT_RATIO); + + uint16 directionProtocolFee = + swapParams.zeroForOne ? slot0.protocolFee().getZeroForOneFee() : slot0.protocolFee().getOneForZeroFee(); + uint24 effectiveSwapFee = + directionProtocolFee == 0 ? swapFee : ProtocolFeeLibrary.calculateSwapFee(directionProtocolFee, swapFee); + + if (priceLimitReverts) { + vm.expectRevert( + abi.encodeWithSelector( + CLPool.InvalidSqrtPriceLimit.selector, slot0.sqrtPriceX96(), swapParams.sqrtPriceLimitX96 + ) + ); + } else if (swapParams.amountSpecified >= 0 && effectiveSwapFee >= LPFeeLibrary.ONE_HUNDRED_PERCENT_FEE) { vm.expectRevert(CLPool.InvalidFeeForExactOut.selector); } @@ -208,6 +216,42 @@ contract PoolTest is Test { } } + /// @notice The exact counterexample CI drew on 2026-09-07, replayed with no fuzzer. + /// + /// `testSwap` reported `InvalidFeeForExactOut()` after 8,194 runs on seed + /// 0x8a9eeedc...59e, and the same suite had passed two days earlier on a different seed. + /// That is the signature of a hole in a test's own expectations rather than a flake, and + /// it was: the fee expectation sat in a branch that could never be reached (see the + /// comment in `testSwap`). Replaying the draw pins it deterministically, so the fix + /// cannot regress into "re-run it and hope for a kinder seed". + /// + /// The draw: an LP fee of exactly 1,000,000 pips - 100%, the top of the bound - and a + /// positive `amountSpecified`, which is an exact-OUTPUT swap. The pool is right to refuse + /// it: a 100% fee consumes the whole input, so no output can be delivered. + function test_testSwapCoversTheHundredPercentFeeExactOutputCase() public { + testSwap( + 645326474426547203313410069153905908525362434349, + CLPool.ModifyLiquidityParams({ + owner: address(0), + tickLower: 0, + tickUpper: 0, + liquidityDelta: 194, + tickSpacing: 32768, + salt: bytes32(uint256(0x16)) + }), + CLPool.SwapParams({ + tickSpacing: -8388608, + zeroForOne: true, + amountSpecified: 6651, + sqrtPriceLimitX96: 288230376151711743, + lpFeeOverride: 300000 + }), + 1_000_000, + 13022, + 256 + ); + } + function testDonate( uint160 sqrtPriceX96, CLPool.ModifyLiquidityParams memory params,