refactor: stable slippage default#446
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe PR refactors slippage handling into exported helper functions (stablecoin detection, context-based default slippage, override activation), introduces a ChangesSlippage logic extraction and component integration
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Form as Borrow/Multiply/Collateral Form
participant useSlippage
participant SwapContext
participant Storage as localStorage
Form->>useSlippage: call with enabled(), fromSymbol(), toSymbol()
useSlippage->>useSlippage: check options.enabled()
alt enabled is true
useSlippage->>SwapContext: update shared swap context
useSlippage->>useSlippage: getDefaultSlippageForContext(context)
useSlippage->>Storage: read persisted SlippageOverride
useSlippage->>useSlippage: isSlippageOverrideActive(override, now, defaultSlippage)
alt override active
useSlippage->>Form: effectiveSlippage = override value
else override inactive/expired
useSlippage->>Storage: clear override + legacy keys
useSlippage->>Form: effectiveSlippage = defaultSlippage
end
else enabled is false
useSlippage-->>SwapContext: skip context update
end
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
🚅 Deployed to the euler-lite-pr-446 environment in euler-lite
|
Seranged
force-pushed
the
fix/stable-slippage-default
branch
from
6e3d000 to
b35f8da
Compare
Seranged
force-pushed
the
fix/stable-slippage-default
branch
from
b35f8da to
57fe4ad
Compare
Seranged
force-pushed
the
fix/stable-slippage-default
branch
from
57fe4ad to
f834784
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
tests/composables/useSlippage.test.ts (2)
21-29: ⚡ Quick winAssert numeric defaults explicitly in this suite.
These expectations reuse
DEFAULT_STABLECOIN_SLIPPAGE/DEFAULT_SLIPPAGE, so a constant regression can still pass here. Add at least one direct numeric assertion (e.g.,0.05/0.3) to pin product intent.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/composables/useSlippage.test.ts` around lines 21 - 29, The test currently only asserts against the constants DEFAULT_STABLECOIN_SLIPPAGE and DEFAULT_SLIPPAGE which could hide regressions; update the cases in the it('uses 0.05% as the default for stablecoin swap pairs', ...) block (the const cases array) to include at least one explicit numeric expectation alongside the constants—e.g., replace or add an entry that expects 0.05 for stablecoin pairs and one that expects 0.3 for non-stablecoin pairs (referencing DEFAULT_STABLECOIN_SLIPPAGE and DEFAULT_SLIPPAGE for clarity), and update the assertion that iterates over cases to check the numeric literal as well as the constant.
36-50: ⚡ Quick winAdd an exact-expiry boundary test.
The suite checks
+1mspast expiry, but not the exactsetAt + SLIPPAGE_EXPIRY_MSboundary. Add one assertion to lock in intended>vs>=behavior.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/composables/useSlippage.test.ts` around lines 36 - 50, Add an exact-expiry boundary assertion for isSlippageOverrideActive: after computing setAt and expiredNow in the test (using SLIPPAGE_EXPIRY_MS), call isSlippageOverrideActive(DEFAULT_SLIPPAGE, setAt, setAt + SLIPPAGE_EXPIRY_MS, DEFAULT_STABLECOIN_SLIPPAGE) and assert the expected boundary behavior (i.e., that the override is still active at exactly setAt + SLIPPAGE_EXPIRY_MS if your logic treats expiry as strictly greater than the window, or assert false if your logic treats it as >=); place this single assertion alongside the existing expiredNow check so the test documents the exact >= vs > behavior for SLIPPAGE_EXPIRY_MS and the isSlippageOverrideActive function.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@tests/composables/useSlippage.test.ts`:
- Around line 21-29: The test currently only asserts against the constants
DEFAULT_STABLECOIN_SLIPPAGE and DEFAULT_SLIPPAGE which could hide regressions;
update the cases in the it('uses 0.05% as the default for stablecoin swap
pairs', ...) block (the const cases array) to include at least one explicit
numeric expectation alongside the constants—e.g., replace or add an entry that
expects 0.05 for stablecoin pairs and one that expects 0.3 for non-stablecoin
pairs (referencing DEFAULT_STABLECOIN_SLIPPAGE and DEFAULT_SLIPPAGE for
clarity), and update the assertion that iterates over cases to check the numeric
literal as well as the constant.
- Around line 36-50: Add an exact-expiry boundary assertion for
isSlippageOverrideActive: after computing setAt and expiredNow in the test
(using SLIPPAGE_EXPIRY_MS), call isSlippageOverrideActive(DEFAULT_SLIPPAGE,
setAt, setAt + SLIPPAGE_EXPIRY_MS, DEFAULT_STABLECOIN_SLIPPAGE) and assert the
expected boundary behavior (i.e., that the override is still active at exactly
setAt + SLIPPAGE_EXPIRY_MS if your logic treats expiry as strictly greater than
the window, or assert false if your logic treats it as >=); place this single
assertion alongside the existing expiredNow check so the test documents the
exact >= vs > behavior for SLIPPAGE_EXPIRY_MS and the isSlippageOverrideActive
function.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: euler-xyz/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 67add0dd-255b-4be1-913b-45ef4bf1db26
📒 Files selected for processing (3)
components/entities/swap/SlippageSettings.vuecomposables/useSlippage.tstests/composables/useSlippage.test.ts
Seranged
force-pushed
the
fix/stable-slippage-default
branch
from
f834784 to
69bcf04
Compare
Seranged
force-pushed
the
fix/stable-slippage-default
branch
from
69bcf04 to
b9f3e6f
Compare
Seranged
force-pushed
the
fix/stable-slippage-default
branch
from
b9f3e6f to
bf2bff0
Compare
Seranged
force-pushed
the
fix/stable-slippage-default
branch
from
bf2bff0 to
af8a974
Compare
Seranged
force-pushed
the
fix/stable-slippage-default
branch
from
af8a974 to
4ddd8e4
Compare
Seranged
force-pushed
the
fix/stable-slippage-default
branch
from
4ddd8e4 to
aa3d699
Compare
Seranged
changed the title
Make slippage override expiry use the active pair default so stale generic overrides do not mask the stablecoin 0.05% default. Align the settings copy with the same pair-aware default and cover stable/non-stable pair cases.
Seranged
force-pushed
the
fix/stable-slippage-default
branch
from
aa3d699 to
8ba875a
Compare
Keep newly saved slippage overrides active in the current tick, prevent stale per-instance storage refs from resurrecting cleared overrides, and reset the modal selection when a custom value is saved back to the active default. Add regressions for the smoke-tested edge cases and complete the repay test harness mocks used by the full suite.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
components/entities/swap/SlippageSettings.vue (1)
86-98: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicated preset/default predicate risks drift across four call sites.
The condition "is this value a preset or the active default" is independently restated at Line 89, Line 95, Line 82, and Line 122. As shown by the bug above, restating it inconsistently already caused a mismatch. Consider extracting a single
isPresetOrDefault(value)helper (closed overpresetValuesanddefaultSlippage) and reusing it in all four places to prevent future divergence.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@components/entities/swap/SlippageSettings.vue` around lines 86 - 98, The preset/default check is duplicated across SlippageSettings.vue and has already drifted, so centralize it into a single helper like isPresetOrDefault(value) within the SlippageSettings setup logic. Update the existing watchEffect, watch(slippage), and the other two call sites mentioned in the diff to use that helper instead of repeating presetValues.includes(value) || value === defaultSlippage.value. Keep the helper closed over presetValues and defaultSlippage so the selection state logic stays consistent in one place.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@components/entities/swap/SlippageSettings.vue`:
- Around line 82-84: The save paths in SlippageSettings.vue are only treating
the exact default as a preset, so custom values that match any other preset stay
marked as custom. Update both onSaveCustom and savePending to use the same
preset-detection logic already used elsewhere in the file, namely checking
whether the saved value matches any entry in presetValues or equals
defaultSlippage.value. This should ensure slippageSelection is set to preset
whenever the saved value corresponds to an existing preset chip, not just the
current default.
---
Nitpick comments:
In `@components/entities/swap/SlippageSettings.vue`:
- Around line 86-98: The preset/default check is duplicated across
SlippageSettings.vue and has already drifted, so centralize it into a single
helper like isPresetOrDefault(value) within the SlippageSettings setup logic.
Update the existing watchEffect, watch(slippage), and the other two call sites
mentioned in the diff to use that helper instead of repeating
presetValues.includes(value) || value === defaultSlippage.value. Keep the helper
closed over presetValues and defaultSlippage so the selection state logic stays
consistent in one place.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: euler-xyz/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 560ae4b6-59bc-4e0b-957d-377142b1546f
📒 Files selected for processing (4)
components/entities/swap/SlippageSettings.vuecomposables/useSlippage.tstests/composables/useSavingsRepay.test.tstests/composables/useSlippage.test.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- tests/composables/useSlippage.test.ts
- composables/useSlippage.ts
LeonardEulerXYZ
left a comment
LeonardEulerXYZ
left a comment
There was a problem hiding this comment.
Reviewed head 5837f6b70d87a7a4a72ef221157c7bbc2d33b930.
Verdict: request changes — I found one user-visible slippage override regression.
Validation run:
npm run test:run -- tests/composables/useSlippage.test.ts tests/utils/swap-slippage-validation.test.ts tests/composables/useSavingsRepay.test.ts— passed, 30 testsnpm run typecheck— passednpm run lint— passednpm run build— passed- Local headed Playwright smoke against an isolated route-level
useSlippage({ fromSymbol, toSymbol })+ realSlippageSettingssave flow — failed on the custom override invariant
Smoke coverage: browser interaction smoke, desktop/mobile harness over the slippage settings lifecycle. No screenshots posted because the finding is state/effective-value based rather than visual; the useful evidence is the failed assertion and state dump.
Scalability / maintainability hygiene: the new shared helper direction is good, and the route call sites now consistently pass swap context across swap/supply/withdraw/borrow/multiply/repay-style surfaces. The missed case is still a composition/lifecycle gap: tests cover isolated helper/composable behavior, but not the common mounted combination where a contextual route instance and the settings component's non-contextual instance both observe the shared override.
Bot feedback: CodeRabbit's preset-equivalence comment is a separate minor UI-state nit; I did not treat it as blocking. The blocker below is independently reproduced against the current head.
| && !hasCompleteSwapContext.value | ||
| ) return | ||
|
|
||
| if (!isOverrideActive.value) { |
There was a problem hiding this comment.
Fixed in 875d690a.
Root cause was a per-instance now ref: the settings instance advanced its clock when saving the override, but the contextual route instance could still see the new setAt as future-dated and clear the shared override. now is now shared via useState("slippage-now"), and tests/composables/useSlippage.test.ts includes keeps a contextual override saved from a second settings instance to pin the route+settings lifecycle.
Also handled CodeRabbit’s preset-equivalence note by centralizing isPresetOrDefault() in SlippageSettings.vue and adding browser coverage for custom input matching preset 0.1%.
Validation after the fix:
npm run test:run— 113 files / 1094 tests passednpm run typecheck— passednpm run build— passed- browser slippage modal matrix — 19/19 passed
Resolve the development merge for the stable slippage PR, keep restored custom slippage selections visible, clear corrupt persisted slippage overrides, and give two SDK-heavy tests explicit timeout budgets.
Share slippage override timestamps across mounted instances, align custom saves with preset/default selection logic, and cover the reviewed lifecycle and boundary cases.
| && !hasCompleteSwapContext.value | ||
| ) return | ||
|
|
||
| if (!isOverrideActive.value) { |
There was a problem hiding this comment.
Fixed in 875d690a.
Root cause was a per-instance now ref: the settings instance advanced its clock when saving the override, but the contextual route instance could still see the new setAt as future-dated and clear the shared override. now is now shared via useState("slippage-now"), and tests/composables/useSlippage.test.ts includes keeps a contextual override saved from a second settings instance to pin the route+settings lifecycle.
Also handled CodeRabbit’s preset-equivalence note by centralizing isPresetOrDefault() in SlippageSettings.vue and adding browser coverage for custom input matching preset 0.1%.
Validation after the fix:
npm run test:run— 113 files / 1094 tests passednpm run typecheck— passednpm run build— passed- browser slippage modal matrix — 19/19 passed
LeonardEulerXYZ
left a comment
LeonardEulerXYZ
left a comment
There was a problem hiding this comment.
Reviewed head 875d690ac0555771701b509a66d9a1c21526c543.
Verdict: request changes — the original useSlippage route/settings lifecycle blocker looks fixed, but the new pair-sensitive default exposes a remaining route-integration bug in the refinance page.
Validation run:
npm run test:run -- tests/composables/useSlippage.test.ts tests/composables/useEulerAccount.test.ts tests/composables/useEulerSdk.test.ts— passed, 30 tests.npm run typecheck— passed.- Direct code trace of the refinance slippage context against the actual collateral/debt quote request assets.
Prior Leonard blocker status: I rechecked the current useSlippage implementation. The shared useState("slippage-now"), the incomplete-context guard, and the new keeps a contextual override saved from a second settings instance test cover the earlier route+settings override-erasure failure, so I am not reposting that issue.
Blocking finding:
Refinance debt quotes can choose the wrong default slippage pair
pages/position/[number]/borrow/swap.vue still provides one route-level slippage context as:
fromSymbol: () => sourceCollateralVault.value?.asset.symbol || sourceDebtVault.value?.asset.symbol,
toSymbol: () => targetCollateralVault.value?.asset.symbol || targetDebtVault.value?.asset.symbol,but the debt quote request actually swaps the target debt asset into the source debt asset:
tokenIn: targetDebtVault.value.asset.address,
tokenOut: sourceDebtVault.value.asset.address,
slippage: slippage.value,Now that this PR makes the default depend on the pair, that mismatch is user-visible. For a debt-only refinance:
- current collateral
USDC, current debtWETH, target debtUSDC⇒ context becomesUSDC -> USDC, so the default is0.05%, while the actual debt quote isUSDC -> WETHand should use the normal0.3%default. - current collateral
WETH, current debtUSDC, target debtUSDT⇒ context becomesWETH -> USDT, so the default is0.3%, while the actual debt quote is stable-to-stableUSDT -> USDCand should use0.05%.
The same single-context problem appears when both collateral and debt legs are active: one displayed/default slippage value is reused for both quote requests even if one leg is stable-stable and the other is not.
Please derive the default from the actual active swap leg(s) on this route. A safe rule would be: use the stablecoin default only when every active quote leg that needs a swap is stable-to-stable; otherwise keep the normal 0.3% default, or split the route into leg-specific slippage values. The invariant is that the slippage default shown/sent for a quote must be based on the assets in that quote, not on whichever collateral/debt symbol happens to win the current fallback order.
Browser smoke: not run — this finding is from code-path validation of the quote request wiring; focused composable tests and typecheck pass.
Derive refinance slippage context from the active quote legs so debt-only and mixed-leg flows use stable defaults only when every active swap is stable-to-stable.
|
Addressed Leonard’s refinance slippage blocker in What changed:
Validation:
|
LeonardEulerXYZ
left a comment
LeonardEulerXYZ
left a comment
There was a problem hiding this comment.
Reviewed head 5e939c8108e5c823187121693d0f5da6c44aae5a.
Verdict: comment — no blocking issues found in this pass. I am not approving automatically.
What I checked:
- Re-read the changed slippage persistence/defaulting path,
SlippageSettings, borrow/multiply/collateral swap call sites, and the refinance-page slippage context fix. - Reconstructed the prior refinance invariant: the slippage default must be derived from the actual active quote leg(s), using the stable default only when every active swap leg is stable-to-stable. The new
getRefinanceSlippageContext()helper and its tests now cover the debt-quote direction and mixed-leg fallback correctly. - Scalability / maintainability hygiene pass: searched the sibling
useSlippage()consumers across lend, withdraw, repay, multiply, collateral swap, generic swap-page logic, and refinance. The patch is no longer a one-off page fallback; the refinance multi-leg decision is isolated in a small tested utility, and borrow/multiply tab coexistence is guarded withenabled. I did not find a sibling surface still carrying the same pair-default bug. - Bot/reviewer feedback pass: CodeRabbit’s older preset-equivalence nit is addressed by the shared
isPresetOrDefault()checks in the modal save paths. I did not find a material active bot finding to amplify.
Validation run:
npm run test:run -- tests/composables/useSlippage.test.ts tests/utils/refinance-slippage.test.ts tests/composables/useEulerAccount.test.ts tests/composables/useEulerSdk.test.ts— passed, 33 tests.npm run typecheck— passed.npx eslint <changed files>— passed.npm run build— passed.npm run lintrepo-wide is still blocked by existing errors outside the PR diff; I did not treat those as PR-specific.- Light headed Playwright route smoke against the Railway preview:
/and/borrow?network=1loaded as the real app on desktop and mobile viewports, with no page/console errors observed.
Smoke coverage: browser route smoke + mobile route smoke only. I did not run wallet/signing coverage, and I did not capture/post screenshots because this pass did not produce a meaningful visual before/after artifact for the slippage-default logic.
Previous Leonard blockers: resolved in the current head based on code trace plus focused regression coverage.
2 participants
Summary
0.05%default slippage in the settings modal and transaction slippage state.swap-slippage*storage keys hydrate into the explicit override shape for compatibility and are cleared after migration/reset so old values do not resurrect.Customand the value.Slippage Product Behavior
Swap slippage defaults follow the selected swap token pair:
USDC -> RLUSD) default to0.05%.wstETH -> WETH) default to0.3%.The slippage control is globally accessible from user settings and is also available inside swap flows. Custom slippage is stored as a user override with the pair-default bucket where it was set:
3%applies within the same default bucket where the user set it.3%override set from global settings or a normal0.3%pair applies to normal0.3%pairs, but does not carry into stable-to-stable0.05%pairs.3%override set from a stable-to-stable0.05%flow applies to stable-to-stable pairs, but does not carry into normal0.3%pairs.0.01%can carry across pair types and do not expire just because 24 hours passed.Existing persisted
swap-slippage*values hydrate into the current override model. Values with a stored stable0.05%bucket remain custom on stable pairs, while values without a stored bucket hydrate as normal0.3%overrides and fall back to the stable0.05%default on stable pairs.Changes
swap-slippage-overrideas the source of truth for persisted custom slippage.useSlippagecoverage for stable/non-stable pairs, override context changes, existing storage hydration, stale/future timestamps, invalid overrides, and clearing custom values equal to the active default.Test plan
npm run test:run -- tests/composables/useSlippage.test.ts tests/utils/swap-slippage-validation.test.ts(28 passing tests)npm run lintnpm run typecheckgit diff --check127.0.0.1:3082with deterministic injected swap contexts:0.05%0.3%3%override and showsresets to 0.05% default after 24h3%override and showsresets to 0.3% default after 24h3%override3%override0.01%lower-than-default overrides3%overrides fall back to0.05%swap-slippage*keys hydrate toCustom 3%swap-slippage*keys fall back to stable0.05%swap-slippage-overrideas JSONCustom 3%chip renders matching dark text for label and valueSummary by CodeRabbit
New Features
Bug Fixes