Skip to content

chore(qa): refresh slippage snapshot for post-PR3 reality - #8

Merged
Hiksang merged 1 commit into
mainfrom
chore/refresh-slippage-snapshot
May 6, 2026
Merged

chore(qa): refresh slippage snapshot for post-PR3 reality#8
Hiksang merged 1 commit into
mainfrom
chore/refresh-slippage-snapshot

Conversation

@Hiksang

@Hiksang Hiksang commented May 6, 2026

Copy link
Copy Markdown
Collaborator

Summary

qa/slippage.test.ts has been red on main ever since the v1.0.12 baseline PRs landed: PR #3 hardened 14 of the 15 sites the 2026-05-05 snapshot catalogued, and PR #4 shifted line numbers in uniswap_v3.ts, so the hardcoded KNOWN_INFINITE_SLIPPAGE set is now entirely stale plus the documentation comment at uniswap_v3.ts:94 self-triggers the regex. PR #7 CI surfaced this — three Build & Test runs red with the exact same error on main HEAD via git stash.

This PR is a no-functional-change refresh of the slippage guard so future PRs stop bouncing off the same drift.

What's changed

1. Re-ground KNOWN_INFINITE_SLIPPAGE

A fresh scan with the existing regex over defi-protocols/src returns exactly one hit:

dex/uniswap_v3.ts:262   amountOutMinimum: 0n

That site sits inside UniswapV3Adapter.quote()'s client.call({...}) fallback — it's an eth_call simulation that never broadcasts a transaction, so a zero floor is correct (and the only sensible default for a price probe).

The 14 broadcast-path sites that the original snapshot tracked are all gone — PR #3 wired them through applyMinSlippage(slippage, quotedAmountOut) or explicit amount_*_min? overrides:

Adapter Old lines (stale) Fix from PR #3
algebra_v3.ts 86, 273, 278, 304 applyMinSlippage / explicit min args
balancer_v3.ts 37 applyMinSlippage
thena_cl.ts 78, 165, 190 applyMinSlippage / explicit min args
uniswap_v3.ts 90, 242, 343, 344, 363, 364, 403 applyMinSlippage / explicit min args

2. Skip comment lines in the regex pass

uniswap_v3.ts:94 is the doc comment PR #3 itself added:

// SSOT Section 7.3: never broadcast `amountOutMinimum: 0n`. If the

The existing slippageKeyPattern matched the substring inside that comment, so the test reported the very documentation that warns against 0n as a "novel unprotected site." A new commentLinePattern = /^\s*(?:\/\/|\*)/ filter skips lines whose first non-whitespace character is // or *, keeping the guard's intent (real broadcast paths) without making it impossible to mention the forbidden literal in docs.

Test plan

  • pnpm -C ts -r build — synced + dist rebuilt clean.
  • pnpm -C ts -r lint — 3 packages, tsc --noEmit clean.
  • pnpm -C ts -r test64/64 passing (was 62/64 on main HEAD).
  • Reviewer to confirm dex/uniswap_v3.ts:262 is in fact a client.call() simulation path (lines 248–266) and not a code path that ends up in executor.execute().

Why this is a separate PR from #7

PR #7 (Monad aggregators) was 1 file / +5 lines / 0 functional impact, but had to bear a chains.toml change that's unrelated to slippage tests. Bundling the snapshot refresh in would mix concerns. Once this lands, PR #7 should rebase cleanly and go green on the first try.

Out of scope

  • Refactoring UniswapV3Adapter.quote() to use a sentinel instead of 0n in the simulation path. That's a follow-up — the test now correctly distinguishes simulation-vs-broadcast via the explicit allowlist entry, so there's no exposure to fix.

🤖 Generated with Claude Code

PR #3 (feat/slippage-protection) closed 14 of the 15 sites that the
2026-05-05 baseline catalogued in qa/slippage.test.ts:KNOWN_INFINITE_SLIPPAGE.
The line numbers also shifted when PR #4 (viem-chain anchor) refactored
uniswap_v3.ts, so every entry in the original snapshot became stale on
2026-05-06 main and the test went red on every new PR (e.g. #7).

This commit re-grounds the snapshot against current main:

  Adapter scan (regex unchanged): exactly 1 hit remains in the entire
  defi-protocols/src tree —

    dex/uniswap_v3.ts:262   amountOutMinimum: 0n  (eth_call simulation
                            inside quote() fallback; never broadcasts a tx)

  PR #3 hardened the other 14 sites:
    algebra_v3.ts:86,273,278,304     -> applyMinSlippage / explicit min args
    balancer_v3.ts:37                -> applyMinSlippage
    thena_cl.ts:78,165,190           -> applyMinSlippage / explicit min args
    uniswap_v3.ts:90,242,343,344,    -> applyMinSlippage / explicit min args
                  363,364,403

In addition to refreshing the set, this commit fixes a self-trigger
bug: uniswap_v3.ts:94 carries a documentation comment that warns
against `amountOutMinimum: 0n`, and that comment matched the regex
on its own — driving a false-positive "novel site" failure. Adding a
`commentLinePattern` skip in both `it` blocks keeps the guard's
intent (catch real broadcast paths) while letting docs reference the
forbidden literal.

Verified:
  - pnpm -C ts -r build      — synced + dist rebuilt clean.
  - pnpm -C ts -r lint       — 3 packages, tsc --noEmit clean.
  - pnpm -C ts -r test       — 64/64 passing (was 62/64 before this fix).

Refs: PR #7 CI failure (3x Build & Test red) was the smoke; root cause
was the line-number drift, not anything Monad-related.
@Hiksang
Hiksang merged commit 9fc4d12 into main May 6, 2026
4 checks passed
@Hiksang
Hiksang deleted the chore/refresh-slippage-snapshot branch May 6, 2026 10:41
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