Feat/delete custom tokens #560
Conversation
WalkthroughThe changes in this pull request introduce several enhancements across multiple components related to token management in a blockchain application. A new method Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant AssetDetailView
participant TokensState
participant NetworkAssetsItem
User->>AssetDetailView: Request to delete custom token
AssetDetailView->>TokensState: removeErc20Token(chainName, address)
TokensState-->>AssetDetailView: Promise<boolean>
AssetDetailView->>NetworkAssetsItem: Emit update:tokens
NetworkAssetsItem->>NetworkAssetsItem: Fetch updated tokens
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
Documentation and Community
|
|
💼 Build Files |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Outside diff range and nitpick comments (7)
packages/extension/src/libs/tokens-state/index.ts (1)
63-66: Consider adding input validation for the address parameter.The method could benefit from validating the address format before processing.
Consider adding validation:
async removeErc20Token( chainName: NetworkNames, address: string, ): Promise<boolean> { + if (!address || !/^0x[a-fA-F0-9]{40}$/.test(address)) { + return false; + } const state: IState | null = await this.storage.get(StorageKeys.customTokens);packages/extension/src/ui/action/views/network-assets/index.vue (1)
Line range hint
1-224: Consider architectural improvements for better maintainability.While the current implementation works, consider these future improvements:
- Extract token management logic into a composable
- Add error handling for token operations
- Add loading states for async token operations
- Consider using Vuex/Pinia for state management if token state is used across multiple components
This would improve maintainability and make the component more focused on UI rendering.
packages/extension/src/ui/action/views/network-assets/components/network-assets-item.vue (1)
144-146: Consider adding loading state management.The async operation could benefit from a loading state to improve UX and prevent potential UI flickers.
+const isLoading = ref(false); onMounted(async () => { + isLoading.value = true; await fetchCustomTokens(); + isLoading.value = false; });packages/extension/src/ui/action/views/asset-detail-view/index.vue (1)
329-333: Enhance warning label visibility and fix layoutThe warning label styling could be more prominent given its importance, and the negative margins might cause layout issues.
Consider these style improvements:
&__action { - margin: 0 0 -12px -12px; - width: calc(~'100% + 24px'); + margin: 0 0 12px 0; + padding: 0 12px; text-align: center; .label { color: @secondaryLabel; + display: block; + margin-top: 8px; + padding: 8px; + background-color: fade(@warning, 10%); + border-radius: 4px; + font-weight: 500; } }packages/extension/src/providers/ethereum/types/evm-network.ts (1)
213-213: Consider using a more robust address comparison method.While the current case-insensitive comparison works, consider using the network's
isAddressmethod and checksum addresses for a more robust comparison. This would align with Ethereum's address handling best practices.Here's a suggested improvement:
-a.contract.toLowerCase() === (token as CustomErc20Token).address.toLowerCase() +this.isAddress(a.contract) && this.isAddress((token as CustomErc20Token).address) && +toChecksumAddress(a.contract) === toChecksumAddress((token as CustomErc20Token).address)packages/extension/src/ui/action/views/network-assets/components/custom-evm-token.vue (2)
50-57: LGTM! Consider extracting the truncation logic.The conditional rendering and truncation of long token symbols with tooltip is well implemented. However, since similar truncation logic is used for both token name and symbol, consider extracting it into a reusable computed property or method.
+ const truncateText = (text: string, limit = 16) => ({ + shouldTruncate: text.length > limit, + truncated: `${text.slice(0, 12)}...`, + original: text + });
Line range hint
1-1: Consider adding token deletion capabilityGiven that this PR is about deleting custom tokens (
Feat/delete custom tokens), consider adding token deletion functionality to this component, possibly through a delete button in the token info section when viewing existing tokens.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (6)
packages/extension/src/libs/tokens-state/index.ts(2 hunks)packages/extension/src/providers/ethereum/types/evm-network.ts(2 hunks)packages/extension/src/ui/action/views/asset-detail-view/index.vue(3 hunks)packages/extension/src/ui/action/views/network-assets/components/custom-evm-token.vue(3 hunks)packages/extension/src/ui/action/views/network-assets/components/network-assets-item.vue(4 hunks)packages/extension/src/ui/action/views/network-assets/index.vue(2 hunks)
🧰 Additional context used
🪛 Biome
packages/extension/src/libs/tokens-state/index.ts
[error] 69-69: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
🔇 Additional comments (11)
packages/extension/src/libs/tokens-state/index.ts (2)
38-38: LGTM!
The formatting change improves readability while maintaining the same functionality.
57-93: Well-implemented token removal functionality!
The implementation is thorough, handling edge cases like empty token arrays and cleaning up empty networks.
🧰 Tools
🪛 Biome
[error] 69-69: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
packages/extension/src/ui/action/views/network-assets/index.vue (1)
21-22: LGTM! Well-structured component communication.
The addition of the network prop and update:tokens event handler follows Vue.js best practices for parent-child component communication, enabling proper token management functionality.
packages/extension/src/ui/action/views/network-assets/components/network-assets-item.vue (3)
54-56: LGTM: Template changes properly integrate custom token management.
The new props are correctly bound to the asset-detail-view component, enabling custom token management functionality.
62-64: LGTM: Imports are properly structured.
The necessary imports for types and functionality are correctly added.
Also applies to: 74-74
84-87: Consider handling undefined network properties.
While the prop type is correctly defined, the empty object default value might lead to runtime errors if code assumes network properties exist without validation.
packages/extension/src/ui/action/views/asset-detail-view/index.vue (1)
72-72: Verify component integration
Let's verify the integration with network-assets-item.vue and other related components.
Also applies to: 88-95
✅ Verification successful
Component integration is properly implemented
The integration between components is correctly implemented with proper prop passing and event handling:
- BaseButton is properly imported and used within asset-detail-view for the delete action
- network-assets-item correctly imports and uses asset-detail-view with all required props:
- token
- isCustomToken
- removeToken
- close event handling
- The custom token deletion UI is conditionally rendered based on isCustomToken prop
- Props are properly typed and have appropriate defaults
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check component integration and event handling
# Check for removeToken event handlers
echo "Checking for removeToken event handlers..."
rg -l "removeToken" --type vue
# Check for isCustomToken usage
echo "Checking for isCustomToken usage..."
rg -l "isCustomToken" --type vue
# Check for related components and their interactions
echo "Checking for related component imports..."
rg -l "asset-detail-view" --type vue
Length of output: 453
Script:
#!/bin/bash
# Let's try a different approach without file type filtering
# Check for removeToken event handlers
echo "Checking for removeToken event handlers..."
rg -l "removeToken"
# Check for isCustomToken usage
echo "Checking for isCustomToken usage..."
rg -l "isCustomToken"
# Check for related component imports and usage
echo "Checking for related component imports..."
rg -l "asset-detail-view"
# Check for BaseButton usage in the component
echo "Checking BaseButton usage in the component..."
rg -C 3 "BaseButton" "packages/extension/src/ui/action/views/asset-detail-view/index.vue"
# Check for network-assets-item references
echo "Checking network-assets-item references..."
rg -l "network-assets-item"
Length of output: 1503
Script:
#!/bin/bash
# Let's examine the actual integration between these components
# Check how BaseButton is used in asset-detail-view
echo "Checking BaseButton usage context..."
rg -C 10 "BaseButton" packages/extension/src/ui/action/views/asset-detail-view/index.vue
# Check the interaction between asset-detail-view and network-assets-item
echo "Checking removeToken implementation in asset-detail-view..."
rg -C 10 "removeToken" packages/extension/src/ui/action/views/asset-detail-view/index.vue
# Check how network-assets-item uses asset-detail-view
echo "Checking asset-detail-view usage in network-assets-item..."
rg -C 10 "asset-detail-view" packages/extension/src/ui/action/views/network-assets/components/network-assets-item.vue
# Check isCustomToken implementation
echo "Checking isCustomToken implementation..."
rg -C 5 "isCustomToken" packages/extension/src/ui/action/views/asset-detail-view/index.vue
Length of output: 4637
packages/extension/src/providers/ethereum/types/evm-network.ts (2)
160-160: LGTM!
The indentation change improves code readability while maintaining the same functionality.
Line range hint 1-284: Verify integration with token deletion functionality.
Given that this file handles custom token management and the PR's objective is to implement token deletion, let's verify the integration between this file and the token deletion functionality.
✅ Verification successful
Token deletion functionality is properly integrated
The verification shows a complete and well-integrated token deletion implementation:
TokensStateclass provides the coreremoveErc20Tokenmethod that handles token deletion from storage- UI components properly integrate this functionality:
network-assets-item.vueimplements the deletion trigger and updates the token listasset-detail-view.vueprovides the deletion UI with appropriate user warning
The file under review correctly uses TokensState for managing custom tokens and properly integrates with the deletion functionality through the getAllTokenInfo method, which filters out deleted tokens from the displayed list.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for token deletion related code and its integration
# Look for token deletion related methods
echo "Searching for token deletion related methods..."
rg -l "removeToken|deleteToken|removeErc20Token"
# Look for files that import TokensState and use deletion methods
echo "Checking TokensState usage..."
ast-grep --pattern 'import { TokensState } from "@/libs/tokens-state"'
# Look for UI components that might trigger token deletion
echo "Checking UI components..."
fd -e vue -e ts -x rg -l "removeToken|deleteToken|removeErc20Token" {}
Length of output: 851
Script:
#!/bin/bash
# Let's examine the token deletion implementation and its integration
# Check the implementation in tokens-state
echo "Checking TokensState implementation..."
rg -A 10 "removeToken|deleteToken" packages/extension/src/libs/tokens-state/index.ts
# Check UI components that trigger deletion
echo "Checking network-assets-item.vue implementation..."
rg -A 10 "removeToken|deleteToken" packages/extension/src/ui/action/views/network-assets/components/network-assets-item.vue
echo "Checking asset-detail-view implementation..."
rg -A 10 "removeToken|deleteToken" packages/extension/src/ui/action/views/asset-detail-view/index.vue
# Check if TokensState is properly imported in the UI components
echo "Checking TokensState imports in UI..."
ast-grep --pattern 'import { TokensState } from "@/libs/tokens-state"' packages/extension/src/ui/action/views/network-assets/components/network-assets-item.vue packages/extension/src/ui/action/views/asset-detail-view/index.vue
Length of output: 2122
Script:
#!/bin/bash
# Let's check the TokensState implementation since it wasn't shown in previous results
echo "Examining TokensState class implementation..."
cat packages/extension/src/libs/tokens-state/index.ts
# Also check for any tests related to token deletion
echo "Checking for related tests..."
fd -e spec.ts -e test.ts -x rg -l "removeToken|deleteToken" {}
Length of output: 3087
packages/extension/src/ui/action/views/network-assets/components/custom-evm-token.vue (2)
432-436: LGTM! Style changes improve layout.
The switch to flex display and consistent font sizing enhances the token info layout and maintains visual harmony.
289-289: Verify content fits after height reduction
The container height has been reduced significantly from 568px to 385px. Please ensure that all content (including error states and warnings) fits properly within the reduced height without overflow or scrollbars.
✅ Verification successful
Let me generate additional scripts to verify the content structure and potential overflow issues.
Height reduction is appropriate and content fits properly
After analyzing the template structure and styles, the height reduction from 568px to 385px is appropriate because:
- The layout has a fixed structure with predictable content heights:
- Header (68px - padding + content)
- Contract input (64px)
- Token info/error section (~100px)
- Warning message (~80px)
- Bottom buttons section (~57px)
- All content sections have proper spacing and no scrollable areas
- Error and warning states are accommodated within the new height
- The component uses absolute positioning for buttons at the bottom
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other height-related styles or potential overflow issues
rg -g '*.vue' -g '*.less' 'height:|overflow:|max-height:' packages/extension/src/ui/action/views/network-assets/
Length of output: 3924
Script:
#!/bin/bash
# Check the template structure and content elements in custom-evm-token.vue
cat packages/extension/src/ui/action/views/network-assets/components/custom-evm-token.vue
Length of output: 12804
packages/extension/src/ui/action/views/network-assets/index.vue
Outdated
Show resolved
Hide resolved
packages/extension/src/ui/action/views/network-assets/components/network-assets-item.vue
Outdated
Show resolved
Hide resolved
packages/extension/src/ui/action/views/network-assets/components/network-assets-item.vue
Outdated
Show resolved
Hide resolved
packages/extension/src/ui/action/views/network-assets/components/network-assets-item.vue
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
packages/extension/src/ui/action/views/network-assets/components/network-assets-item.vue (2)
84-87: Consider a more type-safe default value for the network propThe empty object default value might not satisfy the BaseNetwork type requirements. Consider either making the prop required or providing a proper default value that matches the BaseNetwork interface.
network: { type: Object as PropType<BaseNetwork>, - default: () => ({}), + required: true, },
133-150: Consider using provide/inject for TokensStateThe TokensState instance could be provided at a higher level component and injected here, following Vue's dependency injection pattern. This would:
- Prevent multiple instances of TokensState
- Make it easier to mock for testing
- Follow the single responsibility principle
Also, consider handling component unmount during fetch.
-const tokenState = new TokensState(); +const tokenState = inject('tokenState') as TokensState; +if (!tokenState) throw new Error('TokensState not provided'); const fetchCustomTokens = async () => { + const controller = new AbortController(); try { - return await tokenState.getTokensByNetwork(props.network.name).then(res => { + return await tokenState.getTokensByNetwork(props.network.name, { signal: controller.signal }).then(res => { customTokens.value = res.filter( (token): token is CustomErc20Token => 'address' in token, ); }); } catch { customTokens.value = []; } + return () => controller.abort(); }; onMounted(async () => { - await fetchCustomTokens(); + const cleanup = await fetchCustomTokens(); + onUnmounted(cleanup); });
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
packages/extension/src/ui/action/views/network-assets/components/network-assets-item.vue(4 hunks)packages/extension/src/ui/action/views/network-assets/index.vue(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/extension/src/ui/action/views/network-assets/index.vue
🔇 Additional comments (3)
packages/extension/src/ui/action/views/network-assets/components/network-assets-item.vue (3)
54-56: LGTM: Template props are correctly bound
The new props are properly bound to the asset-detail-view component using the correct Vue syntax.
152-158: LGTM: Robust implementation of isCustomToken
The computed property correctly:
- Guards against undefined contract
- Performs case-insensitive address comparison
- Uses proper type narrowing with the some() predicate
164-173: **** The past review comment about error handling in the removeToken method is still valid and applicable.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
packages/extension/src/libs/tokens-state/index.ts (1)
63-93: Consider improving null safety and defensive programmingWhile the implementation is solid, there are a few improvements that could make it more robust:
- Use optional chaining for safer null handling
- Add defensive checks for the tokens array
Here's a suggested improvement:
async removeErc20Token( chainName: NetworkNames, address: string, ): Promise<boolean> { const state: IState | null = await this.storage.get(StorageKeys.customTokens); - if (state && state[chainName]) { - const tokens = state[chainName]; + const tokens = state?.[chainName]; + if (tokens?.length) { - for (let i = 0; i < tokens!.length; i++) { - const token = tokens![i]; + for (let i = 0; i < tokens.length; i++) { + const token = tokens[i]; if ( token.type === TokenType.ERC20 && (token as CustomErc20Token).address.toLowerCase() === address.toLowerCase() ) { - tokens!.splice(i, 1); + tokens.splice(i, 1); - if (tokens!.length === 0) { + if (tokens.length === 0 && state) { delete state[chainName]; } await this.storage.set(StorageKeys.customTokens, state); return true; } } } return false; }🧰 Tools
🪛 Biome (1.9.4)
[error] 69-69: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
packages/extension/src/libs/tokens-state/index.ts(2 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
packages/extension/src/libs/tokens-state/index.ts
[error] 69-69: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
🔇 Additional comments (2)
packages/extension/src/libs/tokens-state/index.ts (2)
38-38: LGTM!
The formatting change in the address comparison maintains the same functionality while improving code readability.
86-86: Missing await keyword
The storage operation should be awaited.
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Improvements