Release: v2.4.0 #614
Release: v2.4.0 #614
Conversation
…t into fix/ui-freezing
…t into fix/identicon-mismatch
WalkthroughThe changes introduce modifications to several Vue components and stylesheets. In Changes
Possibly related PRs
Suggested reviewers
✨ Finishing Touches
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: 0
🧹 Nitpick comments (1)
packages/extension/src/ui/onboard/App.vue (1)
72-72: Consolidate overflow properties.The
overflow: hidden;on line 72 conflicts withoverflow-y: auto;on line 75. Consider consolidating these properties for better maintainability.body { width: 100vw; height: 100vh; margin: 0; - overflow: hidden; font-size: 0; font-family: 'Roboto', sans-serif; - overflow-y: auto; + overflow-x: hidden; + overflow-y: auto; }Also applies to: 75-75
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/extension/src/ui/onboard/App.vue(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: buildAll
- GitHub Check: test
🔇 Additional comments (1)
packages/extension/src/ui/onboard/App.vue (1)
102-102: Review the height constraints.The container has both
height: 100vh;andmin-height: 700px;. On desktop,100vhwill always be greater than 700px, makingmin-heightredundant. However, on mobile devices with small screens, this could force unnecessary scrolling.Please verify:
- Is the 700px minimum height requirement necessary for all screen sizes?
- Have you tested this on mobile devices to ensure a good user experience?
Also applies to: 108-108
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
packages/extension/src/ui/action/components/network-menu/index.vue (1)
95-95: Consider removing !important flags from styles.The use of
!importantflags in CSS should be avoided as they:
- Make styles harder to override when needed
- Can lead to specificity wars
- Make the codebase harder to maintain
- opacity: 1; + opacity: 1; - opacity: 0.6 !important; + opacity: 0.6; - opacity: 1 !important; + opacity: 1;Also applies to: 103-103, 109-110
packages/extension/src/ui/action/views/network-activity/components/network-activity-action.vue (1)
111-114: Consider using CSS custom properties for focus styles.The focus styles could be more maintainable using CSS custom properties.
+:root { + --focus-ring-color: -webkit-focus-ring-color; + --focus-ring-offset: 2px; +} &:focus { - outline: -webkit-focus-ring-color auto 1px !important; - outline-offset: 2px !important; + outline: var(--focus-ring-color) auto 1px; + outline-offset: var(--focus-ring-offset); }packages/extension/src/ui/action/icons/tabs/activity.vue (1)
27-33: Consider optimizing the inactive state SVG path.The path data for the inactive state contains an excessive number of coordinate points, which could be simplified for better maintainability and potentially smaller file size.
Consider using an SVG optimization tool to simplify the path data while maintaining visual fidelity. Tools like SVGO can help with this optimization.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
packages/extension/src/providers/ethereum/ui/styles/common-popup.less(1 hunks)packages/extension/src/ui/action/components/network-menu/index.vue(4 hunks)packages/extension/src/ui/action/icons/tabs/activity.vue(1 hunks)packages/extension/src/ui/action/icons/tabs/assets.vue(1 hunks)packages/extension/src/ui/action/icons/tabs/dapps.vue(1 hunks)packages/extension/src/ui/action/icons/tabs/nfts.vue(1 hunks)packages/extension/src/ui/action/views/network-activity/components/network-activity-action.vue(4 hunks)packages/extension/src/ui/action/views/network-assets/components/network-assets-item.vue(2 hunks)
✅ Files skipped from review due to trivial changes (1)
- packages/extension/src/providers/ethereum/ui/styles/common-popup.less
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: buildAll
- GitHub Check: test
🔇 Additional comments (8)
packages/extension/src/ui/action/components/network-menu/index.vue (1)
6-6: LGTM! Improved active state handling.The addition of
exact-active-classensures precise route matching for active states.Also applies to: 15-15, 22-22, 32-32
packages/extension/src/ui/action/views/network-activity/components/network-activity-action.vue (1)
4-9: LGTM! Improved semantic markup and accessibility.Great improvements:
- Using
<button>instead of<a>for actions is semantically correct- Added proper focus styles for keyboard navigation
Also applies to: 10-15
packages/extension/src/ui/action/views/network-assets/components/network-assets-item.vue (1)
7-7: LGTM! Improved image rendering performance.The explicit width and height attributes help prevent Cumulative Layout Shift (CLS) and improve Core Web Vitals.
Also applies to: 226-228
packages/extension/src/ui/action/icons/tabs/assets.vue (2)
2-36: LGTM! Clean and consistent SVG implementation.The conditional rendering of SVGs based on the
isActiveprop is well-structured, with proper opacity differentiation between active and inactive states.
39-46: LGTM! Clean prop definition.The
isActiveprop is properly typed and has a sensible default value.packages/extension/src/ui/action/icons/tabs/nfts.vue (1)
2-42: LGTM! Well-structured SVG with proper clipping path.The SVG implementation is clean with proper use of clipping path for the active state.
packages/extension/src/ui/action/icons/tabs/dapps.vue (1)
2-36: LGTM! Clean grid icon implementation.The SVG implementation effectively represents a grid pattern with proper state handling.
packages/extension/src/ui/action/icons/tabs/activity.vue (1)
2-35: LGTM! Consistent with other tab icons.The implementation follows the same pattern as other tab icons, maintaining consistency across the UI.
Also applies to: 38-51
Fix/show active network in tabs
devop: update zerox api
Fix/paste window
Fix/UI freezing
…into feat/backup-detected
feat: add backup page
There was a problem hiding this comment.
Actionable comments posted: 5
🔭 Outside diff range comments (3)
packages/extension/src/ui/onboard/create-wallet/wallet-ready.vue (1)
9-20:⚠️ Potential issueAdd security attributes to external links.
External links opening in new tabs should include
rel="noopener noreferrer"to prevent potential security vulnerabilities.Apply this diff to add the security attributes:
- <a href="https://github.com/myetherwallet" target="_blank"> + <a href="https://github.com/myetherwallet" target="_blank" rel="noopener noreferrer"> - <a href="https://www.instagram.com/myetherwallet/" target="_blank"> + <a href="https://www.instagram.com/myetherwallet/" target="_blank" rel="noopener noreferrer"> - <a href="https://www.reddit.com/r/MyEtherWallet/" target="_blank"> + <a href="https://www.reddit.com/r/MyEtherWallet/" target="_blank" rel="noopener noreferrer"> - <a href="https://twitter.com/myetherwallet" target="_blank"> + <a href="https://twitter.com/myetherwallet" target="_blank" rel="noopener noreferrer">packages/extension/src/ui/action/views/network-assets/index.vue (1)
112-132: 🛠️ Refactor suggestionEnhance error handling and prevent rapid retries.
The error handling could be improved with error type information and debouncing.
Consider these improvements:
+const updateAssetsDebounced = useDebounceFn(async () => { + try { + isFetchError.value = false; + isLoading.value = true; + assets.value = []; + const currentNetwork = selectedNetworkName.value; + if (!props.accountInfo.selectedAccount?.address) return; + + const _assets = await props.network.getAllTokenInfo( + props.accountInfo.selectedAccount.address + ); + + if (selectedNetworkName.value !== currentNetwork) return; + assets.value = _assets; + } catch (error) { + if (error instanceof NetworkError) { + console.error('Network error:', error); + } else { + console.error('Unknown error:', error); + } + isFetchError.value = true; + assets.value = []; + } finally { + isLoading.value = false; + } +}, 1000); + -const updateAssets = () => { - isFetchError.value = false; - isLoading.value = true; - assets.value = []; - const currentNetwork = selectedNetworkName.value; - if (props.accountInfo.selectedAccount?.address) { - props.network - .getAllTokenInfo(props.accountInfo.selectedAccount?.address || '') - .then(_assets => { - if (selectedNetworkName.value !== currentNetwork) return; - assets.value = _assets; - isLoading.value = false; - }) - .catch(() => { - if (selectedNetworkName.value !== currentNetwork) return; - isFetchError.value = true; - isLoading.value = false; - assets.value = [];; - }); - } -}; +const updateAssets = () => { + updateAssetsDebounced(); +};Don't forget to add the necessary imports:
import { useDebounceFn } from '@vueuse/core'; import { NetworkError } from '@/types/errors';packages/extension/src/ui/onboard/create-wallet/routes.ts (1)
43-49: 🛠️ Refactor suggestionAvoid mutating route objects and enhance type safety.
The function correctly transforms routes but has some potential issues:
- Direct mutation of route objects
- Unsafe type assertion on route.name
export default (): RouteRecordRaw[] => { - return Object.values(routes).map(route => { - route.path = `/${namespace}/${route.path}`; - route.name = `${namespace}-${String(route.name)}`; - return route; + return Object.values(routes).map(route => ({ + ...route, + path: `/${namespace}/${route.path}`, + name: `${namespace}-${route.name as string}`, + component: route.component, + })); }); };
🧹 Nitpick comments (39)
packages/extension/src/ui/action/views/network-assets/components/network-assets-error.vue (6)
6-7: Fix typo in class name.The class name contains a typo: "network-assetss__error-button" has an extra 's'.
- class="network-assetss__error-button" + class="network-assets__error-button"
16-18: Add type validation for updateAssets prop.The
updateAssetsprop should have proper type validation to ensure it's a required function.-defineProps({ - updateAssets: Function, -}); +defineProps({ + updateAssets: { + type: Function as PropType<() => void>, + required: true + } +});
40-42: Remove unused 'a' selector in styles.The styles contain an unused 'a' selector. The component uses a button, not an anchor tag.
- a { - width: 150px; - }
48-61: Consider using a composable for error handling logic.The timer-based error detection logic could be reused across components. Consider extracting it into a composable function.
Example implementation:
// useLoadingError.ts import { ref, watchEffect, onBeforeMount } from 'vue'; export function useLoadingError( condition: () => boolean, timeout: number = 30000 ) { let timer: NodeJS.Timeout | null = null; const assumedError = ref(false); watchEffect(() => { if (timer) { clearTimeout(timer); } if (condition()) { timer = setTimeout(() => { assumedError.value = true; }, timeout); } }); onBeforeMount(() => { if (timer) { clearTimeout(timer); } }); return { assumedError }; }Usage in component:
-let timer: NodeJS.Timeout | null = null; -const assumedError = ref(false); - -watchEffect(() => { - if (timer) { - clearTimeout(timer); - } - if (props.cryptoAmount == '~') { - timer = setTimeout(() => { - assumedError.value = true; - }, 30000); - } -}); - -onBeforeMount(() => { - if (timer) { - clearTimeout(timer); - } -}); +const { assumedError } = useLoadingError( + () => props.cryptoAmount === '~' +);
125-130: Enhance error handling with specific error types.The catch block could provide more specific error handling based on the error type, and potentially retry the request for certain error types.
}) - .catch(() => { + .catch((error: Error) => { if (selectedNetworkName.value !== currentNetwork) return; + console.error('Failed to fetch assets:', error); + // Retry for network errors + if (error.name === 'NetworkError' && !error.message.includes('unauthorized')) { + setTimeout(updateAssets, 5000); + return; + } isFetchError.value = true; isLoading.value = false; assets.value = [];
112-132: Consider using async/await for better readability.The promise chain could be refactored to use async/await for improved readability and error handling.
-const updateAssets = () => { +const updateAssets = async () => { isFetchError.value = false; isLoading.value = true; assets.value = []; const currentNetwork = selectedNetworkName.value; - if (props.accountInfo.selectedAccount?.address) { - props.network - .getAllTokenInfo(props.accountInfo.selectedAccount?.address || '') - .then(_assets => { - if (selectedNetworkName.value !== currentNetwork) return; - assets.value = _assets; - isLoading.value = false; - }) - .catch(() => { - if (selectedNetworkName.value !== currentNetwork) return; - isFetchError.value = true; - isLoading.value = false; - assets.value = []; - }); + try { + if (!props.accountInfo.selectedAccount?.address) return; + + const _assets = await props.network.getAllTokenInfo( + props.accountInfo.selectedAccount.address + ); + + if (selectedNetworkName.value !== currentNetwork) return; + + assets.value = _assets; + } catch (error) { + if (selectedNetworkName.value !== currentNetwork) return; + console.error('Failed to fetch assets:', error); + isFetchError.value = true; + assets.value = []; + } finally { + isLoading.value = false; } };packages/utils/src/nacl-encrypt-decrypt.ts (4)
13-14: Consider adding input validation or error handling.
naclDecodeHexrelies onhexToBuffer(msgHex)and subsequent base64 decoding. IfmsgHexis malformed or empty, this could throw an unhandled error. You might want to wrap this operation in try/catch or validate input length upfront to improve robustness.
26-59: Handle null return in decryption more explicitly.
IfnaclBox.openfails, it returnsnull, which causesencodeUTF8(decryptedMessage)to throw. While it's currently caught and rethrown as "Decryption failed.", consider explicitly checking for anullresult before encoding to clarify the error path and potentially log additional context.
61-111: Secure ephemeral key usage and memory cleanup.
naclEncryptappropriately incorporatesrandomBytes(24)for the nonce and ephemeral key pair generation. However, for a fully robust implementation, consider securely cleaning (zeroing) the ephemeral secret key from memory once encryption is complete. This helps reduce the likelihood of sensitive key material lingering in memory longer than necessary.
113-119: Ensure usage constraints are documented.
ExposingnaclDecryptandnaclEncryptto the broader codebase can encourage flexible use but also raises potential misuse concerns (e.g., storing private keys insecurely). Consider emphasizing best practices in documentation or using typed patterns that better guide correct usage.packages/utils/src/random-names.ts (3)
6-100: Extensive adjective list.
Yourleftarray provides a wide range of adjectives that effectively diversify generated names. No issues found, though you might consider a brief note on how these terms were selected to maintain consistency or style if these are used broadly across your product.
586-592: Support for non-ASCII strings.
sumAsciiadds the char codes. IfseedStrincludes extended Unicode characters, those code points could exceed single-byte ranges. Suggest clarifying or limiting the expected character set to avoid unexpected collisions or expanding to handle full Unicode.
594-602: Non-cryptographic randomness.
generateRandomNameWithSeeduses a sine-based pseudo-random formula suitable for reproducible naming but not for security-sensitive operations. Consider explicitly documenting this limitation to avoid confusion with cryptographically secure RNG usage.packages/swap/src/providers/zerox/index.ts (1)
193-195: Adopt defensive checks for transaction fields.
The new approach of usingresponse.transaction.to/value/datais appropriate; just confirm thatresponse.transactionis always present in the API response, or add optional chaining or conditionals to future-proof against missing fields.packages/extension/src/libs/backup-state/index.ts (4)
35-50: Consider alternative error handling strategy.
The#getSignaturemethod relies on console errors. For better resiliency, consider throwing a custom error or returning a structured error object. This prevents potential silent failures if external consumers ignore console output.
224-225: Remove the unnecessary continue statement.
Static analysis flags this as redundant since there’s no logic following this path in the loop iteration. You can safely remove it with no functional impact.Apply this diff to remove the unnecessary statement:
await kr.renameAccount(existingAccount.address, newAccount.name); - continue; } else if (newAccount) {🧰 Tools
🪛 Biome (1.9.4)
[error] 224-224: Unnecessary continue statement
Unsafe fix: Delete the unnecessary continue statement
(lint/correctness/noUnnecessaryContinue)
246-319: Validate successful backups before updating state.
While this block properly handles backup encryption and uploads, it updates the backup state even if the server responds with a non-2xx status (only checksmessage === 'Ok'). Ensure you handle unsuccessful HTTP status codes or unexpected response structures, e.g., if.json()fails or a network error occurs.
321-360: Enhance fallback or default state handling.
When no stored backup info exists, the code creates a new default state. Consider logging or informing the user about generated user IDs or forcibly returning a well-defined object. This ensures clarity in the UX and debugging process.packages/extension/src/libs/backup-state/types.ts (3)
7-11: Add JSDoc comments to document the interface properties.While the property names are descriptive, adding JSDoc comments would help clarify their purpose and any constraints.
export interface IState { + /** Timestamp of the last successful backup */ lastBackupTime: number; + /** Unique identifier of the user */ userId: string; + /** Whether backup functionality is enabled */ enabled: boolean; }
13-20: Consider using number type for timestamps consistently.For consistency with
lastBackupTimeinIState, consider usingnumbertype forupdatedAtto represent timestamps uniformly.export interface ListBackupType { userId: string; - updatedAt: string; + updatedAt: number; }
26-29: Document security implications of excluded fields.Please add a comment explaining why
addressandpublicKeyare excluded from the backup data for security purposes.+/** + * Backup data structure that excludes sensitive account information (address and publicKey) + * for security purposes. + */ export interface BackupData { accounts: Omit<EnkryptAccount, 'address' | 'publicKey'>[]; uuid: string; }packages/extension/src/ui/action/views/settings/views/settings-backups/backup-identicon.vue (1)
18-34: Optimize SVG positioning using transform.Replace negative margins with transform for better performance and maintainability.
.jdenticon-container { border-radius: 50%; overflow: hidden; height: 32px; width: 32px; position: relative; svg { width: 40px; height: 40px; position: absolute; - top: -4px; - left: -4px; + top: 50%; + left: 50%; + transform: translate(-50%, -50%); } }packages/extension/src/ui/action/views/settings/components/settings-switch.vue (1)
45-46: Remove duplicatebox-sizingproperty.The
box-sizingproperty is declared twice on consecutive lines.box-sizing: border-box; - box-sizing: border-box;packages/extension/src/ui/onboard/restore-wallet/type-password.vue (1)
45-61: Consider enhancing error feedback for users.While the error handling is good, consider showing a user-friendly error message instead of just logging to console.
.catch(error => { isInitializing.value = false; console.error('Wallet initialization failed:', error); + // Show user-friendly error message + store.setError('Failed to initialize wallet. Please try again.'); });packages/extension/src/libs/utils/initialize-wallet.ts (1)
62-91: Consider adding error type information and improving error handling.While the backup state implementation is good, the error handling could be enhanced:
- The catch block silently logs errors
- The error type in the catch block is not specified
- } catch (e) { + } catch (e: unknown) { console.error(e); + if (e instanceof Error) { + console.error('Backup initialization failed:', e.message); + } return { backupsFound: false }; }packages/extension/src/ui/action/views/settings/index.vue (1)
60-60: Consider adding state persistence for settings view.The backup settings state is managed in memory but not persisted. Consider saving the last active settings view to improve user experience when reopening settings.
+const SETTINGS_VIEW_KEY = 'last_settings_view'; + +const loadLastView = async () => { + const lastView = await browser.storage.local.get(SETTINGS_VIEW_KEY); + if (lastView[SETTINGS_VIEW_KEY]) { + setAllToFalse(); + switch (lastView[SETTINGS_VIEW_KEY]) { + case 'backups': + isBackups.value = true; + break; + // ... other cases + } + } +}; + const backupsAction = () => { setAllToFalse(); isBackups.value = true; + browser.storage.local.set({ [SETTINGS_VIEW_KEY]: 'backups' }); };Also applies to: 68-68, 117-120
packages/extension/src/ui/action/views/accounts/components/add-account-form.vue (1)
124-127: Consider enhancing error handling for backup failures.While the backup operation is properly implemented as a non-blocking operation, consider enhancing the error handling to provide user feedback when backup fails, rather than just logging to console.
const backupState = new BackupState(); -backupState.backup(false).catch(() => { - console.error('Failed to backup'); -}); +backupState.backup(false).catch((error) => { + console.error('Failed to backup:', error); + // Show a non-blocking notification to the user + showNotification({ + type: 'error', + message: 'Account created successfully, but backup failed. You can retry backup in settings.', + }); +});packages/extension/src/types/provider.ts (1)
134-134: Remove unnecessary constructor.The constructor can be safely removed as it doesn't add any functionality beyond what the default constructor would provide.
- constructor(node: string, options?: unknown) {}🧰 Tools
🪛 Biome (1.9.4)
[error] 134-134: This constructor is unnecessary.
Unsafe fix: Remove the unnecessary constructor.
(lint/complexity/noUselessConstructor)
packages/extension/src/ui/onboard/restore-wallet/backup-detected.vue (3)
11-25: Enhance accessibility for backup selection.The backup selection uses anchor tags (
<a>) for clickable items without proper ARIA attributes or keyboard navigation support.<a v-for="backup in backups" :key="backup.userId" @click="selectBackup(backup)" + role="button" + tabindex="0" + :aria-selected="selectedBackup === backup" + @keyup.enter="selectBackup(backup)" + @keyup.space="selectBackup(backup)" :class="[ { selected: selectedBackup === backup }, 'backup-detected__backup-item', ]" >
84-108: Consider implementing retry logic for backup loading.The component attempts to create a new account if loading the main wallet fails, but doesn't retry loading backups if that fails.
onBeforeMount(async () => { await kr.unlock(unref(password)); - await backupState.getMainWallet().catch(async () => { + await backupState.getMainWallet().catch(async (error) => { + console.error('Failed to get main wallet:', error); await kr.saveNewAccount({ basePath: EthereumNetworks.ethereum.basePath, name: 'EVM Account 1', signerType: EthereumNetworks.ethereum.signer[0], walletType: WalletType.mnemonic, }); }); - const mainWallet = await backupState.getMainWallet(); + let retries = 3; + let mainWallet; + while (retries > 0) { + try { + mainWallet = await backupState.getMainWallet(); + break; + } catch (error) { + console.error(`Failed to get main wallet (${retries} retries left):`, error); + retries--; + if (retries === 0) throw error; + await new Promise(resolve => setTimeout(resolve, 1000)); + } + }
113-130: Enhance error handling in backup restoration.The error handling in
useBackupcould provide more specific error feedback to users.const useBackup = async () => { if (selectedBackup.value) { backupBtnText.value = 'Restoring backup...'; processing.value = true; await backupState .restoreBackup(selectedBackup.value.userId, unref(password)) .then(() => { router.push({ name: routes.walletReady.name, }); }) - .catch(() => { - backupBtnText.value = 'Failed to restore backup'; + .catch((error) => { + console.error('Backup restoration failed:', error); + backupBtnText.value = `Failed to restore backup: ${error.message || 'Unknown error'}`; processing.value = false; selectedBackup.value = undefined; }); } };packages/extension/src/ui/action/views/settings/views/settings-backups/index.vue (2)
19-20: Consider adding aria-labels for better accessibility.The loading states and empty states could benefit from proper aria-labels to improve screen reader support.
- <div class="settings-container__backup"> + <div class="settings-container__backup" role="status" aria-label="Fetching backups"> - <div class="settings-container__backup-header">Fetching backups</div> + <div class="settings-container__backup-header" aria-live="polite">Fetching backups</div> - v-else-if="backups.length === 0" - class="settings-container__backup" + v-else-if="backups.length === 0" + class="settings-container__backup" + role="status" + aria-label="No backups found"Also applies to: 26-27, 36-40
68-74: Add keyboard navigation support for delete button.The delete button should be keyboard accessible and have proper aria-label.
- class="settings-container__backup-status-button" - @click="showDeleteBackup(entity)" + class="settings-container__backup-status-button" + role="button" + tabindex="0" + aria-label="Delete backup" + @click="showDeleteBackup(entity)" + @keyup.enter="showDeleteBackup(entity)"packages/extension/src/ui/action/views/swap/libs/send-transactions.ts (2)
243-243: Remove Unnecessary Labeled Break in Legacy Branch
Static analysis flags the label used in thebreak update_block_hash;statement as unnecessary. Consider replacing it with a plainbreak;for a cleaner control flow.- break update_block_hash; + break;🧰 Tools
🪛 Biome (1.9.4)
[error] 243-243: Unnecessary label.
Safe fix: Remove the unnecessary label.
You can achieve the same result without the label.(lint/complexity/noUselessLabel)
354-354: Remove Unnecessary Labeled Break in Versioned Branch
The break statement using the labelupdate_block_hashis superfluous. Simplify the control flow by removing the label and using a plainbreak;.- break update_block_hash; + break;🧰 Tools
🪛 Biome (1.9.4)
[error] 354-354: Unnecessary label.
Safe fix: Remove the unnecessary label.
You can achieve the same result without the label.(lint/complexity/noUselessLabel)
packages/extension/src/ui/action/views/network-activity/components/network-activity-total.vue (2)
2-13: Enhance error message for better user experience.The error message could be more user-friendly and provide clearer next steps.
Consider this improvement:
- <span>Loading balance error. Please try again later</span> + <span>Unable to load balance. Please check your connection and try refreshing the page.</span>
74-82: Enhance error state visibility.Consider adding more visual cues to make the error state more noticeable.
Consider these style improvements:
&__total-error { padding: 0 20px 12px 20px; h3 { + display: flex; + align-items: center; + gap: 8px; + + &::before { + content: '!'; + width: 20px; + height: 20px; + border-radius: 50%; + background: @error; + color: @white; + display: flex; + align-items: center; + justify-content: center; + font-size: 14px; + } + span { color: @error; } } }packages/extension/src/ui/action/views/network-assets/index.vue (1)
18-21: Consider repositioning the error component.The error component's position in the template could be improved for better user experience.
Consider moving it before the assets list to ensure users see the error message immediately:
- <network-assets-header v-if="!isLoading && assets.length > 0" /> <network-assets-error v-if="isFetchError" :update-assets="updateAssets" /> + <network-assets-header v-if="!isLoading && assets.length > 0" />packages/extension/src/ui/onboard/create-wallet/routes.ts (1)
9-41: Consider enhancing type safety for route paths and names.The routes object is well-structured and covers the complete wallet creation flow. Consider these improvements:
+// Define path constants for type safety +const ROUTES_PATH = { + PICK_PASSWORD: 'pick-password', + TYPE_PASSWORD: 'type-password', + RECOVERY_PHRASE: 'recovery-phrase', + CHECK_PHRASE: 'check-phrase', + USER_ANALYTICS: 'user-analytics', + WALLET_READY: 'wallet-ready', +} as const; + +// Define route names enum for type safety +enum ROUTES_NAME { + PICK_PASSWORD = 'pick-password', + TYPE_PASSWORD = 'type-password', + RECOVERY_PHRASE = 'recovery-phrase', + CHECK_PHRASE = 'check-phrase', + USER_ANALYTICS = 'user-analytics', + WALLET_READY = 'wallet-ready', +} + export const routes = { pickPassword: { - path: 'pick-password', - name: 'pick-password', + path: ROUTES_PATH.PICK_PASSWORD, + name: ROUTES_NAME.PICK_PASSWORD, component: PickPassword, }, // Apply similar changes to other routes... };
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (39)
packages/extension/package.json(3 hunks)packages/extension/src/libs/backup-state/configs.ts(1 hunks)packages/extension/src/libs/backup-state/index.ts(1 hunks)packages/extension/src/libs/backup-state/types.ts(1 hunks)packages/extension/src/libs/keyring/public-keyring.ts(8 hunks)packages/extension/src/libs/utils/initialize-wallet.ts(2 hunks)packages/extension/src/providers/common/ui/send-transaction/send-address-item.vue(1 hunks)packages/extension/src/providers/common/ui/send-transaction/send-contacts-list.vue(4 hunks)packages/extension/src/providers/ethereum/ui/send-transaction/index.vue(1 hunks)packages/extension/src/types/provider.ts(2 hunks)packages/extension/src/ui/action/App.vue(5 hunks)packages/extension/src/ui/action/components/switch/index.vue(1 hunks)packages/extension/src/ui/action/views/accounts/components/add-account-form.vue(2 hunks)packages/extension/src/ui/action/views/network-activity/components/network-activity-total.vue(3 hunks)packages/extension/src/ui/action/views/network-assets/components/network-assets-error.vue(1 hunks)packages/extension/src/ui/action/views/network-assets/index.vue(5 hunks)packages/extension/src/ui/action/views/settings/components/settings-inner-header.vue(2 hunks)packages/extension/src/ui/action/views/settings/components/settings-switch.vue(4 hunks)packages/extension/src/ui/action/views/settings/index.vue(5 hunks)packages/extension/src/ui/action/views/settings/views/settings-backups/backup-identicon.vue(1 hunks)packages/extension/src/ui/action/views/settings/views/settings-backups/index.vue(1 hunks)packages/extension/src/ui/action/views/settings/views/settings-general/index.vue(3 hunks)packages/extension/src/ui/action/views/swap/libs/send-transactions.ts(15 hunks)packages/extension/src/ui/onboard/App.vue(6 hunks)packages/extension/src/ui/onboard/create-wallet/routes.ts(1 hunks)packages/extension/src/ui/onboard/create-wallet/store.ts(1 hunks)packages/extension/src/ui/onboard/create-wallet/wallet-ready.vue(1 hunks)packages/extension/src/ui/onboard/restore-wallet/backup-detected.vue(1 hunks)packages/extension/src/ui/onboard/restore-wallet/routes.ts(2 hunks)packages/extension/src/ui/onboard/restore-wallet/type-password.vue(2 hunks)packages/signers/ethereum/src/index.ts(1 hunks)packages/signers/ethereum/src/utils.ts(0 hunks)packages/swap/src/providers/zerox/index.ts(4 hunks)packages/swap/src/providers/zerox/types.ts(1 hunks)packages/types/src/index.ts(1 hunks)packages/utils/package.json(1 hunks)packages/utils/src/index.ts(2 hunks)packages/utils/src/nacl-encrypt-decrypt.ts(1 hunks)packages/utils/src/random-names.ts(1 hunks)
💤 Files with no reviewable changes (1)
- packages/signers/ethereum/src/utils.ts
✅ Files skipped from review due to trivial changes (4)
- packages/extension/src/libs/backup-state/configs.ts
- packages/extension/src/providers/common/ui/send-transaction/send-address-item.vue
- packages/signers/ethereum/src/index.ts
- packages/extension/src/ui/onboard/create-wallet/store.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- packages/extension/src/ui/onboard/App.vue
- packages/extension/src/ui/action/App.vue
- packages/utils/package.json
- packages/extension/package.json
🧰 Additional context used
🪛 Biome (1.9.4)
packages/extension/src/types/provider.ts
[error] 134-134: This constructor is unnecessary.
Unsafe fix: Remove the unnecessary constructor.
(lint/complexity/noUselessConstructor)
packages/extension/src/libs/backup-state/index.ts
[error] 224-224: Unnecessary continue statement
Unsafe fix: Delete the unnecessary continue statement
(lint/correctness/noUnnecessaryContinue)
packages/extension/src/ui/action/views/swap/libs/send-transactions.ts
[error] 243-243: Unnecessary label.
Safe fix: Remove the unnecessary label.
You can achieve the same result without the label.
(lint/complexity/noUselessLabel)
[error] 354-354: Unnecessary label.
Safe fix: Remove the unnecessary label.
You can achieve the same result without the label.
(lint/complexity/noUselessLabel)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: buildAll
- GitHub Check: test
🔇 Additional comments (38)
packages/utils/src/nacl-encrypt-decrypt.ts (1)
16-19: Validate JSON parsing.
When parsing the return ofhexToBuffer(strData), an invalid or corrupted hex string could raise aSyntaxErrorwith no user-friendly fallback. Consider a try/catch block to return a clear message or fallback.packages/utils/src/index.ts (2)
14-20: Centralized import improvement.
Bringing all NaCl encryption utilities into the main export file helps keep the utility layer cohesive and discoverable. Great addition.
53-58: Check for potential naming collisions.
All newly exported symbols (naclEncrypt,naclDecrypt, etc.) appear unique. However, be mindful as the codebase grows that these cryptographic utility exports might eventually clash with similarly named functions or constants. Consider a naming pattern (e.g., prefixing) if you foresee expansions.packages/utils/src/random-names.ts (1)
102-584: Comprehensive name list.
Therightarray is quite thorough, referencing many historical figures. Just be mindful of potential trademark/copyright concerns with certain names in specialized contexts.packages/swap/src/providers/zerox/index.ts (3)
73-73: Verify the updated API endpoint rigorously.
The change tohttps://partners.mewapi.io/zeroxv2looks fine for a versioned migration, but please confirm all downstream services and integrations are compatible with the new endpoint.
157-157: Confirm renaming the parameter to match API specs.
Ensure the newtakerparameter (changed from the previously usedtakerAddress) matches exactly what the endpoint expects.
168-169: Validate the construction of the query parameters.
Appending the chain ID as a query parameter instead of the path is likely correct given the new API structure. However, please verify that the endpoint consistently expectschainIdin this manner.packages/extension/src/libs/backup-state/index.ts (1)
91-94: Check for missing or invalid signatures.
When!signature, the code logs an error and returns an empty array. Double-check that this behavior is desired or consider throwing an error to inform upstream processes.packages/swap/src/providers/zerox/types.ts (1)
4-10: Good structuring of the transaction fields.
Consolidating related properties intoZeroXResponseTransactionTypeand referencing it viatransactioninZeroXResponseTypeis a cleaner approach. Moving away from flat fields reduces confusion and simplifies usage.Also applies to: 17-17
packages/extension/src/libs/backup-state/types.ts (2)
3-5: LGTM!The enum follows TypeScript naming conventions and uses a descriptive key name.
22-24: LGTM!The interface follows a common pattern for API responses and TypeScript naming conventions.
packages/extension/src/ui/action/views/settings/views/settings-backups/backup-identicon.vue (1)
1-3: LGTM!The template is minimal and follows Vue best practices.
packages/extension/src/ui/onboard/restore-wallet/routes.ts (1)
8-8: LGTM!The new backup detection route follows the established naming conventions and routing patterns.
Also applies to: 41-45
packages/extension/src/ui/action/views/settings/components/settings-switch.vue (1)
2-66: LGTM! The border customization enhances component flexibility.The new
hasBorderprop with conditional styling allows for better component customization while maintaining backward compatibility through the default value.packages/extension/src/ui/action/components/switch/index.vue (1)
3-3: LGTM! Excellent refactor to use Vue's v-model pattern.The changes improve the component by:
- Using Vue's built-in v-model directive for better reactivity
- Simplifying state management with a computed property
- Following Vue 3 best practices for two-way binding
Also applies to: 18-23
packages/extension/src/ui/action/views/settings/components/settings-inner-header.vue (1)
11-11: LGTM! Clean addition of backup settings section.The implementation maintains consistency with existing patterns and properly integrates the new backup settings section.
Also applies to: 47-50
packages/extension/src/ui/onboard/restore-wallet/type-password.vue (1)
30-30: LGTM! Improved reactive state handling and error management.The changes enhance the component by:
- Properly handling reactive references with
unref- Adding error handling for wallet initialization
- Supporting the new backup detection feature
Also applies to: 45-66
packages/extension/src/libs/utils/initialize-wallet.ts (1)
11-25: LGTM! Filtering test wallets enhances account management.The addition of test wallet filtering improves account management by ensuring only production wallets are considered during initialization.
packages/extension/src/ui/action/views/settings/index.vue (1)
42-47: LGTM! Clean integration of backup settings component.The backup settings component is well-integrated with consistent event handling patterns.
packages/types/src/index.ts (1)
72-76: LGTM! Well-typed interface extension.The optional isTestWallet property is appropriately typed and maintains backward compatibility.
packages/extension/src/ui/action/views/settings/views/settings-general/index.vue (2)
52-58: LGTM! Clear and informative backup settings UI.The backup button and description provide clear information about the backup functionality.
76-76: LGTM! Clean event handling implementation.The event emission is well-typed and follows Vue 3 best practices.
Also applies to: 88-90
packages/extension/src/libs/keyring/public-keyring.ts (1)
26-26: LGTM! Test wallet identification added.The
isTestWalletproperty is consistently added to all test wallet accounts, properly scoped within the development environment.Also applies to: 37-37, 48-48, 59-59, 70-70, 84-84, 95-95, 106-106
packages/extension/src/types/provider.ts (1)
56-56: LGTM! Storage namespace added for backup state.The
backupStateenum value is properly added to support the new backup functionality.packages/extension/src/providers/common/ui/send-transaction/send-contacts-list.vue (2)
120-131: Good error handling in address comparison.The
isCheckedmethod properly handles potential errors during address comparison and provides a safe fallback.
42-42: Consistent usage of isChecked method.The
isCheckedmethod is consistently used across all instances where address comparison is needed, improving maintainability.Also applies to: 54-54, 72-72
packages/extension/src/providers/ethereum/ui/send-transaction/index.vue (1)
45-50: Improved UX by closing contact selection after paste.The modification enhances user experience by automatically closing the contact selection interface after pasting an address.
packages/extension/src/ui/action/views/swap/libs/send-transactions.ts (10)
76-78: Type Annotation Update forexecuteSwapSignature
The return type now uses semicolons ({ hash: string; sentAt: number }) instead of commas. This change improves clarity and consistency in your type definitions.
176-177: Consistent Return Value in Substrate Branch
The Substrate branch now returns an object matching the updated type ({ hash: string; sentAt: number }). Ensure that any consuming code reflects this updated structure.
181-181: Updated Type Annotation forsolTxs
The declaration ofsolTxsnow uses semicolon separators in its type definition, which brings it in line with the overall TypeScript style used elsewhere in the codebase.
220-223: Consistent Error Message Formatting in Legacy Branch
The warning log for fee retrieval now uses a consistent template literal format. This small refinement improves readability in error reporting.
332-334: Consistent Error Message Formatting in Versioned Branch
The error message for checking the fee in the versioned transaction branch now follows a consistent format. This enhances clarity when logging issues.
341-343: Enhanced Warning Log for Outdated Block Hash in Versioned Branch
The warning log notifying that the unsigned versioned transaction may have an out-of-date block hash has been updated for clarity and consistency.
369-372: Improved Logging for Blockhash Retrieval Attempts in Versioned Branch
The warning message for a failed attempt to fetch the latest blockhash now clearly includes the attempt number, which makes debugging easier.
507-507: Updated Type Annotation fortxPromises
The variabletxPromisesis now defined with the type{ hash:0x${string}; sentAt: number }[], using semicolons for separation. This aligns the annotation with the overall style used in the code.
569-569: Consistent Structure for EVM Transaction Promises
When pushing totxPromises, the transaction object now adheres to the updated type structure. This ensures consistency across transaction types and improves type safety.
478-478: Consistent Appending tosolTxsArray
The object{ hash: txHash, sentAt: Date.now() }pushed intosolTxsproperly conforms to the updated type definition, maintaining uniformity in the Solana branch.packages/extension/src/ui/onboard/create-wallet/routes.ts (1)
1-8: LGTM!The imports are well-organized, with local components grouped together followed by external dependencies. The empty line provides good visual separation.
packages/extension/src/ui/action/views/settings/views/settings-backups/backup-identicon.vue
Outdated
Show resolved
Hide resolved
packages/extension/src/ui/action/views/settings/views/settings-backups/index.vue
Show resolved
Hide resolved
packages/extension/src/ui/action/views/settings/views/settings-backups/index.vue
Show resolved
Hide resolved
packages/extension/src/ui/action/views/network-activity/components/network-activity-total.vue
Show resolved
Hide resolved
devop: token 2022 support
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
packages/extension/src/providers/ethereum/libs/assets-handlers/solanachain.ts (1)
20-31: Consider adding error handling for token account fetching.While the concurrent fetching is efficient, there's no explicit error handling if one of the requests fails.
- ]).then(([tokenAccounts, tokenAccounts2022]) => { + ]).then(([tokenAccounts, tokenAccounts2022]) => { + if (!tokenAccounts || !tokenAccounts2022) { + throw new Error('Failed to fetch token accounts'); + }packages/swap/src/providers/jupiter/index.ts (1)
351-351: Consider adding a comment explaining TOKEN_2022 referral fee exclusion.While the logic is correct, it would be helpful to document why TOKEN_2022 tokens are excluded from referral fee handling.
- } else if (!referrerATAExists && !isSrcToken2022) { + } else if (!referrerATAExists && !isSrcToken2022) { + // Skip referral fee handling for TOKEN_2022 tokens as they use a different fee mechanismpackages/extension/src/providers/solana/ui/send-transaction/index.vue (2)
529-540: Consider caching the program ID.The program ID is used multiple times in the function. Consider storing it in a variable to avoid potential inconsistencies and improve readability.
+ const tokenProgramId = programId!.owner; const associatedTokenTo = getAssociatedTokenAddressSync( contract, to, undefined, - programId!.owner, + tokenProgramId, ); const associatedTokenFrom = getAssociatedTokenAddressSync( contract, from, undefined, - programId!.owner, + tokenProgramId, );
550-559: Consider extracting token transfer logic.The token transfer logic is duplicated between the cases where the associated token account exists and when it needs to be created. Consider extracting this into a helper function to reduce code duplication.
+ const createTransferInst = (from: PublicKey, to: PublicKey, owner: PublicKey) => + createTransferInstruction( + from, + to, + owner, + toBN(TxInfo.value.value).toNumber(), + [], + tokenProgramId, + ); if (validATA) { - transaction.add( - createTransferInstruction( - associatedTokenFrom, - associatedTokenTo, - from, - toBN(TxInfo.value.value).toNumber(), - [], - programId!.owner, - ), - ); + transaction.add(createTransferInst(associatedTokenFrom, associatedTokenTo, from)); } else { transaction.add( createAssociatedTokenAccountInstruction( from, associatedTokenTo, to, contract, - programId!.owner, + tokenProgramId, ), ); - transaction.add( - createTransferInstruction( - associatedTokenFrom, - associatedTokenTo, - from, - toBN(TxInfo.value.value).toNumber(), - [], - programId!.owner, - ), - ); + transaction.add(createTransferInst(associatedTokenFrom, associatedTokenTo, from));Also applies to: 561-579
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
packages/extension/src/providers/ethereum/libs/assets-handlers/solanachain.ts(1 hunks)packages/extension/src/providers/solana/ui/libs/decode-tx.ts(2 hunks)packages/extension/src/providers/solana/ui/send-transaction/index.vue(4 hunks)packages/extension/src/providers/solana/ui/sol-verify-transaction.vue(1 hunks)packages/swap/src/providers/jupiter/index.ts(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: buildAll
- GitHub Check: test
🔇 Additional comments (7)
packages/extension/src/providers/ethereum/libs/assets-handlers/solanachain.ts (2)
5-6: LGTM! Good addition of TOKEN_2022_PROGRAM_ID support.The imports are correctly structured to support the new token program functionality.
12-19: Efficient concurrent fetching of token accounts.The use of Promise.all for concurrent fetching from both TOKEN_PROGRAM_ID and TOKEN_2022_PROGRAM_ID is a good optimization.
packages/extension/src/providers/solana/ui/libs/decode-tx.ts (1)
81-84: Good enhancement of account filtering logic.The changes correctly:
- Include TOKEN_2022_PROGRAM_ID in account filtering
- Use >= for data length check, which is more flexible and future-proof
packages/extension/src/providers/solana/ui/sol-verify-transaction.vue (1)
282-293: Improved blockhash handling with safety checks.The changes add proper checks to prevent overwriting existing blockhash values for both legacy and versioned transactions.
packages/swap/src/providers/jupiter/index.ts (2)
321-324: Good addition of TOKEN_2022 program detection.The code correctly identifies tokens from the TOKEN_2022 program.
330-330: Proper handling of referral fees for TOKEN_2022 tokens.Setting referrerATAPubkey to undefined for TOKEN_2022 tokens is the correct approach to skip referral fee handling.
packages/extension/src/providers/solana/ui/send-transaction/index.vue (1)
541-546:❓ Verification inconclusive
Verify the account existence check.
The
getAccountcall is used to check if the associated token account exists, but it might throw errors for reasons other than non-existence. Consider using a more specific error check.
🏁 Script executed:
#!/bin/bash # Description: Check if there are any specific error types we should handle in getAccount # Search for getAccount usage patterns in the codebase rg -A 5 "getAccount.*catch"Length of output: 29
Action Required: Refine Error Handling for
getAccountIn the file
packages/extension/src/providers/solana/ui/send-transaction/index.vue(lines 541–546), the call togetAccountis used to verify the existence of the associated token account. However, without a surrounding try/catch or a mechanism to filter specific error cases, any error thrown bygetAccount—whether due to a missing account or another issue (e.g., network issues, misconfiguration)—could lead to ambiguous behavior. Please verify thatgetAccountdistinguishes between expected “account not found” errors and other failures. Depending on the findings, consider adding a try/catch block with refined error checks to ensure that only the intended error case (i.e., a missing account) triggers the specific handling logic.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
packages/extension/src/libs/backup-state/index.ts (3)
77-107: Consider robust error handling for network failures.
ThelistBackupsmethod silently returns an empty array if a signature is unavailable or if the remote fetch fails. You might want to handle HTTP errors more explicitly (e.g., ifrawResponse.okis false).
223-225: Remove the unnecessarycontinuestatement.
Because this is anif/else if/else ifchain, usingcontinue;after renaming the account is redundant—control won't fall into the subsequent branches.if (existingAccount && newAccount) { await kr.renameAccount(existingAccount.address, newAccount.name); - continue; }🧰 Tools
🪛 Biome (1.9.4)
[error] 225-225: Unnecessary continue statement
Unsafe fix: Delete the unnecessary continue statement
(lint/correctness/noUnnecessaryContinue)
247-318: Improve error handling in thebackupmethod.
If the fetch request fails or receives an unexpected status code, the code only logs an error. Consider adding retry logic or explicit handling (e.g., user-facing notifications). This will make the backup process more robust.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/extension/src/libs/backup-state/index.ts(1 hunks)packages/extension/src/libs/backup-state/types.ts(1 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
packages/extension/src/libs/backup-state/index.ts
[error] 225-225: Unnecessary continue statement
Unsafe fix: Delete the unnecessary continue statement
(lint/correctness/noUnnecessaryContinue)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: buildAll
- GitHub Check: test
🔇 Additional comments (1)
packages/extension/src/libs/backup-state/types.ts (1)
1-35: All good here!
These type definitions cleanly express backup-related data structures. Great job defining clear interfaces and types for state management.
Summary by CodeRabbit
Style
New Features