-
Notifications
You must be signed in to change notification settings - Fork 122
feat: update cow.fi
/tokens
page
#5889
New 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
- add `base`, `arbitrum`, `avalanche`, and `polygon` - refactor components to remove some TS warnings
The latest updates on your projects. Learn more about Vercel for Git ↗︎
3 Skipped Deployments
|
WalkthroughThe changes introduce a centralized mapping for blockchain network names, images, and URLs, refactor several components to utilize these mappings for improved type safety and rendering consistency, modularize the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant TokenDetails
participant NetworkTableComponent
participant SwapCardsComponent
participant NetworkItem
participant SwapLinkCard
User->>TokenDetails: View token page
TokenDetails->>TokenDetailsHeading: Render token heading
TokenDetails->>NetworkTableComponent: Render network table
NetworkTableComponent->>NetworkItem: Render each network row
TokenDetails->>SwapCardsComponent: Render swap cards
SwapCardsComponent->>SwapLinkCard: Render card for each supported network
Suggested reviewers
Poem
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
yarn install v1.22.22 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (2)
✨ 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 (
|
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: 2
🧹 Nitpick comments (1)
apps/cow-fi/components/TokenDetails/index.tsx (1)
78-133
: Excellent modularization with comprehensive network support.The extracted SwapCardsComponent properly handles all supported networks with conditional rendering. The addition of new networks (base, arbitrum, avalanche, polygon) aligns with the PR objectives to expand platform coverage.
Consider extracting the network-to-networkId mapping to a constant for better maintainability:
const NETWORK_ID_MAP = { ethereum: 1, xdai: 100, base: 8453, 'arbitrum-one': 42161, avalanche: 43114, 'polygon-pos': 137, } as const
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📥 Commits
Reviewing files that changed from the base of the PR and between 13bd0a9 and 126833 8000 7.
⛔ Files ignored due to path filters (4)
apps/cow-fi/public/images/arbitrum-chain.svg
is excluded by!**/*.svg
apps/cow-fi/public/images/avalanche-chain.svg
is excluded by!**/*.svg
apps/cow-fi/public/images/base-chain.svg
is excluded by!**/*.svg
apps/cow-fi/public/images/polygon-chain.svg
is excluded by!**/*.svg
📒 Files selected for processing (7)
apps/cow-fi/components/NetworkItem/index.tsx
(2 hunks)apps/cow-fi/components/SwapLinkCard/index.tsx
(1 hunks)apps/cow-fi/components/TokenDetails/index.tsx
(3 hunks)apps/cow-fi/components/TokenPageComponent.tsx
(2 hunks)apps/cow-fi/const/networkMap.ts
(1 hunks)apps/cow-fi/next.config.ts
(1 hunks)apps/cow-fi/services/tokens/index.ts
(3 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (3)
apps/cow-fi/services/tokens/index.ts (1)
apps/cow-fi/const/networkMap.ts (1)
NETWORK_MAP
(1-8)
apps/cow-fi/components/TokenDetails/index.tsx (5)
apps/cow-fi/components/TokenDetails/index.styles.ts (5)
DetailHeading
(136-149)TokenTitle
(151-181)NetworkTable
(198-202)SwapCardsWrapper
(236-243)MainContent
(40-47)apps/cow-fi/components/NetworkItem/styles.ts (1)
NetworkHeaderItem
(4-25)apps/cow-fi/components/NetworkItem/index.tsx (1)
NetworkItem
(22-47)apps/cow-fi/const/networkMap.ts (1)
NETWORK_MAP
(1-8)apps/cow-fi/components/SwapLinkCard/index.tsx (1)
SwapLinkCard
(15-40)
apps/cow-fi/components/NetworkItem/index.tsx (2)
apps/cow-fi/const/networkMap.ts (3)
NETWORK_MAP
(1-8)NETWORK_URL_MAP
(19-26)NETWORK_IMAGE_MAP
(10-17)apps/cow-fi/components/NetworkItem/styles.ts (1)
ItemWrapper
(27-110)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Cypress
- GitHub Check: Setup
🔇 Additional comments (21)
apps/cow-fi/next.config.ts (1)
96-101
: LGTM! Proper Next.js image configuration for external sources.The remotePatterns configuration correctly allows loading images from CoinGecko's CDN, which aligns with the network image handling improvements in the refactored components.
apps/cow-fi/const/networkMap.ts (3)
1-8
: Excellent centralization of network constants.The
NETWORK_MAP
provides a clean, maintainable way to manage network identifiers and their human-readable names across the application.
19-26
: LGTM! Explorer URLs are correctly configured.The blockchain explorer URLs are accurate for each respective network and follow the correct address path pattern.
10-17
: Verify that the image files exist at the specified paths.The
NETWORK_IMAGE_MAP
references local image files. Please ensure all these SVG files exist in the/images/
directory to prevent broken images.#!/bin/bash # Verify that all network image files exist echo "Checking for existence of network image files..." images=( "ethereum.svg" "base-chain.svg" "arbitrum-chain.svg" "avalanche-chain.svg" "polygon-chain.svg" "gnosis-chain.svg" ) missing_files=() for image in "${images[@]}"; do if [ ! -f "apps/cow-fi/public/images/$image" ]; then missing_files+=("$image") fi done if [ ${#missing_files[@]} -eq 0 ]; then echo "✅ All network image files exist" else echo "❌ Missing image files:" printf '%s\n' "${missing_files[@]}" fiapps/cow-fi/components/TokenPageComponent.tsx (2)
3-3
: Good addition of explicit JSX import.This improves TypeScript compatibility and follows React 17+ best practices for JSX usage.
27-27
: Excellent explicit return type annotation.The
JSX.Element
return type improves type safety and aligns with the typing improvements across other components in this PR.apps/cow-fi/services/tokens/index.ts (4)
3-3
: Good import organization.Moving the
backOff
import to the top follows standard import ordering conventions.
9-12
: Excellent network expansion with proper typing.The expanded
NETWORKS
array now supports the new platforms (base, arbitrum, avalanche, polygon) as specified in the PR objectives. The typing usingkeyof typeof NETWORK_MAP
ensures type safety and consistency with the centralized network constants.
35-35
: Good immutability improvement.Changing from
let
toconst
forsortedTokens
is correct since the variable is not reassigned after the sort operation.
96-99
: ```shell
#!/bin/bash
set -eecho "------ Index.ts snippet (lines 80-140) ------"
sed -n '80,140p' apps/cow-fi/services/tokens/index.ts || echo "Failed to read index.ts"echo
echo "------ Definition of descriptionFiles ------"
grep -R "descriptionFiles" -n apps/cow-fi/services/tokens/index.ts || echo "No matches for descriptionFiles"echo
echo "------ apps/cow-fi/data directory structure (files) ------"
find apps/cow-fi/data -maxdepth 2 -type f | sed 's/^/ /'echo
echo "------ JSON files under apps/cow-fi/data ------"
find apps/cow-fi/data -type f -name "*.json" | sed 's/^/ /'</details> <details> <summary>apps/cow-fi/components/SwapLinkCard/index.tsx (5)</summary> `1-6`: **Excellent imports for the refactored component.** The addition of `JSX`, `Image`, and network constants imports supports the improved typing and performance optimizations in the refactored component. --- `15-22`: **Improved function signature with proper typing.** The updated function signature with explicit return type `JSX.Element | null` and the use of `NETWORK_IMAGE_MAP` for dynamic image resolution improves both type safety and maintainability. --- `32-36`: **Excellent optimization with Next.js Image component.** The replacement of HTML `<img>` tags with Next.js `<Image>` components with explicit dimensions provides better performance through automatic optimization, lazy loading, and proper sizing. --- `8-13`: ```shell #!/bin/bash # Print lines 80–100 of TokenDetails to inspect full SwapLinkCard props for Ethereum sed -n '80,100p' apps/cow-fi/components/TokenDetails/index.tsx
26-28
: ```shell
#!/bin/bashRetry without type filters to locate swap URL logic and network mappings
echo "Searching for WXDAI and WETH usage across the repo..."
rg "WXDAI|WETH" -n .echo
echo "Searching for all occurrences of networkId..."
rg "networkId" -n .</details> <details> <summary>apps/cow-fi/components/NetworkItem/index.tsx (4)</summary> `1-6`: **Excellent import additions for better performance and type safety.** The imports correctly add React hooks for memoization, Next.js optimized Image component, and centralized network constants for improved maintainability. --- `18-18`: **Great type safety improvement.** Changing the network prop from a generic string to `keyof typeof NETWORK_MAP` ensures compile-time validation and prevents invalid network identifiers from being passed. --- `22-28`: **Well-implemented memoization for performance optimization.** The useMemo hooks appropriately cache derived values and have correct dependencies. This prevents unnecessary recalculations when the component re-renders. --- `32-41`: **Excellent use of Next.js Image components with proper attributes.** The migration from regular img tags to Next.js Image components includes proper width/height specifications and meaningful alt text, which improves performance and accessibility. </details> <details> <summary>apps/cow-fi/components/TokenDetails/index.tsx (2)</summary> `1-12`: **Well-organized imports for the refactored component.** The imports correctly include all necessary dependencies for the modularized component structure and performance optimizations. --- `139-139`: **Excellent component modularization and type safety.** The refactoring successfully extracts concerns into focused subcomponents while maintaining the original functionality. The explicit return type annotation improves type safety. Also applies to: 147-147, 186-186, 191-191 </details> </blockquote></details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
const { network, platformData } = props | ||
const { address, symbol, name } = platformData | ||
|
||
const networkUrl = useMemo(() => `${NETWORK_URL_MAP[network]}/${address}`, [network, address]) | ||
const networkImage = useMemo(() => NETWORK_IMAGE_MAP[network], [network]) | ||
const networkName = useMemo(() => NETWORK_MAP[network], [network]) |
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.
useMemo is redundant here (in all 3 places)
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.
removed
apps/cow-fi/const/networkMap.ts
Outdated
xdai: 'Gnosis Chain', | ||
} | ||
|
||
export const NETWORK_IMAGE_MAP: Record<keyof typeof NETWORK_MAP, string> = { |
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.
Nitpick: to keep it short we could define a type:
type Network = keyof typeof NETWORK_MAP
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.
good idea, done
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.
I see you created the type Network
but it's not here. Did you forget to change or doesn't apply in this part?
([network, platformData]) => | ||
platformData.contractAddress && ( | ||
<NetworkItem | ||
key={`${network}-${platformData.contractAddress}`} // TODO: check if this is correct |
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.
I think it's correct
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.
Question:
- Where are the COW tokens from Arb1 and Polygon? Maybe we need to add them to the CoW Swap token list for them to show up here? I honestly don't know, just asking.
For example, chainlink shows all the chains https://cowfi-git-feat-update-cowfi-tokens-cowswap.vercel.app/tokens/chainlink
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 @cowdan , great!
However, some issues from my side:
- There are no chains in the tiny swap widget on each token page
- (nitpick) I think, 'Swap on' widgets should be displayed in the same order we display chains in the app. WDYT?
- I did not check everything, but noticed that chains info is still missing for some tokens:
- COW: no polygon and Base
- DAI presents on Ethereum only?
- aave is missing on GC:
Could you please check why this is happening?
Thanks
This page is driven by
Yes but I'd prefer to address it in a separate PR |
Yes, it'll be a separate PR
Good suggestion, done
As mentioned above, it's an issue that needs to be address in the |
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.
del
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.
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.
Approved. Just one nitpick comment
apps/cow-fi/const/networkMap.ts
Outdated
xdai: 'Gnosis Chain', | ||
} | ||
|
||
export const NETWORK_IMAGE_MAP: Record<keyof typeof NETWORK_MAP, string> = { |
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.
I see you created the type Network
but it's not here. Did you forget to change or doesn't apply in this part?
Summary
base
,arbitrum
,avalanche
, andpolygon
To Test
/tokens
Summary by CodeRabbit
New Features
Refactor
Chores