feat(slippage): show slippage outside accordion when modified or abov… #6427
Conversation
|
@Rustix69 is attempting to deploy a commit to the cow Team on Vercel. A member of the Team first needs to authorize it. |
|
All contributors have signed the CLA ✍️ ✅ |
WalkthroughIntroduces a new hook Changes
Sequence DiagramsequenceDiagram
participant TradeRateDetails
participant Hook as useShouldShowSlippageProminent
participant Wallet as useWalletInfo
participant Slippage as useTradeSlippage
participant Modified as useIsSlippageModified
participant EthFlow as useIsEoaEthFlow
TradeRateDetails->>Hook: ask shouldShowSlippageProminent
Hook->>Modified: isSlippageModified?
Modified-->>Hook: bool
alt isSlippageModified = true
Hook-->>TradeRateDetails: true
else
Hook->>Slippage: get slippage %
Slippage-->>Hook: % value
Hook->>EthFlow: isEoaEthFlow?
EthFlow-->>Hook: bool
Hook->>Wallet: get chainId
Wallet-->>Hook: chainId
Note over Hook: choose threshold (ERC20=200bps or ETH_FLOW per chain)
alt slippage_bps > threshold
Hook-->>TradeRateDetails: true
else
Hook-->>TradeRateDetails: false
end
end
alt true
TradeRateDetails->>TradeRateDetails: render slippageRow outside accordion
else
TradeRateDetails->>TradeRateDetails: render slippageRow inside accordion
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
@elena-zh Have a look on this PR and let me know if any changes required. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
apps/cowswap-frontend/src/modules/tradeSlippage/hooks/useShouldShowSlippageProminent.ts (1)
49-65: Threshold logic is correct and well-structured.The logic correctly implements the different thresholds for regular orders (2%), ETH-flow on Mainnet (5%), and ETH-flow on other chains (uses
ETH_FLOW_SLIPPAGE_WARNING_THRESHOLD). While there's some redundancy since non-Mainnet ETH-flow currently uses the same 2% threshold as regular orders, keeping the logic explicit makes the intent clear and allows for future differentiation per chain.The structure is clean, but if you want to simplify in the future, you could collapse cases with identical thresholds. For now, the explicit approach is fine and self-documenting.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
apps/cowswap-frontend/src/modules/tradeSlippage/hooks/useShouldShowSlippageProminent.ts(1 hunks)apps/cowswap-frontend/src/modules/tradeSlippage/index.tsx(1 hunks)apps/cowswap-frontend/src/modules/tradeWidgetAddons/containers/TradeRateDetails/index.tsx(4 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
apps/cowswap-frontend/src/modules/tradeSlippage/hooks/useShouldShowSlippageProminent.ts (6)
libs/wallet/src/api/hooks.ts (1)
useWalletInfo(24-26)apps/cowswap-frontend/src/modules/tradeSlippage/hooks/useTradeSlippage.ts (1)
useTradeSlippage(36-40)apps/cowswap-frontend/src/modules/tradeSlippage/hooks/useIsSlippageModified.ts (1)
useIsSlippageModified(3-5)apps/cowswap-frontend/src/modules/trade/hooks/useIsEoaEthFlow.ts (1)
useIsEoaEthFlow(5-7)libs/common-utils/src/misc.ts (1)
percentToBps(193-195)libs/common-const/src/common.ts (1)
ETH_FLOW_SLIPPAGE_WARNING_THRESHOLD(86-88)
apps/cowswap-frontend/src/modules/tradeWidgetAddons/containers/TradeRateDetails/index.tsx (2)
apps/cowswap-frontend/src/modules/tradeSlippage/hooks/useShouldShowSlippageProminent.ts (1)
useShouldShowSlippageProminent(37-70)apps/cowswap-frontend/src/modules/trade/containers/TradeTotalCostsDetails/index.tsx (1)
TradeTotalCostsDetails(21-41)
🔇 Additional comments (7)
apps/cowswap-frontend/src/modules/tradeSlippage/index.tsx (1)
11-11: LGTM!The export follows the existing pattern and correctly exposes the new hook.
apps/cowswap-frontend/src/modules/tradeWidgetAddons/containers/TradeRateDetails/index.tsx (3)
16-16: LGTM!The import and hook usage are correct and follow React best practices.
Also applies to: 46-46
86-93: LGTM!Extracting the slippage row into a variable improves readability and enables clean conditional rendering. The null check appropriately handles the case when slippage is not available.
103-104: LGTM!The conditional rendering logic correctly ensures the slippage row appears in exactly one location—either prominently outside the accordion or inside when not prominent. The mutual exclusion logic is sound, and the padding wrapper is consistent with existing patterns.
Also applies to: 110-122
apps/cowswap-frontend/src/modules/tradeSlippage/hooks/useShouldShowSlippageProminent.ts (3)
1-23: LGTM!The imports and threshold constants are well-defined and clearly documented. The values correctly match the PR requirements (2% for regular orders, 5% for ETH-flow on Mainnet).
37-47: LGTM!The hook correctly implements the requirement that manually modified slippage should always be shown prominently. The early return pattern is clean and efficient.
67-70: LGTM!The final comparison and memoization are correctly implemented. The use of strict greater-than comparison aligns with the PR requirements, and all necessary dependencies are included in the useMemo array.
|
@Rustix69 , thank you. will review. Until then, could you please sign the agreement #6427 (comment)? |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
I have read the CLA Document and I hereby sign the CLA |
There was a problem hiding this comment.
Hey @Rustix69 , thank you, great job!
3 issues I've found:
- It would be great to move the slippage field closer to the action button
- When setting ETH token on Ethereum, the slippage banner (when slippage is >2%) is displayed while the slippage field is not.
A bit more background on this: now the slippage banner is displayed for ETH-flow orders when slippage is higher than 2%. Threshold of 5% was removed 3 weeks ago.
Could you please add it like here?
- Could you please hide the slippage field (if is was changed) for swap and bridge orders when price/fee accordion is expanded? The field is duplicated in this case
- In case of swap and bridge order. the field's name should be
Max. swap slippage - One more possible option to solve 3 and 4 cases is to hide Slippage tolerance field(if manually modified) for swap and bridge orders. :)
Thank you
|
@elena-zh I had fixed the changes plz review once... Show Slippage Prominently for Limit/TWAP OrdersChanges
Behavior
Files Modified
|
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
apps/cowswap-frontend/src/modules/tradeSlippage/hooks/useShouldShowSlippageProminent.ts (1)
59-62: Clarify threshold variation in comment.The comment states ">2%" but the actual threshold varies by order type (2% for regular, 5% for ETH-flow per requirements).
Apply this diff:
// Show prominently when: // 1. User manually modified slippage, OR - // 2. Slippage exceeds threshold (>2%) + // 2. Slippage exceeds order-specific threshold (2% regular / 5% ETH-flow)apps/cowswap-frontend/src/modules/tradeWidgetAddons/containers/TradeRateDetails/index.tsx (2)
103-104: Clarify comment about conditional slippage rendering.The comment "Always show slippage inside accordion" is misleading—it suggests unconditional rendering, but the actual intent is that slippage is available inside the accordion (visible when expanded) and may also appear outside when prominent.
Apply this diff:
- {/* Always show slippage inside accordion */} + {/* Slippage inside accordion (visible when expanded) */} {slippageRow}
121-122: Consider optimization: avoid rendering slippageRow twice.Currently,
slippageRowis rendered both inside the accordion (line 104) and outside (line 122) when prominent. Although only one is visible at a time due to accordion state, this creates unnecessary DOM nodes and component instances.Refactor to render in only one location:
<TradeFeesAndCosts receiveAmountInfo={receiveAmountInfo} networkCostsSuffix={shouldPayGas ? <NetworkCostsSuffix /> : null} networkCostsTooltipSuffix={<NetworkCostsTooltipSuffix />} /> - {/* Slippage inside accordion (visible when expanded) */} - {slippageRow} + {/* Show slippage inside only when not shown prominently outside, or when accordion is open */} + {(!shouldShowSlippageProminent || isFeeDetailsOpen) && slippageRow} <RowDeadline deadline={deadline} />This ensures
slippageRowrenders in exactly one location:
- Inside when
!shouldShowSlippageProminent(any accordion state)- Inside when
shouldShowSlippageProminent && isFeeDetailsOpen(open accordion)- Outside when
shouldShowSlippageProminent && !isFeeDetailsOpen(closed accordion, line 122)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
apps/cowswap-frontend/src/modules/tradeSlippage/hooks/useShouldShowSlippageProminent.ts(1 hunks)apps/cowswap-frontend/src/modules/tradeWidgetAddons/containers/TradeRateDetails/index.tsx(4 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-10-10T20:28:16.565Z
Learnt from: fairlighteth
PR: cowprotocol/cowswap#6347
File: apps/cowswap-frontend/src/modules/trade/pure/TradeConfirmation/index.tsx:49-49
Timestamp: 2025-10-10T20:28:16.565Z
Learning: In apps/cowswap-frontend/src/modules/trade, TradeConfirmation follows a two-layer architecture: TradeConfirmationView (pure/stateless) in pure/TradeConfirmation/index.tsx renders the UI, while TradeConfirmation (container) in containers/TradeConfirmation/index.tsx wraps it to freeze props during pending trades (via useStableTradeConfirmationProps), wire in signing state (useSigningStep), and inject trade confirmation state (useTradeConfirmState). Consuming modules should import the container TradeConfirmation from 'modules/trade' to preserve this stateful behavior.
<!-- [add_learning]
When reviewing component refactoring in apps/cowswap-frontend/src/modules/trade, recognize the pattern where a pure view component (e.g., TradeConfirmationView) is separated from a stateful container (e.g., TradeConfirmation) that wraps it. The container adds runtime state management (prop stabilization, signing state, etc.) while the view remains testable and composable. Do not flag usages that import th...
Applied to files:
apps/cowswap-frontend/src/modules/tradeWidgetAddons/containers/TradeRateDetails/index.tsx
🧬 Code graph analysis (2)
apps/cowswap-frontend/src/modules/tradeWidgetAddons/containers/TradeRateDetails/index.tsx (2)
apps/cowswap-frontend/src/modules/tradeSlippage/hooks/useShouldShowSlippageProminent.ts (1)
useShouldShowSlippageProminent(31-66)apps/cowswap-frontend/src/modules/trade/containers/TradeTotalCostsDetails/index.tsx (1)
TradeTotalCostsDetails(21-41)
apps/cowswap-frontend/src/modules/tradeSlippage/hooks/useShouldShowSlippageProminent.ts (8)
libs/wallet/src/api/hooks.ts (1)
useWalletInfo(24-26)apps/cowswap-frontend/src/modules/tradeSlippage/hooks/useTradeSlippage.ts (1)
useTradeSlippage(36-40)apps/cowswap-frontend/src/modules/tradeSlippage/hooks/useIsSlippageModified.ts (1)
useIsSlippageModified(3-5)apps/cowswap-frontend/src/modules/trade/hooks/useIsEoaEthFlow.ts (1)
useIsEoaEthFlow(5-7)apps/cowswap-frontend/src/modules/trade/hooks/useTradeTypeInfo.ts (1)
useTradeTypeInfo(6-8)apps/cowswap-frontend/src/modules/trade/hooks/useIsCurrentTradeBridging.ts (1)
useIsCurrentTradeBridging(5-9)libs/common-utils/src/misc.ts (1)
percentToBps(193-195)libs/common-const/src/common.ts (1)
ETH_FLOW_SLIPPAGE_WARNING_THRESHOLD(86-88)
🔇 Additional comments (2)
apps/cowswap-frontend/src/modules/tradeWidgetAddons/containers/TradeRateDetails/index.tsx (1)
86-93: Nice refactor for reusability.Extracting
slippageRowas a constant eliminates duplication and makes the conditional rendering logic cleaner.apps/cowswap-frontend/src/modules/tradeSlippage/hooks/useShouldShowSlippageProminent.ts (1)
39-44: Verify the exclusion of SWAP orders from prominent slippage display with the PR author or design documentation.The code explicitly excludes
TradeType.SWAPand bridge orders with a comment ("Never show prominently for Swap or Bridge orders"), indicating intentional design. However, I could not locate supporting rationale in the codebase or public resources. Confirm with the PR author or linked issue whether this exclusion aligns with the stated objectives and is properly documented in the broader architecture or design specs.
| /** | ||
| * Hook to determine if slippage should be displayed prominently (outside the accordion) | ||
| * | ||
| * Slippage should be prominent when: | ||
| * 1. User has manually modified the slippage value, OR | ||
| * 2. Slippage exceeds warning thresholds (>2% for all orders) | ||
| * | ||
| * EXCEPT: Never show prominently for Swap or Bridge orders (to avoid duplication) | ||
| * | ||
| * Note: When accordion is expanded, prominent slippage is hidden to avoid duplication | ||
| * | ||
| * @returns {boolean} true if slippage should be shown outside the accordion | ||
| */ |
There was a problem hiding this comment.
Critical: Documentation contradicts stated requirements.
The JSDoc on line 23 states slippage should exceed ">2% for all orders," but the PR objectives clearly specify different thresholds: ">2% for regular orders" and ">5% for ETH-flow orders on Mainnet."
Apply this diff to correct the documentation:
* Slippage should be prominent when:
* 1. User has manually modified the slippage value, OR
- * 2. Slippage exceeds warning thresholds (>2% for all orders)
+ * 2. Slippage exceeds warning thresholds (>2% for regular ERC20 orders, >5% for ETH-flow orders)🤖 Prompt for AI Agents
In
apps/cowswap-frontend/src/modules/tradeSlippage/hooks/useShouldShowSlippageProminent.ts
around lines 18 to 30, the JSDoc incorrectly states a single ">2% for all
orders" threshold; update the documentation to reflect the PR objectives:
slippage should be prominent when the user manually modified it OR when slippage
exceeds >2% for regular orders and >5% for ETH-flow orders on Mainnet; retain
the exceptions noting never show prominently for Swap or Bridge orders and hide
prominent slippage when the accordion is expanded.
| if (isEoaEthFlow) { | ||
| // For ETH-flow orders, use 2% threshold | ||
| warningThresholdBps = ETH_FLOW_SLIPPAGE_WARNING_THRESHOLD[chainId] | ||
| } else { | ||
| // For regular ERC20 orders, use 2% threshold | ||
| warningThresholdBps = REGULAR_ORDER_SLIPPAGE_WARNING_BPS | ||
| } |
There was a problem hiding this comment.
Critical: ETH-flow threshold implementation contradicts requirements.
The implementation uses 2% for ETH-flow orders (line 53), but the PR objectives and linked issue #6317 explicitly state the threshold should be ">5% for ETH-flow orders on Mainnet."
Looking at libs/common-const/src/common.ts, ETH_FLOW_SLIPPAGE_WARNING_THRESHOLD is hardcoded to 200 bps (2%) for all chains. This constant needs to be updated to 500 bps (5%) to match requirements.
You'll need to update the constant definition in libs/common-const/src/common.ts:
export const ETH_FLOW_SLIPPAGE_WARNING_THRESHOLD: Record<SupportedChainId, number> = mapSupportedNetworks(
500, // 5%
)Then update the comment on line 52:
if (isEoaEthFlow) {
- // For ETH-flow orders, use 2% threshold
+ // For ETH-flow orders, use 5% threshold
warningThresholdBps = ETH_FLOW_SLIPPAGE_WARNING_THRESHOLD[chainId]🤖 Prompt for AI Agents
In
apps/cowswap-frontend/src/modules/tradeSlippage/hooks/useShouldShowSlippageProminent.ts
around lines 51–57, the ETH-flow threshold is set to 2% but requirements require
>5% on Mainnet; update the constant definition in
libs/common-const/src/common.ts to set ETH_FLOW_SLIPPAGE_WARNING_THRESHOLD to
500 (bps) via mapSupportedNetworks(500) so chains use 5% instead of 2%, and then
update the inline comment at line ~52 in useShouldShowSlippageProminent.ts to
reflect that ETH-flow orders use a 5% threshold on Mainnet (e.g., “For ETH-flow
orders, use 5% threshold”) ensuring types still match SupportedChainId mapping.
2 participants
Show Slippage Outside Accordion When Modified or Above Threshold
Fixes #6317
Summary
Slippage tolerance now displays outside the accordion when it requires user attention, making high slippage warnings and manual adjustments immediately visible.
Changes
useShouldShowSlippageProminenthook for smart visibility logicWhen Slippage Shows Outside Accordion
Testing
Snapshot
Files Modified
useShouldShowSlippageProminent.tshooktradeSlippage/index.tsx(exports)TradeRateDetails/index.tsx(conditional rendering)Before/After
Before: Slippage always hidden → users miss important warnings
After: Slippage prominent when needed → better transparency & safety
Summary by CodeRabbit