-
Notifications
You must be signed in to change notification settings - Fork 122
feat(bridge): display account proxy #5881
8000New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
Note Currently processing new changes in this PR. This may take a few minutes, please wait... 📒 Files selected for processing (1)
WalkthroughThis update introduces a comprehensive "Account Proxy" (CoW Shed) feature to the application. It adds new React components, hooks, styled elements, and state management for interacting with account proxies, including viewing proxy details, recovering funds, and displaying proxy-related information. The change also restructures balances and allowances caching to be account-specific, updates route handling, and unifies address display logic via reusable components. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant App
participant Wallet
participant CoWShedWidget
participant ProxyContract
participant Blockchain
User->>App: Navigate to /account-proxy/:tab?/:token?
App->>CoWShedWidget: Render widget/modal (tab, token)
CoWShedWidget->>Wallet: Fetch current account
CoWShedWidget->>ProxyContract: Fetch proxy address for account
CoWShedWidget->>Blockchain: Check if proxy is deployed
alt Tab = "about"
CoWShedWidget->>App: Display proxy info, tokens, FAQ
else Tab = "recover-funds"
User->>CoWShedWidget: Select token to recover
CoWShedWidget->>Blockchain: Fetch token balance
User->>CoWShedWidget: Click "Recover funds"
CoWShedWidget->>ProxyContract: Execute recovery transaction
ProxyContract->>Blockchain: Transfer funds to user
end
CoWShedWidget->>App: Update balances context
Possibly related PRs
Suggested reviewers
Poem
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
Hey @shoom3301 , thank you.
Thanks |
@elena-zh thank you!
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
♻️ Duplicate comments (3)
apps/cowswap-frontend/src/modules/cowShed/pure/CoWShedFAQ.tsx (3)
12-45
: Extract FAQ data for better maintainability.The FAQ data remains hardcoded inside the component, making it difficult to maintain and reuse. This issue was previously identified and should be addressed.
55-58
: Improve accessibility and avoid interfering with native details behavior.The preventDefault() call still interferes with native keyboard navigation and screen reader functionality. This accessibility concern was previously raised and should be addressed.
72-74
: Improve type safety and consistency for answer property.The answer property still uses inconsistent typing (JSX vs functions), creating potential runtime issues. This type safety concern was previously identified and remains unresolved.
🧹 Nitpick comments (4)
apps/cowswap-frontend/src/modules/cowShed/pure/TokensInProxyBanner/index.tsx (4)
39-39
: Fix grammatical error in banner text.Missing the article "the" before "following tokens".
- Your proxy account contains following tokens: + Your proxy account contains the following tokens:
51-51
: Improve banner message clarity.The current message is grammatically awkward and could be clearer about the purpose and action.
- Something could go wrong and you can withdraw your funds at <strong>Recover funds</strong> tab + If something goes wrong, you can withdraw your funds using the <strong>Recover funds</strong> tab.
14-14
: Replace magic numbers with design tokens.Consider using design system tokens or named constants instead of hardcoded pixel values for better maintainability and consistency.
const TokenLogoStyled = styled(TokenLogo)` display: inline-block; position: relative; - top: 3px; + top: ${({ theme }) => theme.spacing.xxs}; // or appropriate design token ` const TokensList = styled.ul` margin: 0; - padding-left: 10px; + padding-left: ${({ theme }) => theme.spacing.sm}; // or appropriate design tokenAlso applies to: 19-19
40-50
: Consider performance optimization for token amount calculations.Creating
CurrencyAmount
instances on every render could impact performance. Consider memoizing these calculations, especially if the token list is large.+import { useMemo } from 'react' export function TokensInProxyBanner({ tokensToRefund }: TokensInProxyBannerProps): ReactNode { if (tokensToRefund.length === 0) return null + const tokenAmounts = useMemo(() => + tokensToRefund.map(({ token, balance }) => ({ + token, + amount: CurrencyAmount.fromRawAmount(token, balance.toString()) + })), [tokensToRefund]) return ( <InlineBanner bannerType={StatusColorVariant.Warning} orientation={BannerOrientation.Horizontal}> <div> Your proxy account contains the following tokens: <TokensList> - {tokensToRefund.map(({ token, balance }) => { - const amount = CurrencyAmount.fromRawAmount(token, balance.toString()) - + {tokenAmounts.map(({ token, amount }) => { return ( <li key={token.address}> <TokenAmount amount={amount} tokenSymbol={token} /> <TokenLogoStyled size={16} token={token} /> </li> ) })}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
yarn.lock
is excluded by!**/yarn.lock
,!**/*.lock
📒 Files selected for processing (11)
apps/cowswap-frontend/src/legacy/components/TopLevelModals/index.tsx
(2 hunks)apps/cowswap-frontend/src/modules/account/containers/CowShedInfo/index.tsx
(1 hunks)apps/cowswap-frontend/src/modules/cowShed/containers/CoWShedModal/index.tsx
(1 hunks)apps/cowswap-frontend/src/modules/cowShed/containers/CoWShedTabs/index.tsx
(1 hunks)apps/cowswap-frontend/src/modules/cowShed/containers/CoWShedWidget/index.tsx
(1 hunks)apps/cowswap-frontend/src/modules/cowShed/containers/ProxyRecipient/index.tsx
(1 hunks)apps/cowswap-frontend/src/modules/cowShed/index.ts
(1 hunks)apps/cowswap-frontend/src/modules/cowShed/pure/CoWShedFAQ.tsx
(1 hunks)apps/cowswap-frontend/src/modules/cowShed/pure/TokensInProxyBanner/index.tsx
(1 hunks)apps/cowswap-frontend/src/modules/tradeFormValidation/pure/TradeFormButtons/tradeButtonsMap.tsx
(4 hunks)package.json
(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- apps/cowswap-frontend/src/legacy/components/TopLevelModals/index.tsx
- package.json
🚧 Files skipped from review as they are similar to previous changes (4)
- apps/cowswap-frontend/src/modules/cowShed/index.ts
- apps/cowswap-frontend/src/modules/cowShed/containers/ProxyRecipient/index.tsx
- apps/cowswap-frontend/src/modules/cowShed/containers/CoWShedWidget/index.tsx
- apps/cowswap-frontend/src/modules/account/containers/CowShedInfo/index.tsx
🧰 Additional context used
🧬 Code Graph Analysis (3)
apps/cowswap-frontend/src/modules/tradeFormValidation/pure/TradeFormButtons/tradeButtonsMap.tsx (1)
apps/cowswap-frontend/src/modules/tradeFormValidation/pure/TradeFormBlankButton/index.tsx (1)
TradeFormBlankButton
(63-127)
apps/cowswap-frontend/src/modules/cowShed/containers/CoWShedTabs/index.tsx (3)
libs/widget-lib/src/types.ts (1)
SupportedChainId
(4-4)apps/cowswap-frontend/src/common/hooks/useNavigate.ts (1)
useNavigate
(10-23)apps/cowswap-frontend/src/common/pure/Tabs/index.tsx (1)
Tabs
(5-11)
apps/cowswap-frontend/src/modules/cowShed/containers/CoWShedModal/index.tsx (2)
apps/cowswap-frontend/src/modules/cowShed/state/cowShedModalAtom.ts (1)
cowShedModalAtom
(7-7)apps/cowswap-frontend/src/modules/cowShed/containers/CoWShedWidget/index.tsx (1)
CoWShedWidget
(30-111)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Lint
- GitHub Check: Test
- GitHub Check: Cypress
🔇 Additional comments (8)
apps/cowswap-frontend/src/modules/cowShed/containers/CoWShedModal/index.tsx (1)
9-22
: LGTM! Clean modal implementation with proper state management.The modal component follows React best practices with:
- Proper Jotai state management
- Early return for closed state
- Correct use of React portal for modal rendering
- Clean prop passing to child components
apps/cowswap-frontend/src/modules/cowShed/containers/CoWShedTabs/index.tsx (1)
21-64
: LGTM! Well-structured tab component with proper mode handling.The implementation effectively handles both modal and navigation modes:
- Clean separation of concerns between modal state and routing
- Proper token selector cleanup on tab changes
- Correct synchronization between external props and internal state
- Appropriate use of replace navigation for tab switching
apps/cowswap-frontend/src/modules/cowShed/pure/TokensInProxyBanner/index.tsx (1)
32-34
: Fix the empty array check logic.The current logic only checks if the first token exists, but should check if the array is empty. This could cause incorrect behavior if
tokensToRefund
is an empty array.- const firstToken = tokensToRefund[0] - - if (!firstToken) return null + if (tokensToRefund.length === 0) return nullLikely an incorrect or invalid review comment.
apps/cowswap-frontend/src/modules/tradeFormValidation/pure/TradeFormButtons/tradeButtonsMap.tsx (5)
4-5
: LGTM: Bridge error handling imports added correctly.The new imports for
BridgeProviderQuoteError
andBridgeQuoteErrors
are appropriately added to support enhanced bridge quote error handling.
32-32
: LGTM: Centralized default error message improves maintainability.The
DEFAULT_QUOTE_ERROR
constant provides a consistent fallback message across different error scenarios.
35-35
: LGTM: Consistent use of centralized error constant.Good refactoring to use the centralized
DEFAULT_QUOTE_ERROR
constant instead of hardcoded strings.
45-54
: LGTM: Comprehensive bridge error mapping with appropriate specificity.The
bridgeQuoteErrorTexts
mapping covers allBridgeQuoteErrors
enum values with appropriate user-friendly messages. The specific messages for route-related errors ("No routes found") and unsupported operations ("Only 'sell' orders are supported") provide better user guidance than generic error messages.
112-132
: Let’s locate the definitions and usages of bothBridgeProviderQuoteError
and theBridgeQuoteErrors
enum more thoroughly:#!/bin/bash # Search for class or interface definitions of BridgeProviderQuoteError rg -g "*.ts" -n "cla 9E88 ss BridgeProviderQuoteError" -A 5 -B 5 rg -g "*.ts" -n "interface BridgeProviderQuoteError" -A 5 -B 5 # Search all occurrences to find where message is declared or assigned rg -g "*.ts" -n "BridgeProviderQuoteError" -A 5 -B 5 # Locate the BridgeQuoteErrors enum rg -g "*.ts" -n "enum BridgeQuoteErrors" -A 5 -B 5
apps/cowswap-frontend/src/modules/cowShed/pure/TokensInProxyBanner/index.tsx
Show resolved
Hide resolved
@elena-zh we can improve those points after, now we need to give Mindy some version for user testing. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @Shoom, thanks.
I've opened these tasks to address them later:
- (new) https://www.notion.so/cownation/Account-Proxy-page-does-not-cover-the-screen-in-a-mobile-view-21e8da5f04ca800abeadfa93b7b4f752
- https://www.notion.so/cownation/Account-Proxy-navigation-issues-21e8da5f04ca8079a621f90905c0997c
- https://www.notion.so/cownation/Account-Proxy-separate-FAQ-between-tabs-21e8da5f04ca80289b50e0a7ec5af938
- https://www.notion.so/cownation/Account-Proxy-Add-link-to-the-warning-21e8da5f04ca805db541c68c2d7b16ca
… fix/cowshed-displaying # Conflicts: # package.json # yarn.lock
… fix/cowshed-displaying # Conflicts: # apps/cowswap-frontend/src/modules/account/containers/Transaction/ActivityDetails.tsx # apps/cowswap-frontend/src/modules/bridge/pure/RecipientDisplay/index.tsx
Summary
Fixes: https://www.notion.so/cownation/Recipient-address-is-confusing-2108da5f04ca809d9a51d7bcf09e8f10?source=copy_link
To Test
about
Background
Optional: Give background information for changes you've made, that might be difficult to explain via comments
Summary by CodeRabbit
New Features
Enhancements
Bug Fixes
Refactor
Chores