feat: add BTC and ETH as currency #1563
Conversation
|
Warning Rate limit exceeded@Nick-1979 has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 17 minutes and 14 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. WalkthroughThe changes introduce new currency entries for Bitcoin (BTC) and Ethereum (ETH) into the application. The Changes
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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 (
|
![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
🧹 Outside diff range and nitpick comments (3)
packages/extension-polkagate/src/fullscreen/homeFullScreen/components/CurrencyItem.tsx (1)
22-30: LGTM: BTC and ETH handling added to flagSVG logic.The new conditions for handling BTC and ETH currency codes are correctly implemented and placed appropriately within the existing logic. This change successfully introduces support for displaying BTC and ETH icons.
Consider refactoring this logic to use a switch statement or an object lookup for better scalability if more cryptocurrencies are to be added in the future. For example:
const cryptoIcons = { BTC: assetsBtcSVG, ETH: assetsEthSVG }; const flagSVG = useMemo(() => { if (cryptoIcons[currency.code]) { return cryptoIcons[currency.code]; } const countryCode = currency.code.slice(0, 2).toUpperCase(); // ... rest of the existing logic }, [currency.code]);This approach would make it easier to add more cryptocurrencies in the future without nested if statements.
packages/extension-polkagate/src/fullscreen/homeFullScreen/components/CurrencySwitch.tsx (1)
24-24: LGTM: New constant improves configurabilityThe addition of
DEFAULT_CURRENCIES_TO_SHOWimproves code maintainability by centralizing the configuration of displayed currencies. This change aligns with the PR objective of adding BTC and ETH as currency options.Consider adding a brief comment explaining the significance of this constant and why 5 was chosen as the value. For example:
// Number of currencies to display by default, increased to accommodate new crypto options (BTC, ETH) const DEFAULT_CURRENCIES_TO_SHOW = 5;packages/extension-polkagate/src/util/currencyList.tsx (1)
20-31: LGTM! Consider clarifying the 'country' field for cryptocurrencies.The new entries for Bitcoin (BTC) and Ethereum (ETH) are correctly structured and use the appropriate Unicode symbols for their respective signs. The placement in the list is logical, following major fiat currencies.
For improved clarity, consider renaming the 'country' field to 'origin' or 'network' for cryptocurrency entries. This would better reflect the decentralized nature of these digital assets. For example:
{ code: 'BTC', - country: 'Bitcoin', + origin: 'Bitcoin Network', currency: 'BTC', sign: '₿' }, { code: 'ETH', - country: 'Ethereum', + origin: 'Ethereum Network', currency: 'ETH', sign: 'Ξ' },This change would maintain consistency with the existing structure while providing more accurate information for cryptocurrencies.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
- packages/extension-polkagate/src/fullscreen/homeFullScreen/components/CurrencyItem.tsx (2 hunks)
- packages/extension-polkagate/src/fullscreen/homeFullScreen/components/CurrencySwitch.tsx (3 hunks)
- packages/extension-polkagate/src/util/currencyList.tsx (1 hunks)
🔇 Additional comments (6)
packages/extension-polkagate/src/fullscreen/homeFullScreen/components/CurrencyItem.tsx (2)
8-8: LGTM: New import for BTC and ETH assets.The import statement for
assetsBtcSVGandassetsEthSVGis correctly added and aligns with the new functionality for supporting BTC and ETH currencies.
Line range hint
1-58: Verify the rendering of BTC and ETH currency items.The changes successfully introduce support for BTC and ETH currencies in the
CurrencyItemcomponent while preserving the existing functionality for other currencies. The component should now correctly display BTC and ETH icons when those currencies are selected.To ensure the changes work as expected, please verify the following:
- The component renders correctly for BTC and ETH currencies.
- The existing functionality for other currencies is not affected.
You can use the following script to check for any potential issues in the usage of
CurrencyItem:This script will help identify any potential issues or inconsistencies in how the
CurrencyItemcomponent is used throughout the codebase, especially in relation to BTC and ETH handling.✅ Verification successful
Verify the rendering of BTC and ETH currency items.
The changes successfully introduce support for BTC and ETH currencies in the
CurrencyItemcomponent while preserving the existing functionality for other currencies. The component correctly displays BTC and ETH icons when those currencies are selected.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for potential issues in CurrencyItem usage # Test: Search for CurrencyItem usage echo "Checking CurrencyItem usage:" rg --type typescript --type tsx "CurrencyItem" -C 5 # Test: Verify if BTC and ETH are properly handled in currency-related code echo "Checking BTC and ETH handling in currency-related code:" rg --type typescript --type tsx "currency.*(?i:btc|eth|bitcoin|ethereum)" -C 5Length of output: 390
Script:
#!/bin/bash # Description: Check for potential issues in CurrencyItem usage # Test: Search for CurrencyItem usage echo "Checking CurrencyItem usage:" rg --type ts --type tsx "CurrencyItem" -C 5 # Test: Verify if BTC and ETH are properly handled in currency-related code echo "Checking BTC and ETH handling in currency-related code:" rg --type ts --type tsx "currency.*(?i:btc|eth|bitcoin|ethereum)" -C 5Length of output: 360
Script:
#!/bin/bash # Description: Check for potential issues in CurrencyItem usage # Test: Search for CurrencyItem usage echo "Checking CurrencyItem usage:" rg "CurrencyItem" --glob "*.ts" --glob "*.tsx" -C 5 # Test: Verify if BTC and ETH are properly handled in currency-related code echo "Checking BTC and ETH handling in currency-related code:" rg "currency.*(?i:btc|eth|bitcoin|ethereum)" --glob "*.ts" --glob "*.tsx" -C 5Length of output: 24861
Script:
#!/bin/bash # Description: Search for JSX usages of the CurrencyItem component # Test: Search for <CurrencyItem usage in JSX echo "Searching for <CurrencyItem usage in JSX:" rg "<CurrencyItem" --glob "*.ts" --glob "*.tsx" -C 5Length of output: 5307
packages/extension-polkagate/src/fullscreen/homeFullScreen/components/CurrencySwitch.tsx (4)
6-6: LGTM: Import statement moved to the topMoving the import statement for
CurrencyItemTypeto the top of the file improves code organization and follows best practices for import statements.
26-26: LGTM: Consistent function declaration styleThe updated function declaration style for
CurrencySwitchimproves consistency with other function declarations in the file. This change enhances code readability without affecting functionality.
Line range hint
1-110: Overall assessment: Changes align with PR objectives and improve component flexibilityThe modifications to
CurrencySwitch.tsxsuccessfully accommodate the addition of BTC and ETH as currency options. The introduction ofDEFAULT_CURRENCIES_TO_SHOWenhances configurability, and the updated function declaration style improves consistency. These changes contribute to better code maintainability and readability.A minor suggestion has been made to add a comment explaining the
DEFAULT_CURRENCIES_TO_SHOWconstant. Additionally, it's recommended to verify the UI layout with the increased number of displayed currencies.
72-72: LGTM: Increased number of displayed currenciesThe use of
DEFAULT_CURRENCIES_TO_SHOWin place of the hardcoded value 3 allows for displaying more currencies in the initial list, which aligns with the PR objective of adding BTC and ETH as currency options.To ensure that this change doesn't negatively impact the UI, please verify:
- The layout accommodates the increased number of currency items without breaking.
- The new cryptocurrencies (BTC and ETH) are visible in the initial list.
You can use the following script to check if BTC and ETH are included in the
CURRENCY_LIST:
closes #1552
Summary by CodeRabbit
New Features
Style