feat(spaceward): decouple walletconnect storage #1074
Conversation
📝 WalkthroughWalkthroughThe pull request introduces several changes across multiple files in the project. It adds a new dependency, Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant App
participant WalletConnect
participant StatusSidebar
participant env
User->>App: Initiate Wallet Connection
App->>WalletConnect: Use env.wcWalletRelayUrl
WalletConnect->>env: Fetch WebSocket URL
env-->>WalletConnect: Return WebSocket URL
WalletConnect->>User: Connect to Wallet
User->>StatusSidebar: Check Connection Status
StatusSidebar->>WalletConnect: Query for Status
WalletConnect-->>StatusSidebar: Return Status
StatusSidebar->>User: Display Connection Status
Possibly related PRs
Suggested labels
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
|
Hey @alex-nax and thank you for opening this pull request! 👋🏼 It looks like you forgot to add a changelog entry for your changes. Make sure to add a changelog entry in the 'CHANGELOG.md' file. |
There was a problem hiding this comment.
Actionable comments posted: 10
🧹 Outside diff range and nitpick comments (12)
spaceward/src/utils/custom-kv-storage.ts (2)
9-16: Add TypeScript types and JSDoc comments for better maintainabilityThe JSON serialization utilities work correctly but could benefit from better documentation and type safety.
Consider applying these improvements:
-const JSONStringify = (data: any) => +/** + * Stringifies data with special handling for BigInt values + * @param data The data to stringify + * @returns JSON string with BigInt values preserved as strings with 'n' suffix + */ +const JSONStringify = (data: unknown): string => JSON.stringify(data, (_, value) => typeof value === "bigint" ? value.toString() + "n" : value, ); -export function safeJsonStringify(value: any): string { +/** + * Safely stringifies a value, ensuring the result is always a string + * @param value The value to stringify + * @returns A string representation of the value + */ +export function safeJsonStringify(value: unknown): string { return typeof value === "string" ? value : JSONStringify(value) || ""; }
35-42: Optimize memory usage in getEntries methodThe current implementation loads all entries at once, which could cause memory issues with large datasets.
Consider implementing pagination or streaming for large datasets:
-public async getEntries<T = any>(): Promise<[string, T][]> { +public async getEntries<T = any>( + options?: { limit?: number; offset?: number } +): Promise<[string, T][]> { const entries = await this.indexedDb.getItems( await this.indexedDb.getKeys(), ); + if (options?.limit) { + const start = options.offset || 0; + return entries + .slice(start, start + options.limit) + .map((item: any) => [item.key, item.value] as [string, T]); + } return entries.map( (item: any) => [item.key, item.value] as [string, T], ); }spaceward/src/utils/contract.tsx (4)
28-29: Add null coalescing for more defensive chain ID retrievalThe chain ID retrieval could be more defensive against potential undefined values.
-const chainId = chains?.[0]?.id; +const chainId = chains?.[0]?.id ?? null;
42-44: Improve error message readabilityThe error message could be more user-friendly and structured better.
-throw new Error("Failed to switch chain; if problem persists, visit chainlist to manually add network to your wallet", { +throw new Error("Unable to switch to the required network. You may need to add this network to your wallet first.", { cause: { chainId }, });
Line range hint
108-109: Address TODO comment in error handlingThere's an unimplemented TODO comment in the error cause object.
Would you like me to help implement proper error cause handling for failed transactions? This could include:
- Transaction receipt details
- Gas usage information
- Error codes and messages
Line range hint
115-119: Improve error type handlingThe error type casting could be more specific and type-safe.
-description: - (e as any)?.message ?? "An unexpected error has occured", +description: + (e instanceof Error ? e.message : "An unexpected error has occurred"),spaceward/src/features/modals/ConnectedModal.tsx (1)
Line range hint
89-91: Optimize localStorage access and add error handlingThe component makes multiple direct calls to
localStorage.getItemwith the same key during render, which is inefficient and could cause performance issues. Additionally, there's no error handling for localStorage access.Consider extracting the localStorage access to a custom hook or memoized value:
const getSessionId = useCallback((topic: string) => { try { return localStorage.getItem(`WALLETCONNECT_SESSION_WS_${topic}`) || ""; } catch (error) { console.error('Failed to access localStorage:', error); return ""; } }, []);Then use it in the render:
-localStorage.getItem(`WALLETCONNECT_SESSION_WS_${s.topic}`) || "" +getSessionId(s.topic)Also applies to: 98-100
🧰 Tools
🪛 Biome
[error] 10-10: Don't use '{}' as a type.
Prefer explicitly define the object shape. '{}' means "any non-nullable value".
(lint/complexity/noBannedTypes)
spaceward/src/features/modals/ApproveActionModal.tsx (2)
Line range hint
82-106: Enhance error handling and user feedback.The error handling in the approval flow could be improved to provide better user feedback. Consider:
- Displaying specific error messages instead of silently catching errors
- Allowing cancellation even when approve action is loading
try { const close = await approveRequest({ w, eth, cosm, req, client, }); if (close) { setModal({ type: undefined, params: undefined, }); } -} finally { +} catch (error) { + // Display user-friendly error message + console.error('Transaction approval failed:', error); + // Add error state handling here +} finally { setLoading(false); }🧰 Tools
🪛 Biome
[error] 15-15: Don't use '{}' as a type.
Prefer explicitly define the object shape. '{}' means "any non-nullable value".
(lint/complexity/noBannedTypes)
Line range hint
108-127: Consider UX improvement for cancel action.The cancel button is currently disabled during loading state, which might trap users in a hanging transaction. Consider enabling cancellation even during loading state.
-disabled={!w || loading} +disabled={!w} onClick={() => { - if (!request || !w || loading) { + if (!request || !w) { return; } + setLoading(false); // Cancel ongoing operation if any w.respondSessionRequest({ topic: request.topic, response: {🧰 Tools
🪛 Biome
[error] 15-15: Don't use '{}' as a type.
Prefer explicitly define the object shape. '{}' means "any non-nullable value".
(lint/complexity/noBannedTypes)
spaceward/src/hooks/useWeb3Wallet.ts (3)
Line range hint
156-158: Address TODO comment and improve ping handlingThe TODO comment and basic console.log for session ping should be properly implemented.
Would you like me to help implement proper session ping handling? This should include:
- Proper error handling
- Session state validation
- Reconnection logic if needed
Line range hint
146-152: Review keepalive interval configurationThe 15-second keepalive interval might be too aggressive and could impact performance.
Consider:
- Making the interval configurable
- Implementing exponential backoff
- Adding error handling for failed pings
- }, 15000) as any; + }, process.env.WALLET_CONNECT_KEEPALIVE_INTERVAL || 30000) as any;
Line range hint
188-196: Optimize state update frequency and type safetyThe current implementation has several concerns:
- 1-second interval for state updates is very frequent
- Multiple type assertions (
as any) reduce type safetySuggestions:
- Implement debouncing for state updates
- Replace type assertions with proper type definitions
- Consider using React Query or SWR for better state management
- const t: number = setInterval(() => { + const t: NodeJS.Timeout = setInterval(() => { if (!w) { return; } updateState(); - }, 1000) as any; + }, 5000);
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
⛔ Files ignored due to path filters (1)
spaceward/pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (11)
spaceward/package.json(1 hunks)spaceward/src/App.tsx(3 hunks)spaceward/src/env.ts(2 hunks)spaceward/src/features/actions/StatusSidebar.tsx(2 hunks)spaceward/src/features/modals/ApproveActionModal.tsx(1 hunks)spaceward/src/features/modals/ConnectedModal.tsx(1 hunks)spaceward/src/features/modals/WalletConnect.tsx(2 hunks)spaceward/src/features/walletconnect/WalletConnect.tsx(1 hunks)spaceward/src/hooks/useWeb3Wallet.ts(2 hunks)spaceward/src/utils/contract.tsx(2 hunks)spaceward/src/utils/custom-kv-storage.ts(1 hunks)
🧰 Additional context used
🪛 Biome
spaceward/src/features/modals/ApproveActionModal.tsx
[error] 15-15: Don't use '{}' as a type.
Prefer explicitly define the object shape. '{}' means "any non-nullable value".
(lint/complexity/noBannedTypes)
spaceward/src/features/modals/ConnectedModal.tsx
[error] 10-10: Don't use '{}' as a type.
Prefer explicitly define the object shape. '{}' means "any non-nullable value".
(lint/complexity/noBannedTypes)
🔇 Additional comments (17)
spaceward/src/env.ts (3)
28-30: LGTM! Type annotation improves type safety
The addition of the union type annotation for networkVisibility enhances type safety and clearly documents the allowed values.
56-56: LGTM! Proper integration into env object
The wcWalletRelayUrl is correctly exported as part of the env object, maintaining consistency with other environment configurations.
32-34: Verify security implications of using WalletConnect relay
While using WalletConnect's official relay is common practice, please ensure:
- The connection is always over WSS (secure WebSocket)
- Sensitive data is properly encrypted before transmission
- The relay server is trusted and reliable for production use
Let's verify the usage of this relay URL:
✅ Verification successful
WalletConnect implementation follows security best practices
Based on the code analysis, the WalletConnect implementation is secure:
- Uses WSS (secure WebSocket) by default with
wss://relay.walletconnect.org - Implements proper encryption through WalletConnect's Core client with secure storage
- Uses standard session management with proper request/response handling
- Includes secure pairing and authentication flows
- Maintains session keepalive with secure ping mechanism
The implementation follows WalletConnect v2 protocol specifications and security standards. No security issues were found.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for places where the relay URL is used to ensure secure usage
rg -l "wcWalletRelayUrl" | xargs rg -B 3 -A 3 "wcWalletRelayUrl"
# Check if there's any non-WSS usage of WalletConnect relay
rg -i "ws://" | grep -i "walletconnect"
Length of output: 3524
Script:
#!/bin/bash
# Let's check the implementation of useWeb3Wallet to verify secure usage
ast-grep --pattern 'function useWeb3Wallet($_) {
$$$
}'
# Let's also check for any encryption or security-related code around WalletConnect
rg -A 5 -B 5 "WalletConnect" --type typescript
Length of output: 10701
spaceward/src/utils/contract.tsx (1)
Line range hint 1-124: Verify the impact of error handling changes
Let's verify that these error handling changes are consistently implemented across other contract interaction points.
✅ Verification successful
Error handling changes are consistently implemented
The verification shows that:
- The
assertChainfunction is properly imported and used across all contract interaction points in the codebase (spaces, actions, and intents features) - The
handleContractWriteutility is consistently used withwriteContractAsyncacross different components - All contract interactions follow the same error handling pattern through these utility functions
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other contract write operations and chain assertions
# to ensure consistent error handling
echo "Checking for other contract write implementations..."
rg -l "writeContract" --type ts --type tsx
echo "Checking for chain assertion usage..."
rg -l "assertChain" --type ts --type tsx
echo "Checking for similar toast notifications..."
rg -l "toast\({.*duration:\s*999999" --type ts --type tsx
Length of output: 615
Script:
#!/bin/bash
# Search for other contract write operations and chain assertions
# to ensure consistent error handling
echo "Checking for other contract write implementations..."
rg "writeContract" -t typescript -t tsx
echo "Checking for chain assertion usage..."
rg "assertChain" -t typescript -t tsx
echo "Checking for similar toast notifications..."
rg "toast.*duration.*999999" -t typescript -t tsx
Length of output: 531
Script:
#!/bin/bash
# Search for other contract write operations and chain assertions
# using basic file extensions since typescript type is not recognized
echo "Checking for other contract write implementations..."
rg "writeContract" -g "*.ts" -g "*.tsx"
echo "Checking for chain assertion usage..."
rg "assertChain" -g "*.ts" -g "*.tsx"
echo "Checking for similar toast notifications..."
rg "toast.*duration.*999999" -g "*.ts" -g "*.tsx"
Length of output: 3188
spaceward/src/features/modals/WalletConnect.tsx (1)
11-11: Good improvement: Replaced hardcoded WebSocket URL with environment variable.
The change improves configurability and security by moving the WalletConnect relay URL to environment configuration.
Let's verify the environment variable setup:
Also applies to: 24-24
✅ Verification successful
Let me check the environment variable definition to ensure it's properly set up.
Environment variable setup is properly configured and consistently used
The wcWalletRelayUrl environment variable:
- Is correctly defined in
env.tswith a sensible default value - Uses the standard
VITE_WC_WALLET_RELAY_URLenvironment variable - Is consistently used across 6 different components through the
envimport
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the environment variable definition and usage
# Expected: Find the wcWalletRelayUrl definition in env files and its usage
# Check env files for the variable definition
echo "Checking env files for wcWalletRelayUrl definition:"
rg -l "wcWalletRelayUrl" --type ts
# Check for other usages to ensure consistency
echo "Checking for all usages of wcWalletRelayUrl:"
rg "wcWalletRelayUrl" --type ts
Length of output: 1192
Script:
#!/bin/bash
# Check the environment variable definition in env.ts
cat spaceward/src/env.ts
Length of output: 2182
spaceward/src/features/modals/ConnectedModal.tsx (1)
11-11: Verify environment variable handling
The WebSocket URL is now properly sourced from environment configuration. However, we should ensure proper error handling if the environment variable is missing.
spaceward/src/features/modals/ApproveActionModal.tsx (1)
13-13: LGTM! Environment variable usage improves configurability.
The change from hardcoded WebSocket URL to environment variable is a good practice for configuration management.
Let's verify the environment variable validation:
Also applies to: 17-17
✅ Verification successful
Environment variable validation looks good
The environment variable VITE_WC_WALLET_RELAY_URL has a sensible default fallback to wss://relay.walletconnect.org if not set, which is the standard WalletConnect relay URL. The usage is consistent across all components that need WebSocket connectivity.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if wcWalletRelayUrl is properly validated in env.ts
rg -A 5 "wcWalletRelayUrl" --type ts
Length of output: 2883
spaceward/src/features/walletconnect/WalletConnect.tsx (1)
10-10: Verify environment configuration for WalletConnect relay URL
The change from hardcoded WebSocket URL to environment variable is good for configuration management. However, we should ensure proper fallbacks are in place.
Also applies to: 14-14
✅ Verification successful
Environment variable configuration for WalletConnect relay URL is properly configured with fallback
The codebase shows proper implementation of the WalletConnect relay URL configuration:
- The environment variable is defined in
src/env.tswith a default fallback towss://relay.walletconnect.org - The configuration is consistently used across multiple components through
env.wcWalletRelayUrl - Usage found in:
- WalletConnect.tsx
- ConnectedModal.tsx
- ApproveActionModal.tsx
- StatusSidebar.tsx
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify environment variable configuration and usage
# Check for environment variable definition and potential fallbacks
# Look for env.ts or similar files defining wcWalletRelayUrl
fd -e ts -e js env$ -x cat {} | grep -A 5 wcWalletRelayUrl
# Check for usage of this environment variable across the codebase
rg "wcWalletRelayUrl" -A 2
Length of output: 1592
spaceward/src/hooks/useWeb3Wallet.ts (2)
Line range hint 67-77: Security and configuration improvements needed
Several concerns in the initialization code:
- Hardcoded WalletConnect project ID should be moved to environment configuration
- Mixing localStorage with IndexedDB storage seems inconsistent
Suggested improvements:
- projectId: "4fda584de3c28e97dfa5847023e337c8",
+ projectId: process.env.WALLET_CONNECT_PROJECT_ID,Let's check for other instances of hardcoded configuration:
#!/bin/bash
# Search for hardcoded project IDs
rg "projectId: ['\"][\w\d]+['\"]"
# Check environment variable usage
rg "process\.env\.WALLET_CONNECT"19-19: Verify custom storage implementation compatibility
The switch to a custom IndexedDB storage implementation requires verification of WalletConnect's storage interface requirements.
Let's verify the custom storage implementation:
Consider:
- Extracting the storage identifier to a constant or configuration
- Adding a fallback mechanism if IndexedDB is not available
Also applies to: 62-64
✅ Verification successful
Custom storage implementation is correctly implemented
The IndexedDb class properly implements all required WalletConnect IKeyValueStorage interface methods:
- getKeys()
- getEntries()
- getItem()
- setItem()
- removeItem()
The implementation uses the battle-tested unstorage library with IndexedDB driver, and includes proper handling of JSON serialization, including BigInt values. The storage identifier concern is not critical as it's used consistently throughout the application.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if the custom storage implements all required WalletConnect storage methods
ast-grep --pattern 'class $_ implements IKeyValueStorage {
$$$
}'
# Search for WalletConnect storage interface usage examples
rg -A 5 "IKeyValueStorage"
Length of output: 418
Script:
#!/bin/bash
# Let's check the custom storage implementation
rg -A 10 "class IndexedDb"
# Check the storage interface implementation details
fd "custom-kv-storage" --exec cat {}
# Look for any storage-related interfaces or types
rg -A 5 "interface.*Storage"
Length of output: 7512
spaceward/package.json (3)
125-125: Check for potential conflicts with localforage
The project includes both unstorage and localforage for storage functionality. While they can coexist, we should verify there's no redundancy or conflicts in their usage.
125-125: Verify web3wallet compatibility
Since this is for "custom kvStorage for web3wallet", let's verify compatibility with the WalletConnect packages.
✅ Verification successful
Let me gather more information about the custom storage implementation and its usage.
Custom storage implementation is compatible with WalletConnect
The implementation is verified to be compatible because:
- The custom storage class correctly implements
IKeyValueStorageinterface from@walletconnect/types - The code is based on WalletConnect's official implementation
- The project uses compatible versions:
@walletconnect/web3wallet: 1.10.0@walletconnect/core: 2.11.0unstorage: 1.12.0 (used for IndexedDB implementation)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check compatibility between unstorage and WalletConnect packages
# Get WalletConnect core version requirements
echo "=== WalletConnect Dependencies ==="
curl -s https://registry.npmjs.org/@walletconnect/web3wallet/1.10.0 | jq '.dependencies'
# Look for wallet storage implementation
echo -e "\n=== Wallet Storage Implementation ==="
rg -n "class.*Storage|interface.*Storage"
Length of output: 1836
Script:
#!/bin/bash
# Check custom storage implementation details and usage
echo "=== Custom Storage Implementation ==="
cat spaceward/src/utils/custom-kv-storage.ts
echo -e "\n=== Storage Usage ==="
rg -n "IndexedDb|IKeyValueStorage" -A 5
echo -e "\n=== Package Dependencies ==="
cat spaceward/package.json | jq '.dependencies'
Length of output: 7354
125-125: Verify unstorage version and peer dependencies
The added unstorage package is pinned to version 1.12.0. Let's verify the version and dependencies.
✅ Verification successful
Consider upgrading unstorage to latest version 1.13.1
The current version 1.12.0 is slightly outdated compared to the latest version 1.13.1 released on November 1st, 2024. While no security vulnerabilities were found, it's recommended to use the latest stable version for potential bug fixes and improvements.
Let me verify if any of the peer dependencies are actually used in the project:
Let me check the actual storage driver configuration to understand the usage:
Version 1.12.0 of unstorage is appropriate for this usage
The implementation only uses the IndexedDB driver from unstorage with basic key-value operations. No peer dependencies are required as the code only imports the core package and IndexedDB driver. The current version (1.12.0) is stable and compatible with the implementation's needs.
While there is a newer version (1.13.1), upgrading is optional since:
- No security vulnerabilities were found
- The code uses basic functionality that's stable across versions
- The implementation is based on WalletConnect's reference code
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check latest version, release date, and peer dependencies of unstorage
# Check NPM for latest versions and peer dependencies
curl -s https://registry.npmjs.org/unstorage | jq '{
"latest_version": .["dist-tags"].latest,
"current_version_date": .versions["1.12.0"].time,
"latest_version_date": .time.modified,
"peer_dependencies": .versions["1.12.0"].peerDependencies
}'
# Check for potential security advisories
gh api graphql -f query='
{
securityVulnerabilities(first: 5, ecosystem: NPM, package: "unstorage") {
nodes {
advisory {
summary
severity
publishedAt
}
vulnerableVersionRange
firstPatchedVersion {
identifier
}
}
}
}'
Length of output: 1202
Script:
#!/bin/bash
# Check for usage of peer dependencies in the codebase
echo "Searching for imports of peer dependencies..."
rg -l "import.*from.*(?:ioredis|@vercel/kv|idb-keyval|@azure|@netlify|@upstash|@planetscale|@capacitor)" --type ts --type js
echo -e "\nChecking storage driver configuration..."
rg -l "unstorage.*driver" --type ts --type js
Length of output: 413
Script:
#!/bin/bash
# Check the storage driver implementation
cat spaceward/src/utils/custom-kv-storage.ts
Length of output: 1705
spaceward/src/App.tsx (2)
196-279: LGTM! Well-structured routing implementation
The routing structure is well-organized with clear path definitions and proper component mapping. Each route is properly nested under the Root component, and the paths are logically grouped.
186-188: Verify the impact of empty wallets array
The wallets prop is now set to an empty array, which might affect the wallet functionality. Please ensure this change aligns with the intended behavior and doesn't break existing wallet integrations.
spaceward/src/features/actions/StatusSidebar.tsx (2)
63-63: LGTM: Environment-based configuration improves flexibility
Moving from a hardcoded WebSocket URL to an environment variable improves configurability and follows best practices for deployment across different environments.
291-291: LGTM: Improved variable declaration style
The destructured array assignment with type annotations is more concise while maintaining type safety.
| import { createStorage } from "unstorage"; | ||
| import indexedDbDriver from "unstorage/drivers/indexedb"; | ||
| import { ICore } from "@walletconnect/types"; |
There was a problem hiding this comment.
Fix typo in import path: 'indexedb' should be 'indexeddb'
There's a typo in the import path that could cause module resolution issues.
Apply this fix:
-import indexedDbDriver from "unstorage/drivers/indexedb";
+import indexedDbDriver from "unstorage/drivers/indexeddb";📝 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.
| import { createStorage } from "unstorage"; | |
| import indexedDbDriver from "unstorage/drivers/indexedb"; | |
| import { ICore } from "@walletconnect/types"; | |
| import { createStorage } from "unstorage"; | |
| import indexedDbDriver from "unstorage/drivers/indexeddb"; | |
| import { ICore } from "@walletconnect/types"; |
| public async setItem<T = any>(key: string, value: T): Promise<void> { | ||
| await this.indexedDb.setItem(key, safeJsonStringify(value)); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling for setItem method
The method should handle storage errors and validate input.
Add error handling and input validation:
public async setItem<T = any>(key: string, value: T): Promise<void> {
+ if (!key) {
+ throw new Error('Key cannot be empty');
+ }
+ try {
await this.indexedDb.setItem(key, safeJsonStringify(value));
+ } catch (error) {
+ console.error(`Failed to store item for key ${key}:`, error);
+ throw new Error(`Storage operation failed: ${error.message}`);
+ }
}📝 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.
| public async setItem<T = any>(key: string, value: T): Promise<void> { | |
| await this.indexedDb.setItem(key, safeJsonStringify(value)); | |
| } | |
| public async setItem<T = any>(key: string, value: T): Promise<void> { | |
| if (!key) { | |
| throw new Error('Key cannot be empty'); | |
| } | |
| try { | |
| await this.indexedDb.setItem(key, safeJsonStringify(value)); | |
| } catch (error) { | |
| console.error(`Failed to store item for key ${key}:`, error); | |
| throw new Error(`Storage operation failed: ${error.message}`); | |
| } | |
| } |
| export class IndexedDb implements IKeyValueStorage { | ||
| private indexedDb; | ||
| constructor(name: string) { | ||
| this.indexedDb = createStorage({ | ||
| driver: indexedDbDriver({ | ||
| dbName: name, | ||
| storeName: DB_STORE_NAME, | ||
| }), | ||
| }); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling for storage initialization
The constructor should handle potential initialization failures of IndexedDB.
Consider adding error handling:
constructor(name: string) {
+ if (!window.indexedDB) {
+ throw new Error('IndexedDB is not supported in this environment');
+ }
this.indexedDb = createStorage({
driver: indexedDbDriver({
dbName: name,
storeName: DB_STORE_NAME,
}),
});
}📝 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.
| export class IndexedDb implements IKeyValueStorage { | |
| private indexedDb; | |
| constructor(name: string) { | |
| this.indexedDb = createStorage({ | |
| driver: indexedDbDriver({ | |
| dbName: name, | |
| storeName: DB_STORE_NAME, | |
| }), | |
| }); | |
| } | |
| export class IndexedDb implements IKeyValueStorage { | |
| private indexedDb; | |
| constructor(name: string) { | |
| if (!window.indexedDB) { | |
| throw new Error('IndexedDB is not supported in this environment'); | |
| } | |
| this.indexedDb = createStorage({ | |
| driver: indexedDbDriver({ | |
| dbName: name, | |
| storeName: DB_STORE_NAME, | |
| }), | |
| }); | |
| } |
| public async getItem<T = any>(key: string): Promise<T | undefined> { | ||
| const item = await this.indexedDb.getItem(key); | ||
| if (item === null) { | ||
| return undefined; | ||
| } | ||
| return item as T; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve type safety in getItem method
The current implementation uses unsafe type casting.
Consider adding type validation:
public async getItem<T = any>(key: string): Promise<T | undefined> {
const item = await this.indexedDb.getItem(key);
if (item === null) {
return undefined;
}
+ try {
+ return typeof item === 'string' ? JSON.parse(item) : item as T;
+ } catch (error) {
+ console.error(`Failed to parse stored item for key ${key}:`, error);
+ return undefined;
+ }
- return item as T;
}📝 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.
| public async getItem<T = any>(key: string): Promise<T | undefined> { | |
| const item = await this.indexedDb.getItem(key); | |
| if (item === null) { | |
| return undefined; | |
| } | |
| return item as T; | |
| } | |
| public async getItem<T = any>(key: string): Promise<T | undefined> { | |
| const item = await this.indexedDb.getItem(key); | |
| if (item === null) { | |
| return undefined; | |
| } | |
| try { | |
| return typeof item === 'string' ? JSON.parse(item) : item as T; | |
| } catch (error) { | |
| console.error(`Failed to parse stored item for key ${key}:`, error); | |
| return undefined; | |
| } | |
| } |
| // copied from https://github.com/WalletConnect/walletconnect-utils/blob/master/misc/keyvaluestorage/src/browser/lib/indexedDb.ts | ||
| // with the addition of a custom db name | ||
| import { createStorage } from "unstorage"; | ||
| import indexedDbDriver from "unstorage/drivers/indexedb"; | ||
| import { ICore } from "@walletconnect/types"; | ||
|
|
||
| type IKeyValueStorage = ICore["storage"]; | ||
|
|
||
| const JSONStringify = (data: any) => | ||
| JSON.stringify(data, (_, value) => | ||
| typeof value === "bigint" ? value.toString() + "n" : value, | ||
| ); | ||
|
|
||
| export function safeJsonStringify(value: any): string { | ||
| return typeof value === "string" ? value : JSONStringify(value) || ""; | ||
| } | ||
|
|
||
| const DB_STORE_NAME = "keyvaluestorage"; | ||
|
|
||
| export class IndexedDb implements IKeyValueStorage { | ||
| private indexedDb; | ||
| constructor(name: string) { | ||
| this.indexedDb = createStorage({ | ||
| driver: indexedDbDriver({ | ||
| dbName: name, | ||
| storeName: DB_STORE_NAME, | ||
| }), | ||
| }); | ||
| } | ||
|
|
||
| public async getKeys(): Promise<string[]> { | ||
| return this.indexedDb.getKeys(); | ||
| } | ||
|
|
||
| public async getEntries<T = any>(): Promise<[string, T][]> { | ||
| const entries = await this.indexedDb.getItems( | ||
| await this.indexedDb.getKeys(), | ||
| ); | ||
| return entries.map( | ||
| (item: any) => [item.key, item.value] as [string, T], | ||
| ); | ||
| } | ||
|
|
||
| public async getItem<T = any>(key: string): Promise<T | undefined> { | ||
| const item = await this.indexedDb.getItem(key); | ||
| if (item === null) { | ||
| return undefined; | ||
| } | ||
| return item as T; | ||
| } | ||
|
|
||
| public async setItem<T = any>(key: string, value: T): Promise<void> { | ||
| await this.indexedDb.setItem(key, safeJsonStringify(value)); | ||
| } | ||
|
|
||
| public async removeItem(key: string): Promise<void> { | ||
| await this.indexedDb.removeItem(key); | ||
| } | ||
| } |
There was a problem hiding this comment.
💡 Codebase verification
Sensitive data stored in IndexedDB without encryption poses security risks
The current implementation stores data in IndexedDB without encryption:
- Data is only stringified using
safeJsonStringifybefore storage - No encryption/decryption layer is implemented in the
IndexedDbclass - Raw access to stored data is possible from any JavaScript code running on the same origin
Recommended security improvements:
- Implement client-side encryption before storing sensitive data
- Add a method to clear storage on session end
- Consider using the Web Crypto API for encryption operations
🔗 Analysis chain
Verify security implications of storing wallet data in IndexedDB
While IndexedDB is persistent, it's accessible to any JavaScript code running on the same origin. Consider the following security aspects:
- Sensitive data encryption before storage
- Clear storage on logout/session end
- Access control mechanisms
Let's check for any sensitive data handling in the codebase:
Let me verify the encryption implementation by checking for any encryption-related code in the IndexedDb class.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for wallet-related sensitive data storage
rg -i "wallet.*\.(key|secret|private)"
# Check for encryption usage
rg -i "encrypt|cipher|crypto"
# Look for secure storage patterns
ast-grep --pattern 'class $_ implements IKeyValueStorage {
$$$
encrypt($$$) {
$$$
}
$$$
}'
Length of output: 102148
Script:
#!/bin/bash
# Search for encryption-related code in the IndexedDb class implementation
ast-grep --pattern 'class IndexedDb {
$$$
encrypt($$$) {
$$$
}
$$$
}'
# Check for any encryption usage in the storage implementation
rg -A 5 "safeJsonStringify|setItem" spaceward/src/utils/custom-kv-storage.ts
# Check if there are any other storage implementations that handle encryption
rg -l "implements IKeyValueStorage"
Length of output: 653
| toastResult = toast({ | ||
| title: "Error", | ||
| description: | ||
| (e as any)?.message ?? "An unexpected error has occured", | ||
| action: <a href={`https://chainlist.org/chain/${id}`} target="_blank" onClick={() => toastResult.dismiss()}>ChainList</a>, | ||
| duration: 999999, | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Adjust toast duration and enhance error presentation
The toast duration is set very high (999999ms ≈ 16.7 minutes) and the error presentation could be improved.
toastResult = toast({
title: "Error",
description:
(e as any)?.message ?? "An unexpected error has occured",
action: <a href={`https://chainlist.org/chain/${id}`} target="_blank" onClick={() => toastResult.dismiss()}>ChainList</a>,
- duration: 999999,
+ duration: 10000, // 10 seconds is more reasonable
});📝 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.
| toastResult = toast({ | |
| title: "Error", | |
| description: | |
| (e as any)?.message ?? "An unexpected error has occured", | |
| action: <a href={`https://chainlist.org/chain/${id}`} target="_blank" onClick={() => toastResult.dismiss()}>ChainList</a>, | |
| duration: 999999, | |
| }); | |
| toastResult = toast({ | |
| title: "Error", | |
| description: | |
| (e as any)?.message ?? "An unexpected error has occured", | |
| action: <a href={`https://chainlist.org/chain/${id}`} target="_blank" onClick={() => toastResult.dismiss()}>ChainList</a>, | |
| duration: 10000, // 10 seconds is more reasonable | |
| }); |
| @@ -5,11 +5,10 @@ import { SessionTypes } from "@walletconnect/types"; | |||
| import { useModalState } from "./state"; | |||
| import { ModalParams } from "./types"; | |||
| import AddressAvatar from "@/components/AddressAvatar"; | |||
| import { env } from "@/env"; | |||
|
|
|||
| export default function ConnectedModal(props: ModalParams<{}>) { | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Replace empty object type with proper interface definition
The empty object type {} should be replaced with a proper interface definition for better type safety and code clarity.
-export default function ConnectedModal(props: ModalParams<{}>) {
+interface ConnectedModalProps {}
+export default function ConnectedModal(props: ModalParams<ConnectedModalProps>) {Committable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 Biome
[error] 10-10: Don't use '{}' as a type.
Prefer explicitly define the object shape. '{}' means "any non-nullable value".
(lint/complexity/noBannedTypes)
| @@ -10,10 +10,11 @@ import { ModalParams } from "./types"; | |||
| import { useModalState } from "./state"; | |||
| import clsx from "clsx"; | |||
| import { usePublicClient } from "wagmi"; | |||
| import { env } from "@/env"; | |||
|
|
|||
| export default function ApproveModal({ hidden }: ModalParams<{}>) { | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Define explicit type for modal parameters.
Using {} as a type is discouraged as it means "any non-nullable value". Consider defining an explicit interface for the modal parameters.
-export default function ApproveModal({ hidden }: ModalParams<{}>) {
+interface ApproveModalParams {
+ // Add specific modal parameters here
+}
+export default function ApproveModal({ hidden }: ModalParams<ApproveModalParams>) {Committable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 Biome
[error] 15-15: Don't use '{}' as a type.
Prefer explicitly define the object shape. '{}' means "any non-nullable value".
(lint/complexity/noBannedTypes)
| "wss://relay.walletconnect.org", | ||
| ); | ||
|
|
||
| const { sessionRequests, activeSessions, w } = useWeb3Wallet(env.wcWalletRelayUrl); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Remove unused destructured variable 'w'
The destructured variable 'w' from useWeb3Wallet is not used anywhere in the component.
-const { sessionRequests, activeSessions, w } = useWeb3Wallet(env.wcWalletRelayUrl);
+const { sessionRequests, activeSessions } = useWeb3Wallet(env.wcWalletRelayUrl);📝 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 { sessionRequests, activeSessions, w } = useWeb3Wallet(env.wcWalletRelayUrl); | |
| const { sessionRequests, activeSessions } = useWeb3Wallet(env.wcWalletRelayUrl); |
| const injectedWallet = injectedModule(); | ||
|
|
||
| const walletConnectWallet = walletConnect({ | ||
| projectId, | ||
| dappUrl: "https://wardenprotocol.org/", | ||
| }); |
There was a problem hiding this comment.
Move sensitive configuration to environment variables
The wallet configuration contains hardcoded values that should be managed through environment variables for better security and configurability.
Apply this diff to use environment variables:
const injectedWallet = injectedModule();
const walletConnectWallet = walletConnect({
- projectId,
- dappUrl: "https://wardenprotocol.org/",
+ projectId: env.walletConnectProjectId,
+ dappUrl: env.dappUrl,
});Then add these variables to your environment configuration:
// env.ts
export interface Env {
walletConnectProjectId: string;
dappUrl: string;
// ... existing env vars
}…n-protocol/wardenprotocol into feature/automated-orders-scheduler * 'feature/automated-orders-scheduler' of github.com:warden-protocol/wardenprotocol: fix walletconnect and snap handlers (#1055) custom kvStorage for web3wallet; add suggestion to go to chainlink to select chain (#1074) Migrate to foundry (#1040)
custom kvStorage for web3wallet; add suggestion to go to chainlink to select chain
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Documentation
These changes aim to improve user experience and system flexibility.