Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. 🗂️ Base branches to auto review (3)
Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThe changes introduce payment ID support and enhanced multi-chain resolution in the name resolution system. The Changes
Sequence Diagram(s)sequenceDiagram
participant UI
participant GenericNameResolver
participant NameResolver
participant SIDResolver
participant Web3NameSDK
UI->>GenericNameResolver: resolveName(name, coins, providerChain)
GenericNameResolver->>NameResolver: resolveAddress(name, coin, providerChain)
alt Name is Payment ID
NameResolver->>SIDResolver: resolveAddress(name, providerChain)
SIDResolver->>Web3NameSDK: handlePaymentIdGetAddress(name, providerChain)
Web3NameSDK-->>SIDResolver: address or null
else Name is EVM/Solana TLD
NameResolver->>SIDResolver: resolveAddress(name, providerChain)
SIDResolver->>Web3NameSDK: resolve via web3NameResolver/solanaNameResolver
Web3NameSDK-->>SIDResolver: address or null
end
SIDResolver-->>NameResolver: address or null
NameResolver-->>GenericNameResolver: address or null
GenericNameResolver-->>UI: address or null
Possibly related PRs
Suggested reviewers
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
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: 10
🔭 Outside diff range comments (1)
packages/hw-wallets/src/trezor/bitcoin/index.ts (1)
46-46: 🛠️ Refactor suggestionImprove error handling consistency.
Similar to the Ethereum implementation, the error handling uses
(result.payload as any).errorcasting. Consider applying the same type-safe error handling approach suggested for the Ethereum file.Also applies to: 84-84, 113-113
🧹 Nitpick comments (4)
packages/extension/src/providers/common/ui/send-transaction/send-contacts-list.vue (1)
128-130: Good improvement to error logging specificity.The more descriptive error message helps with debugging and aligns well with the name resolution enhancements in this PR. The message clearly indicates the likely cause of the error.
Consider adding structured logging to preserve error details while maintaining the improved message:
} catch (err) { - console.error( - 'Error checking if address is checked, probably name resolver', - ); + console.error( + 'Error checking if address is checked, probably name resolver:', + err instanceof Error ? err.message : String(err) + ); return false; }packages/extension/src/providers/common/ui/styles/verify-transaction.less (1)
177-181: Enhanced readability with improved typography.The styling improvements for the "To" account info section enhance readability with larger font size (16px) and better contrast. However, the extensive use of
!importantdeclarations suggests there may be specificity issues in the CSS cascade.Consider reviewing the CSS architecture to reduce reliance on
!importantdeclarations by increasing selector specificity or restructuring the stylesheets for better maintainability.packages/hw-wallets/src/trezor/trezorConnect.ts (1)
4-4: Use optional chaining for cleaner Chrome detection.The static analysis tool correctly identifies that the Chrome detection logic can be simplified using optional chaining.
Apply this diff to improve readability:
- if (chrome && chrome.runtime && chrome.runtime.getPlatformInfo) { + if (chrome?.runtime?.getPlatformInfo) {🧰 Tools
🪛 Biome (1.9.4)
[error] 4-4: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
packages/name-resolution/src/sid/types.ts (1)
6-6: Add missing semicolon.- timeout?: number + timeout?: number;
📜 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 (46)
package.json(1 hunks)packages/extension-bridge/package.json(2 hunks)packages/extension/package.json(6 hunks)packages/extension/src/libs/name-resolver/index.ts(1 hunks)packages/extension/src/providers/common/libs/new-features.ts(1 hunks)packages/extension/src/providers/common/ui/send-transaction/send-contacts-list.vue(1 hunks)packages/extension/src/providers/common/ui/styles/verify-transaction.less(2 hunks)packages/extension/src/providers/ethereum/libs/activity-handlers/providers/etherscan/configs.ts(1 hunks)packages/extension/src/providers/ethereum/libs/transaction/gas-utils.ts(2 hunks)packages/extension/src/providers/ethereum/libs/transaction/index.ts(2 hunks)packages/extension/src/providers/ethereum/networks/hemi.ts(1 hunks)packages/extension/src/providers/ethereum/networks/index.ts(2 hunks)packages/extension/src/providers/ethereum/ui/eth-verify-transaction.vue(5 hunks)packages/extension/src/providers/ethereum/ui/send-transaction/components/send-address-input.vue(1 hunks)packages/extension/src/providers/ethereum/ui/send-transaction/index.vue(1 hunks)packages/extension/src/providers/solana/ui/send-transaction/components/send-address-input.vue(1 hunks)packages/extension/src/providers/solana/ui/send-transaction/index.vue(1 hunks)packages/extension/src/ui/action/views/swap/components/send-address-input.vue(1 hunks)packages/extension/src/ui/action/views/swap/index.vue(2 hunks)packages/hw-wallets/package.json(4 hunks)packages/hw-wallets/src/trezor/bitcoin/index.ts(6 hunks)packages/hw-wallets/src/trezor/ethereum/index.ts(7 hunks)packages/hw-wallets/src/trezor/solana/index.ts(4 hunks)packages/hw-wallets/src/trezor/trezorConnect.ts(1 hunks)packages/keyring/package.json(2 hunks)packages/name-resolution/package.json(3 hunks)packages/name-resolution/src/index.ts(1 hunks)packages/name-resolution/src/sid/index.ts(1 hunks)packages/name-resolution/src/sid/types.ts(1 hunks)packages/name-resolution/src/sid/utils.ts(1 hunks)packages/name-resolution/src/types.ts(2 hunks)packages/name-resolution/src/ud/index.ts(2 hunks)packages/name-resolution/tests/ens.test.ts(3 hunks)packages/name-resolution/tests/sid.test.ts(4 hunks)packages/name-resolution/tests/ud.test.ts(2 hunks)packages/name-resolution/tsconfig.json(1 hunks)packages/request/package.json(2 hunks)packages/signers/bitcoin/package.json(2 hunks)packages/signers/ethereum/package.json(2 hunks)packages/signers/kadena/package.json(2 hunks)packages/signers/polkadot/package.json(2 hunks)packages/storage/package.json(2 hunks)packages/swap/package.json(2 hunks)packages/types/package.json(2 hunks)packages/types/src/networks.ts(1 hunks)packages/utils/package.json(2 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (5)
packages/extension/src/libs/name-resolver/index.ts (1)
packages/name-resolution/src/types.ts (1)
CoinType(4-140)
packages/extension/src/providers/ethereum/libs/transaction/index.ts (2)
packages/utils/src/index.ts (1)
numberToHex(46-46)packages/extension/src/providers/ethereum/libs/transaction/gas-utils.ts (2)
collectiveGasConfig(247-247)safeGasForStaking(246-246)
packages/extension/src/providers/ethereum/networks/hemi.ts (2)
packages/extension/src/providers/ethereum/types/evm-network.ts (2)
EvmNetworkOptions(25-56)EvmNetwork(58-293)packages/extension/src/providers/ethereum/libs/activity-handlers/index.ts (1)
EtherscanActivity(10-10)
packages/name-resolution/src/sid/index.ts (2)
packages/name-resolution/src/sid/types.ts (3)
SIDOptions(1-7)TIMEOUT_PRESETS(34-38)PAYMENT_ID_CHAINS_MAP(18-25)packages/name-resolution/src/sid/utils.ts (1)
isValidPaymentId(2-2)
packages/hw-wallets/src/trezor/solana/index.ts (3)
packages/hw-wallets/src/types.ts (3)
getAddressRequest(74-76)AddressResponse(31-34)SolSignTransaction(61-63)packages/hw-wallets/src/trezor/solana/configs.ts (1)
supportedPaths(8-8)packages/utils/src/index.ts (1)
bufferToHex(38-38)
🪛 Biome (1.9.4)
packages/hw-wallets/src/trezor/trezorConnect.ts
[error] 4-4: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
packages/name-resolution/src/sid/index.ts
[error] 102-104: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Unsafe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 107-109: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Unsafe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: buildAll
🔇 Additional comments (75)
package.json (1)
37-37:Details
✅ Verification successful
Verify the @swc/core version update for compatibility and security.
The @swc/core dependency has been updated from ^1.11.24 to ^1.11.29. Please ensure this version is valid and check for any security advisories or breaking changes.
🏁 Script executed:
#!/bin/bash # Description: Verify @swc/core version exists and check for security advisories # Check if the version exists on npm npm view @swc/core@1.11.29 version # Check for security advisories npm audit --audit-level moderate --package-lock-only 2>/dev/null || echo "No package-lock.json found, skipping audit" # Get recent versions to verify this is a valid progression npm view @swc/core versions --json | jq '.[-10:]'Length of output: 493
Verified @swc/core update
- Version 1.11.29 exists on npm and follows the valid progression from 1.11.24.
- No security advisories were reported (audit skipped due to missing lock file).
No further changes needed. Consider adding a lock file and running
npm audit(oryarn audit/pnpm audit) in CI to catch any dev‐dependency issues.packages/types/src/networks.ts (1)
114-114:Details
❓ Verification inconclusive
Good addition of Hemi network, but consider CoingeckoPlatform integration.
The addition of
Hemi = "Hemi"to the NetworkNames enum looks correct and follows the established naming convention.However, I notice that Hemi is not added to the
CoingeckoPlatformenum below. This might be intentional if Hemi is not yet supported by CoinGecko, but please verify if this is the intended behavior.
🏁 Script executed:
#!/bin/bash # Description: Check if Hemi network configuration exists in the codebase # Look for Hemi network configuration files fd -t f -e ts -e js | xargs rg -l "Hemi" | head -10 # Check for any existing CoingeckoPlatform references to Hemi rg "Hemi" --type ts -A 3 -B 3Length of output: 3007
Great addition of Hemi network – please confirm CoingeckoPlatform mapping
The new entry
Hemi = "Hemi",in
packages/types/src/networks.tsis correctly named and placed. References to Hemi have been added elsewhere:
packages/extension/src/providers/ethereum/networks/hemi.tspackages/extension/src/providers/ethereum/libs/activity-handlers/providers/etherscan/configs.tspackages/extension/src/providers/common/libs/new-features.tsHowever, the
CoingeckoPlatformenum inpackages/types/src/networks.tsstill lacks a Hemi entry. If you intend to fetch Hemi prices from CoinGecko, add something like:export enum CoingeckoPlatform { Ethereum = "ethereum", Bsc = "bsc", Polygon = "polygon-pos", Avalanche = "avalanche", Fantom = "fantom", Arbitrum = "arbitrum-one", Optimism = "optimistic-ethereum", + Hemi = "hemi", }Please verify whether CoinGecko supports Hemi—or, if this omission is intentional, document the limitation or handle missing mapping gracefully.
packages/types/package.json (1)
27-41:Details
❓ Verification inconclusive
Development dependency updates look good, but verify eslint v9 compatibility.
The coordinated update of development dependencies across the monorepo is well-executed. All version increments appear reasonable.
However, the eslint update from v8 to v9 is a major version change that could introduce breaking changes. Please ensure the configuration is compatible:
🏁 Script executed:
#!/bin/bash # Description: Check for eslint configuration and potential compatibility issues # Find eslint configuration files fd -t f "(\.eslintrc|eslint\.config)" | head -5 # Check if there are any eslint-related errors in the build rg "eslint" package.json -A 2 -B 2 # Look for any lint scripts that might be affected rg "lint" --type json -A 2 -B 2Length of output: 40134
ESLint v9 upgrade: limited workspace impact
Most packages use Prettier-only lint scripts—only two workspaces invoke ESLint directly:
• packages/name-resolution (scripts.lint:
eslint . --ext .js,.jsx,.ts,.tsx --fix)
• packages/extension (scripts.lint:eslint . --fix && yarn format)Please run these lint commands and resolve any new errors:
yarn workspace name-resolution lint yarn workspace extension lintIf both pass cleanly, the upgrade to ESLint v9 is safe to merge.
packages/request/package.json (1)
34-49:Details
❓ Verification inconclusive
Excellent coordination of dependency updates across the monorepo.
The development dependency updates are consistent with those in the types package, demonstrating good maintenance practices across the monorepo. The additional vitest update to ^3.1.4 is appropriate for this package's testing needs.
Same concern as mentioned for the types package: the eslint v9 update requires verification for compatibility. Please ensure the lint scripts and configurations work correctly with the new major version:
🏁 Script executed:
#!/bin/bash # Description: Test lint script functionality # Check if lint script runs without errors in this package cd packages/request && npm run lint --dry-run 2>&1 | head -20 || echo "Lint script check completed" # Verify the script definition cat packages/request/package.json | jq '.scripts.lint'Length of output: 549
🏁 Script executed:
#!/bin/bash # Retrieve all npm scripts defined in the request package cd packages/request && jq '.scripts' package.jsonLength of output: 170
Excellent coordination of dependency updates across the monorepo.
- The devDependencies in
packages/request/package.jsonmirror those in the types package, including the new vitest version for testing.- The
lintscript here is defined asprettier --write .and does not invoke ESLint.Please confirm whether ESLint is run elsewhere (e.g., via CI or git hooks) or if you’d like to add an
eslintscript in this package. If ESLint is intended, ensure your configurations and scripts fully support the upgrade to ESLint v9.27.0. Otherwise, the prettier-only lint script remains unaffected.packages/swap/package.json (1)
42-44: Dev dependency versions bumped in@enkryptcom/swap
The updates to@types/node,@typescript-eslint/*,eslint,tsup, andvitestalign with the monorepo-wide maintenance. No runtime dependencies changed, and the build/test scripts remain intact.
Please ensure that the lockfile (e.g.,pnpm-lock.yaml/yarn.lock) is updated accordingly and that the CI pipeline passes with these new versions.Also applies to: 54-54, 56-57
packages/signers/ethereum/package.json (1)
35-38: Sync dev tooling versions for@enkryptcom/signer-ethereum
The bumps to@types/node,@typescript-eslint/*,eslint,tsup, andvitestmirror changes across other packages. No functional impact is expected.
Verify that all lint and test runs succeed under these updated versions.Also applies to: 47-47, 49-50
packages/storage/package.json (1)
30-33: Update devDependencies for@enkryptcom/storage
Consistent version upgrades for TypeScript and lint/test tooling have been applied. The production code is unaffected.
Ensure that local development and CI pipelines still pass without errors.Also applies to: 42-42, 44-45
packages/signers/bitcoin/package.json (1)
35-38: Bump devDependencies in@enkryptcom/signer-bitcoin
The versions of core dev tools (@types/node, ESLint,tsup,vitest, etc.) have been aligned with the rest of the monorepo. No changes to runtime behavior.
Please run the full test suite to confirm compatibility.Also applies to: 47-47, 49-50
packages/signers/polkadot/package.json (2)
27-28: Upgrade Polkadot utilities
@polkadot/utiland@polkadot/util-cryptohave been bumped from^13.4.4to^13.5.1. Review the changelog for any breaking or deprecated APIs to confirm this is a safe patch-level update.
34-37: Align devDependencies with monorepo versions
The development tooling versions (@types/node, ESLint,tsup,vitest, etc.) now match other signer packages. No production code changes here.
Verify that formatting, linting, and tests run cleanly under these new versions.Also applies to: 46-46, 48-49
packages/utils/package.json (4)
27-27: Upgrade@polkadot/util-cryptoto ^13.5.1
This bump aligns with the monorepo-wide update and pulls in the latest fixes.
33-36: Bump ESLint and TypeScript ESLint plugins to latest patch
Updating@types/node,@typescript-eslint/eslint-plugin,@typescript-eslint/parser, andeslintto the newest patch versions ensures consistent linting behavior across packages.
45-45: Updatetsupbuild tool to ^8.5.0
Matches other packages’ tsup version for uniform build tooling.
48-49: Bumptypescript-eslintmeta‐package andvitesttest runner
Keeping the test runner and tooling metadata in sync with the rest of the monorepo.packages/keyring/package.json (4)
32-32: Upgrade@polkadot/utilto ^13.5.1
Consistent with the latest Polkadot runtime utilities across the codebase.
37-40: Refresh linting dependencies
@types/node,@typescript-eslint/eslint-plugin,@typescript-eslint/parser, andeslintare now at their current patch versions, aligning with monorepo standards.
49-49: Bumptsupto ^8.5.0
Ensures the same build output behavior across packages.
51-52: Updatetypescript-eslintmeta‐package andvitest
Maintains version consistency for testing and TS tooling.packages/extension-bridge/package.json (4)
47-47: Update@types/nodeto ^22.15.24
Keeps Node type definitions up to date across all packages.
49-52: Refresh ESLint, parser, and bumpbumpp
@typescript-eslint/eslint-plugin,@typescript-eslint/parser,bumpp, andeslintare now at their latest patch versions for consistent tooling.
61-61: Upgradetsupto ^8.5.0
Aligns the build tool version across the monorepo.
64-64: Bumptypescript-eslintmeta‐package
Maintains the meta‐tooling version in sync with other packages.packages/extension/package.json (7)
3-3: Increment package version to 2.8.0
Reflects the new release containing feature additions and dependency updates.
26-26: Upgrade key runtime dependencies
Bumped@amplitude/analytics-browser,@ledgerhq/hw-transport-webusb, and@metaplex-foundation/mpl-bubblegumto their latest patch versions.Also applies to: 41-43
46-53: Update Polkadot ecosystem packages
All@polkadot/*dependencies are now at ^15.x / ^13.x, ensuring compatibility with the latest Polkadot releases.
81-81: Bump Vue to ^3.5.16
Keeps the Vue framework version up to date with the latest improvements and bug fixes.
102-108: Refresh TypeScript definition packages
Updated@types/chrome,@types/lodash, and@types/nodeto their newest versions for accurate typings.
119-121: Bump core devDependencies (@vueuse/core,eslint)
Ensures development tooling consistency and the latest linting rules.
127-134: Update build and testing tools
npm-run-all2,rollup-plugin-visualizer,systeminformation,tsup, andvitesthave all been upgraded to align with other packages in the monorepo.Also applies to: 140-140
packages/signers/kadena/package.json (4)
31-31: Upgrade@polkadot/util-cryptoto ^13.5.1
Keeps cryptographic utilities in line with other packages.
33-36: Refresh linting and TypeScript plugins
@types/node,@typescript-eslint/eslint-plugin,@typescript-eslint/parser, andeslintare now at the latest patch versions for consistent development experience.
45-45: Updatetsupto ^8.5.0
Aligns bundling behavior with the rest of the monorepo.
47-48: Bumptypescript-eslintmeta‐package andvitest
Ensures testing and tooling metadata are consistent across packages.packages/extension/src/providers/ethereum/libs/activity-handlers/providers/etherscan/configs.ts (1)
77-77:Details
✅ Verification successful
LGTM! Verify the endpoint URL is accessible.
The addition follows the established pattern and correctly maps the Hemi network to its explorer endpoint.
Please verify that the Hemi explorer endpoint is accessible and functional:
🏁 Script executed:
#!/bin/bash # Description: Verify the Hemi explorer endpoint is accessible # Expected: HTTP 200 response indicating the endpoint is live curl -I https://explorer.hemi.xyz/Length of output: 1025
🏁 Script executed:
#!/bin/bash # Verify that NetworkNames.Hemi exists in the enum definition rg "enum NetworkNames" -n rg "Hemi" -nLength of output: 594
Approve Hemi network endpoint mapping—verified
The addition of
[NetworkNames.Hemi]: 'https://explorer.hemi.xyz/',correctly follows the existing pattern.
•NetworkNames.Hemiis defined inpackages/types/src/networks.ts
• The explorer URL returns HTTP 200No further changes required.
packages/name-resolution/tests/ens.test.ts (2)
1-1: Good addition of missing semicolon.Adding the semicolon to the import statement improves code consistency and follows JavaScript/TypeScript best practices.
23-24: Good addition of trailing commas.Adding trailing commas to function parameters improves git diff readability and makes it easier to add/remove parameters in the future.
Also applies to: 33-35
packages/extension/src/providers/ethereum/ui/send-transaction/components/send-address-input.vue (1)
74-74: Excellent UX improvement for short address display.This change prevents unnecessary ellipsis truncation for short addresses, improving readability when the full address can be comfortably displayed. The 13-character threshold is reasonable and aligns with similar improvements in other UI components.
packages/name-resolution/tests/ud.test.ts (2)
1-1: Good addition of missing semicolon.Adding the semicolon to the import statement maintains consistency with the ENS test file and follows JavaScript/TypeScript best practices.
17-18: Good addition of trailing commas.Adding trailing commas to function parameters improves code maintainability and consistency across the test suite.
Also applies to: 22-23
packages/name-resolution/tsconfig.json (1)
5-5:Details
✅ Verification successful
Verify compatibility impact of module system modernization.
The changes from
"node"to"bundler"module resolution and"commonjs"to"ES2015"modules are significant modernization steps that improve tree-shaking and bundler compatibility. However, these changes could potentially break compatibility with Node.js environments expecting CommonJS output.Please verify that all consumers of this package can handle ES2015 modules:
Also applies to: 11-11
🏁 Script executed:
#!/bin/bash # Description: Check if the name-resolution package is consumed in Node.js contexts that might require CommonJS # Expected: Find usage patterns and determine if they support ES modules echo "=== Checking package.json files that depend on @enkryptcom/name-resolution ===" fd package.json --exec jq -r 'select(.dependencies["@enkryptcom/name-resolution"] or .devDependencies["@enkryptcom/name-resolution"]) | .name' echo -e "\n=== Checking import/require statements for name-resolution ===" rg -A 2 -B 2 "from.*name-resolution|require.*name-resolution" echo -e "\n=== Checking if other packages use similar module config ===" fd tsconfig.json --exec grep -l "bundler\|ES2015"Length of output: 1032
Compatibility of ES2015 & “bundler” Resolution Verified
No Node.js/CommonJS consumers of @enkryptcom/name-resolution were found—only the browser‐extension package imports it, and it’s built with a bundler that fully supports ES modules. These TS config updates can be merged as-is.
packages/extension/src/providers/ethereum/networks/index.ts (2)
79-79: LGTM! Import follows established pattern.The import statement is consistent with other network imports in the file.
169-169: LGTM! Export properly adds Hemi network support.The export follows the established pattern and is appropriately placed at the end of the networks object. This change aligns with the coordinated effort to add Hemi network support across the extension.
packages/extension/src/providers/common/libs/new-features.ts (1)
3-3: LGTM! Appropriate update to feature the new Hemi network.Updating the
newNetworksarray to highlight Hemi makes sense as part of introducing this new network. Removing previously featured networks is also appropriate as they're no longer "new".packages/extension/src/ui/action/views/swap/components/send-address-input.vue (1)
83-83: LGTM! Improved address display logic.The addition of the length check prevents unnecessary ellipsis truncation for short addresses, which improves the user experience by showing the full address when it's already concise enough to display completely.
packages/extension/src/providers/solana/ui/send-transaction/index.vue (1)
707-711: LGTM! Properly updated for enhanced name resolution API.The addition of
props.network.provider as stringas the third parameter correctly integrates with the enhanced name resolution system that now accepts provider chain information for more precise address resolution.packages/name-resolution/tests/sid.test.ts (1)
14-18: LGTM! Tests properly updated for enhanced API.The test calls to
resolveAddresshave been correctly updated to include the additional parameters ("ETH" and "ethereum") that match the enhanced method signature, ensuring the tests remain valid and comprehensive.Also applies to: 48-52
packages/extension/src/providers/solana/ui/send-transaction/components/send-address-input.vue (1)
71-75: Excellent UX improvement for address display!The enhanced logic prevents unnecessary ellipsis truncation for short addresses (< 13 characters) and shows the full address when focused. This provides a better user experience by ensuring readability without compromising the interface for longer addresses.
packages/name-resolution/src/types.ts (1)
2-2: LGTM!The SIDOptions import is correctly added to support the enhanced SID resolution functionality.
packages/extension/src/providers/ethereum/ui/send-transaction/index.vue (1)
681-685:Details
✅ Verification successful
Verify the type safety of provider casting.
The addition of
props.network.provider as stringas the third parameter correctly provides network context for enhanced name resolution. However, the type casting suggests thatprovidermight not always be a string type.Please verify that
props.network.provideris always a string or add proper type checking:
🏁 Script executed:
#!/bin/bash # Description: Verify the type of network.provider in EvmNetwork interface # Search for EvmNetwork interface definition ast-grep --pattern 'interface EvmNetwork { $$$ provider: $_ $$$ }' # Search for network provider usage patterns rg -A 3 -B 3 "\.provider"Length of output: 66038
🏁 Script executed:
#!/bin/bash # Search for defineProps usage in Ethereum send-transaction component rg "defineProps" -n packages/extension/src/providers/ethereum/ui/send-transaction/index.vueLength of output: 123
🏁 Script executed:
#!/bin/bash # Display defineProps context to inspect the type of props.network.provider rg "const props = defineProps" -n -A20 -B5 packages/extension/src/providers/ethereum/ui/send-transaction/index.vueLength of output: 848
🏁 Script executed:
#!/bin/bash # Locate EvmNetwork references and its definition across the repo rg "EvmNetwork" -nLength of output: 46375
🏁 Script executed:
#!/bin/bash # Display BaseNetwork definition to verify provider type sed -n '1,200p' packages/extension/src/types/base-network.tsLength of output: 4216
🏁 Script executed:
#!/bin/bash # Locate ProviderName definition in provider types rg "export.*ProviderName" -n packages/extension/src/types/provider.ts # Show its definition context sed -n '1,200p' packages/extension/src/types/provider.tsLength of output: 5849
ProviderName is already a string—remove the redundant cast
props.network.provideris typed asProviderName(a string-valued enum), so it’s always a string literal. You can safely drop theas string:.resolveName( text, [props.network.name as CoinType, 'ETH'], - props.network.provider as string, + props.network.provider, )packages/extension/src/providers/common/ui/styles/verify-transaction.less (1)
10-16: Good addition of consistent label styling.The new
.provider-verify-transaction__labelstyle provides consistent formatting for UI labels with appropriate typography and spacing.packages/extension/src/providers/ethereum/libs/transaction/index.ts (3)
17-18: LGTM! Clean import of gas utilities.The imports for
safeGasForStakingandcollectiveGasConfigare properly added to support RSK staking transaction handling.
21-22: LGTM! RSK network imports for chain ID validation.The RSK and RSK Testnet network imports are correctly added to access their chain IDs for conditional logic.
112-118:Details
✅ Verification successful
Verify RSK staking gas estimation logic and error handling.
The implementation adds special gas handling for RSK staking transactions. However, there are several concerns:
- Missing error handling: The
safeGasForStakingcall lacks error handling and could throw exceptions- Potential runtime error: Accessing
collectiveGasConfig[this.tx.chainId].methodIdcould fail if the chainId is not in the config- Gas limit conversion: Converting between hex and number multiple times may introduce precision issues
Consider this safer implementation:
- let gasLimit = + let gasLimit = this.tx.gasLimit || (numberToHex(await this.estimateGas()) as `0x${string}`); - if ([rsk.chainID, rskTestnet.chainID].includes(this.tx.chainId) && this.tx.data && - this.tx.data.startsWith(collectiveGasConfig[this.tx.chainId].methodId)) { - gasLimit = (numberToHex(await safeGasForStaking(this.tx.chainId, Number(gasLimit))) as `0x${string}`) - } + if ([rsk.chainID, rskTestnet.chainID].includes(this.tx.chainId) && this.tx.data) { + const config = collectiveGasConfig[this.tx.chainId]; + if (config && this.tx.data.startsWith(config.methodId)) { + try { + const adjustedGas = await safeGasForStaking(this.tx.chainId, parseInt(gasLimit, 16)); + gasLimit = numberToHex(adjustedGas) as `0x${string}`; + } catch (error) { + console.warn('Failed to adjust gas for RSK staking transaction:', error); + // Fall back to original gas limit + } + } + }
🏁 Script executed:
#!/bin/bash # Description: Verify the collectiveGasConfig structure and safeGasForStaking implementation # Expected: Find the config structure and error handling patterns # Search for collectiveGasConfig definition and usage rg -A 10 "collectiveGasConfig" --type ts # Search for safeGasForStaking implementation ast-grep --pattern $'export async function safeGasForStaking($_, $_) { $$$ }' # Look for error handling patterns in gas utilities rg -A 5 -B 5 "try|catch.*gas" packages/extension/src/providers/ethereum/libs/transaction/gas-utils.tsLength of output: 6092
No changes needed for RSK staking gas estimation logic
All three concerns have been verified against the current code:
- The
safeGasForStakingfunction wraps its fetch/JSON logic in atry/catchand always falls back to the original gas estimate on error, so the external call will never reject.- The
if ([rsk.chainID, rskTestnet.chainID].includes(this.tx.chainId))guard ensurescollectiveGasConfig[this.tx.chainId]is always defined before accessing.methodId.- Gas limits for staking transactions remain well under JavaScript’s safe-integer range (block gas limits are in the millions), so converting between hex and number will not introduce precision issues.
No fixes are required.
packages/name-resolution/package.json (2)
25-42: LGTM! Dependency updates and new packages for enhanced name resolution.The updates include:
- New runtime dependency
@web3-name-sdk/corefor improved name resolution- New dev dependencies
@bonfida/spl-name-serviceandviemfor Solana name services and Ethereum interactions- Standard version bumps for dev tools (TypeScript, ESLint, etc.)
These changes align with the enhanced name resolution capabilities mentioned in the AI summary.
55-55:Details
✅ Verification successful
Verify the new web3-name-sdk/core dependency usage.
The new
@web3-name-sdk/coredependency should be properly utilized in the codebase for the enhanced name resolution features.
🏁 Script executed:
#!/bin/bash # Description: Verify usage of the new @web3-name-sdk/core package # Expected: Find imports and usage of this package in the name-resolution module # Search for imports of the new package rg "@web3-name-sdk/core" packages/name-resolution/src/ # Look for any related configuration or setup fd "sid" packages/name-resolution/src/ --exec cat {} \;Length of output: 481
🏁 Script executed:
#!/bin/bash # Display the sid index file to verify how the new imports are utilized cat packages/name-resolution/src/sid/index.ts # Search for any invocation or export of the imported create*Name functions rg "createWeb3Name|createSolName|createPaymentIdName" -n packages/name-resolution/srcLength of output: 4106
Verified @web3-name-sdk/core Usage in SID Resolver
The
@web3-name-sdk/coredependency is correctly imported and utilized withinpackages/name-resolution/src/sid/index.ts:
- Imports of
createWeb3Name,createSolName, andcreatePaymentIdNameare present.- Resolvers are instantiated in the constructor:
this.web3NameResolver = createWeb3Name();this.solanaNameResolver = createSolName({ timeout: this.timeout });this.paymentIdNameResolver = createPaymentIdName();- Resolver methods (
getDomainName,getAddress) are invoked in bothresolveReverseNameandresolveAddress.No further action required.
packages/extension/src/providers/ethereum/networks/hemi.ts (1)
1-26: LGTM! Complete and well-structured Hemi network configuration.The network configuration follows the standard EVM network pattern with all required fields:
- Proper chain ID (0xa867 = 43111)
- WebSocket RPC endpoint for real-time updates
- Block explorer URLs with correct placeholders
- Activity handler using wrapped Etherscan handler
- Appropriate currency configuration (ETH/Ethereum)
The configuration appears complete and ready for integration.
packages/extension/src/providers/ethereum/ui/eth-verify-transaction.vue (5)
14-16: LGTM! Clear labeling for transaction sender.Adding an explicit "From" label improves the UI clarity by making it obvious which section shows the sender information.
40-40: Good improvement: Fixed favicon image dimensions.Adding explicit width and height attributes (32px) ensures consistent favicon display regardless of the original image size.
83-96: Excellent UX improvement: Added recipient information display.The new "To" section clearly shows the transaction recipient with:
- Proper labeling
- Recipient identicon for visual identification
- Full address display (or fallback to '~' if unavailable)
This significantly improves transaction verification by showing both parties involved.
203-203: LGTM! Proper initialization of recipient identicon.The
identiconToreactive variable is correctly initialized with a default empty identicon using the network's identicon generator.
237-239: LGTM! Correct recipient identicon generation.The recipient identicon is properly updated during transaction decoding using the lowercase recipient address, which follows standard address formatting conventions.
packages/hw-wallets/src/trezor/trezorConnect.ts (2)
8-9: Verify hardcoded configuration values.The configuration contains hardcoded email addresses and URLs that should be verified for correctness:
- Line 8:
info@enkrypt.com- Line 9:
https://www.enkrypt.com- Line 22:
http://www.myetherwallet.comNote the inconsistency in protocol usage (https vs http) and different domains being used.
Please verify these configuration values are correct and intentional. The mixed protocols and different domains should be reviewed for consistency.
Also applies to: 21-22
3-27: Well-implemented environment-aware dynamic loading.The function correctly implements environment detection and loads the appropriate Trezor Connect library with proper initialization parameters. The type casting to
TrezorConnectTypeensures type safety across different implementations.🧰 Tools
🪛 Biome (1.9.4)
[error] 4-4: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
packages/extension/src/libs/name-resolver/index.ts (1)
17-21: Clean implementation of provider chain support.The addition of the optional
providerChainparameter to theresolveNamemethod is well-implemented. It maintains backward compatibility while enabling chain-specific name resolution by forwarding the parameter to the underlyingresolveAddresscall.packages/name-resolution/src/ud/index.ts (2)
10-307: Excellent expansion of supported TLD coverage.The massive expansion of
supportedTLDsfrom a single entry to hundreds of domains significantly improves the resolver's coverage. The list includes:
- Popular crypto domains (
bitcoin,crypto,dao,eth,polygon)- Traditional domains (
com,org,net,io)- Web3-specific domains (
wallet,nft,blockchain)- Modern TLDs (
ai,app,xyz)This enhancement greatly improves the user experience by supporting a much broader range of domain names.
340-340: Good formatting consistency.The addition of the trailing comma after the
coinparameter improves code consistency and makes future parameter additions cleaner.packages/extension/src/ui/action/views/swap/index.vue (2)
407-411: Proper integration with enhanced name resolution.The update correctly passes the network provider to the
resolveNamemethod, enabling chain-specific name resolution. The parameter is properly cast to string and aligns with the updated method signature in the name resolver.
780-780:Details
❓ Verification inconclusive
Verify the safety of null assertion operators.
The non-null assertion operators (
!) added tofromToken.value.balanceandfromToken.value.decimalsassume these properties will never be null/undefined. This could hide potential runtime errors if these assumptions are incorrect.Please verify that
fromToken.value.balanceandfromToken.value.decimalsare guaranteed to be defined at this point in the code execution. Consider using optional chaining or proper null checks instead of assertions if there's any possibility these could be undefined.
🏁 Script executed:
#!/bin/bash # Search for fromToken.value usage patterns to verify null safety rg -A 3 -B 3 "fromToken\.value\.(balance|decimals)" --type ts --type vueLength of output: 102
I’ll search directly in the .vue file and pull the surrounding context:
🏁 Script executed:
#!/bin/bash # Find all occurrences of fromToken.value in .vue files rg -n "fromToken\.value" -g '*.vue' # Show lines 750–800 from the swap view to inspect fromToken’s declaration and usage sed -n '750,800p' packages/extension/src/ui/action/views/swap/index.vueLength of output: 3349
Let’s inspect how
fromTokenis declared and initialized at the top of this component:#!/bin/bash # Locate how fromToken is defined or pulled in rg -n "const fromToken" -g 'packages/extension/src/ui/action/views/swap/index.vue' rg -n "useSwap" -g 'packages/extension/src/ui/action/views/swap/index.vue' # Dump the first 200 lines to see its initialization context sed -n '1,200p' packages/extension/src/ui/action/views/swap/index.vue
🏁 Script executed:
#!/bin/bash # Print the setup block to locate fromToken declaration sed -n '1,200p' packages/extension/src/ui/action/views/swap/index.vue # Search for all lines where fromToken is defined or referenced rg -n "fromToken" -g 'packages/extension/src/ui/action/views/swap/index.vue'Length of output: 9715
Ensure
fromToken.value.balanceand.decimalsare always defined
- In
packages/extension/src/ui/action/views/swap/index.vueat line 780 you callbut the initial placeholder forfromBase(fromToken.value!.balance!.toString(), fromToken.value!.decimals)fromTokenonly setsdecimals, notbalance.- The real
balancecomes fromswap.getFromTokens()(around lines 306–307), yetisDisabled(which uses line 780) can run before that data arrives.- A missing
balancewill cause a runtime exception when calling.toString().Consider one of the following:
- Initialize your placeholder token with a default
balance(e.g. zero).- Add an explicit guard or fallback before using
fromBase, for example:const rawBalance = fromToken.value?.balance; if (!rawBalance) return true; // disable or handle empty balance const humanBalance = fromBase(rawBalance.toString(), fromToken.value.decimals);- Replace
!assertions with optional chaining and sensible defaults.Please verify that
fromToken.value.balanceand.decimalsare always set before these lines execute, or add the necessary null checks.packages/name-resolution/src/index.ts (2)
48-52: LGTM: Clean implementation of payment ID chain support.The addition of the optional
paymentIdChainparameter maintains backward compatibility while enabling new functionality. The parameter typing and positioning are appropriate.
58-59:Details
✅ Verification successful
Verify SID resolver implementation supports the new parameter.
The conditional logic correctly passes the
paymentIdChainparameter to the SID resolver, but ensure that thesid.resolveAddressmethod has been updated to handle this parameter properly.
🏁 Script executed:
#!/bin/bash # Description: Verify that SID resolver's resolveAddress method accepts paymentIdChain parameter # Expected: Method signature should include optional paymentIdChain parameter ast-grep --pattern 'class SIDResolver { $$$ resolveAddress($_, $paymentIdChain) { $$$ } $$$ }'Length of output: 108
🏁 Script executed:
#!/bin/bash # Locate the SIDResolver class and its resolveAddress method signature rg -n "class SIDResolver" -n packages/name-resolution/src rg -n "resolveAddress" -n packages/name-resolution/srcLength of output: 1036
🏁 Script executed:
#!/bin/bash # Display the signature and surrounding lines of the SIDResolver.resolveAddress method rg -n "public async resolveAddress" -n packages/name-resolution/src/sid/index.ts -C2Length of output: 184
SIDResolver.resolveAddress supports
paymentIdChain
TheresolveAddressmethod inpackages/name-resolution/src/sid/index.ts(lines 92–94) declarespaymentIdChain?: string, so the parameter is already supported. No further changes needed.packages/hw-wallets/package.json (2)
3-3: Consider the significant version jump from 0.0.4 to 0.0.9.The version increase from 0.0.4 to 0.0.9 is substantial for a patch/minor release. Ensure this aligns with your versioning strategy and that the changes justify this increment.
63-65: LGTM: Trezor dependency updates support the new dynamic import architecture.The addition of
@trezor/connect-weband updates to related Trezor packages align well with the dynamic import refactoring implemented in the source files.packages/hw-wallets/src/trezor/ethereum/index.ts (1)
6-6: LGTM: Well-structured dynamic import refactoring.The transition from static to dynamic imports via
getTrezorConnectis well-implemented. Using type imports and storing the instance as a class property provides good separation of concerns.Also applies to: 16-16, 20-20
packages/hw-wallets/src/trezor/bitcoin/index.ts (1)
5-5: LGTM: Consistent refactoring pattern with Ethereum implementation.The dynamic import refactoring follows the same pattern as the Ethereum implementation, providing consistency across the Trezor hardware wallet integrations.
Also applies to: 16-16, 20-20
packages/extension/src/providers/ethereum/libs/transaction/gas-utils.ts (1)
182-195: ****
kvhnuke
force-pushed
the
feat/space-id-support
branch
from
cc00fe3 to
1316df5
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
packages/name-resolution/src/sid/index.ts (2)
75-90: Address the error handling issue identified in previous reviews.The silent error handling in the catch block still makes debugging difficult, as noted in previous reviews. The error should be logged or provide more context.
} catch (error) { + console.warn(`Failed to resolve reverse name for ${address}:`, error); return null; }
92-113:⚠️ Potential issueFix switch case variable scoping and improve address validation.
The switch statement has variable declaration scoping issues as identified in previous reviews and static analysis. The variables need to be wrapped in block scope.
switch (tld) { - case "sol": - const solAddress = await this.solanaNameResolver.getAddress({ - name, - }); - return solAddress; - default: - const address = await this.web3NameResolver.getAddress(name, { - timeout: this.timeout, - }); - return parseInt(address, 16) === 0 ? null : address; + case "sol": { + const solAddress = await this.solanaNameResolver.getAddress({ + name, + }); + return solAddress; + } + default: { + const address = await this.web3NameResolver.getAddress(name, { + timeout: this.timeout, + }); + // Improved zero address check + return address === "0x0" || address === "0x0000000000000000000000000000000000000000" ? null : address; + } }🧰 Tools
🪛 Biome (1.9.4)
[error] 103-105: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.The declaration is defined in this switch clause:
Unsafe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 108-110: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.The declaration is defined in this switch clause:
Unsafe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
🧹 Nitpick comments (1)
packages/name-resolution/src/sid/index.ts (1)
15-39: Consider using camelCase for better TypeScript consistency.The TLD list is comprehensive, but consider renaming
evm_tldstoevmTldsto follow TypeScript naming conventions.-const evm_tlds = [ +const evmTlds = [Also update the reference on line 45:
- supportedTLDs = ["sol", ...evm_tlds]; + supportedTLDs = ["sol", ...evmTlds];
📜 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 (4)
packages/name-resolution/package.json(2 hunks)packages/name-resolution/src/index.ts(1 hunks)packages/name-resolution/src/sid/index.ts(1 hunks)packages/name-resolution/tests/sid.test.ts(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/name-resolution/tests/sid.test.ts
- packages/name-resolution/src/index.ts
- packages/name-resolution/package.json
🧰 Additional context used
🪛 Biome (1.9.4)
packages/name-resolution/src/sid/index.ts
[error] 103-105: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Unsafe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 108-110: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Unsafe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: buildAll
- GitHub Check: test
- GitHub Check: test
🔇 Additional comments (4)
packages/name-resolution/src/sid/index.ts (4)
1-13: LGTM! Clean import structure with helpful context.The new SDK imports are well-organized and the demo URL comment provides valuable context for developers.
41-58: Well-structured class with proper initialization.The class structure is clean with good separation of concerns. The timeout configuration with defaults and proper resolver initialization looks solid.
63-73: Solid implementation of payment ID resolution.The method correctly handles payment ID address resolution with a sensible fallback to Ethereum when no specific chain is provided.
114-117: Clean implementation of dual name support.The method correctly supports both TLD-based names and payment IDs with clear logic and helpful comments.
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
2 participants
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Chores
Tests