refactor(swap): consolidate min-out/max-in slippage math into a shared helper (#715) - #829
Merged
Merged
Conversation
…d helper (#715) The exact-IN min-out floor and the exact-OUT max-in ceiling were copy-pasted across five call sites. Both size a fund-safety bound baked into calldata the user signs, so a fix applied to one copy silently leaves the others wrong — the recurrence surface ARCHITECTURE.md 5.5 calls out. New src/modules/shared/slippage.ts exports applyMinOut / applyMaxIn with the byte-identical arithmetic (floor truncates down; ceiling keeps the + 9_999n round-up bias). Call sites now import it: - uniswap-swap/index.ts (module-private applySlippageExactIn/Out deleted) - curve/actions.ts (add_liquidity minLpOut, swap minDy) - swap/index.ts (1inch dstAmount minOut, LiFi exact-out approval cap) - tron/sunswap-swap.ts (SunSwap V2 minOut — a 5th copy the issue's enumeration predates) Pure refactor: no value, no rounding and no gate changes. The helper deliberately adds no bps validation — every call site already runs its own slippage gate before reaching here. Closes #715
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #715
What changed and why
The exact-IN min-out floor
(quotedOut * BigInt(10_000 - bps)) / 10_000nand the exact-OUT max-in ceiling(quotedIn * BigInt(10_000 + bps) + 9_999n) / 10_000nwere copy-pasted across the swap modules. Both size a fund-safety bound that ends up baked into calldata the user signs on a Ledger, so a correction applied to one copy silently leaves the others wrong — the recurrence surface ARCHITECTURE.md §5.5 names, and the shape behind the #685-class arithmetic bug.New
src/modules/shared/slippage.tsexportsapplyMinOut(quotedOut, slippageBps)andapplyMaxIn(quotedIn, slippageBps)with byte-identical arithmetic to the code they replace, including the deliberate asymmetric rounding (floor truncates DOWN via BigInt division; ceiling keeps the+ 9_999nround-up bias so an approval cap is never one wei short).Call sites converted:
src/modules/uniswap-swap/index.tsapplySlippageExactIn/applySlippageExactOutdeleted; the two call sites now use the shared helpersrc/modules/curve/actions.tsadd_liquidityminLpOut,swapminDysrc/modules/swap/index.tsdstAmountmin-out, LiFi exact-out approval capsrc/modules/tron/sunswap-swap.tsminOutBase— see "beyond the issue's scope" belowThis is a pure refactor: no value changes, no rounding changes, no gate changes.
Beyond the issue's enumerated sites
The issue enumerated four files. A fifth verbatim copy of the exact-IN floor lives at
src/modules/tron/sunswap-swap.ts(SunSwap V2), outside the acceptance criterion's{shared,swap,uniswap-swap,curve}grep scope — it postdates the recon the issue was written from. It is converted here too, since leaving a known copy of the same fund-safety formula defeats the point of the unit. Happy to split it out if you would rather keep this PR to the enumerated four.Deliberately NOT added
The helper adds no
slippageBpsvalidation. Every call site already runs its own gate before reaching this arithmetic (assertSlippageOkinswap/, reused byuniswap-swap/; Curve's inline bps cap; LiFi'sMAX_EFFECTIVE_SLIPPAGE). Adding a throw here would be a user-visible behaviour change, which a consolidation unit should not smuggle in. The precondition is documented in the helper's header instead. If you want a centralised gate, that is a follow-up with its own product call.Out of scope, unchanged, per the issue:
src/modules/lp/uniswap-v3/computes its bounds via a separate position-math path.Acceptance criterion
The issue's transcribed criterion now holds on this branch:
Exactly the two shared-helper definitions, both separator spellings and both operators covered, helper directory included.
src/modules/tronis also clean.The falsifier test
test/slippage-shared-helper-715.test.ts, two halves:applyMinOut/applyMaxInare asserted equal to the pre-refactor inline formulas (kept in the test as the equivalence oracle) across 14(amount, bps)pairs spanning zero, dust, exact multiples, 18-decimal amounts and2^128-1; plus literal pinned values for the cases where the rounding direction is the whole point (applyMinOut(1n, 50) === 0n,applyMaxIn(1n, 1) === 2n,applyMaxIn(1n, 0) === 1n).src/modules/{shared,swap,uniswap-swap,curve,tron}: exactly two hits, both inslippage.ts, and each converted call site is asserted to import../shared/slippage.js.Why it is a falsifier, not a tautology: on unfixed code
src/modules/shared/slippage.tsdoes not exist (half 1 fails at import) and the walk finds the seven pre-refactor arithmetic sites instead of two. It goes RED again if any future call site re-inlines either formula. The structural half also asserts the scan matches the helper's own two definitions, so a broken regex — which would make "no duplicates found" vacuously true — fails loudly rather than passing green.Tests were NOT run locally: Node/npm are not installed on the authoring machine, so no build, lint or test run happened here. CI (Build & Test on Node 20 + 22) is the authoritative check. Nothing in this PR should be read as a claim of a green local run.
No existing test was weakened or updated. Behaviour is unchanged, so no existing assertion needed to move.
Blast radius
Every EVM/TRON swap-preparation path that derives a slippage bound:
prepare_uniswap_swap,prepare_curve_add_liquidity,prepare_curve_swap,prepare_swap(1inch direct + LiFi exact-out approval sizing),prepare_tron_sunswap_swap. Read paths and quote paths are untouched. LiFi's own bakedtoAmountMin(and the #685 integrator-fee handling around it) is not this formula and is untouched.Residual concerns / review asks
swap/,uniswap-swap/,curve/andtron/creates a new (leaf, dependency-free) shared module. Worth confirming it does not disturb the module-boundary intent of ARCHITECTURE.md §7 decision record 1 (swap/anduniswap-swap/kept distinct,assertSlippageOkdefined once and imported) — this follows the same pattern rather than breaking it.tron/conversion is the one judgement call in the diff; see above.