feat: add metamask #936
feat: add metamask #936
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
WalkthroughThe pull request introduces several modifications across multiple components and utility files. Key changes include enhancements to wallet connection handling in the Changes
Possibly related PRs
Suggested reviewers
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (12)
utils/callData/resolverCalls.ts (2)
17-17: Document the hint argument's purposeThe comment "zero is hint argument" doesn't explain the purpose or significance of this hint. Consider adding more detailed documentation about what this hint represents and why it's needed.
16-16: Address the TODO comment regarding contract versionThe TODO comment suggests there's a pending update related to the naming contract version on mainnet. This should be tracked and addressed.
Would you like me to create a GitHub issue to track this TODO item?
utils/callData/autoRenewalCalls.ts (1)
28-29: Document the sponsor parameter's purposeThe comment "sponsor" doesn't provide enough context about this parameter's role in the renewal process.
utils/callData/solanaCalls.ts (2)
29-29: Document the root domain encodingThe comment "sol encoded" doesn't provide enough context about how this value was derived or what it represents in the Solana naming system.
Line range hint
1-1: Consider documenting the calldata standardization patternThere's a consistent pattern across these files for handling calldata:
- Converting numeric values to strings
- Using hint arguments
- Converting hex addresses to decimal
Consider adding documentation in a central location (e.g., README or contributing guidelines) about these patterns to ensure consistency across the codebase.
utils/callData/identityChangeCalls.ts (1)
Line range hint
115-119: Consider adding type assertions for better type safetyThe string conversions are correct, but we could enhance type safety by ensuring the input types are numbers before conversion.
- timestamp.toString(), + typeof timestamp === 'number' ? timestamp.toString() : timestamp, - dataToWrite.toString(), + (typeof dataToWrite === 'number' || typeof dataToWrite === 'string') + ? dataToWrite.toString() + : dataToWrite,components/identities/actions/subdomainModal.tsx (2)
59-62: LGTM: Consider extracting the increment operation for clarityThe string conversion is correct, but we could improve readability by extracting the increment operation.
+ const incrementedValue = Number(callDataEncodedDomain[0]) + 1; - numberToString(Number(callDataEncodedDomain[0]) + 1), + numberToString(incrementedValue),
77-80: LGTM: Consider the same extraction for consistencySimilar to the previous segment, extracting the increment operation would improve readability.
+ const incrementedValue = Number(callDataEncodedDomain[0]) + 1; - numberToString(Number(callDataEncodedDomain[0]) + 1), + numberToString(incrementedValue),pages/identities.tsx (1)
40-40: LGTM: Consider memoizing randomTokenIdThe string conversion is correct, but randomTokenId could be memoized to prevent unnecessary regeneration.
- const randomTokenId: number = Math.floor(Math.random() * 1000000000000); + const randomTokenId = useMemo(() => + Math.floor(Math.random() * 1000000000000) + , []);utils/callData/registrationCalls.ts (1)
36-42: Consider documenting the metadata format requirementsThe metadata is now converted using
hexToDecimal. This conversion requirement should be documented to prevent integration issues.Add a comment explaining that metadata must be in hex format and will be converted to decimal for the contract call.
components/identities/actions/clickable/clickablePersonhoodIcon.tsx (1)
Line range hint
1-237: Consider documenting the numeric value handling standardsThe codebase has been updated to follow a consistent pattern for handling numeric values in contract calls:
- Contract addresses are converted using
hexToDecimal- Numeric parameters are converted to strings
- Default values are provided as strings
Consider adding documentation about these standards to help future contributors maintain consistency.
components/UI/walletConnect.tsx (1)
Line range hint
1-138: Consider adding error boundary for wallet connection failuresWhile the component has good separation of concerns, adding an error boundary would improve the user experience by gracefully handling wallet connection failures.
Consider wrapping the wallet connection logic in an error boundary component to:
- Catch and handle wallet-specific errors
- Provide meaningful error messages to users
- Prevent the entire UI from crashing on connection failures
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (14)
components/UI/navbar.tsx(0 hunks)components/UI/walletConnect.tsx(3 hunks)components/identities/actions/clickable/clickablePersonhoodIcon.tsx(1 hunks)components/identities/actions/subdomainModal.tsx(2 hunks)hooks/contracts.ts(1 hunks)package.json(3 hunks)pages/identities.tsx(1 hunks)tsconfig.json(0 hunks)utils/callData/autoRenewalCalls.ts(3 hunks)utils/callData/identityChangeCalls.ts(3 hunks)utils/callData/registrationCalls.ts(9 hunks)utils/callData/resolverCalls.ts(1 hunks)utils/callData/solanaCalls.ts(2 hunks)utils/connectorWrapper.ts(4 hunks)
💤 Files with no reviewable changes (2)
- components/UI/navbar.tsx
- tsconfig.json
✅ Files skipped from review due to trivial changes (1)
- hooks/contracts.ts
🔇 Additional comments (16)
package.json (1)
25-25: Verify compatibility of updated dependencies
Several key dependencies have been updated:
- Added: @starknet-io/[email protected]
- Updated: @starknet-react/core from 3.5.0 to 3.6.2
- Downgraded: get-starknet-core from 4.0.0 to 3.3.4
- Updated: starknetkit from 2.3.3 to 2.6.2
The downgrade of get-starknet-core requires verification.
Also applies to: 27-27, 36-36, 51-51
✅ Verification successful
Let me analyze the dependencies and their compatibility based on the version information we received:
@starknet-io/[email protected]- This is a recent version, though 4.0.5 is available@starknet-react/coreupgrade to 3.6.2 - This is the latest stable versionget-starknet-coredowngrade from 4.0.0 to 3.3.4 - This is intentional as it's the last stable version before the 4.0.0-next releasesstarknetkitupgrade to 2.6.2 - This is the latest stable version (2.7.0-beta.1 exists but is beta)
Let me generate the final response:
Dependencies are compatible, but security fixes needed
The dependency versions are compatible and strategically chosen:
- The downgrade of get-starknet-core to 3.3.4 is valid as it's the last stable version before 4.0.0
- All other packages are at their latest stable versions
- However, there are 9 low severity vulnerabilities that should be addressed using
npm audit fix
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check compatibility between versions and any security advisories
# Check latest versions
echo "Checking latest versions..."
npm view @starknet-io/get-starknet-core versions --json
npm view @starknet-react/core versions --json
npm view get-starknet-core versions --json
npm view starknetkit versions --json
# Check for known vulnerabilities
echo "Checking for vulnerabilities..."
npm audit
Length of output: 5264
utils/callData/autoRenewalCalls.ts (1)
12-12: Verify hexToDecimal conversion for contract addresses
The conversion of contract addresses using hexToDecimal is important for proper interaction with the contract.
✅ Verification successful
Let me gather more context about the hexToDecimal implementation to ensure it's handling contract addresses correctly.
Let me check the implementation of hexToDecimal in feltService.ts to verify its handling of contract addresses.
hexToDecimal implementation is correct for contract addresses
The implementation in feltService.ts correctly handles contract addresses by:
- Validating hex string input
- Properly removing '0x' prefix with slice(2)
- Using BN (Big Number) for accurate conversion from base 16 to base 10
- Consistent usage across the codebase for contract addresses
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for consistent hexToDecimal usage across the codebase
rg -A 2 "hexToDecimal\(" | grep -v "test" | grep "contract"
Length of output: 603
Script:
#!/bin/bash
# Find the implementation of hexToDecimal
ast-grep --pattern 'function hexToDecimal($_) {
$$$
}'
# Also check for any other implementations or exports
rg "hexToDecimal"
Length of output: 6422
Script:
#!/bin/bash
# Get the implementation from feltService.ts
cat utils/feltService.ts | grep -A 10 "hexToDecimal"
Length of output: 375
utils/callData/identityChangeCalls.ts (2)
55-55: LGTM: Consistent string conversion in setUserData
The string conversion for calldata is consistent with other similar changes.
17-17: LGTM: String conversion for calldata
The conversion of numeric 0 to string "0" in calldata aligns with Starknet's requirements for string-based parameters.
✅ Verification successful
The previous search might have been too specific. Let's broaden our search to understand the calldata pattern usage in the codebase and verify the consistency of string vs numeric values.
String-based calldata values are consistently used across the codebase
The verification confirms that string-based values (including "0") are consistently used in calldata arrays across the codebase:
autoRenewalCalls.ts: Uses string values including"0"andtoString()conversionssolanaCalls.ts: Uses string "0" for hint argumentidentityChangeCalls.ts: Consistently uses string "0" in bothtransferandsetUserDatafunctions
No instances of numeric zero were found in calldata arrays, confirming the correct usage pattern.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Verify consistent string conversion in transfer calls
rg -g '*.ts' -g '*.tsx' 'transferFrom.*calldata.*0"'
Length of output: 54
Script:
#!/bin/bash
# Let's search for any calldata arrays containing "0" as a string
rg -g '*.ts' -g '*.tsx' 'calldata.*"0"'
# Also search for numeric 0 in calldata to ensure consistency
rg -g '*.ts' -g '*.tsx' 'calldata.*[^"]0[^"]'
# Let's also check the specific file for context
cat utils/callData/identityChangeCalls.ts
Length of output: 4527
utils/callData/registrationCalls.ts (4)
100-100: LGTM: Consistent token ID handling
The conversion of tokenId using numberToStringHex is consistent with other token ID handling in the codebase.
109-111: LGTM: Consistent numeric value handling
The conversion of contract address and amount values follows the established pattern for numeric handling in contract calls.
10-12: Verify the purpose of the hardcoded "0" parameter
The hardcoded "0" parameter in the approve calldata array might have a specific purpose that should be documented.
✅ Verification successful
The hardcoded "0" parameter is correct for ERC20 approve
Looking at the ERC20 ABI and the implementation, the "0" parameter is correct. The approve function in the ERC20 contract takes a spender address and an amount in Uint256 format, which requires two parameters: the low and high bits. The "0" represents the high bits of the Uint256 amount, which is appropriate when the amount fits within a single felt (low bits).
The approve function is consistently used across the codebase in registration, renewal, and discount scenarios, always following this pattern:
- First parameter: naming contract address (spender)
- Second parameter: price amount (low bits)
- Third parameter: "0" (high bits)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other approve function implementations to understand the purpose of the "0" parameter
ast-grep --pattern 'function approve($_, $_) {
$$$
return {
$$$
calldata: [$$$]
}
}'
Length of output: 100
Script:
#!/bin/bash
# Let's try a broader search for approve-related code and calldata
rg -A 5 "approve.*calldata"
# Also search for any approve function definitions
ast-grep --pattern 'function approve'
# Search for files containing approve in their name
fd approve
# Let's also check the contract interface or ABI if available
fd -e abi -e json | xargs rg -l "approve"
Length of output: 219
Script:
#!/bin/bash
# Let's examine the ERC20 ABI which typically defines the approve function
cat abi/starknet/erc20_abi.json
# Also check the file content where the approve function is implemented
rg -B 5 -A 10 "approve.*price"
Length of output: 9774
74-80: Verify quote data handling consistency
The quote data handling mixes direct usage (quoteData.quote) with conversions (numberToString(quoteData.max_quote_validity)). Ensure this mixed approach is intentional and matches the contract's expectations.
✅ Verification successful
Let me gather more information about the contract's expectations and the numberToString utility function.
Let me try a different approach to understand the data type handling.
Based on the analysis of the codebase, I can now generate the final response:
Mixed quote data handling is intentional and correct
The mixed approach in handling quote data is intentional and consistent with the contract's expectations:
quoteData.quoteis used directly because it's already in the correct string format for price calculations, as evidenced by its usage ingetDomainPriceAltcoinquoteData.max_quote_validityrequires conversion vianumberToStringbecause it's a numeric timestamp that needs to be converted to a decimal string format for the contract call
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other quote data handling implementations
rg -A 5 'quoteData\.quote|max_quote_validity'
Length of output: 5795
Script:
#!/bin/bash
# Search for contract interface or ABI that defines the parameter types
rg -A 3 'interface.*Registration|contract.*Registration'
# Search for numberToString implementation
ast-grep --pattern 'function numberToString'
Length of output: 105
Script:
#!/bin/bash
# Search for the usage of quote and max_quote_validity in the codebase
rg -B 2 -A 2 'function.*altcoinBuy|function.*register.*With.*Altcoin'
# Look for utility functions that handle conversions
rg -B 1 -A 3 'hexToDecimal|numberToString|getDomainPriceAltcoin'
Length of output: 39367
components/identities/actions/clickable/clickablePersonhoodIcon.tsx (1)
129-131: LGTM: Consistent contract address handling
The conversion of VERIFIER_POP_CONTRACT address using hexToDecimal aligns with the standardized approach for handling contract addresses in calls.
utils/connectorWrapper.ts (4)
8-8: LGTM: MetaMask initialization added correctly
The getStarknet import and initialization are properly placed to support MetaMask wallet integration.
Also applies to: 11-12
27-27: LGTM: MetaMask connector added correctly
The MetaMask connector is properly integrated using InjectedConnector with the correct ID.
165-166: LGTM: Keplr icon standardized to base64
The change standardizes the icon format by replacing the file path with a base64-encoded string, consistent with other wallet icons.
175-186: LGTM: MetaMask wallet configuration complete and well-structured
The MetaMask wallet configuration is properly structured with all necessary properties:
- Correct wallet ID and name
- Base64 encoded SVG icon
- Download links for Chrome, Firefox, and Edge
- Official website link
components/UI/walletConnect.tsx (3)
13-13: LGTM!
The import statement is correctly formatted and the imported function is properly utilized throughout the component.
108-112: Verify MetaMask icon handling
The conditional icon selection needs to properly handle the MetaMask connector icon. Let's verify that the getConnectorIcon utility includes MetaMask support.
✅ Verification successful
Let me gather more information to verify MetaMask icon handling properly.
Based on the search results, I can now provide the final response:
MetaMask icon handling is properly configured
The codebase has complete MetaMask support with:
- MetaMask connector properly defined with ID "metamask" in connectorWrapper.ts
- MetaMask icon configured in the wallets array with base64 encoded SVG
- Dedicated MetaMask icon file at public/ens/metamask.svg
- The
getConnectorIcon()utility correctly handles the MetaMask case by returning the icon from the wallets configuration
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if MetaMask icon is properly configured
rg -A 5 "getConnectorIcon" --type ts --type tsx
# Look for MetaMask-related assets
fd -e svg -e png -e jpg "metamask|MetaMask"
Length of output: 145
Script:
#!/bin/bash
# Search for getConnectorIcon implementation and usage
rg -A 5 "getConnectorIcon"
# Check for connector ID related code
rg -A 3 "connector\.id"
# Look for any MetaMask related configuration
rg -A 3 "metamask|MetaMask" -i
Length of output: 26565
39-42: Verify MetaMask connector handling in Argent mobile browser
Given that this PR adds MetaMask support, the current filtering logic might need to be updated. When in Argent mobile browser, the filter only allows 'argentMobile' or 'argentX' connectors, which could potentially exclude the MetaMask connector.
Let's verify the connector handling:
✅ Verification successful
MetaMask connector filtering is correctly implemented
The current filtering logic in walletConnect.tsx is actually correct and doesn't need modification:
- In Argent mobile browser: Only Argent connectors are shown (argentMobile, argentX) - this is the correct behavior as MetaMask shouldn't be available in Argent's browser.
- On desktop: All connectors are shown, including MetaMask
- On mobile (non-Argent browser): All connectors except argentX are shown, which means MetaMask is properly included
The filtering logic aligns with the wallet-specific behaviors and the MetaMask support added in this PR is not impacted by these filters.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if MetaMask connector is properly handled in other wallet-related files
rg -A 5 "MetaMask|metamask" --type ts --type tsx
# Check for any connector filtering logic in other files
ast-grep --pattern 'filter((connector|$_) => $_)'
Length of output: 130
Script:
#!/bin/bash
# Search for MetaMask and metamask patterns with correct file extensions
rg -A 5 "MetaMask|metamask" -t typescript -t tsx
# Search for connector filtering logic in TypeScript files
rg -A 5 "\.filter.*connector" -t typescript -t tsx
# Look for wallet connection related files
fd -e ts -e tsx wallet
Length of output: 312
Script:
#!/bin/bash
# Check contents of wallet-related files
cat components/UI/walletConnect.tsx
cat components/UI/modalWallet.tsx
# Search for MetaMask patterns without file type specification
rg "MetaMask|metamask" --no-heading
# Search for connector filtering logic without file type specification
rg "\.filter.*connector" --no-heading
Length of output: 11944
| @@ -27,7 +27,7 @@ function setResolving(encodedDomain: string, targetAddr: string): Call { | |||
|
|
|||
| function setAsMainDomain(encodedDomain: string): Call { | |||
| const rootDomain = "16434"; // sol encoded | |||
| const calldata = [2, encodedDomain, rootDomain, 0]; // zero is hint argument | |||
| const calldata = ["2", encodedDomain, rootDomain, "0"]; // zero is hint argument | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Extract magic numbers into named constants
The code contains several magic numbers that should be extracted into named constants with clear documentation:
"16434"(sol encoded)"2"(purpose unknown)
Consider refactoring like this:
+ // Constants for Solana domain operations
+ const SOL_ROOT_DOMAIN = "16434"; // sol encoded
+ const OPERATION_TYPE_DOMAIN = "2"; // Add documentation about what "2" represents
+
function setAsMainDomain(encodedDomain: string): Call {
- const rootDomain = "16434"; // sol encoded
- const calldata = ["2", encodedDomain, rootDomain, "0"]; // zero is hint argument
+ const calldata = [OPERATION_TYPE_DOMAIN, encodedDomain, SOL_ROOT_DOMAIN, "0"]; // zero is hint argumentCommittable suggestion skipped: line range outside the PR's diff.
fricoben
added
the
🚧 In progress do not merge
fricoben
added
❌ Change request
irisdv
added
🔥 Ready for review
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (4)
components/domains/externalDomainActions.tsx (1)
39-42: LGTM! Consider adding type annotations for clarity.The conversion of domain length to string is consistent with the standardization of domain data handling across the application. For better type safety and readability, consider adding explicit type annotations:
- const callDataEncodedDomain: string[] = [encodedDomains.length.toString()]; + const callDataEncodedDomain: readonly string[] = [encodedDomains.length.toString()]; encodedDomains.forEach((domain) => { callDataEncodedDomain.push(domain.toString(10)); });components/identities/actions/changeAddressModal.tsx (1)
15-15: LGTM! Consider enhancing type safety.The type change to
string[]aligns with the standardization of domain data handling. Consider making it readonly for additional type safety since the array shouldn't be modified after creation.- callDataEncodedDomain: string[]; + callDataEncodedDomain: readonly string[];components/identities/actions/identityActions.tsx (2)
101-104: LGTM! Consider enhancing type safety.The conversion to string array is consistent with the standardization effort. Consider making the array readonly and using a constant for the domain length check.
- const callDataEncodedDomain: string[] = [encodedDomains.length.toString()]; + const SINGLE_DOMAIN = "1"; + const callDataEncodedDomain: readonly string[] = [encodedDomains.length.toString()]; encodedDomains.forEach((domain) => { callDataEncodedDomain.push(domain.toString(10)); });
Line range hint
250-259: Consider using a constant for domain length comparison.The string literal "1" is used in multiple places for comparison. Using a constant would improve maintainability and reduce the risk of typos.
+ const SINGLE_DOMAIN = "1"; - {callDataEncodedDomain[0] === "1" && !isAutoRenewalEnabled ? ( + {callDataEncodedDomain[0] === SINGLE_DOMAIN && !isAutoRenewalEnabled ? ( // ... rest of the code - {callDataEncodedDomain[0] === "1" ? ( + {callDataEncodedDomain[0] === SINGLE_DOMAIN ? ( // ... rest of the code - {callDataEncodedDomain[0] === "1" && isAutoRenewalEnabled ? ( + {callDataEncodedDomain[0] === SINGLE_DOMAIN && isAutoRenewalEnabled ? (Also applies to: 319-325
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
components/domains/externalDomainActions.tsx(1 hunks)components/identities/actions/changeAddressModal.tsx(1 hunks)components/identities/actions/identityActions.tsx(5 hunks)components/identities/actions/subdomainModal.tsx(3 hunks)utils/callData/identityChangeCalls.ts(4 hunks)utils/callData/resolverCalls.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- utils/callData/resolverCalls.ts
- components/identities/actions/subdomainModal.tsx
- utils/callData/identityChangeCalls.ts
🔇 Additional comments (1)
components/identities/actions/identityActions.tsx (1)
54-54: LGTM! Style consistency improvement.
The change from single to double quotes improves consistency with the rest of the codebase.
Summary by CodeRabbit
New Features
Bug Fixes
callDataEncodedDomainacross multiple components to ensure string consistency.Chores
package.jsonfor compatibility and new features.