feat(universal-router-sdk): pay fees to up to 4 recipients from encodeSwaps - #694
feat(universal-router-sdk): pay fees to up to 4 recipients from encodeSwaps#694claude[bot] wants to merge 9 commits into
Conversation
239694c to
108e72b
Compare
32aec7e to
1be1b4a
Compare
codyborn
left a comment
There was a problem hiding this comment.
Part of the coordinated multi-fee stack review. encodeSwaps validation is thorough and fails closed — empty array rejected with a correct custody rationale, >4 rejected, fee+flatFee exclusivity kept, mixed portion/flat arrays impossible via per-entry trade-type invariants (both directions tested), and validate → compute → plan ordering confirmed so invariant codes fire before ABI encoding. fee: F vs fee: [F] byte-identity is tested on V2_0, V2_1_1, exact-out, and flat.
Two findings inline: the same sequential-flooring sweep-floor defect as the sibling comment on #693 (here on the exact-input deduction), and the byte-identity changeset claim that this PR's own test edit disproves. Plus one diff-scope nit.
| exactOrMaxAmountIn: routingAmount, | ||
| grossMinOrExactAmountOut, | ||
| netMinOrExactAmountOut: grossMinOrExactAmountOut, | ||
| netMinOrExactAmountOut: grossMinOrExactAmountOut.sub(feeAmount), |
There was a problem hiding this comment.
Same root cause as the finding on #693's uniswap.ts, on the exact-input path: Σ floor(grossMin·f_i) understates what the encoded sequential PAY_PORTION_FULL_PRECISION commands actually pay by 1–3 wei for a meaningful fraction of amounts (verified by integer simulation — [216, 519, 917, 3292] bps on gross 533206710 pays 1 wei more than this deduction). netMinOrExactAmountOut is then a SWEEP floor the router can't meet when the swap fills at (or within a couple wei above) grossMinOrExactAmountOut — a fill that satisfied the user's slippage tolerance reverts on-chain. Lower practical severity than the exact-output case (slippage headroom usually dwarfs the dust), but the encoded floor should be a true worst-case invariant: derive it by simulating the cascade with the encoded (truncated) portions at grossMinOrExactAmountOut. That is also byte-identical to the old single-fee quantized deduction.
There was a problem hiding this comment.
fixed in 76a7cca: computeEncodeSwapsAmounts now reuses simulatePortionFeeDeduction (shared with #693's exact-output path), replaying the cascade with the exact encoded portions. reproducer pinned at both the compute and encodeSwaps boundaries, plus an end-to-end fuzz in 8e2e01b that decodes the portions from the produced calldata and checks the SWEEP floor in that same calldata.
|
|
||
| Every existing per-fee invariant applies per entry: portion pairs with `EXACT_INPUT`, flat with `EXACT_OUTPUT`, fractional bips require UR >= 2.1.1, and portion fees require router custody under `allowDirectTransfers`. A mixed portion/flat array is rejected by those same invariants. An empty array (`AT_LEAST_ONE_FEE_RECIPIENT_REQUIRED`) and more than `MAX_FEE_RECIPIENTS` entries (`TOO_MANY_FEE_RECIPIENTS`) are rejected in `validateEncodeSwaps`. | ||
|
|
||
| Passing a single `Fee` is unchanged (a single portion needs no rescaling): calldata is byte-identical across UR 2.0 / 2.1.1, both trade types, portion and flat fees, `safeMode`, and the `ApproveProxy` wrapper. |
There was a problem hiding this comment.
This PR's own diff disproves the "byte-identical" claim for single fees: test/unit/encodeSwaps.test.ts (~1221–1223) changes an existing single-fee test's expected settlement from the old 1e18-quantized deduction to the exact one (floor(3 · 1/3) = 1 vs 2) and deletes the expect(...).not.to.equal('2') assertion that pinned the old bytes. Any single portion fee whose fraction isn't exactly representable in 1e18 now emits a different sweep floor. Qualify the claim, or adopt the simulate-the-encoded-commands deduction (see the computeEncodeSwapsAmounts.ts comment), which restores it unconditionally.
There was a problem hiding this comment.
you were right, that test edit was the tell. the deduction change had broken single-fee byte-identity for 1e18-unrepresentable fractions. fixed in 76a7cca: the edited expectation (and the deleted .not.to.equal guard) is reverted to main's quantized value, and single-fee output is now pinned byte-identical against goldens captured from unmodified main. changeset updated.
| @@ -0,0 +1,29 @@ | |||
| { | |||
There was a problem hiding this comment.
Nit: this new foundry.lock (29 lines of forge dependency pins) is unrelated to the fee-encoding change and unmentioned in the description. If it's a deliberate pin produced by the forge version test:hardhat runs, say so; otherwise drop it to keep the diff scoped.
There was a problem hiding this comment.
dropped in 76a7cca, it was stray (main has none and the build doesn't use it).
| } | ||
| } | ||
|
|
||
| /** |
There was a problem hiding this comment.
Remove comment (this one, and all comments in this PR + its stack)
There was a problem hiding this comment.
i'd rather keep most of these: the rescaling/deduction comments document the invariants the sweep-floor fix depends on (and one bit us already). happy to trim specific ones you point at, but stripping all comments across the stack feels like a call for david.
There was a problem hiding this comment.
Went with condensing rather than a blanket strip, on David's call — every comment across this PR and #693 is now a single line, and only survives where it states something the code can't: the rescaling formula (scaled_i = f_i / (1 - sum of earlier fees)), why the sweep floor replays the encoded cascade instead of summing floors, why the version gate uses enum order rather than string comparison, and where the golden calldata came from. Narration, restated code, and anything already in the PR description are gone (1441f9d here, 03401a2 on #693): 80 lines removed on this PR, 113 on #693. This specific one is now one line describing why the single-or-array field is normalized at all. Happy to strip whichever of the survivors you still think are noise — point at them and they go.
wkoutre
left a comment
There was a problem hiding this comment.
Requesting changes for the multi-recipient portion-fee sweep-floor rounding issue.
| } | ||
|
|
||
| const exactOrMaxAmountIn = routingQuote.mul(slippageDenominator.add(slippageNumerator)).div(slippageDenominator) | ||
| const grossMinOrExactAmountOut = routingAmount |
There was a problem hiding this comment.
This sweep floor is not conservative once portion fees are rescaled and executed sequentially. Rounding dust from an earlier PAY_PORTION_FULL_PRECISION remains in the router and can be consumed by a later scaled portion, so sum(floor(gross * f_i)) can be smaller than the actual total paid.
For example, with a 1-wei minimum output and fees of 60% then 40%, the first command pays 0 and the second is encoded as 100% of the remaining balance, paying 1 wei. This calculation sets the sweep minimum to 1 (1 - 0 - 0), but the router balance is 0, causing the final SWEEP to revert.
Please derive the floor by simulating the actual encoded portions sequentially at grossMinOrExactAmountOut (including 1e18 quantization), and add execution-level dust tests for this case and fractional splits such as thirds.
There was a problem hiding this comment.
yep, same root cause as codyborn's finding. fixed in 76a7cca by replaying the encoded cascade instead of summing gross fractions; the dust-capture case is a pinned test now.
wkoutre
left a comment
There was a problem hiding this comment.
Two follow-ups on compatibility and API surface.
| } | ||
|
|
||
| const exactOrMaxAmountIn = routingQuote.mul(slippageDenominator.add(slippageNumerator)).div(slippageDenominator) | ||
| const grossMinOrExactAmountOut = routingAmount |
There was a problem hiding this comment.
This also changes existing single-fee behavior for fractions that cannot be represented exactly. For example, a 1/3 fee on an output of 3 now sets the sweep minimum to 2 instead of 3, even though the encoded fee command is unchanged. Please either preserve the old behavior or call out this compatibility change.
There was a problem hiding this comment.
reverted. the 1/3 test is back to main's quantized expectation (settlement 3), byte-identical to main, and pinned by a golden fixture. for a single fee the cascade replay reduces exactly to the old quantized deduction.
| export type { UniversalRouterCommand, UniversalRouterCall, Param, CommandsDefinition } from './utils/commandParser' | ||
| export type { Permit2Permit } from './utils/inputTokens' | ||
| export { normalizeEncodeSwapsSpec } from './utils/normalizeEncodeSwapsSpec' | ||
| export { normalizeEncodeSwapsSpec, toFeeList } from './utils/normalizeEncodeSwapsSpec' |
There was a problem hiding this comment.
Do callers need toFeeList as part of the public API? It looks like an internal helper; keeping it private would avoid expanding the supported surface.
There was a problem hiding this comment.
agreed, removed it from the public exports in 8e2e01b. it stays a module export for internal imports only.
c28bb31 to
a3d3cb2
Compare
0a27a66 to
deb8714
Compare
a3d3cb2 to
210e11c
Compare
…eSwaps `SwapSpecification.fee` now accepts an array of up to MAX_FEE_RECIPIENTS entries alongside the single `Fee` it has always taken. Each entry becomes its own command — PAY_PORTION / PAY_PORTION_FULL_PRECISION for portions, TRANSFER for flat — emitted in the caller's order ahead of the settlement SWEEP. The sweep floor subtracts the sum of the fees rather than a single one. Portions are floored individually against the same pre-fee gross amount and then added; on-chain each PAY_PORTION reads the router's current balance, so the portions compound downward and the sum is an upper bound on what is actually taken — the floor can only be conservative, never short. Flat fees are absolute transfers, so their sum is exact. A total exceeding the output is rejected instead of underflowing into ABI encoding. Every per-fee invariant is applied per entry, and MAX_FEE_RECIPIENTS is reused from UniswapTrade so the two fee-encoding surfaces cannot drift. Single-recipient calldata is byte-identical, verified by capturing 22 encodeSwaps cases (UR 2.0 / 2.1.1, both trade types, portion and flat, safeMode, ApproveProxy) before and after and diffing. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DCbnHXwSBtB6PNntVowc2w
…Swaps multi-recipient fees
Portion fee entries now mean "this fraction of the gross swap output".
The encoder rescales fee i via the shared scalePortionFees helper to
f_i / (1 - sum(f_0..f_{i-1})) and emits PAY_PORTION_FULL_PRECISION, so
each recipient receives exactly their stated fraction of gross. More
than one portion fee therefore requires urVersion >= 2.1.1
(MULTIPLE_FEE_RECIPIENTS_REQUIRE_UR_V2_1_1 otherwise); a single fee is
unchanged, including PAY_PORTION bips on v2.0.
computeEncodeSwapsAmounts now sums the gross-based fees exactly
(sum of floor(gross * f_i)) instead of flooring encoded portions.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…and 100% fee boundary - Pin single portion-fee (V2_1_1 and V2_0), single flat-fee, and no-fee encodeSwaps calldata against golden hex captured from unmodified main (48dea05), proving the multi-recipient change leaves single-fee encodings byte-identical - Boundary tests: fees summing to exactly 100% encode the last portion as the full remaining balance (1e18) and zero the settlement sweep floor; 100% + 1 bps throws FEE_TOTAL_GT_AMOUNT_OUT - computeEncodeSwapsAmounts: exactly 100% yields a zero net output without underflow; just over throws at the call site Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…encoded cascade leaves
computeEncodeSwapsAmounts derived the exact-input net min-out as
grossMin - sum(floor(grossMin * f_i)) over the caller-supplied gross fractions,
but the encoded commands are sequential PAY_PORTION_FULL_PRECISION ops with
rescaled portions, each flooring against the router's RUNNING balance. Dust
left by an earlier fee's floor can be captured by a later (rescaled-larger)
portion, so the on-chain payments can exceed that sum by a few wei — a fill at
(or within a couple wei above) the gross minimum then fails the final SWEEP's
floor even though it satisfied the user's slippage.
Reproducer (now tests at both the computeEncodeSwapsAmounts and encodeSwaps
boundaries, failed before this change): fees [216, 519, 917, 3292] bps on
gross 533206710 — sequential rescaled payments total 263617396 while the sum
gave 263617395, so the floor was 269589315 against an actual remaining balance
of 269589314. Exactly-100% totals on a non-divisible gross (60% + 40% on 101)
likewise demanded a floor of 1 from an empty router; they now floor at 0.
The deduction now reuses simulatePortionFeeDeduction (shared with
UniswapTrade's exact-output path), replaying the cascade with the exact portion
values swapRouter ABI-encodes. For a single fee this reduces exactly to the
previous quantized floor(gross * encodedFee / SCALE), restoring unconditional
single-fee byte-identity: the Percent(1, 3) test this PR had edited to the
exact-math value (settlement 2, with its `.not.to.equal('2')` guard deleted)
is reverted to main's quantized expectation (settlement 3), byte-identical to
main again.
Behavioral notes, reflected in the updated tests and changeset:
- portion totals over 100% now throw scalePortionFees'
'Portion fees together exceed 100% of the swap output' (the one shared
invariant) instead of FEE_TOTAL_GT_AMOUNT_OUT, which remains for flat fees
- the 4-recipient exact-input floor test now pins 475312500000000003 (the
naive sum gave a floor three wei looser than what the router actually holds)
Also drops the stray foundry.lock this branch added (main has none and the
build does not use it).
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ps sweep floor; unexport toFeeList - 150-run seeded-PRNG property test through the full encodeSwaps path: random fee sets (1-4 recipients, fractional-bps fees, random gross), decode the ABI-encoded portions from the produced calldata, replay them against the gross minimum, and require the SWEEP floor in that calldata to equal exactly what the replay leaves — and stay met when the fill lands a few wei above the minimum - drop toFeeList from the package's public exports: it is an internal encoding helper, not API surface (internal imports unchanged) Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Same pass as the base branch: one line per comment, kept only where it states an invariant (the cascade replay behind the sweep floor, the flat-fee total, the empty-array rejection). Comments only — no behavior change. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…tion; guard negatives; document the gross-quote convention - The fee-path checks used tiny-invariant, which strips the message when NODE_ENV is production, while backend callers classify these errors by message: TOO_MANY_FEE_RECIPIENTS and friends existed in source but not in a production build. They throw plain Errors now. - The fractional-bips refusal keyed on urVersion === V2_0; any pre-2.1.1 version lacks PAY_PORTION_FULL_PRECISION, so it keys on !isAtLeastV2_1_1. - scalePortionFees rejects negative fees explicitly instead of relying on ethers' toHex throwing downstream. - encodeSwaps' docstring states the amount convention the sweep floor depends on: routing.quote is the GROSS output on EXACT_INPUT; a net quote double-counts fees and floors the sweep low. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…n the encodeSwaps path too Follows the strict-remainder rule introduced on the portion-fees base branch. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
210e11c to
4e4558b
Compare
Requested by David Schwartz · Slack thread
Same multi-recipient fees as #693, but for the
SwapRouter.encodeSwapssurface:SwapSpecification.feenow also takes an array of up to 4Feeentries (portion or flat). Portions reuse the shared rescaling from #693, and the sweep floor comes from replaying the encoded commands, so a fill at the gross minimum always clears it.Stacked on #693; retarget to
mainonce that merges. Callers need both PRs since the SDK has two independent fee encoders.What to look at:
computeEncodeSwapsAmounts: net min-out viasimulatePortionFeeDeduction(the cascade replay), plus the over-100% and flat-fee-total guardstest/unit/fixtures/encodeSwapsGolden.ts): single-fee output pinned byte-identical to unmodifiedmain(48dea05)Byte-identity scope: a single fee (or one-element array) encodes byte-identically to the previous release in every case; only multi-fee arrays are new bytes. Full story: reviewer guide.
Linear: https://linear.app/uniswap/issue/ECO-760
No breaking changes. Non-fork hardhat suite green; the
uniswapTradesfork suite needsFORK_URLand fails the same onmain.