Skip to content

test(pool-cl): testSwap's InvalidFeeForExactOut expectation was dead code - #3

Merged
danvaneijck merged 1 commit into
injectivefrom
fix/testswap-dead-exactout-branch
Sep 7, 2026
Merged

test(pool-cl): testSwap's InvalidFeeForExactOut expectation was dead code#3
danvaneijck merged 1 commit into
injectivefrom
fix/testswap-dead-exactout-branch

Conversation

@danvaneijck

Copy link
Copy Markdown
Member

Unblocks #2, which is red for a reason that has nothing to do with it: that PR touches a workflow file and a comment and zero Solidity, src/ identical to pinned upstream passes on it, and testSwap still reported InvalidFeeForExactOut() after 8,194 runs. The same suite passed on 2026-09-04 with a different seed.

It is not a flake. It is a hole in the test's own expectations, and it was always there.

The defect

if (zeroForOne)       { …price limit… }
else if (!zeroForOne) { …price limit… }
else if (amountSpecified <= 0 && swapFee == ONE_HUNDRED_PERCENT_FEE) {
    vm.expectRevert(CLPool.InvalidFeeForExactOut.selector);   // ← unreachable
}

The first two branches are exhaustive, so the third can never execute, for any input. The test could not arm that expectation, and therefore failed rather than passing whenever the fuzzer drew a 100% fee alongside a valid price limit and a non-negative amountSpecified.

Two more bugs were hiding behind the dead branch, and both are fixed here:

was is
the exactness test amountSpecified <= 0 >= 0 — the pool reverts when the swap is not exact input, and exactInput is amountSpecified < 0. The test bounds amountSpecified to [0, int128.max], so the old condition could only ever have matched exactly zero
the fee compared the LP fee alone, == 100% state.swapFee — the LP fee composited with this direction's protocol fee — >= 100%, as CLPool.swap does

Rewritten to mirror CLPool.swap's own order: the price limit is checked first, the fee case second, and only one of the two can be the revert the pool actually produces.

The proof, rather than a re-run on a kinder seed

test_testSwapCoversTheHundredPercentFeeExactOutputCase replays the exact CI counterexample with no fuzzer involved. Its five Bound result logs match the CI trace exactly — 1000000, 1019, 256, 6651, 1 — so the replay is faithful, not approximate.

  • against the unfixed testSwap: [FAIL: InvalidFeeForExactOut()]
  • against the fixed one: [PASS]

The draw was lpFee = 1,000,000 (100%, the top of the test's own bound) with a positive amountSpecified, i.e. an exact-output swap. The pool is right to refuse it: a 100% fee consumes the whole input, so no output can be delivered.

⚠️ What I could not establish: a failure rate. The draw only reproduces under the full 75-suite run — narrowing to --match-test testSwap or even to this one suite file changes forge's per-test input stream, and 25 random seeds at 10,000 runs each produced zero failures on the unfixed code that way. So sampling says nothing here in either direction. The case rests on the structural argument above, which needs no sampling: the branch is unreachable by inspection.

Checks

  • Full suite, FOUNDRY_PROFILE=ci forge test --isolate: 844 passed, 0 failed (CI's failing run was 837 passed + 1 failed)
  • forge fmt --check: clean
  • src/ is untouchedgit diff --name-only -- src/ is empty, so upstream-guard is unaffected and the audits still describe the deployed bytecode

Upstream

⚠️ This defect is upstream's, not ourstest/ is not covered by the byte-identical rule, which is the only reason it can be fixed here. It is worth reporting to PancakeSwap so the next pin move does not reintroduce it. Not filed yet.

🤖 Generated with Claude Code

…code

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) <noreply@anthropic.com>
@danvaneijck
danvaneijck merged commit d5bf4ac into injective Sep 7, 2026
4 checks passed
@danvaneijck
danvaneijck deleted the fix/testswap-dead-exactout-branch branch September 7, 2026 07:56
danvaneijck added a commit that referenced this pull request Sep 7, 2026
This PR is CI-only - a workflow file and a comment, zero Solidity - and
its Forge Tests job was red on an upstream test defect it did not cause:
testSwap's InvalidFeeForExactOut expectation sat in an unreachable
else-branch. Fixed on injective by #3.

MERGE rather than rebase: a rebase rewrites the fork commits under new
SHAs, GitHub reports the PR CONFLICTING and then creates NO workflow runs
at all - the guard and the test suite both silently do not run.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant