feat: add_all_tlds_and paymentid#682
Conversation
|
""" WalkthroughThis update enhances the name resolution logic by introducing support for PaymentId names and expanding TLD support through the integration of the Changes
Sequence Diagram(s)sequenceDiagram
participant UI as UI Component
participant NameResolver as GenericNameResolver
participant SDK as Web3NameSDK/SolanaSDK/PaymentIdSDK
UI->>NameResolver: resolveName(name, coins, providerChain)
NameResolver->>NameResolver: iterate coins
NameResolver->>SDK: resolveAddress(name, coin, providerChain)
SDK-->>NameResolver: resolved address or null
NameResolver-->>UI: resolved address or null
sequenceDiagram
participant App as Application
participant SIDResolver
participant PaymentIdSDK
participant SolanaSDK
participant Web3NameSDK
App->>SIDResolver: resolveAddress(name, coin, paymentIdChain)
alt PaymentId name
SIDResolver->>PaymentIdSDK: handlePaymentIdGetAddress(name, paymentIdChain)
PaymentIdSDK-->>SIDResolver: address or null
else Solana TLD
SIDResolver->>SolanaSDK: resolveAddress(name, coin)
SolanaSDK-->>SIDResolver: address or null
else EVM/Other TLD
SIDResolver->>Web3NameSDK: resolveAddress(name, coin)
Web3NameSDK-->>SIDResolver: address or null
end
SIDResolver-->>App: address or null
Suggested reviewers
Tip ⚡️ Faster reviews with caching
Enjoy the performance boost—your workflow just got faster. 📜 Recent review detailsConfiguration used: .coderabbit.yaml ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🔭 Outside diff range comments (1)
packages/extension/src/libs/name-resolver/index.ts (1)
17-26:⚠️ Potential issueCritical issue: providerChain parameter is not used
The method signature has been updated to accept the providerChain parameter, but this parameter is not passed to the underlying
resolveAddressmethod call. This will prevent PaymentId name resolution from working correctly despite UI components passing the provider.Apply this fix to pass the providerChain parameter to the resolveAddress method:
async resolveName(name: string, coins: CoinType[], providerChain?: string): Promise<string | null> { let response: string | null = null; for (const coin of coins) { response = await this.nameResolver - .resolveAddress(name, coin) + .resolveAddress(name, coin, providerChain) .catch(() => null); if (response) return response; } return response; }
🧹 Nitpick comments (7)
packages/name-resolution/src/sid/utils.ts (1)
1-2: PaymentId validation implementation looks good.The regex correctly validates the basic PaymentId format (alphanumeric@alphanumeric), aligning with the PR objective to add PaymentId support.
Consider adding support for common special characters in the local part of the PaymentId if required by the standard specification. For example:
-const paymentIdRegex = /^[a-zA-Z0-9]+@[a-zA-Z0-9]+$/ +const paymentIdRegex = /^[a-zA-Z0-9._-]+@[a-zA-Z0-9]+$/Additionally, consider adding unit tests for this utility function to ensure correctness.
packages/name-resolution/src/types.ts (1)
155-155: Type inconsistency between method signaturesThe
isSupportedNamemethod uses thePaymentIdChaintype for the paymentIdChain parameter, whileresolveAddressusesstring. This inconsistency could lead to type confusion.Consider using the same type for consistency:
- public abstract isSupportedName(name: string, paymentIdChain?:PaymentIdChain): boolean; + public abstract isSupportedName(name: string, paymentIdChain?: string): boolean;Or adjust resolveAddress to use the more specific type if appropriate:
- public abstract resolveAddress(name: string, coint: CoinType, paymentIdChain?: string): Promise<string | null>; + public abstract resolveAddress(name: string, coint: CoinType, paymentIdChain?: PaymentIdChain): Promise<string | null>;packages/name-resolution/src/sid/types.ts (2)
6-7: Terminate thetimeoutproperty with a semicolon for stylistic consistencyAll other members in the
SIDOptionsinterface are terminated with semicolons (;). Although TypeScript’s ASI handles the missing delimiter, keeping the interface homogeneous avoids unnecessary diffs and potential ESLint/TS-lint noise.- timeout?: number + timeout?: number;
18-25: Freeze and strongly-type the PAYMENT_ID_CHAINS_MAPToday the map is a mutable plain object whose value type is an inferred union of the numeric literals (
0 | 1 | …). Converting it to a readonlyas constmap (orRecord<string, PaymentIdChain>) eliminates accidental mutation and guarantees that look-ups return only validPaymentIdChainvalues.-export const PAYMENT_ID_CHAINS_MAP = { +export const PAYMENT_ID_CHAINS_MAP = { "bitcoin": 0, "ethereum": 1, "solana": 2, "tron": 3, "aptos": 4, "sui": 5, -}; +} as const;You may also want to give it an explicit type:
export const PAYMENT_ID_CHAINS_MAP: Record<string, PaymentIdChain> = { … };packages/name-resolution/src/sid/index.ts (3)
85-93: Remove strayconsole.logor gate it behind a debug flag
console.login library code can spam consumer logs. Either remove it or guard it with a debug flag/environment variable.
86-89: Typo:coint→coin(or remove the param)The parameter is named
coint, which is likely a misspelling ofcoin. If it’s unused (as it appears), consider deleting it to satisfynoUnusedParameters; otherwise, rename for clarity.-public async resolveAddress( - name: string, - coint: CoinType, - paymentIdChain?: string, +public async resolveAddress( + name: string, + coin: CoinType, + paymentIdChain?: string,
110-112: Normalize TLD case to avoid false negatives
getTLD(name)may return mixed-case strings (user input). Convert to lowercase when checking membership:-return this.supportedTLDs.includes(getTLD(name)) || isValidPaymentId(name); +return ( + this.supportedTLDs.includes(getTLD(name).toLowerCase()) || + isValidPaymentId(name) +);
📜 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 (14)
packages/extension/package.json(2 hunks)packages/extension/src/libs/name-resolver/index.ts(1 hunks)packages/extension/src/providers/ethereum/ui/send-transaction/index.vue(1 hunks)packages/extension/src/providers/kadena/ui/send-transaction/index.vue(1 hunks)packages/extension/src/providers/polkadot/ui/send-transaction/index.vue(1 hunks)packages/extension/src/providers/solana/ui/send-transaction/index.vue(1 hunks)packages/extension/src/ui/action/views/swap/index.vue(1 hunks)packages/name-resolution/package.json(1 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/tsconfig.json(1 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
packages/name-resolution/src/sid/index.ts
[error] 98-100: 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] 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)
🔇 Additional comments (14)
packages/name-resolution/package.json (2)
52-52:Details
❓ Verification inconclusive
Verify Solana dependency compatibility and version security.
The addition of the Solana web3.js library supports the integration of Solana blockchain-based name resolution.
🏁 Script executed:
#!/bin/bash # Check for the latest version of @solana/web3.js and any known security advisories # Get the latest version from npm echo "Checking latest version of @solana/web3.js:" npm view @solana/web3.js version # Check for any known vulnerabilities echo "Checking for known vulnerabilities:" npm audit --package @solana/web3.jsLength of output: 606
Review Solana web3.js Dependency Version and Security
- packages/name-resolution/package.json (Line 52) pins
@solana/web3.js: "^1.98.0".
Latest patch release is 1.98.2, which falls within this range—no immediate version bump is required.- The
npm audit --package @solana/web3.jscheck failed due to a missing lockfile. To fully vet for vulnerabilities:
- Generate a lockfile (e.g.,
npm install --package-lock-only).- Run
npm auditin the project root or integrate a dependency-scanning step in your CI.- Address any advisories before merging.
Please verify that a lockfile is committed and that the audit report is clean.
54-54:Details
❓ Verification inconclusive
Verify web3-name-sdk dependency compatibility.
The addition of the
@web3-name-sdk/corepackage enables the expanded TLD handling and PaymentId resolution support as specified in the PR objectives.
🏁 Script executed:
#!/bin/bash # Check for the latest version of @web3-name-sdk/core and its GitHub repository status # Get the latest version from npm echo "Checking latest version of @web3-name-sdk/core:" npm view @web3-name-sdk/core version # Check for GitHub repository info echo "Checking GitHub repository info and activity:" npm view @web3-name-sdk/core repository.urlLength of output: 296
Confirm @web3-name-sdk/core Version & Maintenance Status
The npm registry shows you’re already using the latest release (0.4.1), but the package metadata does not expose a repository URL, making it hard to audit maintenance activity and future breaking changes. Please verify:
- File:
packages/name-resolution/package.json(line 54): dependency is"@web3-name-sdk/core": "^0.4.1".- That 0.4.1 fully supports the expanded TLD handling and PaymentId resolution your PR requires.
- The upstream GitHub repository is active, matches this package, and has no pending breaking changes.
- Whether you should pin or widen the version range for future compatibility.
packages/extension/package.json (2)
27-27:Details
❓ Verification inconclusive
Verify Bonfida SPL name service compatibility.
The addition of Bonfida's SPL name service library supports Solana-based domain resolution in the extension.
🏁 Script executed:
#!/bin/bash # Check for the latest version of @bonfida/spl-name-service and any known security advisories # Get the latest version from npm echo "Checking latest version of @bonfida/spl-name-service:" npm view @bonfida/spl-name-service version # Check for any known vulnerabilities echo "Checking for known vulnerabilities:" npm audit --package @bonfida/spl-name-serviceLength of output: 646
Verify @bonfida/spl-name-service Version and Security Audit
packages/extension/package.json lists
"@bonfida/spl-name-service": "^3.0.10", which is the latest published version (3.0.10).The audit command failed due to a missing lockfile. Please generate one and rerun the audit:
cd packages/extension npm install --package-lock-only npm audit --package @bonfida/spl-name-serviceor manually review npm advisories for any vulnerabilities before merging.
No further changes are needed if no security issues are found.
82-82:Details
❓ Verification inconclusive
Verify viem dependency compatibility.
The addition of viem, a modern TypeScript interface for Ethereum, provides improved type safety and performance for blockchain interactions in the extension.
🏁 Script executed:
#!/bin/bash # Check for the latest version of viem and any known security advisories # Get the latest version from npm echo "Checking latest version of viem:" npm view viem version # Check for any known vulnerabilities echo "Checking for known vulnerabilities:" npm audit --package viemLength of output: 562
Dependency Compatibility Verified for viem
The caret range
^2.23.12already covers the latestviem@2.29.1. No known advisories were flagged for viem itself, but since the audit in this sandbox failed for lack of a lockfile, please confirm in your project:
- Generate your lockfile and run the audit in your repo context:
npm install --package-lock-only npm audit --package viem- Ensure no vulnerabilities are reported before merging.
packages/extension/src/providers/ethereum/ui/send-transaction/index.vue (1)
673-673:Details
✅ Verification successful
Check name resolution protocol support.
The code specifies an array of supported protocols (
[props.network.name as CoinType, 'ETH']).
🏁 Script executed:
#!/bin/bash # Verify that the array of supported protocols is consistent with other implementations # and that all the required protocols are included echo "Checking for other CoinType arrays in name resolution calls:" find src/providers -name "*.vue" -o -name "*.ts" | xargs grep -n "CoinType" --color=always echo "Checking if PaymentId is included in name resolution protocols:" find packages/name-resolution/src -name "*.ts" | xargs grep -n "PaymentId" --color=alwaysLength of output: 3380
#!/bin/bash # Display the context around line 673 in the send-transaction component sed -n '650,740p' packages/extension/src/providers/ethereum/ui/send-transaction/index.vue
🏁 Script executed:
#!/bin/bash # Search for all CoinType usages in the extension provider code grep -R "CoinType" -n packages/extension/src/providers # Show the surrounding lines around the protocol array in send-transaction component sed -n '650,720p' packages/extension/src/providers/ethereum/ui/send-transaction/index.vueLength of output: 2862
Ethereum name resolution protocols are correct.
The array[props.network.name as CoinType, 'ETH']matches the pattern used across providers (e.g. Polkadot, Solana, Kadena) and properly includes both the coin-type and the ENS ticker for ENS lookups. No additional protocols (such as PaymentIdChain) are required here.packages/extension/src/providers/solana/ui/send-transaction/index.vue (1)
696-701: Good addition of provider parameter to support expanded name resolution capabilities.The addition of the network provider parameter to the
resolveNamemethod call is consistent with the enhancement of name resolution capabilities in the PR. This change enables the method to utilize provider-specific information when resolving addresses.packages/extension/src/providers/polkadot/ui/send-transaction/index.vue (1)
380-385: Correctly implemented provider parameter for name resolution.The addition of the network provider parameter to the
resolveNamemethod call is consistent with changes in other components and supports the enhanced name resolution capabilities being introduced, particularly for PaymentId support.packages/extension/src/ui/action/views/swap/index.vue (1)
406-411: Properly aligned with name resolution enhancements.The third parameter (provider) has been correctly added to the
resolveNamemethod call, maintaining consistency with similar changes in other components. This change supports the expanded TLD handling and PaymentId integration in the name resolution system.packages/name-resolution/tsconfig.json (1)
5-5: Module configuration updated to support the Web3 Name SDK integration.The TypeScript configuration has been properly updated to support the integration of the
@web3-name-sdk/corepackage. The changes include:
- Changing
moduleResolutionto "bundler" for modern module resolution- Updating
moduleto "es2020" to enable modern JavaScript features- Adding TypeScript declaration file generation
- Adding paths mapping for the SDK to ensure proper module resolution
These changes are necessary to support the refactoring of the SIDResolver to use SDK-based resolvers for EVM, Solana, and PaymentId protocols.
Also applies to: 11-18
packages/extension/src/providers/kadena/ui/send-transaction/index.vue (1)
402-406: LGTM - Added provider context for name resolutionThe code correctly passes the network provider as a third parameter to the name resolver, which enables protocol-specific name resolution including PaymentId.
packages/name-resolution/src/index.ts (2)
49-52: LGTM - Added PaymentId chain support to resolveAddress methodThe parameter addition is consistent with the updated BaseResolver interface and allows for PaymentId name resolution.
54-54: LGTM - Updated SID resolver call to utilize the new parameterThe code correctly passes the paymentIdChain parameter to the SID resolver, enabling PaymentId name resolution.
packages/name-resolution/src/types.ts (2)
2-2: LGTM - Added PaymentIdChain type importThe import has been correctly updated to include the PaymentIdChain type from sid/types.
149-151: LGTM - Updated BaseResolver.resolveAddress signatureThe method signature has been correctly updated to support the optional paymentIdChain parameter.
| const address = await this.web3NameResolver.getAddress(name, { | ||
| timeout: this.timeout, | ||
| }); | ||
| return parseInt(address, 16) === 0 ? null : address; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid parseInt for zero-address detection
parseInt('0x…', 16) can overflow Number.MAX_SAFE_INTEGER, and any non-numeric characters after the first invalid hex digit are silently ignored. A safer test is to compare against the canonical zero address:
-return parseInt(address, 16) === 0 ? null : address;
+return /^0x0{40}$/i.test(address) ? null : address;If the SDK already returns null/undefined on failure, you can omit the check entirely.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const address = await this.web3NameResolver.getAddress(name, { | |
| timeout: this.timeout, | |
| }); | |
| return parseInt(address, 16) === 0 ? null : address; | |
| } | |
| const address = await this.web3NameResolver.getAddress(name, { | |
| timeout: this.timeout, | |
| }); | |
| return /^0x0{40}$/i.test(address) ? 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)
| 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; | ||
| } | ||
| } |
There was a problem hiding this comment.
Fix lexical-declaration leak inside switch cases
Declaring const variables directly in a case clause without wrapping it in braces violates the ECMAScript spec and is flagged by Biome (noSwitchDeclarations).
Wrap each clause in its own block to contain the 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,
+ });
+ return parseInt(address, 16) === 0 ? null : address;
+ }
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| 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; | |
| } | |
| } | |
| 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; | |
| } | |
| } | |
| } |
🧰 Tools
🪛 Biome (1.9.4)
[error] 98-100: 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] 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)
| "compilerOptions": { | ||
| "lib": ["ESNext", "dom"], | ||
| "moduleResolution": "node", | ||
| "moduleResolution": "bundler", |
There was a problem hiding this comment.
this change cannot be done
| "sourceMap": true, | ||
| "target": "es5", | ||
| "module": "commonjs", | ||
| "module": "es2020", |
There was a problem hiding this comment.
same here
we have to maintain commonjs
packages/extension/package.json
Outdated
| "switch-ts": "^1.1.1", | ||
| "url-parse": "^1.5.10", | ||
| "uuid": "^11.1.0", | ||
| "viem": "^2.23.12", |
There was a problem hiding this comment.
doesnt seem like you are using this package
| .resolveName( | ||
| text, | ||
| [props.network.name as CoinType], | ||
| props.network?.provider as string, | ||
| ) |
There was a problem hiding this comment.
you dont seem to support kadena so this change is not necessary
| .resolveName( | ||
| text, | ||
| [props.network.name as CoinType, 'DOT', 'KSM'], | ||
| props.network?.provider as string, | ||
| ) |
There was a problem hiding this comment.
you dont seem to support polkadot so this change is not necessary
|
merged into #691 |
2 participants
Summary by CodeRabbit
New Features
Improvements
Bug Fixes
Chores