fix: switch to sell order if wallet changed from safe to EOA #6610
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
WalkthroughAdds a dev-server host binding and introduces EOA ETH-flow detection in SwapWidget to reset swap state to Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (5 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
🧰 Additional context used🧠 Learnings (2)📚 Learning: 2025-12-03T19:23:35.765ZApplied to files:
📚 Learning: 2025-09-11T08:25:51.460ZApplied to files:
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
🔇 Additional comments (1)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
apps/cowswap-frontend/project.json(1 hunks)apps/cowswap-frontend/src/modules/swap/containers/SwapWidget/index.tsx(2 hunks)
🧰 Additional context used
🧠 Learnings (5)
📓 Common learnings
Learnt from: shoom3301
Repo: cowprotocol/cowswap PR: 5443
File: apps/cowswap-frontend/src/modules/swap/containers/ConfirmSwapModalSetup/index.tsx:71-71
Timestamp: 2025-02-20T15:59:33.749Z
Learning: The swap module in apps/cowswap-frontend/src/modules/swap/ is marked for deletion in PR #5444 as part of the swap widget unification effort.
📚 Learning: 2025-10-10T20:28:16.565Z
Learnt from: fairlighteth
Repo: cowprotocol/cowswap PR: 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/swap/containers/SwapWidget/index.tsx
📚 Learning: 2025-02-20T15:59:33.749Z
Learnt from: shoom3301
Repo: cowprotocol/cowswap PR: 5443
File: apps/cowswap-frontend/src/modules/swap/containers/ConfirmSwapModalSetup/index.tsx:71-71
Timestamp: 2025-02-20T15:59:33.749Z
Learning: The swap module in apps/cowswap-frontend/src/modules/swap/ is marked for deletion in PR #5444 as part of the swap widget unification effort.
Applied to files:
apps/cowswap-frontend/src/modules/swap/containers/SwapWidget/index.tsx
📚 Learning: 2025-10-13T19:41:31.440Z
Learnt from: limitofzero
Repo: cowprotocol/cowswap PR: 6351
File: apps/cowswap-frontend/src/modules/erc20Approve/containers/TradeApproveModal/useTradeApproveCallback.ts:87-121
Timestamp: 2025-10-13T19:41:31.440Z
Learning: In apps/cowswap-frontend/src/modules/erc20Approve, useApproveCallback returns Promise<TransactionResponse | undefined> and is distinct from useApproveCurrency, which can return Promise<TransactionReceipt | SafeMultisigTransactionResponse>. When reviewing approval flows, verify which hook is actually being used before flagging Safe wallet concerns.
Applied to files:
apps/cowswap-frontend/src/modules/swap/containers/SwapWidget/index.tsx
📚 Learning: 2025-06-23T07:03:50.760Z
Learnt from: shoom3301
Repo: cowprotocol/cowswap PR: 5859
File: apps/cowswap-frontend/src/modules/tradeQuote/hooks/useTradeQuotePolling.ts:76-82
Timestamp: 2025-06-23T07:03:50.760Z
Learning: In the useTradeQuotePolling hook, there are two useLayoutEffect hooks that work together: one resets the counter to 0 when the confirmation modal closes, and another automatically triggers pollQuote(false, true) whenever the counter reaches 0. This creates an intentional chain reaction for immediate quote updates.
Applied to files:
apps/cowswap-frontend/src/modules/swap/containers/SwapWidget/index.tsx
🧬 Code graph analysis (1)
apps/cowswap-frontend/src/modules/swap/containers/SwapWidget/index.tsx (1)
apps/cowswap-frontend/src/modules/trade/hooks/useIsEoaEthFlow.ts (1)
useIsEoaEthFlow(5-7)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Setup
- GitHub Check: Cypress
🔇 Additional comments (3)
apps/cowswap-frontend/src/modules/swap/containers/SwapWidget/index.tsx (3)
15-15: Imports look correct.The new imports
SELL_ETH_RESET_STATEanduseIsEoaEthFloware properly added to support the ETH-flow fix.Note: Based on learnings, the swap module was previously marked for deletion as part of a swap widget unification effort (PR #5444 from February 2025). If this module is still planned for removal, consider whether this fix should be applied in a different location.
Also applies to: 21-21
102-102: LGTM!The hook usage is correct and retrieves the EOA ETH flow state as needed.
109-113: Verify scope ofisEoaEthFlowto prevent incorrect resetting of non-ETH orders and confirm the fix addresses the wallet switch scenario.The useEffect correctly resets to a sell order when
isEoaEthFlowis active and a buy order is detected. However, before merging, please verify:
isEoaEthFlowscope: Confirm thatisEoaEthFlowonly becomes true when ETH is involved as the sell or buy currency. If it can be true for non-ETH token pairs, this effect might incorrectly reset unrelated buy orders.
SELL_ETH_RESET_STATEcompleteness: Verify this constant properly initializes all necessary swap state (currencies, amounts, decimals) for a sell ETH order.Wallet switch scenario: Test that the fix handles the reported issue where switching from Safe to EOA wallet properly resets a buy order to a sell order without requiring manual action.
| "buildTarget": "cowswap-frontend:build", | ||
| "host": true |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n apps/cowswap-frontend/project.json | sed -n '30,50p'Repository: cowprotocol/cowswap
Length of output: 722
🏁 Script executed:
git log --oneline -20 -- apps/cowswap-frontend/project.jsonRepository: cowprotocol/cowswap
Length of output: 133
🏁 Script executed:
git show HEAD:apps/cowswap-frontend/project.json | head -50Repository: cowprotocol/cowswap
Length of output: 1246
🏁 Script executed:
git show HEAD~1:apps/cowswap-frontend/project.json | sed -n '34,50p'Repository: cowprotocol/cowswap
Length of output: 99
🏁 Script executed:
git log -p --all -S '"host": true' -- apps/cowswap-frontend/project.json | head -100Repository: cowprotocol/cowswap
Length of output: 45
🏁 Script executed:
rg -A 5 -B 5 "host.*true" apps/cowswap-frontend/Repository: cowprotocol/cowswap
Length of output: 50377
🏁 Script executed:
git log --all --oneline | head -20Repository: cowprotocol/cowswap
Length of output: 1312
🏁 Script executed:
rg -B 5 -A 5 "host.*true" --type jsonRepository: cowprotocol/cowswap
Length of output: 766
🏁 Script executed:
fd -name "*.md" | xargs rg -l "host" 2>/dev/null | head -5Repository: cowprotocol/cowswap
Length of output: 474
Clarify the relevance of this dev server configuration change.
This change sets "host": true for the Vite dev server, binding it to all network interfaces instead of localhost. This doesn't appear related to the PR's stated objective of fixing ETH-flow order handling when switching from Safe to EOA wallets.
If this configuration is needed for testing the fix (e.g., in CI/CD or specific development setups), please document that in the PR description. Otherwise, consider moving it to a separate PR.
🤖 Prompt for AI Agents
In apps/cowswap-frontend/project.json around lines 38-39, the dev server "host":
true change binds Vite to all network interfaces but appears unrelated to the
Safe→EOA order-handling fix; either remove/revert this change from this PR (keep
dev server config unchanged) or, if it is required for testing the bugfix
(CI/dev machines, remote device testing, or reproducing the issue), document
that requirement clearly in the PR description (explain why binding to all
interfaces is necessary, where/how tests rely on it, and any security/port
notes); do not leave the unrelated config change in this PR without
justification — move it to a separate config/infra PR if it’s not strictly
needed here.
There was a problem hiding this comment.
True, this was needed for my setup (Windows + Ubuntu running through WSL), otherwise I could not load http://localhost:3000
There was a problem hiding this comment.
@kernelwhisperer, thank you for the clarification! That makes perfect sense—WSL setups often require binding to all network interfaces (0.0.0.0) for the Windows host to access services running in the Linux subsystem. The "host": true option is exactly what's needed for this scenario.
This is a reasonable change to include, especially if team members use similar development setups. You might consider adding a brief note in the repository's development documentation (if one exists) mentioning that this configuration supports WSL-based development workflows, which could help other developers in similar environments.
🐰✨
✏️ Learnings added
Learnt from: kernelwhisperer
Repo: cowprotocol/cowswap PR: 6610
File: apps/cowswap-frontend/project.json:38-39
Timestamp: 2025-12-03T19:23:35.765Z
Learning: In the cowswap-frontend project configuration (apps/cowswap-frontend/project.json), the Vite dev server option `"host": true` is set to support Windows + WSL development environments, where binding to all network interfaces is required for the Windows host to access the dev server running in WSL.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
|
Works as expected test6663.mov |
kernelwhisperer
force-pushed
the
fix/disallow-ethflow-buy-order
branch
from
ba3bb32 to
a74cee9
Compare
kernelwhisperer
force-pushed
the
fix/disallow-ethflow-buy-order
branch
from
a74cee9 to
0ad0b30
Compare
Summary
Fixes #6394
To Test
Background
Before this change, the output would get disabled, but you were still able to click swap and place an invalid buy order.
Summary by CodeRabbit
Bug Fixes
Chores
✏️ Tip: You can customize this high-level summary in your review settings.