WalletConnect #15803
WalletConnect #15803
Conversation
|
🚀 Expo preview is ready!
|
|
👍 Dependency issues cleared. Learn more about Socket for GitHub ↗︎ This PR previously contained dependency changes with security issues that have been resolved, removed, or ignored. Ignoring: Next stepsTake a deeper look at the dependencyTake a moment to review the security alert above. Review the linked package source code to understand the potential risk. Ensure the package is not malicious before proceeding. If you're unsure how to proceed, reach out to your security team or ask the Socket team for help at support [AT] socket [DOT] dev. Remove the packageIf you happen to install a dependency that Socket reports as Known Malware you should immediately remove it and select a different dependency. For other alert types, you may may wish to investigate alternative packages or consider if there are other ways to mitigate the specific risk posed by the dependency. Mark a package as acceptable riskTo ignore an alert, reply with a comment starting with |
martykan
force-pushed
the
feat/walletconnect
branch
from
1eb19e8 to
915e87a
Compare
martykan
force-pushed
the
feat/walletconnect
branch
from
915e87a to
bb35715
Compare
martykan
force-pushed
the
feat/walletconnect
branch
from
bb35715 to
59e12ce
Compare
martykan
force-pushed
the
feat/walletconnect
branch
6 times, most recently
from
e8a05a2 to
f42e19a
Compare
martykan
force-pushed
the
feat/walletconnect
branch
3 times, most recently
from
ee37e70 to
f25b23e
Compare
WalkthroughThis pull request implements extensive modifications across several packages to integrate and enhance WalletConnect functionality. Notable changes include the addition of new properties and types for enhanced type safety in components, particularly within notes and protocol actions. The changes introduce thunks for managing WalletConnect sessions and proposals, along with new actions and middleware to handle session authentication, account switching, and connection requests. Multiple UI components and modal views are updated or added to support wallet connection proposals and initialization. Additionally, the configuration files have been updated to include new allowed domains, dependency declarations, TypeScript references, and deep link constants, ensuring proper integration of the WalletConnect feature across the application. Suggested labels
Suggested reviewers
Tip 🌐 Web search-backed reviews and chat
✨ Finishing Touches
🪧 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 (
|
martykan
force-pushed
the
feat/walletconnect
branch
2 times, most recently
from
d9cc7a8 to
c1e7473
Compare
|
@SocketSecurity ignore-all |
martykan
requested review from
MiroslavProchazka,
marekrjpolak,
a team and
Nodonisko
as code owners
martykan
added
connect
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 13
🧹 Nitpick comments (18)
packages/components/src/components/Note/Note.tsx (1)
19-32: Consider adding documentation for variants.The implementation looks good with consistent variant application and proper default values. However, it would be helpful to add JSDoc comments explaining:
- Available variant options
- Visual effects of each variant
- The relationship between Icon and Paragraph variants
Example documentation:
+/** + * A component that displays a note with an icon and text. + * @param {IconVariant} variant - Controls the visual style of both icon and text. + * Available options: 'primary', 'secondary', 'tertiary'. + * Defaults to 'tertiary'. + */ export const Note = ({packages/suite-walletconnect/src/adapters/ethereum.ts (2)
19-32: ValidatechainIdmore robustly and clarify error messages.
Currently, thegetAccountfunction throws a generic "Account not found" error if the chain does not match. Consider validatingchainIdexplicitly and providing a clearer error message to help users debug issues.
85-93: Relying on a hard-coded gas limit may cause transaction failures for certain interactions.
Whengasis missing, the code defaults to'21000'. This might be insufficient for contract interactions or more complex transfers. Consider using an on-chain gas estimation service for a more accurate result if feasible.packages/suite-walletconnect/src/walletConnectThunks.ts (3)
84-116: Expose device error details.
When the adapter operation or the device call fails, we throw a generic'Device request failed'. Including the actual error message or error code would aid in diagnosing root causes during session requests.
171-196: Extend event data beyond just chain ID and accounts if needed.
For chain switching or account switching, consider whether additional network or account details (e.g., chain name, network status) might benefit the connected sessions.
280-284: Log pairing errors with contextual details.
Pairing can fail for multiple reasons (invalid URI, network issues, etc.). Providing a more descriptive console log or user-facing error message can help with debugging.packages/suite-walletconnect/src/walletConnectConstants.ts (1)
5-10: Consider making URLs configurable.The hardcoded URLs in
WALLETCONNECT_METADATAmight need to change across different environments. Consider making them configurable through environment variables or a configuration file.packages/suite-walletconnect/src/walletConnectMiddleware.ts (1)
26-33: Consider adding timeout for session proposal modal.The session proposal should have a timeout to prevent indefinitely pending proposals.
dispatch( extra.actions.openModal({ type: 'walletconnect-proposal', eventId: action.payload.eventId, + timeout: 300000, // 5 minutes timeout }), );packages/suite-walletconnect/src/walletConnectTypes.ts (2)
6-12: Consider adding version field to WalletConnectAdapter.For future compatibility, adding a version field would help manage adapter updates and migrations.
export interface WalletConnectAdapter { networkType: string; methods: string[]; + version: string; requestThunk: SuiteCompatibleThunk<{ event: WalletKitTypes.SessionRequest; }>; }
42-50: Add validation timestamp to PendingConnectionProposal.Track when the validation status was last updated to handle stale validations.
export interface PendingConnectionProposal { eventId: number; params: ProposalTypes.Struct; origin: string; validation: 'UNKNOWN' | 'VALID' | 'INVALID'; verifyUrl: string; isScam?: boolean; expired: boolean; + validationTimestamp?: number; }packages/suite-walletconnect/src/adapters/index.ts (1)
7-11: Consider using a Map for adapter registry.Using a Map with network types as keys would provide more efficient lookups and better type safety.
-export const adapters: WalletConnectAdapter[] = [ - ethereumAdapter, - // TODO: solanaAdapter - // TODO: bitcoinAdapter -]; +export const adapters = new Map<string, WalletConnectAdapter>([ + ['eip155', ethereumAdapter], + // TODO: Add Solana adapter with 'solana' key + // TODO: Add Bitcoin adapter with 'bip122' key +]);packages/suite/src/components/suite/modals/ReduxModal/UserContextModal/ConnectPopupModal.tsx (1)
6-12: Add data-test attributes for e2e testing.Include data-test attributes to facilitate end-to-end testing of the modal interactions.
interface ConnectPopupModalProps { method: string; processName?: string; origin?: string; onConfirm: () => void; onCancel: () => void; + 'data-test'?: string; }packages/suite-walletconnect/src/walletConnectReducer.ts (2)
24-34: Add validation for duplicate session topics.The
saveSessioncase should validate that the topic is unique before adding a new session. While the code handles updates correctly, it might be better to explicitly prevent duplicate topics..addCase(walletConnectActions.saveSession, (state, { payload }) => { const { topic, ...rest } = payload; const exists = state.sessions.find(session => session.topic === topic); if (exists) { state.sessions = state.sessions.map(session => session.topic === topic ? { ...session, ...rest } : session, ); } else { + // Ensure topic is not empty or invalid + if (!topic?.trim()) { + throw new Error('Invalid session topic'); + } state.sessions.push(payload); } })
39-47: Add error handling for session proposal actions.The proposal-related actions lack error handling for edge cases. Consider adding validation and error handling.
.addCase(walletConnectActions.createSessionProposal, (state, { payload }) => { + if (state.pendingProposal && !state.pendingProposal.expired) { + throw new Error('Another proposal is already pending'); + } state.pendingProposal = payload; }) .addCase(walletConnectActions.clearSessionProposal, state => { state.pendingProposal = undefined; }) .addCase(walletConnectActions.expireSessionProposal, state => { + if (!state.pendingProposal) { + return; + } + if (state.pendingProposal.expired) { + return; + } if (state.pendingProposal) state.pendingProposal.expired = true; });packages/suite/src/views/settings/SettingsDebug/WalletConnect.tsx (1)
48-68: Add loading states and error handling for session management.The session list lacks loading states and error handling for the disconnect action.
Consider adding loading states and error handling:
<Row key={session.topic} gap={spacings.md}> <StatusLight variant="primary" /> <Column flex="1"> <Text>{session.peer.metadata.name}</Text> <Text variant="tertiary">{session.peer.metadata.url}</Text> <Text variant="tertiary">{session.topic}</Text> </Column> <Button + isLoading={session.isDisconnecting} onClick={() => { dispatch( walletConnectDisconnectThunk({ topic: session.topic }), - ); + ).catch(error => { + console.error('Failed to disconnect:', error); + }); }} size="small" variant="destructive" > Disconnect </Button> </Row>packages/suite/src/components/suite/modals/ReduxModal/UserContextModal/WalletConnectProposalModal.tsx (2)
43-45: Add loading state for proposal actions.The confirm button should show a loading state during approval/rejection.
Add loading state:
isDisabled={ - !pendingProposal || pendingProposal.expired || pendingProposal.isScam + !pendingProposal || + pendingProposal.expired || + pendingProposal.isScam || + pendingProposal.isLoading } +isLoading={pendingProposal?.isLoading}
57-71: Add aria labels for accessibility.The validation status notes lack proper accessibility attributes.
Add aria labels:
{!pendingProposal?.isScam && pendingProposal?.validation === 'VALID' && ( - <Note variant="info" iconName="shieldCheckFilled"> + <Note + variant="info" + iconName="shieldCheckFilled" + role="status" + aria-label="Service is verified" + > <Translation id="TR_WALLETCONNECT_SERVICE_VERIFIED" /> </Note> )}packages/suite/src/support/messages.ts (1)
8388-8429: LGTM! The WalletConnect messages cover key user flows.The messages provide good coverage of connection requests, service verification states, and error handling. They are well organized and use clear, user-friendly language.
Consider adding a message for successful WalletConnect connection state, e.g.:
TR_WALLETCONNECT_CONNECTED: { id: 'TR_WALLETCONNECT_CONNECTED', defaultMessage: 'Successfully connected to {serviceName}.', }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (31)
packages/components/src/components/Note/Note.tsx(1 hunks)packages/suite-desktop-connect-popup/src/connectPopupThunks.ts(3 hunks)packages/suite-desktop-core/src/config.ts(1 hunks)packages/suite-walletconnect/package.json(1 hunks)packages/suite-walletconnect/src/adapters/ethereum.ts(1 hunks)packages/suite-walletconnect/src/adapters/index.ts(1 hunks)packages/suite-walletconnect/src/index.ts(1 hunks)packages/suite-walletconnect/src/walletConnectActions.ts(1 hunks)packages/suite-walletconnect/src/walletConnectConstants.ts(1 hunks)packages/suite-walletconnect/src/walletConnectMiddleware.ts(1 hunks)packages/suite-walletconnect/src/walletConnectReducer.ts(1 hunks)packages/suite-walletconnect/src/walletConnectThunks.ts(1 hunks)packages/suite-walletconnect/src/walletConnectTypes.ts(1 hunks)packages/suite-walletconnect/tsconfig.json(1 hunks)packages/suite/jest.config.js(1 hunks)packages/suite/package.json(1 hunks)packages/suite/src/actions/suite/__tests__/initAction.test.ts(4 hunks)packages/suite/src/actions/suite/initAction.ts(2 hunks)packages/suite/src/actions/suite/protocolActions.ts(3 hunks)packages/suite/src/components/suite/modals/ReduxModal/UserContextModal/ConnectPopupModal.tsx(1 hunks)packages/suite/src/components/suite/modals/ReduxModal/UserContextModal/UserContextModal.tsx(2 hunks)packages/suite/src/components/suite/modals/ReduxModal/UserContextModal/WalletConnectProposalModal.tsx(1 hunks)packages/suite/src/middlewares/wallet/index.ts(2 hunks)packages/suite/src/reducers/wallet/index.ts(3 hunks)packages/suite/src/support/messages.ts(1 hunks)packages/suite/src/views/settings/SettingsDebug/SettingsDebug.tsx(2 hunks)packages/suite/src/views/settings/SettingsDebug/WalletConnect.tsx(1 hunks)packages/suite/tsconfig.json(1 hunks)packages/urls/src/deeplinks.ts(1 hunks)scripts/list-outdated-dependencies/connect-dependencies.txt(1 hunks)suite-common/suite-types/src/modal.ts(1 hunks)
✅ Files skipped from review due to trivial changes (4)
- scripts/list-outdated-dependencies/connect-dependencies.txt
- packages/suite-walletconnect/src/index.ts
- packages/suite-walletconnect/tsconfig.json
- packages/suite-walletconnect/package.json
⏰ Context from checks skipped due to timeout of 90000ms (17)
- GitHub Check: Build suite-desktop-mac
- GitHub Check: Build suite-desktop-linux
- GitHub Check: Build suite-desktop-win
- GitHub Check: build-deploy
- GitHub Check: build-deploy
- GitHub Check: transport-e2e-test
- GitHub Check: Setup and Cache Dependencies
- GitHub Check: prepare_android_test_app
- GitHub Check: run-desktop-tests (@group=wallet, trezor-user-env-unix bitcoin-regtest)
- GitHub Check: run-desktop-tests (@group=other, trezor-user-env-unix)
- GitHub Check: run-desktop-tests (@group=settings, trezor-user-env-unix bitcoin-regtest)
- GitHub Check: build-web
- GitHub Check: run-desktop-tests (@group=device-management, trezor-user-env-unix)
- GitHub Check: run-desktop-tests (@group=suite, trezor-user-env-unix)
- GitHub Check: test
- GitHub Check: Analyze with CodeQL (javascript)
- GitHub Check: build-web
🔇 Additional comments (20)
packages/components/src/components/Note/Note.tsx (2)
7-7: LGTM! Clean import addition.The
IconVarianttype import is correctly added alongside related Icon imports.
13-17: LGTM! Type definition is well-structured.The
variantprop is correctly typed as optionalIconVariant, maintaining type safety and consistency.packages/suite-walletconnect/src/adapters/ethereum.ts (2)
34-34: Consider providing a default case in the switch statement.
For unrecognized or unsupported Ethereum RPC methods, it's recommended to handle them gracefully or return a helpful error response.
155-164: Overall adapter structure looks solid.
The exportedethereumAdapterobject is well-defined and neatly encapsulates the supported methods, network type, and request logic.packages/suite-walletconnect/src/walletConnectThunks.ts (1)
23-24: Guard against uninitialized or nullwalletKit.
Access to thewalletKitglobal object presumes successful initialization withinwalletConnectInitThunk. If this thunk is called out of order,walletKitmight be undefined. Adding a guard check or fallback ensures stability.packages/urls/src/deeplinks.ts (1)
2-2: Deeplink addition confirmed.
AddingSUITE_WALLETCONNECT_DEEPLINKis straightforward and consistent with the naming convention used for other deep links.packages/suite-walletconnect/src/walletConnectActions.ts (1)
1-39: LGTM! Well-structured Redux actions.The implementation follows Redux best practices with:
- Clear action namespacing
- Type-safe payloads
- Consistent action naming
packages/suite-desktop-core/src/config.ts (1)
31-31: Verify the security implications of allowing the WalletConnect domain.Please confirm that:
- The domain is strictly required for WalletConnect verification.
- The domain's access is properly restricted by CSP rules.
Run the following script to check CSP rules for the domain:
✅ Verification successful
WalletConnect Domain and CSP Verification
- The domain
verify.walletconnect.orgis required for WalletConnect verification.- The CSP configuration in
packages/suite-desktop-core/src/config.tsuses a permissiveconnect-src *rule. This design choice (as noted by the comment "Allow all API calls (Can't be restricted bc of custom backends)") means that while there are restrictions on default content (default-src 'self'), API calls—including those to WalletConnect—are intentionally allowed from any origin.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check if CSP rules properly restrict the WalletConnect domain. # Test: Search for CSP rules that might affect WalletConnect domain. rg -A 5 $'verify\.walletconnect\.org|connect-src|default-src'Length of output: 11291
packages/suite/src/middlewares/wallet/index.ts (1)
9-9: Verify the middleware order.The WalletConnect middleware is added at the end of the chain. Please confirm that:
- The middleware order is correct (e.g., no dependencies on this middleware from other middleware).
- The middleware doesn't need to run before any existing middleware.
Run the following script to analyze middleware dependencies:
Also applies to: 32-32
✅ Verification successful
Middleware order verified.
The WalletConnect middleware is correctly imported and appended at the end of the middleware chain without any evidence of dependency requirements by preceding middleware. The search results confirm that no other middleware refers to or depends on it, hence the current order is appropriate.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for potential dependencies between middleware. # Test: Search for WalletConnect imports or usage in other middleware. rg -A 5 $'walletConnect|WalletConnect' packages/suite/src/middlewares/wallet/Length of output: 783
packages/suite-walletconnect/src/walletConnectMiddleware.ts (1)
11-17: Verify account switching behavior across chains.The account switching logic needs to handle cross-chain scenarios appropriately.
✅ Verification successful
Account switching correctly forwards to a thunk that handles cross-chain logic.
The middleware simply dispatches the action, and the thunk implementation inwalletConnectThunks.tsincludes chain-specific references (e.g., usingsupportedNamespaces.eip155.chainsand constructingchainIdvalues) to manage cross-chain scenarios.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for cross-chain account switching implementations rg -l "switchSelectedAccount.*chain" --type tsLength of output: 48
Script:
#!/bin/bash # Search for any "chain" references within the walletConnectMiddleware.ts file. echo "Searching for 'chain' references in packages/suite-walletconnect/src/walletConnectMiddleware.ts..." rg -n "chain" --type ts packages/suite-walletconnect/src/walletConnectMiddleware.tsLength of output: 296
Script:
#!/bin/bash # Locate the implementation details for switchSelectedAccountThunk within the codebase. echo "Searching for occurrences of switchSelectedAccountThunk in the repository..." rg -n "switchSelectedAccountThunk" --type tsLength of output: 596
Script:
#!/bin/bash # Search for "chain" references within the switchSelectedAccountThunk implementation. echo "Searching for 'chain' references in packages/suite-walletconnect/src/walletConnectThunks.ts..." rg -n "chain" --type ts packages/suite-walletconnect/src/walletConnectThunks.tsLength of output: 616
packages/suite/src/reducers/wallet/index.ts (1)
13-13: LGTM! Clean integration of WalletConnect reducer.The WalletConnect reducer is properly integrated following the existing patterns:
- Import from the new package
- Instantiation with extraDependencies
- Integration into the combined reducers
Also applies to: 34-34, 54-54
packages/suite/jest.config.js (1)
74-74: LGTM! Necessary Jest configuration updates.The configuration changes properly handle the new WalletConnect dependencies and ESM modules.
Also applies to: 77-77
packages/suite/src/views/settings/SettingsDebug/SettingsDebug.tsx (1)
88-90: LGTM!The WalletConnect section is properly integrated into the debug settings.
packages/suite/src/actions/suite/initAction.ts (1)
14-14: LGTM! WalletConnect initialization is properly integrated.The initialization is placed at a logical point in the sequence, just before the suite is marked as ready.
Also applies to: 122-122
suite-common/suite-types/src/modal.ts (1)
199-202: LGTM! Type definition for WalletConnect proposal is well-structured.The type follows the existing pattern and includes the necessary eventId field.
packages/suite/src/components/suite/modals/ReduxModal/UserContextModal/UserContextModal.tsx (1)
54-54: LGTM! Modal integration is clean and consistent.The WalletConnect proposal modal is properly integrated into the existing modal system.
Also applies to: 224-225
packages/suite/src/actions/suite/__tests__/initAction.test.ts (2)
27-27: LGTM! Test coverage for WalletConnect initialization is comprehensive.The walletConnectInitThunk is properly tested across all relevant initialization scenarios.
Let's verify the test coverage for WalletConnect initialization:
Also applies to: 125-125, 172-172, 217-217
✅ Verification successful
Test Coverage for WalletConnect Initialization is Comprehensive
The results confirm that the only file containing tests for
walletConnectInitThunkispackages/suite/src/actions/suite/__tests__/initAction.test.ts, covering all the intended initialization scenarios.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check test coverage for WalletConnect initialization # Search for WalletConnect related test files fd -e test.ts -e test.tsx walletconnect # Search for WalletConnect initialization tests in other test files rg -l "walletConnectInitThunk" -g "*.test.ts" -g "*.test.tsx"Length of output: 165
1-226: Verify implementation status of WalletConnect integration.Since this is marked as a WIP PR, please ensure:
- All EVM chain methods are properly implemented and tested
- The deeplink scheme
trezorsuite://walletconnectis fully functional- The connection manager UI in the Debug section is working as expected
Let's check the implementation status:
packages/suite/tsconfig.json (1)
111-111: Integrating WalletConnect Package Reference
The new reference entry for WalletConnect ({ "path": "../suite-walletconnect" }) has been added correctly to thereferencesarray. Ensure that the relative path aligns with the repository structure so that the TypeScript compiler can properly resolve types from the new package.packages/suite/package.json (1)
81-81: New WalletConnect Dependency Added
The dependency"@trezor/suite-walletconnect": "workspace:*"has been added in thedependenciessection, which integrates the new WalletConnect package into the suite. Verify that the package is correctly configured and that its version (managed via workspace) meets your requirements.
| value={connectionUrl} | ||
| onChange={e => setConnectionUrl(e.target.value)} | ||
| size="small" | ||
| placeholder="New Connection URL" | ||
| /> |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add input validation for connection URL.
The connection URL input lacks validation, which could lead to invalid connection attempts.
Add validation before dispatching the pair action:
<Input
value={connectionUrl}
onChange={e => setConnectionUrl(e.target.value)}
size="small"
placeholder="New Connection URL"
+ isInvalid={connectionUrl !== '' && !connectionUrl.startsWith('wc:')}
/>📝 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.
| value={connectionUrl} | |
| onChange={e => setConnectionUrl(e.target.value)} | |
| size="small" | |
| placeholder="New Connection URL" | |
| /> | |
| <Input | |
| value={connectionUrl} | |
| onChange={e => setConnectionUrl(e.target.value)} | |
| size="small" | |
| placeholder="New Connection URL" | |
| isInvalid={connectionUrl !== '' && !connectionUrl.startsWith('wc:')} | |
| /> |
| onClick={() => { | ||
| dispatch(walletConnectPairThunk({ uri: connectionUrl })); | ||
| }} |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling for failed connections.
The connection action lacks error handling and feedback to the user.
Add error handling and user feedback:
onClick={() => {
+ if (!connectionUrl.startsWith('wc:')) {
+ return;
+ }
dispatch(walletConnectPairThunk({ uri: connectionUrl }));
+ setConnectionUrl(''); // Clear input after attempt
}}📝 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.
| onClick={() => { | |
| dispatch(walletConnectPairThunk({ uri: connectionUrl })); | |
| }} | |
| onClick={() => { | |
| if (!connectionUrl.startsWith('wc:')) { | |
| return; | |
| } | |
| dispatch(walletConnectPairThunk({ uri: connectionUrl })); | |
| setConnectionUrl(''); // Clear input after attempt | |
| }} |
| desktopApi.on('connect-popup/call', async params => { | ||
| const response = await dispatch(connectPopupCallThunk(params)).unwrap(); | ||
| desktopApi.connectPopupResponse(response); | ||
| }); |
There was a problem hiding this comment.
💡 Codebase verification
Race Condition Issue in the 'connect-popup/call' Handler
- Our search confirmed that the only event handler for
'connect-popup/call'is defined inpackages/suite-desktop-connect-popup/src/connectPopupThunks.tswithout any mechanism to prevent overlapping processing. - There’s no request-tracking logic to handle concurrent calls, which could lead to race conditions if multiple events are triggered simultaneously.
- The proposed refactoring using a
pendingRequestsmap would help mitigate these issues.
🔗 Analysis chain
Handle potential race conditions in event handling.
The event handler doesn't handle potential race conditions from multiple concurrent calls.
Add a verification script to check for potential race conditions:
Consider adding request tracking:
+const pendingRequests = new Map();
+
desktopApi.on('connect-popup/call', async params => {
+ if (pendingRequests.has(params.id)) {
+ console.warn(`Request ${params.id} is already being processed`);
+ return;
+ }
+ pendingRequests.set(params.id, true);
const response = await dispatch(connectPopupCallThunk(params)).unwrap();
desktopApi.connectPopupResponse(response);
+ pendingRequests.delete(params.id);
});🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other event handlers that might conflict
rg -A 5 "desktopApi.on\('connect-popup/call'" --type ts
Length of output: 725
...suite/src/components/suite/modals/ReduxModal/UserContextModal/WalletConnectProposalModal.tsx
Show resolved
Hide resolved
martykan
force-pushed
the
feat/walletconnect
branch
from
c1e7473 to
476b2b8
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
packages/suite-walletconnect/src/adapters/ethereum.ts (1)
60-70:⚠️ Potential issueHandle malformed JSON data gracefully.
The code invokes
JSON.parse(data)without a try-catch block, which could throw a runtime error if the payload is malformed.Apply this diff to add error handling:
- const [address, data] = event.params.request.params; + const [address, rawData] = event.params.request.params; + let parsedData; + try { + parsedData = JSON.parse(rawData); + } catch (parseError) { + throw new Error('Invalid JSON in request params'); + } const account = getAccount(address); const response = await dispatch( trezorConnectPopupActions.connectPopupCallThunk({ id: 0, method: 'ethereumSignTypedData', payload: { path: account.path, - data: JSON.parse(data), + data: parsedData,packages/suite-walletconnect/src/walletConnectThunks.ts (1)
237-240:⚠️ Potential issueHandle WalletKit initialization failures.
If
WalletKit.initfails, the error is not caught or handled properly.- walletKit = await WalletKit.init({ - core, - metadata: WALLETCONNECT_METADATA, - }); + try { + walletKit = await WalletKit.init({ + core, + metadata: WALLETCONNECT_METADATA, + }); + } catch (error) { + console.error('Failed to initialize WalletKit:', error); + throw new Error('Failed to initialize WalletConnect. Please try again.'); + }
🧹 Nitpick comments (3)
packages/suite-walletconnect/src/adapters/ethereum.ts (2)
27-32: Enhance error message for account lookup.The error message "Account not found" could be more descriptive to help with debugging.
- throw new Error('Account not found'); + throw new Error(`Account not found for address ${address}${chainId ? ` and chainId ${chainId}` : ''}`);
130-133: Improve error messages for transaction failures.The current error messages are too generic. Include more context about what failed to help with debugging.
if (!signResponse.success) { console.error('eth_sendTransaction error', signResponse); - throw new Error('eth_sendTransaction error'); + throw new Error(`Failed to sign transaction: ${signResponse.payload.error}`); } const pushResponse = await TrezorConnect.pushTransaction({ coin: account.symbol, identity: getAccountIdentity(account), tx: signResponse.payload.serializedTx, }); if (!pushResponse.success) { console.error('eth_sendTransaction push error', pushResponse); - throw new Error('eth_sendTransaction push error'); + throw new Error(`Failed to broadcast transaction: ${pushResponse.payload.error}`); }Also applies to: 140-143
packages/suite-walletconnect/src/walletConnectThunks.ts (1)
41-44: Enhance error handling for ETH account lookup.The error message could be more descriptive, and the account type should be validated.
const ethAccount = accounts.find(a => a.symbol === 'eth'); if (!ethAccount) { - throw new Error('No ETH account'); + throw new Error('No Ethereum account found. Please add an Ethereum account before authenticating.'); + } + if (ethAccount.networkType !== 'ethereum') { + throw new Error('Invalid account type. Expected Ethereum account.'); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (30)
packages/suite-desktop-connect-popup/src/connectPopupThunks.ts(3 hunks)packages/suite-desktop-core/src/config.ts(1 hunks)packages/suite-walletconnect/package.json(1 hunks)packages/suite-walletconnect/src/adapters/ethereum.ts(1 hunks)packages/suite-walletconnect/src/adapters/index.ts(1 hunks)packages/suite-walletconnect/src/index.ts(1 hunks)packages/suite-walletconnect/src/walletConnectActions.ts(1 hunks)packages/suite-walletconnect/src/walletConnectConstants.ts(1 hunks)packages/suite-walletconnect/src/walletConnectMiddleware.ts(1 hunks)packages/suite-walletconnect/src/walletConnectReducer.ts(1 hunks)packages/suite-walletconnect/src/walletConnectThunks.ts(1 hunks)packages/suite-walletconnect/src/walletConnectTypes.ts(1 hunks)packages/suite-walletconnect/tsconfig.json(1 hunks)packages/suite/jest.config.js(1 hunks)packages/suite/package.json(1 hunks)packages/suite/src/actions/suite/__tests__/initAction.test.ts(4 hunks)packages/suite/src/actions/suite/initAction.ts(2 hunks)packages/suite/src/actions/suite/protocolActions.ts(3 hunks)packages/suite/src/components/suite/modals/ReduxModal/UserContextModal/ConnectPopupModal.tsx(1 hunks)packages/suite/src/components/suite/modals/ReduxModal/UserContextModal/UserContextModal.tsx(2 hunks)packages/suite/src/components/suite/modals/ReduxModal/UserContextModal/WalletConnectProposalModal.tsx(1 hunks)packages/suite/src/middlewares/wallet/index.ts(2 hunks)packages/suite/src/reducers/wallet/index.ts(3 hunks)packages/suite/src/support/messages.ts(1 hunks)packages/suite/src/views/settings/SettingsDebug/SettingsDebug.tsx(2 hunks)packages/suite/src/views/settings/SettingsDebug/WalletConnect.tsx(1 hunks)packages/suite/tsconfig.json(1 hunks)packages/urls/src/deeplinks.ts(1 hunks)scripts/list-outdated-dependencies/connect-dependencies.txt(1 hunks)suite-common/suite-types/src/modal.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (28)
- scripts/list-outdated-dependencies/connect-dependencies.txt
- packages/suite-desktop-core/src/config.ts
- packages/urls/src/deeplinks.ts
- packages/suite-walletconnect/tsconfig.json
- packages/suite/src/middlewares/wallet/index.ts
- packages/suite/src/actions/suite/initAction.ts
- packages/suite/jest.config.js
- packages/suite-walletconnect/src/index.ts
- packages/suite/tsconfig.json
- packages/suite/src/views/settings/SettingsDebug/SettingsDebug.tsx
- suite-common/suite-types/src/modal.ts
- packages/suite/src/reducers/wallet/index.ts
- packages/suite/package.json
- packages/suite/src/components/suite/modals/ReduxModal/UserContextModal/ConnectPopupModal.tsx
- packages/suite/src/views/settings/SettingsDebug/WalletConnect.tsx
- packages/suite-walletconnect/src/walletConnectMiddleware.ts
- packages/suite/src/components/suite/modals/ReduxModal/UserContextModal/UserContextModal.tsx
- packages/suite-walletconnect/src/walletConnectActions.ts
- packages/suite-walletconnect/package.json
- packages/suite/src/components/suite/modals/ReduxModal/UserContextModal/WalletConnectProposalModal.tsx
- packages/suite-walletconnect/src/walletConnectTypes.ts
- packages/suite/src/actions/suite/protocolActions.ts
- packages/suite-walletconnect/src/adapters/index.ts
- packages/suite-walletconnect/src/walletConnectReducer.ts
- packages/suite-walletconnect/src/walletConnectConstants.ts
- packages/suite-desktop-connect-popup/src/connectPopupThunks.ts
- packages/suite/src/actions/suite/tests/initAction.test.ts
- packages/suite/src/support/messages.ts
⏰ Context from checks skipped due to timeout of 90000ms (18)
- GitHub Check: PR-check / node-authorizeCoinjoin cancelCoinjoinAuthorization passphrase unlockPath setBusy checkFirmwareAuthenticity keepSession cancel.test info.test resetDevice-api
- GitHub Check: PR-check / node-override init-api-flaky
- GitHub Check: PR-check / web-override init-api-flaky
- GitHub Check: PR-check / web-authorizeCoinjoin cancelCoinjoinAuthorization passphrase unlockPath setBusy checkFirmwareAuthenticity keepSession cancel.test info.test resetDevice-api
- GitHub Check: build-deploy
- GitHub Check: build-deploy
- GitHub Check: run-desktop-tests (@group=wallet, trezor-user-env-unix bitcoin-regtest)
- GitHub Check: run-desktop-tests (@group=settings, trezor-user-env-unix bitcoin-regtest)
- GitHub Check: EAS Update
- GitHub Check: run-desktop-tests (@group=device-management, trezor-user-env-unix)
- GitHub Check: transport-e2e-test
- GitHub Check: test
- GitHub Check: run-desktop-tests (@group=suite, trezor-user-env-unix)
- GitHub Check: build-web
- GitHub Check: build-web
- GitHub Check: Analyze with CodeQL (javascript)
- GitHub Check: prepare_android_test_app
- GitHub Check: Socket Security: Pull Request Alerts
🔇 Additional comments (2)
packages/suite-walletconnect/src/adapters/ethereum.ts (1)
155-164: LGTM!The adapter export is well-structured and type-safe.
packages/suite-walletconnect/src/walletConnectThunks.ts (1)
206-228: LGTM!The session update implementation correctly handles account modifications and namespace updates.
martykan
force-pushed
the
feat/walletconnect
branch
from
8d2bdce to
45ef5b0
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (1)
packages/suite/src/components/suite/modals/ReduxModal/UserContextModal/WalletConnectProposalModal.tsx (1)
21-28:⚠️ Potential issueAdd error handling for failed proposal actions.
Both accept and reject handlers need proper error handling to manage potential failures.
Apply this diff to both handlers:
const handleAccept = () => { - dispatch(sessionProposalApproveThunk({ eventId })); - dispatch(onCancel()); + dispatch(sessionProposalApproveThunk({ eventId })) + .unwrap() + .then(() => dispatch(onCancel())) + .catch(error => { + console.error('Failed to approve proposal:', error); + // Show error to user + }); }; const handleReject = () => { - dispatch(sessionProposalRejectThunk({ eventId })); - dispatch(onCancel()); + dispatch(sessionProposalRejectThunk({ eventId })) + .unwrap() + .then(() => dispatch(onCancel())) + .catch(error => { + console.error('Failed to reject proposal:', error); + // Show error to user + }); };
🧹 Nitpick comments (7)
packages/suite-desktop-connect-popup/src/connectPopupThunks.ts (1)
42-43: Consider improving type safety for TrezorConnect method calls.The
@ts-expect-errorcomments indicate potential type safety issues with dynamic method calls.Consider using a type-safe approach:
type TrezorMethod = keyof typeof TrezorConnect; function callTrezorMethod<M extends TrezorMethod>( method: M, params: CallMethodParams<M> ): Promise<CallMethodResponse<M>> { return TrezorConnect[method](params); }Also applies to: 66-67
packages/suite-walletconnect/src/adapters/ethereum.ts (2)
20-32: Enhance error handling in getAccount helper.The error message could be more descriptive to help debug issues in production.
- throw new Error('Account not found'); + throw new Error( + `Account not found for address ${address}${ + chainId ? ` and chainId ${chainId}` : '' + }` + );
85-146: Consider adding transaction validation.While the transaction handling is comprehensive, consider adding validation for required transaction fields before processing.
+ const requiredFields = ['to', 'value', 'from']; + for (const field of requiredFields) { + if (!transaction[field]) { + throw new Error(`Missing required transaction field: ${field}`); + } + } + if (account.networkType !== 'ethereum') {packages/suite/src/components/suite/modals/ReduxModal/UserContextModal/WalletConnectProposalModal.tsx (2)
57-71: Simplify validation state conditions.The validation state checks can be simplified for better readability and maintainability.
Consider refactoring to:
- {!pendingProposal?.isScam && pendingProposal?.validation === 'VALID' && ( + {pendingProposal?.validation === 'VALID' && !pendingProposal?.isScam && ( <Note variant="info" iconName="shieldCheckFilled"> <Translation id="TR_WALLETCONNECT_SERVICE_VERIFIED" /> </Note> )} - {!pendingProposal?.isScam && pendingProposal?.validation === 'UNKNOWN' && ( + {pendingProposal?.validation === 'UNKNOWN' && !pendingProposal?.isScam && ( <Note variant="warning" iconName="shieldWarningFilled"> <Translation id="TR_WALLETCONNECT_SERVICE_UNKNOWN" /> </Note> )} - {(pendingProposal?.isScam || pendingProposal?.validation === 'INVALID') && ( + {pendingProposal?.validation === 'INVALID' && ( <Note variant="destructive" iconName="shieldWarningFilled"> <Translation id="TR_WALLETCONNECT_SERVICE_DANGEROUS" /> </Note> )}
77-92: Consider consolidating banner conditions.The banner conditions for scam, invalid, and expired states could be consolidated using a switch statement or early returns for better maintainability.
Consider refactoring to:
const getBannerContent = () => { if (pendingProposal?.isScam) { return { variant: 'destructive' as const, translationId: 'TR_WALLETCONNECT_IS_SCAM' }; } if (pendingProposal?.validation === 'INVALID') { return { variant: 'destructive' as const, translationId: 'TR_WALLETCONNECT_UNABLE_TO_VERIFY' }; } if (pendingProposal?.expired) { return { variant: 'warning' as const, translationId: 'TR_WALLETCONNECT_REQUEST_EXPIRED' }; } return null; }; const bannerContent = getBannerContent(); {bannerContent && ( <Banner variant={bannerContent.variant} margin={{ top: spacings.xs }}> <Translation id={bannerContent.translationId} /> </Banner> )}packages/suite-walletconnect/src/walletConnectThunks.ts (2)
23-23: Consider encapsulating the WalletKit instance.The global mutable
walletKitvariable could lead to race conditions or initialization issues. Consider encapsulating it within a service class or using a Redux slice to manage its lifecycle.
76-76: Improve error logging.Replace
console.errorwith a proper logging mechanism that includes context about the operation being performed.- console.error(error); + console.error('WalletConnect: Failed to authenticate session', { + error, + eventId: event.id, + });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (33)
packages/components/src/components/Note/Note.tsx(1 hunks)packages/connect/src/events/call.ts(1 hunks)packages/suite-desktop-connect-popup/src/connectPopupThunks.ts(2 hunks)packages/suite-desktop-core/src/config.ts(1 hunks)packages/suite-walletconnect/package.json(1 hunks)packages/suite-walletconnect/src/adapters/ethereum.ts(1 hunks)packages/suite-walletconnect/src/adapters/index.ts(1 hunks)packages/suite-walletconnect/src/index.ts(1 hunks)packages/suite-walletconnect/src/walletConnectActions.ts(1 hunks)packages/suite-walletconnect/src/walletConnectConstants.ts(1 hunks)packages/suite-walletconnect/src/walletConnectMiddleware.ts(1 hunks)packages/suite-walletconnect/src/walletConnectReducer.ts(1 hunks)packages/suite-walletconnect/src/walletConnectThunks.ts(1 hunks)packages/suite-walletconnect/src/walletConnectTypes.ts(1 hunks)packages/suite-walletconnect/tsconfig.json(1 hunks)packages/suite/jest.config.js(1 hunks)packages/suite/package.json(1 hunks)packages/suite/src/actions/suite/__tests__/initAction.test.ts(4 hunks)packages/suite/src/actions/suite/initAction.ts(2 hunks)packages/suite/src/actions/suite/protocolActions.ts(3 hunks)packages/suite/src/components/suite/modals/ReduxModal/UserContextModal/ConnectPopupModal.tsx(1 hunks)packages/suite/src/components/suite/modals/ReduxModal/UserContextModal/UserContextModal.tsx(2 hunks)packages/suite/src/components/suite/modals/ReduxModal/UserContextModal/WalletConnectProposalModal.tsx(1 hunks)packages/suite/src/middlewares/wallet/discoveryMiddleware.ts(2 hunks)packages/suite/src/middlewares/wallet/index.ts(2 hunks)packages/suite/src/reducers/wallet/index.ts(3 hunks)packages/suite/src/support/messages.ts(1 hunks)packages/suite/src/views/settings/SettingsDebug/SettingsDebug.tsx(2 hunks)packages/suite/src/views/settings/SettingsDebug/WalletConnect.tsx(1 hunks)packages/suite/tsconfig.json(1 hunks)packages/urls/src/deeplinks.ts(1 hunks)scripts/list-outdated-dependencies/connect-dependencies.txt(1 hunks)suite-common/suite-types/src/modal.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (29)
- packages/suite-walletconnect/tsconfig.json
- scripts/list-outdated-dependencies/connect-dependencies.txt
- packages/suite-walletconnect/src/index.ts
- packages/urls/src/deeplinks.ts
- packages/suite/src/views/settings/SettingsDebug/SettingsDebug.tsx
- packages/suite/package.json
- packages/connect/src/events/call.ts
- packages/suite/src/middlewares/wallet/discoveryMiddleware.ts
- packages/suite/src/components/suite/modals/ReduxModal/UserContextModal/ConnectPopupModal.tsx
- packages/suite/src/reducers/wallet/index.ts
- packages/suite-walletconnect/src/walletConnectMiddleware.ts
- packages/suite-walletconnect/package.json
- packages/suite-desktop-core/src/config.ts
- suite-common/suite-types/src/modal.ts
- packages/suite/src/components/suite/modals/ReduxModal/UserContextModal/UserContextModal.tsx
- packages/suite/src/middlewares/wallet/index.ts
- packages/suite-walletconnect/src/walletConnectConstants.ts
- packages/suite/jest.config.js
- packages/suite/src/views/settings/SettingsDebug/WalletConnect.tsx
- packages/suite/src/actions/suite/initAction.ts
- packages/suite-walletconnect/src/walletConnectActions.ts
- packages/components/src/components/Note/Note.tsx
- packages/suite/src/actions/suite/tests/initAction.test.ts
- packages/suite-walletconnect/src/walletConnectTypes.ts
- packages/suite/src/actions/suite/protocolActions.ts
- packages/suite-walletconnect/src/walletConnectReducer.ts
- packages/suite/tsconfig.json
- packages/suite-walletconnect/src/adapters/index.ts
- packages/suite/src/support/messages.ts
🧰 Additional context used
📓 Learnings (1)
packages/suite-walletconnect/src/adapters/ethereum.ts (1)
Learnt from: martykan
PR: trezor/trezor-suite#15803
File: packages/suite-walletconnect/src/adapters/ethereum.ts:113-113
Timestamp: 2025-02-03T10:44:26.876Z
Learning: In WalletConnect integration (packages/suite-walletconnect), gas limit for Ethereum transactions should be specified by the caller (dApp). The fallback value of '21000' is only used when the caller doesn't provide a gas limit.
⏰ Context from checks skipped due to timeout of 90000ms (23)
- GitHub Check: Unit Tests
- GitHub Check: Type Checking
- GitHub Check: Other Checks
- GitHub Check: Build libs for publishing
- GitHub Check: Releases revision Checks
- GitHub Check: Linting and formatting
- GitHub Check: PR-check / node-authorizeCoinjoin cancelCoinjoinAuthorization passphrase unlockPath setBusy checkFirmwareAuthenticity keepSession cancel.test info.test resetDevice-api
- GitHub Check: PR-check / node-override init-api-flaky
- GitHub Check: PR-check / web-override init-api-flaky
- GitHub Check: PR-check / web-authorizeCoinjoin cancelCoinjoinAuthorization passphrase unlockPath setBusy checkFirmwareAuthenticity keepSession cancel.test info.test resetDevice-api
- GitHub Check: run-desktop-tests (@group=wallet, trezor-user-env-unix bitcoin-regtest)
- GitHub Check: run-desktop-tests (@group=settings, trezor-user-env-unix bitcoin-regtest)
- GitHub Check: run-desktop-tests (@group=device-management, trezor-user-env-unix)
- GitHub Check: run-desktop-tests (@group=suite, trezor-user-env-unix)
- GitHub Check: build-web
- GitHub Check: build-deploy
- GitHub Check: prepare_android_test_app
- GitHub Check: build-web
- GitHub Check: EAS Update
- GitHub Check: transport-e2e-test
- GitHub Check: test
- GitHub Check: Analyze with CodeQL (javascript)
- GitHub Check: Socket Security: Pull Request Alerts
🔇 Additional comments (9)
packages/suite-desktop-connect-popup/src/connectPopupThunks.ts (4)
1-24: LGTM! Great improvement in type safety.The new type definitions with generics provide excellent type safety and clear structure for the thunk's parameters and response.
35-40: Enhance device selection error handling.The TODO comment suggests incomplete device selection handling. Consider implementing a device selection wait mechanism.
Would you like me to help implement a device selection wait mechanism that uses a deferred promise similar to the confirmation handling?
95-101: LGTM! Clean type-safe implementation.The generic thunk wrapper provides excellent type safety while maintaining a clean implementation.
107-111: Handle potential race conditions in event handling.The event handler still lacks protection against concurrent calls with the same ID.
Consider implementing request tracking as suggested in the previous review:
+const pendingRequests = new Map(); + desktopApi.on('connect-popup/call', async params => { + if (pendingRequests.has(params.id)) { + console.warn(`Request ${params.id} is already being processed`); + return; + } + pendingRequests.set(params.id, true); const response = await dispatch(connectPopupCallThunk(params)).unwrap(); desktopApi.connectPopupResponse(response); + pendingRequests.delete(params.id); });packages/suite-walletconnect/src/adapters/ethereum.ts (3)
1-12: LGTM! Dependencies are well-organized.All imports are necessary and properly structured for the WalletConnect Ethereum adapter implementation.
60-84: Handle malformed JSON data gracefully.The code invokes
JSON.parse(data)without a try-catch block, which could throw a runtime error if the payload is malformed.- const [address, data] = event.params.request.params; - const account = getAccount(address); - const response = await dispatch( + const [address, rawData] = event.params.request.params; + let parsedData; + try { + parsedData = JSON.parse(rawData); + } catch (parseError) { + throw new Error('Invalid JSON in request params'); + } + const account = getAccount(address); + const response = await dispatch( trezorConnectPopupActions.connectPopupCallThunk({ id: 0, method: 'ethereumSignTypedData', payload: { path: account.path, - data: JSON.parse(data), + data: parsedData,
155-164: LGTM! Adapter interface is well-defined.The adapter export correctly implements the WalletConnectAdapter interface and lists all supported methods.
packages/suite/src/components/suite/modals/ReduxModal/UserContextModal/WalletConnectProposalModal.tsx (1)
1-15: LGTM! Clean imports and well-typed interface.The imports are properly organized and the interface is well-defined with appropriate typing.
packages/suite-walletconnect/src/walletConnectThunks.ts (1)
237-240: EnsureWalletKit.initfailures are handled.If
walletKit = await WalletKit.init({...})fails for any reason (e.g., network or config issues), the current logic will silently exit or throw unhandled exceptions. Consider catching and surfacing this error to the user.
| <H2>{pendingProposal?.params.proposer.metadata.name}</H2> | ||
|
|
||
| <Paragraph>{pendingProposal?.params.proposer.metadata.url}</Paragraph> | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add null checks for pendingProposal metadata.
The metadata access could throw if pendingProposal is null. Use optional chaining consistently.
Apply this diff:
- <H2>{pendingProposal?.params.proposer.metadata.name}</H2>
+ <H2>{pendingProposal?.params?.proposer?.metadata?.name}</H2>
- <Paragraph>{pendingProposal?.params.proposer.metadata.url}</Paragraph>
+ <Paragraph>{pendingProposal?.params?.proposer?.metadata?.url}</Paragraph>📝 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.
| <H2>{pendingProposal?.params.proposer.metadata.name}</H2> | |
| <Paragraph>{pendingProposal?.params.proposer.metadata.url}</Paragraph> | |
| <H2>{pendingProposal?.params?.proposer?.metadata?.name}</H2> | |
| <Paragraph>{pendingProposal?.params?.proposer?.metadata?.url}</Paragraph> |
| error: { | ||
| code: 1, | ||
| message: error.message, | ||
| }, |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance error handling security.
Exposing raw error messages to clients could leak sensitive information. Consider using predefined error codes and messages.
- error: {
- code: 1,
- message: error.message,
- },
+ error: {
+ code: 4001, // Standard JSON-RPC error code for user rejection
+ message: 'User rejected the request',
+ },📝 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.
| error: { | |
| code: 1, | |
| message: error.message, | |
| }, | |
| error: { | |
| code: 4001, // Standard JSON-RPC error code for user rejection | |
| message: 'User rejected the request', | |
| }, |
| const network = getNetwork(account.symbol); | ||
| const sessions = await walletKit.getActiveSessions(); | ||
| for (const topic in sessions) { | ||
| walletKit.emitSessionEvent({ | ||
| topic, | ||
| event: { | ||
| name: 'chainChanged', | ||
| data: network.chainId, | ||
| }, | ||
| chainId: `eip155:${network.chainId}`, | ||
| }); |
There was a problem hiding this comment.
Add network validation before emitting events.
The code assumes the network exists for the account symbol. Add validation to prevent potential runtime errors.
const network = getNetwork(account.symbol);
+ if (!network?.chainId) {
+ console.error('WalletConnect: Invalid network for account', { account });
+ return;
+ }
const sessions = await walletKit.getActiveSessions();📝 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 network = getNetwork(account.symbol); | |
| const sessions = await walletKit.getActiveSessions(); | |
| for (const topic in sessions) { | |
| walletKit.emitSessionEvent({ | |
| topic, | |
| event: { | |
| name: 'chainChanged', | |
| data: network.chainId, | |
| }, | |
| chainId: `eip155:${network.chainId}`, | |
| }); | |
| const network = getNetwork(account.symbol); | |
| if (!network?.chainId) { | |
| console.error('WalletConnect: Invalid network for account', { account }); | |
| return; | |
| } | |
| const sessions = await walletKit.getActiveSessions(); | |
| for (const topic in sessions) { | |
| walletKit.emitSessionEvent({ | |
| topic, | |
| event: { | |
| name: 'chainChanged', | |
| data: network.chainId, | |
| }, | |
| chainId: `eip155:${network.chainId}`, | |
| }); |
| export const walletConnectPairThunk = createThunk<void, { uri: string }>( | ||
| `${WALLETCONNECT_MODULE}/walletConnectPairThunk`, | ||
| async ({ uri }) => { | ||
| if (desktopApi.available) { | ||
| desktopApi.appFocus(); | ||
| } | ||
|
|
||
| try { | ||
| await walletKit.pair({ uri }); | ||
| } catch (error) { | ||
| console.error(error); | ||
| } | ||
| }, | ||
| ); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add URI validation and improve error handling.
The URI should be validated before attempting to pair, and errors should be properly handled and communicated to the user.
async ({ uri }) => {
+ if (!uri?.startsWith('wc:')) {
+ throw new Error('Invalid WalletConnect URI format');
+ }
+
if (desktopApi.available) {
desktopApi.appFocus();
}
try {
await walletKit.pair({ uri });
} catch (error) {
- console.error(error);
+ throw new Error('Failed to pair with WalletConnect: ' + 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.
| export const walletConnectPairThunk = createThunk<void, { uri: string }>( | |
| `${WALLETCONNECT_MODULE}/walletConnectPairThunk`, | |
| async ({ uri }) => { | |
| if (desktopApi.available) { | |
| desktopApi.appFocus(); | |
| } | |
| try { | |
| await walletKit.pair({ uri }); | |
| } catch (error) { | |
| console.error(error); | |
| } | |
| }, | |
| ); | |
| export const walletConnectPairThunk = createThunk<void, { uri: string }>( | |
| `${WALLETCONNECT_MODULE}/walletConnectPairThunk`, | |
| async ({ uri }) => { | |
| if (!uri?.startsWith('wc:')) { | |
| throw new Error('Invalid WalletConnect URI format'); | |
| } | |
| if (desktopApi.available) { | |
| desktopApi.appFocus(); | |
| } | |
| try { | |
| await walletKit.pair({ uri }); | |
| } catch (error) { | |
| throw new Error('Failed to pair with WalletConnect: ' + error.message); | |
| } | |
| }, | |
| ); |
martykan
force-pushed
the
feat/walletconnect
branch
from
45ef5b0 to
f5c1814
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/suite-desktop-connect-popup/src/connectPopupThunks.ts (1)
95-103: Consider removing the type assertion.The
as anytype assertion in the wrapper could be unsafe. Consider using a more precise type assertion or restructuring the types to avoid the need for assertion.-): AsyncThunkAction< - ConnectPopupCallThunkResponse<M>, - ConnectPopupCallThunkParams<M>, - CustomThunkAPI -> => connectPopupCallThunkInner(params) as any; +): AsyncThunkAction< + ConnectPopupCallThunkResponse<M>, + ConnectPopupCallThunkParams<M>, + CustomThunkAPI +> => connectPopupCallThunkInner(params) as AsyncThunkAction< + ConnectPopupCallThunkResponse<M>, + ConnectPopupCallThunkParams<M>, + CustomThunkAPI +>;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
packages/connect/src/events/call.ts(1 hunks)packages/suite-desktop-connect-popup/src/connectPopupThunks.ts(2 hunks)packages/suite/src/middlewares/wallet/discoveryMiddleware.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/connect/src/events/call.ts
- packages/suite/src/middlewares/wallet/discoveryMiddleware.ts
⏰ Context from checks skipped due to timeout of 90000ms (16)
- GitHub Check: build-deploy
- GitHub Check: build-deploy
- GitHub Check: run-desktop-tests (@group=wallet, trezor-user-env-unix bitcoin-regtest)
- GitHub Check: EAS Update
- GitHub Check: run-desktop-tests (@group=settings, trezor-user-env-unix bitcoin-regtest)
- GitHub Check: transport-e2e-test
- GitHub Check: build
- GitHub Check: run-desktop-tests (@group=device-management, trezor-user-env-unix)
- GitHub Check: prepare_android_test_app
- GitHub Check: build-web
- GitHub Check: Setup and Cache Dependencies
- GitHub Check: run-desktop-tests (@group=suite, trezor-user-env-unix)
- GitHub Check: test
- GitHub Check: Analyze with CodeQL (javascript)
- GitHub Check: build-web
- GitHub Check: Socket Security: Pull Request Alerts
🔇 Additional comments (3)
packages/suite-desktop-connect-popup/src/connectPopupThunks.ts (3)
12-24: LGTM! Well-structured type definitions.The generic type definitions provide strong type safety and proper constraints for TrezorConnect methods.
38-38: Address the TODO comment about device selection.The TODO indicates a potential edge case when device is not found. Consider implementing a device selection wait mechanism or documenting why it's deferred.
Would you like me to help implement the device selection wait mechanism or create an issue to track this task?
109-113: Race condition in event handler still needs addressing.The event handler doesn't handle potential race conditions from multiple concurrent calls.
Consider improving desktop API types.
The
@ts-expect-errorcomment suggests incomplete typing in the desktop API. Consider enhancing the types to avoid the need for type assertions.
martykan
force-pushed
the
feat/walletconnect
branch
from
f5c1814 to
1cb0ba6
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (10)
suite-common/walletconnect/src/adapters/ethereum.ts (1)
34-59: Validate input for'personal_sign'.
Currently, any message of any length is passed to Trezor Connect. Consider validating or limiting message size to mitigate potential performance or security concerns, especially with huge messages.suite-common/walletconnect/src/adapters/index.ts (1)
21-43: Verify consistency ingetNamespaces.
For multi-chain expansions (e.g., Solana, Bitcoin), confirm that thechainIdnaming scheme remains consistent. A mismatch could cause cross-adapter conflicts if non-EVM chains are added.suite-common/walletconnect/src/walletConnectThunks.ts (4)
1-23: Consider storingwalletKitmore safely.Defining
walletKitas a top-level variable might lead to issues if the file is imported multiple times or if parallel initialization occurs. Consider storing it in the Redux store or returning it from an initialization thunk to reduce the risk of unexpected reinitializations.
84-122: Provide more context in error messages.When the method is unsupported (line 93), consider adding the method name to the error message for easier debugging:
- throw new Error('Unsupported method'); + throw new Error(`Unsupported method: ${event.params.request.method}`);
124-163: Improve user feedback for session proposal errors.Currently, errors in
sessionProposalApproveThunkare just logged to the console. Consider displaying a user-facing error or integrating a logging/telemetry solution to communicate and track failures more effectively.
229-277: Prevent silent failures during WalletKit initialization.If there's an exception in
WalletKit.init, the code flow will silently fail. Wrap this block in a try/catch, log or handle the error, and ensure the rest of the method handles partial initialization states gracefully.suite-common/walletconnect/src/walletConnectActions.ts (1)
7-38: Consider adding JSDoc comments for better documentation.While the action implementations are correct and type-safe, adding JSDoc comments would improve maintainability by documenting the purpose and payload structure of each action.
Example addition:
+/** + * Saves or updates a WalletConnect session. + * @param payload The session details to save + */ const saveSession = createAction( `${ACTION_PREFIX}/saveSession`, (payload: WalletConnectSession) => ({ payload, }), );suite-common/walletconnect/src/walletConnectTypes.ts (1)
42-50: Consider adding expiration timestamp.The
PendingConnectionProposalinterface tracks expiration status but not when it expires.Apply this improvement:
export interface PendingConnectionProposal { eventId: number; params: ProposalTypes.Struct; origin: string; validation: 'UNKNOWN' | 'VALID' | 'INVALID'; verifyUrl: string; isScam?: boolean; expired: boolean; + expiresAt?: number; // Unix timestamp }suite-common/walletconnect/src/walletConnectReducer.ts (2)
20-49: Consider adding session expiry cleanup.The reducer handles session management but doesn't clean up expired sessions.
Add a new action and case to handle session expiry:
+const cleanupExpiredSessions = createAction(`${ACTION_PREFIX}/cleanupExpiredSessions`); export const prepareWalletConnectReducer = createReducerWithExtraDeps( walletConnectInitialState, (builder, _extra) => { builder + .addCase(cleanupExpiredSessions, (state) => { + const now = Date.now() / 1000; + state.sessions = state.sessions.filter(session => session.expiry > now); + }) // ... rest of the cases } );
51-55: Consider adding memoized selectors.The selectors are correct but could benefit from memoization for performance.
Consider using
createSelectorfrom Redux Toolkit:+import { createSelector } from '@reduxjs/toolkit'; -export const selectSessions = (state: WalletConnectStateRootState) => - state.wallet.walletConnect.sessions; +export const selectSessions = createSelector( + (state: WalletConnectStateRootState) => state.wallet.walletConnect.sessions, + sessions => sessions +); -export const selectPendingProposal = (state: WalletConnectStateRootState) => - state.wallet.walletConnect.pendingProposal; +export const selectPendingProposal = createSelector( + (state: WalletConnectStateRootState) => state.wallet.walletConnect.pendingProposal, + proposal => proposal +);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (33)
packages/components/src/components/Note/Note.tsx(1 hunks)packages/connect/src/events/call.ts(1 hunks)packages/suite-desktop-connect-popup/src/connectPopupThunks.ts(2 hunks)packages/suite-desktop-core/src/config.ts(1 hunks)packages/suite/jest.config.js(1 hunks)packages/suite/package.json(1 hunks)packages/suite/src/actions/suite/__tests__/initAction.test.ts(4 hunks)packages/suite/src/actions/suite/initAction.ts(2 hunks)packages/suite/src/actions/suite/protocolActions.ts(3 hunks)packages/suite/src/components/suite/modals/ReduxModal/UserContextModal/ConnectPopupModal.tsx(1 hunks)packages/suite/src/components/suite/modals/ReduxModal/UserContextModal/UserContextModal.tsx(2 hunks)packages/suite/src/components/suite/modals/ReduxModal/UserContextModal/WalletConnectProposalModal.tsx(1 hunks)packages/suite/src/middlewares/wallet/discoveryMiddleware.ts(2 hunks)packages/suite/src/middlewares/wallet/index.ts(2 hunks)packages/suite/src/reducers/wallet/index.ts(3 hunks)packages/suite/src/support/messages.ts(1 hunks)packages/suite/src/views/settings/SettingsDebug/SettingsDebug.tsx(2 hunks)packages/suite/src/views/settings/SettingsDebug/WalletConnect.tsx(1 hunks)packages/suite/tsconfig.json(1 hunks)packages/urls/src/deeplinks.ts(1 hunks)scripts/list-outdated-dependencies/connect-dependencies.txt(1 hunks)suite-common/suite-types/src/modal.ts(1 hunks)suite-common/walletconnect/package.json(1 hunks)suite-common/walletconnect/src/adapters/ethereum.ts(1 hunks)suite-common/walletconnect/src/adapters/index.ts(1 hunks)suite-common/walletconnect/src/index.ts(1 hunks)suite-common/walletconnect/src/walletConnectActions.ts(1 hunks)suite-common/walletconnect/src/walletConnectConstants.ts(1 hunks)suite-common/walletconnect/src/walletConnectMiddleware.ts(1 hunks)suite-common/walletconnect/src/walletConnectReducer.ts(1 hunks)suite-common/walletconnect/src/walletConnectThunks.ts(1 hunks)suite-common/walletconnect/src/walletConnectTypes.ts(1 hunks)suite-common/walletconnect/tsconfig.json(1 hunks)
✅ Files skipped from review due to trivial changes (3)
- suite-common/walletconnect/tsconfig.json
- suite-common/walletconnect/package.json
- suite-common/walletconnect/src/index.ts
🚧 Files skipped from review as they are similar to previous changes (21)
- scripts/list-outdated-dependencies/connect-dependencies.txt
- packages/suite-desktop-core/src/config.ts
- packages/suite/jest.config.js
- packages/suite/tsconfig.json
- packages/urls/src/deeplinks.ts
- packages/suite/src/components/suite/modals/ReduxModal/UserContextModal/ConnectPopupModal.tsx
- packages/suite/src/actions/suite/initAction.ts
- suite-common/suite-types/src/modal.ts
- packages/suite/package.json
- packages/suite/src/actions/suite/tests/initAction.test.ts
- packages/connect/src/events/call.ts
- packages/suite/src/middlewares/wallet/index.ts
- packages/suite/src/middlewares/wallet/discoveryMiddleware.ts
- packages/suite/src/components/suite/modals/ReduxModal/UserContextModal/UserContextModal.tsx
- packages/suite/src/reducers/wallet/index.ts
- packages/suite/src/views/settings/SettingsDebug/WalletConnect.tsx
- packages/suite/src/components/suite/modals/ReduxModal/UserContextModal/WalletConnectProposalModal.tsx
- packages/components/src/components/Note/Note.tsx
- packages/suite/src/actions/suite/protocolActions.ts
- packages/suite/src/views/settings/SettingsDebug/SettingsDebug.tsx
- packages/suite/src/support/messages.ts
🧰 Additional context used
📓 Learnings (1)
suite-common/walletconnect/src/walletConnectConstants.ts (1)
Learnt from: martykan
PR: trezor/trezor-suite#15803
File: packages/suite-walletconnect/src/walletConnectConstants.ts:3-3
Timestamp: 2025-02-03T12:29:19.007Z
Learning: The WalletConnect project ID in Trezor Suite is not considered sensitive information and can be stored in constants rather than environment variables.
⏰ Context from checks skipped due to timeout of 90000ms (15)
- GitHub Check: build-deploy
- GitHub Check: build-deploy
- GitHub Check: run-desktop-tests (@group=wallet, trezor-user-env-unix bitcoin-regtest)
- GitHub Check: EAS Update
- GitHub Check: run-desktop-tests (@group=settings, trezor-user-env-unix bitcoin-regtest)
- GitHub Check: build-web
- GitHub Check: transport-e2e-test
- GitHub Check: run-desktop-tests (@group=device-management, trezor-user-env-unix)
- GitHub Check: build-web
- GitHub Check: Socket Security: Pull Request Alerts
- GitHub Check: Analyze with CodeQL (javascript)
- GitHub Check: prepare_android_test_app
- GitHub Check: test
- GitHub Check: run-desktop-tests (@group=suite, trezor-user-env-unix)
- GitHub Check: Setup and Cache Dependencies
🔇 Additional comments (26)
suite-common/walletconnect/src/walletConnectConstants.ts (2)
5-10: Verify WalletConnect client metadata structure.Let's verify if the metadata structure matches WalletConnect requirements and if all required fields are present.
✅ Verification successful
WalletConnect client metadata structure verified.
The metadata constant includes all required fields—name,description,url, andicons—which aligns perfectly with WalletConnect specifications. Optional fields likeverifyUrlorredirectaren’t mandatory.🏁 Scripts executed
The following scripts were executed for the analysis:
Web query:
What are the required fields and format for WalletConnect client metadata?Length of output: 3740
3-3: Verify WalletConnect Project ID.Let's verify if this is a valid WalletConnect project ID and check for any associated requirements or limitations.
✅ Verification successful
WalletConnect Project ID is Valid
The
PROJECT_IDvalue203549d0480d0f24d994780f34889b03is a 32-character lowercase hexadecimal string, which meets the WalletConnect format requirements as verified via structure analysis and web documentation. No adjustments are needed.🏁 Scripts executed
The following scripts were executed for the analysis:
Web query:
What is the format of a valid WalletConnect Project ID and how can I verify if a project ID is valid?Length of output: 4379
suite-common/walletconnect/src/adapters/ethereum.ts (5)
1-9: Imports structure looks solid.
No obvious issues here, and the dependencies align well with the code’s functionality.
20-32: Guard against invalid chains ingetAccount.
IfgetNetwork(a.symbol)returnsundefined,getNetwork(a.symbol).chainIdwill throw an error. Consider adding a guard check to handle unknown networks proactively.
85-146: Review EIP-1559 fee structure and minimum gas limit.
- Some EVM-based networks no longer use
gasPriceexclusively but instead usemaxFeePerGasandmaxPriorityFeePerGas. Consider updating the fee estimation logic to support EIP-1559 transactions.- The fallback of
21000gas might be too low for transactions that interact with contracts. Confirm whether a higher default or dynamic approach is needed.
147-151: Confirm side-effects forwallet_switchEthereumChain.
This switch case only returnschainId, but it does not perform any actual chain switching logic on the device or in the application. If that is intentional, confirm that no additional handling is required.
155-164: Adapter configuration looks consistent.
The exportedethereumAdaptersatisfies theWalletConnectAdapterinterface properly and integrates seamlessly with the rest of the module.suite-common/walletconnect/src/adapters/index.ts (2)
1-11: Adapters array is well-structured.
The placeholder comments for future Solana and Bitcoin adapters indicate extensibility, which is good for future-proofing.
13-19: Utility methods are straightforward and clear.
getAdapterByMethodandgetAdapterByNetworkare concise and ensure a direct lookup. No issues here.suite-common/walletconnect/src/walletConnectThunks.ts (6)
24-82: Add a safety check for undefined device.Within
sessionAuthenticateThunk, the code retrieves the selected device but never checks whether it’s undefined before callingTrezorConnect.ethereumSignMessage. If no device is selected or none is found, this call could fail. Include a guard or throw an explicit error to enhance robustness.
165-175: Logic for rejecting session proposals is clear.This code correctly rejects the session proposal if the user opts out. No further issues spotted.
178-202: Verify thatgetNetworkalways returns a valid network.If
getNetwork(account.symbol)returnsundefined, then accessingnetwork.chainIdwill cause a runtime error. Add a guard or fallback to handle cases where the network may be unknown.
205-227: Handle missing or alternative namespaces.If the existing session’s
oldNamespacesdoes not containeip155, extending it might result in runtime errors. Consider adding checks or conditionally merging namespaces to avoid inconsistent session state.
279-284: Add error handling for invalid pairing URIs.
walletConnectPairThunkcallswalletKit.pairwithout a try/catch. If the URI is invalid or pairing fails, it may produce an unhandled error. A wrapped error check would improve reliability.
286-292: Consider catching errors when disconnecting sessions.If
walletKit.disconnectSessionfails, you may want to notify the user or log the error. A try/catch would ensure that partial failures do not break the app flow silently.packages/suite-desktop-connect-popup/src/connectPopupThunks.ts (7)
1-1: No concerns with the new import.Importing
AsyncThunkActionfrom Redux Toolkit is appropriate for typed thunk definitions.
3-5: Imports for custom thunk and TrezorConnect are valid.These additions align with the new typed thunk usage and are consistent with the rest of the code.
12-16: Good approach to typed thunk response.Defining
ConnectPopupCallThunkResponsegenerically improves type safety and reduces unintentional misuse of return payloads.
18-24: Well-defined thunk parameters.Splitting out
ConnectPopupCallThunkParamswith a generic method type helps ensure calls align correctly with TrezorConnect’s method signatures.
26-93: Persisting concurrency concerns from past review.Handling multiple concurrent
connect-popup/callrequests still risks race conditions, as previously noted. A request-tracking approach (e.g., apendingRequestsmap) would help mitigate overlapping calls:+const pendingRequests = new Map<number, boolean>(); desktopApi.on('connect-popup/call', async params => { + if (pendingRequests.has(params.id)) { + console.warn(`Request ${params.id} is already in progress.`); + return; + } + pendingRequests.set(params.id, true); const response = await dispatch(connectPopupCallThunk(params)).unwrap(); desktopApi.connectPopupResponse(response); + pendingRequests.delete(params.id); });
95-104: The wrapper thunk is fine.Exposing a typed thunk via generics is a sensible approach, preserving the flexibility of method-specific response typing while enabling standard usage with
.fulfilledor.rejectedin the Redux flow.
109-112: Recurring concurrency issue on'connect-popup/call'.This event listener dispatches
connectPopupCallThunkdirectly, which can lead to race conditions if multiple calls arrive almost simultaneously. Refer to the previous concurrency suggestion above.suite-common/walletconnect/src/walletConnectActions.ts (1)
1-5: LGTM! Well-structured imports and action prefix.The imports are properly organized, and the action prefix follows the standard namespace pattern.
suite-common/walletconnect/src/walletConnectTypes.ts (2)
6-12: LGTM! Well-defined adapter interface.The adapter interface properly defines the contract for network-specific implementations.
21-40: Consider adding validation for metadata URLs.The session interface includes URLs in metadata, but there's no validation to ensure they're HTTPS.
Run this script to check for any hardcoded HTTP URLs:
✅ Verification successful
Review Feedback: URL Validation for WalletConnect Metadata
The
WalletConnectSessioninterface insuite-common/walletconnect/src/walletConnectTypes.tsis solely a type definition and, as such, does not include runtime validation logic. This is expected since interfaces describe the shape of data rather than enforce constraints at runtime. Our grep search confirms that this file contains no hardcoded HTTP URLs, and many intentional usages of HTTP (e.g., for localhost/test endpoints) appear elsewhere in the codebase.If enforcing HTTPS for metadata URLs is required, the appropriate approach would be to add the validation in the runtime code where
WalletConnectSessionobjects are created or processed—not in the type definition itself.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for HTTP URLs in the codebase rg -i "http://" --type tsLength of output: 20841
suite-common/walletconnect/src/walletConnectReducer.ts (1)
6-18: LGTM! Clean state structure.The state types and initial state are well-defined and properly typed.
| case 'eth_signTypedData_v4': { | ||
| const [address, data] = event.params.request.params; | ||
| const account = getAccount(address); | ||
| const response = await dispatch( | ||
| trezorConnectPopupActions.connectPopupCallThunk({ | ||
| id: 0, | ||
| method: 'ethereumSignTypedData', | ||
| payload: { | ||
| path: account.path, | ||
| data: JSON.parse(data), | ||
| metamask_v4_compat: true, | ||
| device, | ||
| useEmptyPassphrase: device?.useEmptyPassphrase, | ||
| }, | ||
| processName: 'WalletConnect', | ||
| origin: event.verifyContext.verified.origin, | ||
| }), | ||
| ).unwrap(); | ||
| if (!response.success) { | ||
| console.error('eth_signTypedData_v4 error', response); | ||
| throw new Error('eth_signTypedData_v4 error'); | ||
| } | ||
|
|
||
| return response.payload.signature; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Handle JSON parsing errors in 'eth_signTypedData_v4'.
If data is invalid JSON, calling JSON.parse(data) will throw. Wrap the parsing in a try/catch block to provide clearer diagnostics and avoid unhandled exceptions.
+let parsedData;
+try {
+ parsedData = JSON.parse(data);
+} catch (error) {
+ console.error('Failed to parse typed data', error);
+ throw new Error('eth_signTypedData_v4 parsing error');
+}
...
- data: JSON.parse(data),
+ data: parsedData,📝 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.
| case 'eth_signTypedData_v4': { | |
| const [address, data] = event.params.request.params; | |
| const account = getAccount(address); | |
| const response = await dispatch( | |
| trezorConnectPopupActions.connectPopupCallThunk({ | |
| id: 0, | |
| method: 'ethereumSignTypedData', | |
| payload: { | |
| path: account.path, | |
| data: JSON.parse(data), | |
| metamask_v4_compat: true, | |
| device, | |
| useEmptyPassphrase: device?.useEmptyPassphrase, | |
| }, | |
| processName: 'WalletConnect', | |
| origin: event.verifyContext.verified.origin, | |
| }), | |
| ).unwrap(); | |
| if (!response.success) { | |
| console.error('eth_signTypedData_v4 error', response); | |
| throw new Error('eth_signTypedData_v4 error'); | |
| } | |
| return response.payload.signature; | |
| } | |
| case 'eth_signTypedData_v4': { | |
| const [address, data] = event.params.request.params; | |
| const account = getAccount(address); | |
| let parsedData; | |
| try { | |
| parsedData = JSON.parse(data); | |
| } catch (error) { | |
| console.error('Failed to parse typed data', error); | |
| throw new Error('eth_signTypedData_v4 parsing error'); | |
| } | |
| const response = await dispatch( | |
| trezorConnectPopupActions.connectPopupCallThunk({ | |
| id: 0, | |
| method: 'ethereumSignTypedData', | |
| payload: { | |
| path: account.path, | |
| data: parsedData, | |
| metamask_v4_compat: true, | |
| device, | |
| useEmptyPassphrase: device?.useEmptyPassphrase, | |
| }, | |
| processName: 'WalletConnect', | |
| origin: event.verifyContext.verified.origin, | |
| }), | |
| ).unwrap(); | |
| if (!response.success) { | |
| console.error('eth_signTypedData_v4 error', response); | |
| throw new Error('eth_signTypedData_v4 error'); | |
| } | |
| return response.payload.signature; | |
| } |
| export const prepareWalletConnectMiddleware = createMiddlewareWithExtraDeps( | ||
| async (action, { dispatch, next, extra }) => { | ||
| await next(action); | ||
|
|
||
| if (accountsActions.updateSelectedAccount.match(action) && action.payload.account) { | ||
| dispatch( | ||
| walletConnectThunks.switchSelectedAccountThunk({ | ||
| account: action.payload.account, | ||
| }), | ||
| ); | ||
| } | ||
|
|
||
| if ( | ||
| accountsActions.createAccount.match(action) || | ||
| accountsActions.removeAccount.match(action) | ||
| ) { | ||
| dispatch(walletConnectThunks.updateAccountsThunk()); | ||
| } | ||
|
|
||
| if (walletConnectActions.createSessionProposal.match(action)) { | ||
| dispatch( | ||
| extra.actions.openModal({ | ||
| type: 'walletconnect-proposal', | ||
| eventId: action.payload.eventId, | ||
| }), | ||
| ); | ||
| } | ||
| if (walletConnectActions.clearSessionProposal.match(action)) { | ||
| dispatch(extra.actions.onModalCancel()); | ||
| } | ||
|
|
||
| return action; | ||
| }, | ||
| ); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider adding error handling for async operations.
The middleware implementation is well-structured, but it lacks error handling for the async operations.
Apply this improvement:
export const prepareWalletConnectMiddleware = createMiddlewareWithExtraDeps(
async (action, { dispatch, next, extra }) => {
+ try {
await next(action);
if (accountsActions.updateSelectedAccount.match(action) && action.payload.account) {
dispatch(
walletConnectThunks.switchSelectedAccountThunk({
account: action.payload.account,
}),
);
}
// ... rest of the middleware logic ...
return action;
+ } catch (error) {
+ console.error('WalletConnect middleware error:', error);
+ throw error;
+ }
},
);📝 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 const prepareWalletConnectMiddleware = createMiddlewareWithExtraDeps( | |
| async (action, { dispatch, next, extra }) => { | |
| await next(action); | |
| if (accountsActions.updateSelectedAccount.match(action) && action.payload.account) { | |
| dispatch( | |
| walletConnectThunks.switchSelectedAccountThunk({ | |
| account: action.payload.account, | |
| }), | |
| ); | |
| } | |
| if ( | |
| accountsActions.createAccount.match(action) || | |
| accountsActions.removeAccount.match(action) | |
| ) { | |
| dispatch(walletConnectThunks.updateAccountsThunk()); | |
| } | |
| if (walletConnectActions.createSessionProposal.match(action)) { | |
| dispatch( | |
| extra.actions.openModal({ | |
| type: 'walletconnect-proposal', | |
| eventId: action.payload.eventId, | |
| }), | |
| ); | |
| } | |
| if (walletConnectActions.clearSessionProposal.match(action)) { | |
| dispatch(extra.actions.onModalCancel()); | |
| } | |
| return action; | |
| }, | |
| ); | |
| export const prepareWalletConnectMiddleware = createMiddlewareWithExtraDeps( | |
| async (action, { dispatch, next, extra }) => { | |
| try { | |
| await next(action); | |
| if (accountsActions.updateSelectedAccount.match(action) && action.payload.account) { | |
| dispatch( | |
| walletConnectThunks.switchSelectedAccountThunk({ | |
| account: action.payload.account, | |
| }), | |
| ); | |
| } | |
| if ( | |
| accountsActions.createAccount.match(action) || | |
| accountsActions.removeAccount.match(action) | |
| ) { | |
| dispatch(walletConnectThunks.updateAccountsThunk()); | |
| } | |
| if (walletConnectActions.createSessionProposal.match(action)) { | |
| dispatch( | |
| extra.actions.openModal({ | |
| type: 'walletconnect-proposal', | |
| eventId: action.payload.eventId, | |
| }), | |
| ); | |
| } | |
| if (walletConnectActions.clearSessionProposal.match(action)) { | |
| dispatch(extra.actions.onModalCancel()); | |
| } | |
| return action; | |
| } catch (error) { | |
| console.error('WalletConnect middleware error:', error); | |
| throw error; | |
| } | |
| }, | |
| ); |
martykan
force-pushed
the
feat/walletconnect
branch
from
1cb0ba6 to
aa52ef7
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (1)
packages/suite/src/actions/suite/protocolActions.ts (1)
55-64: 🛠️ Refactor suggestionEnhance error handling and user feedback for WalletConnect URIs.
The current implementation lacks proper error handling and user feedback.
Apply this diff to improve error handling:
} else if (uri?.startsWith(SUITE_WALLETCONNECT_DEEPLINK)) { // This feature is currently only available in debug mode const isDebug = selectIsDebugModeActive(getState()); - if (!isDebug) return; + if (!isDebug) { + dispatch(notificationsActions.addToast({ + type: 'error', + message: 'WalletConnect is only available in debug mode', + })); + return; + } const parsedUri = parseUri(uri); + if (!parsedUri) { + dispatch(notificationsActions.addToast({ + type: 'error', + message: 'Invalid WalletConnect URI format', + })); + return; + } + const wcUri = parsedUri?.searchParams?.get('uri'); if (wcUri) { + // Validate WalletConnect URI format + if (!wcUri.startsWith('wc:')) { + dispatch(notificationsActions.addToast({ + type: 'error', + message: 'Invalid WalletConnect URI format', + })); + return; + } dispatch(walletConnectActions.walletConnectPairThunk({ uri: wcUri })); + } else { + dispatch(notificationsActions.addToast({ + type: 'error', + message: 'Missing WalletConnect URI parameter', + })); }
🧹 Nitpick comments (8)
packages/suite-desktop-connect-popup/src/connectPopupThunks.ts (2)
35-40: Consider implementing device selection wait logic.The TODO comment indicates missing functionality for waiting for device selection. This could lead to a poor user experience if the device is temporarily unavailable.
Would you like me to help implement the device selection wait logic? This could involve:
- Adding a timeout mechanism
- Implementing a device selection listener
- Adding retry logic with backoff
95-103: Consider removing theas anytype cast.The type cast could hide potential type mismatches. Consider using a more specific type assertion or restructuring the types to avoid the need for casting.
-): AsyncThunkAction< - ConnectPopupCallThunkResponse<M>, - ConnectPopupCallThunkParams<M>, - CustomThunkAPI -> => connectPopupCallThunkInner(params) as any; +): AsyncThunkAction< + ConnectPopupCallThunkResponse<M>, + ConnectPopupCallThunkParams<M>, + CustomThunkAPI +> => connectPopupCallThunkInner(params) as AsyncThunkAction< + ConnectPopupCallThunkResponse<M>, + ConnectPopupCallThunkParams<M>, + CustomThunkAPI +>;suite-common/walletconnect/src/adapters/ethereum.ts (1)
20-32: Add input validation for the address parameter.The
getAccounthelper function should validate that the input address is a valid Ethereum address before performing the comparison.const getAccount = (address: string, chainId?: number) => { + if (!address || !/^0x[a-fA-F0-9]{40}$/.test(address)) { + throw new Error('Invalid Ethereum address'); + } const account = selectAccounts(getState()).find(suite-common/walletconnect/src/walletConnectThunks.ts (5)
22-22: Consider improving the globalwalletKitvariable management.The global mutable
walletKitvariable could lead to race conditions and initialization issues. Consider:
- Moving it into Redux state for better state management
- Adding initialization checks
- Using a singleton pattern if global state is necessary
74-81: Enhance error handling in session authentication.The current error handling logs to console and sends a generic rejection. Consider:
- Distinguishing between different error types (user rejection vs. technical failure)
- Providing more specific error messages to the client
- Implementing proper error logging/monitoring
} catch (error) { - console.error(error); + // Log error with proper context + console.error('Session authentication failed:', { + error, + eventId: event.id, + }); + const errorResponse = error.message === 'Failed to sign message' + ? getSdkError('USER_REJECTED') + : getSdkError('GENERIC_ERROR', error.message); + await walletKit.rejectSessionAuthenticate({ id: event.id, - reason: getSdkError('USER_REJECTED'), + reason: errorResponse, }); }
110-120: Standardize error responses in session requests.The error response uses a hard-coded error code and doesn't follow a standard format. Consider:
- Using standard WalletConnect error codes
- Including more context in error responses
await walletKit.respondSessionRequest({ topic: event.topic, response: { id: event.id, jsonrpc: '2.0', error: { - code: 1, - message: error.message, + code: error.code || getSdkError('GENERIC_ERROR').code, + message: error.message, + data: { + type: error.name, + stack: process.env.NODE_ENV === 'development' ? error.stack : undefined, + }, }, }, });
213-220: Optimize namespace updates in account management.The current implementation creates unnecessary object spreads. Consider:
- Using object destructuring
- Implementing batch updates for multiple sessions
-const namespaces = { - ...oldNamespaces, - eip155: { - ...oldNamespaces.eip155, - accounts: updatedNamespaces.eip155.accounts, - chains: updatedNamespaces.eip155.chains, - }, -}; +const { accounts, chains } = updatedNamespaces.eip155; +const namespaces = { + ...oldNamespaces, + eip155: { + ...oldNamespaces.eip155, + accounts, + chains, + }, +};
279-292: Add error handling and success feedback to connection management.The connection management thunks could be enhanced with proper error handling and success notifications.
export const walletConnectPairThunk = createThunk<void, { uri: string }>( `${WALLETCONNECT_MODULE}/walletConnectPairThunk`, async ({ uri }) => { + try { await walletKit.pair({ uri }); + return { success: true }; + } catch (error) { + console.error('Failed to pair:', error); + throw error; + } }, );
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (33)
packages/components/src/components/Note/Note.tsx(1 hunks)packages/connect/src/events/call.ts(1 hunks)packages/suite-desktop-connect-popup/src/connectPopupThunks.ts(2 hunks)packages/suite-desktop-core/src/config.ts(1 hunks)packages/suite/jest.config.js(1 hunks)packages/suite/package.json(1 hunks)packages/suite/src/actions/suite/__tests__/initAction.test.ts(4 hunks)packages/suite/src/actions/suite/initAction.ts(2 hunks)packages/suite/src/actions/suite/protocolActions.ts(3 hunks)packages/suite/src/components/suite/modals/ReduxModal/UserContextModal/ConnectPopupModal.tsx(1 hunks)packages/suite/src/components/suite/modals/ReduxModal/UserContextModal/UserContextModal.tsx(2 hunks)packages/suite/src/components/suite/modals/ReduxModal/UserContextModal/WalletConnectProposalModal.tsx(1 hunks)packages/suite/src/middlewares/wallet/discoveryMiddleware.ts(2 hunks)packages/suite/src/middlewares/wallet/index.ts(2 hunks)packages/suite/src/reducers/wallet/index.ts(3 hunks)packages/suite/src/support/messages.ts(1 hunks)packages/suite/src/views/settings/SettingsDebug/SettingsDebug.tsx(2 hunks)packages/suite/src/views/settings/SettingsDebug/WalletConnect.tsx(1 hunks)packages/suite/tsconfig.json(1 hunks)packages/urls/src/deeplinks.ts(1 hunks)scripts/list-outdated-dependencies/connect-dependencies.txt(1 hunks)suite-common/suite-types/src/modal.ts(1 hunks)suite-common/walletconnect/package.json(1 hunks)suite-common/walletconnect/src/adapters/ethereum.ts(1 hunks)suite-common/walletconnect/src/adapters/index.ts(1 hunks)suite-common/walletconnect/src/index.ts(1 hunks)suite-common/walletconnect/src/walletConnectActions.ts(1 hunks)suite-common/walletconnect/src/walletConnectConstants.ts(1 hunks)suite-common/walletconnect/src/walletConnectMiddleware.ts(1 hunks)suite-common/walletconnect/src/walletConnectReducer.ts(1 hunks)suite-common/walletconnect/src/walletConnectThunks.ts(1 hunks)suite-common/walletconnect/src/walletConnectTypes.ts(1 hunks)suite-common/walletconnect/tsconfig.json(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (29)
- scripts/list-outdated-dependencies/connect-dependencies.txt
- packages/suite/package.json
- packages/suite/src/components/suite/modals/ReduxModal/UserContextModal/ConnectPopupModal.tsx
- packages/connect/src/events/call.ts
- packages/suite/src/middlewares/wallet/discoveryMiddleware.ts
- packages/suite/tsconfig.json
- packages/suite-desktop-core/src/config.ts
- packages/suite/src/components/suite/modals/ReduxModal/UserContextModal/UserContextModal.tsx
- packages/suite/src/actions/suite/tests/initAction.test.ts
- packages/suite/src/reducers/wallet/index.ts
- packages/suite/src/actions/suite/initAction.ts
- suite-common/suite-types/src/modal.ts
- suite-common/walletconnect/src/walletConnectMiddleware.ts
- packages/suite/src/views/settings/SettingsDebug/WalletConnect.tsx
- packages/suite/src/middlewares/wallet/index.ts
- packages/suite/jest.config.js
- suite-common/walletconnect/src/walletConnectActions.ts
- packages/urls/src/deeplinks.ts
- packages/suite/src/components/suite/modals/ReduxModal/UserContextModal/WalletConnectProposalModal.tsx
- packages/suite/src/views/settings/SettingsDebug/SettingsDebug.tsx
- packages/components/src/components/Note/Note.tsx
- suite-common/walletconnect/tsconfig.json
- suite-common/walletconnect/src/walletConnectConstants.ts
- suite-common/walletconnect/package.json
- suite-common/walletconnect/src/walletConnectTypes.ts
- suite-common/walletconnect/src/index.ts
- suite-common/walletconnect/src/walletConnectReducer.ts
- suite-common/walletconnect/src/adapters/index.ts
- packages/suite/src/support/messages.ts
⏰ Context from checks skipped due to timeout of 90000ms (18)
- GitHub Check: PR-check / node-override init-api-flaky
- GitHub Check: PR-check / node-authorizeCoinjoin cancelCoinjoinAuthorization passphrase unlockPath setBusy checkFirmwareAuthenticity keepSession cancel.test info.test resetDevice-api
- GitHub Check: PR-check / web-authorizeCoinjoin cancelCoinjoinAuthorization passphrase unlockPath setBusy checkFirmwareAuthenticity keepSession cancel.test info.test resetDevice-api
- GitHub Check: PR-check / web-override init-api-flaky
- GitHub Check: build-deploy
- GitHub Check: build-deploy
- GitHub Check: run-desktop-tests (@group=wallet, trezor-user-env-unix bitcoin-regtest)
- GitHub Check: EAS Update
- GitHub Check: run-desktop-tests (@group=settings, trezor-user-env-unix bitcoin-regtest)
- GitHub Check: transport-e2e-test
- GitHub Check: build-web
- GitHub Check: run-desktop-tests (@group=device-management, trezor-user-env-unix)
- GitHub Check: test
- GitHub Check: Analyze with CodeQL (javascript)
- GitHub Check: prepare_android_test_app
- GitHub Check: run-desktop-tests (@group=suite, trezor-user-env-unix)
- GitHub Check: build-web
- GitHub Check: Socket Security: Pull Request Alerts
🔇 Additional comments (7)
packages/suite/src/actions/suite/protocolActions.ts (2)
4-5: LGTM!The new imports are well-organized and necessary for the WalletConnect integration.
Also applies to: 9-11
37-37: LGTM!The addition of
getStateparameter is appropriate for accessing the debug mode state.packages/suite-desktop-connect-popup/src/connectPopupThunks.ts (2)
1-24: LGTM! Well-structured type definitions.The new type definitions provide excellent type safety through proper use of generics and method-specific type constraints.
109-113: Race Condition Issue in the 'connect-popup/call' HandlerThe event handler still lacks request tracking to handle concurrent calls.
Consider improving type definitions instead of using @ts-expect-error.
The
@ts-expect-errorcomment suggests that the desktop API types need improvement.Consider defining proper types for the desktop API parameters:
interface ConnectPopupCallParams<M extends keyof typeof TrezorConnect> { id: number; method: M; payload: Omit<CallMethodParams<M>, 'method'>; processName?: string; origin?: string; }suite-common/walletconnect/src/adapters/ethereum.ts (3)
1-12: LGTM! Clean and well-organized imports.The imports are properly organized and use named imports for better tree-shaking.
60-84: Handle JSON parsing errors ineth_signTypedData_v4.If
datais invalid JSON, callingJSON.parse(data)will throw. Wrap the parsing in a try/catch block.
155-164: LGTM! Well-typed adapter export.The adapter export correctly uses the
satisfiesoperator to ensure type safety and lists all implemented methods.
| case 'wallet_switchEthereumChain': { | ||
| const [chainId] = event.params.request.params; | ||
|
|
||
| return chainId; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add chain ID validation in wallet_switchEthereumChain.
The function should validate that the chain ID is supported before returning it.
case 'wallet_switchEthereumChain': {
const [chainId] = event.params.request.params;
+ const supportedChains = ['1', '5', '137']; // Add your supported chain IDs
+ if (!supportedChains.includes(chainId)) {
+ throw new Error(`Unsupported chain ID: ${chainId}`);
+ }
return chainId;
}📝 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.
| case 'wallet_switchEthereumChain': { | |
| const [chainId] = event.params.request.params; | |
| return chainId; | |
| } | |
| case 'wallet_switchEthereumChain': { | |
| const [chainId] = event.params.request.params; | |
| const supportedChains = ['1', '5', '137']; // Add your supported chain IDs | |
| if (!supportedChains.includes(chainId)) { | |
| throw new Error(`Unsupported chain ID: ${chainId}`); | |
| } | |
| return chainId; | |
| } |
| case 'eth_sendTransaction': { | ||
| const chainId = Number(event.params.chainId.replace('eip155:', '')); | ||
| const transaction = event.params.request.params[0]; | ||
| const account = getAccount(transaction.from, chainId); | ||
| if (account.networkType !== 'ethereum') { | ||
| throw new Error('Account is not Ethereum'); | ||
| } | ||
| if (!transaction.gasPrice) { | ||
| // Fee not provided, estimate it | ||
| const feeLevels = await TrezorConnect.blockchainEstimateFee({ | ||
| coin: account.symbol, | ||
| identity: getAccountIdentity(account), | ||
| request: { | ||
| blocks: [2], | ||
| specific: { | ||
| from: account.descriptor, | ||
| }, | ||
| }, | ||
| }); | ||
| if (!feeLevels.success) { | ||
| throw new Error('eth_sendTransaction cannot estimate fee'); | ||
| } | ||
| transaction.gasPrice = feeLevels.payload.levels[0]?.feePerUnit; | ||
| } | ||
| const payload = { | ||
| path: account.path, | ||
| transaction: { | ||
| ...transaction, | ||
| gasLimit: transaction.gas ?? '21000', | ||
| nonce: account.misc.nonce, | ||
| chainId, | ||
| push: true, | ||
| }, | ||
| device, | ||
| useEmptyPassphrase: device?.useEmptyPassphrase, | ||
| }; | ||
| const signResponse = await dispatch( | ||
| trezorConnectPopupActions.connectPopupCallThunk({ | ||
| id: 0, | ||
| method: 'ethereumSignTransaction', | ||
| payload, | ||
| processName: 'WalletConnect', | ||
| origin: event.verifyContext.verified.origin, | ||
| }), | ||
| ).unwrap(); | ||
| if (!signResponse.success) { | ||
| console.error('eth_sendTransaction error', signResponse); | ||
| throw new Error('eth_sendTransaction error'); | ||
| } | ||
|
|
||
| const pushResponse = await TrezorConnect.pushTransaction({ | ||
| coin: account.symbol, | ||
| identity: getAccountIdentity(account), | ||
| tx: signResponse.payload.serializedTx, | ||
| }); | ||
| if (!pushResponse.success) { | ||
| console.error('eth_sendTransaction push error', pushResponse); | ||
| throw new Error('eth_sendTransaction push error'); | ||
| } | ||
|
|
||
| return pushResponse.payload.txid; | ||
| } |
There was a problem hiding this comment.
Potential race condition with nonce in eth_sendTransaction.
Using account.misc.nonce directly could lead to incorrect nonce if multiple transactions are sent in quick succession. Consider fetching the latest nonce from the network.
- nonce: account.misc.nonce,
+ nonce: await TrezorConnect.getAccountInfo({
+ coin: account.symbol,
+ descriptor: account.descriptor,
+ }).then(response =>
+ response.success ? response.payload.nonce :
+ Promise.reject(new Error('Failed to fetch nonce'))
+ ),📝 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.
| case 'eth_sendTransaction': { | |
| const chainId = Number(event.params.chainId.replace('eip155:', '')); | |
| const transaction = event.params.request.params[0]; | |
| const account = getAccount(transaction.from, chainId); | |
| if (account.networkType !== 'ethereum') { | |
| throw new Error('Account is not Ethereum'); | |
| } | |
| if (!transaction.gasPrice) { | |
| // Fee not provided, estimate it | |
| const feeLevels = await TrezorConnect.blockchainEstimateFee({ | |
| coin: account.symbol, | |
| identity: getAccountIdentity(account), | |
| request: { | |
| blocks: [2], | |
| specific: { | |
| from: account.descriptor, | |
| }, | |
| }, | |
| }); | |
| if (!feeLevels.success) { | |
| throw new Error('eth_sendTransaction cannot estimate fee'); | |
| } | |
| transaction.gasPrice = feeLevels.payload.levels[0]?.feePerUnit; | |
| } | |
| const payload = { | |
| path: account.path, | |
| transaction: { | |
| ...transaction, | |
| gasLimit: transaction.gas ?? '21000', | |
| nonce: account.misc.nonce, | |
| chainId, | |
| push: true, | |
| }, | |
| device, | |
| useEmptyPassphrase: device?.useEmptyPassphrase, | |
| }; | |
| const signResponse = await dispatch( | |
| trezorConnectPopupActions.connectPopupCallThunk({ | |
| id: 0, | |
| method: 'ethereumSignTransaction', | |
| payload, | |
| processName: 'WalletConnect', | |
| origin: event.verifyContext.verified.origin, | |
| }), | |
| ).unwrap(); | |
| if (!signResponse.success) { | |
| console.error('eth_sendTransaction error', signResponse); | |
| throw new Error('eth_sendTransaction error'); | |
| } | |
| const pushResponse = await TrezorConnect.pushTransaction({ | |
| coin: account.symbol, | |
| identity: getAccountIdentity(account), | |
| tx: signResponse.payload.serializedTx, | |
| }); | |
| if (!pushResponse.success) { | |
| console.error('eth_sendTransaction push error', pushResponse); | |
| throw new Error('eth_sendTransaction push error'); | |
| } | |
| return pushResponse.payload.txid; | |
| } | |
| case 'eth_sendTransaction': { | |
| const chainId = Number(event.params.chainId.replace('eip155:', '')); | |
| const transaction = event.params.request.params[0]; | |
| const account = getAccount(transaction.from, chainId); | |
| if (account.networkType !== 'ethereum') { | |
| throw new Error('Account is not Ethereum'); | |
| } | |
| if (!transaction.gasPrice) { | |
| // Fee not provided, estimate it | |
| const feeLevels = await TrezorConnect.blockchainEstimateFee({ | |
| coin: account.symbol, | |
| identity: getAccountIdentity(account), | |
| request: { | |
| blocks: [2], | |
| specific: { | |
| from: account.descriptor, | |
| }, | |
| }, | |
| }); | |
| if (!feeLevels.success) { | |
| throw new Error('eth_sendTransaction cannot estimate fee'); | |
| } | |
| transaction.gasPrice = feeLevels.payload.levels[0]?.feePerUnit; | |
| } | |
| const payload = { | |
| path: account.path, | |
| transaction: { | |
| ...transaction, | |
| gasLimit: transaction.gas ?? '21000', | |
| - nonce: account.misc.nonce, | |
| + nonce: await TrezorConnect.getAccountInfo({ | |
| + coin: account.symbol, | |
| + descriptor: account.descriptor, | |
| + }).then(response => | |
| + response.success ? response.payload.nonce : | |
| + Promise.reject(new Error('Failed to fetch nonce')) | |
| + ), | |
| chainId, | |
| push: true, | |
| }, | |
| device, | |
| useEmptyPassphrase: device?.useEmptyPassphrase, | |
| }; | |
| const signResponse = await dispatch( | |
| trezorConnectPopupActions.connectPopupCallThunk({ | |
| id: 0, | |
| method: 'ethereumSignTransaction', | |
| payload, | |
| processName: 'WalletConnect', | |
| origin: event.verifyContext.verified.origin, | |
| }), | |
| ).unwrap(); | |
| if (!signResponse.success) { | |
| console.error('eth_sendTransaction error', signResponse); | |
| throw new Error('eth_sendTransaction error'); | |
| } | |
| const pushResponse = await TrezorConnect.pushTransaction({ | |
| coin: account.symbol, | |
| identity: getAccountIdentity(account), | |
| tx: signResponse.payload.serializedTx, | |
| }); | |
| if (!pushResponse.success) { | |
| console.error('eth_sendTransaction push error', pushResponse); | |
| throw new Error('eth_sendTransaction push error'); | |
| } | |
| return pushResponse.payload.txid; | |
| } |
| const pendingProposal = selectPendingProposal(getState()); | ||
| if (!pendingProposal || pendingProposal.eventId !== eventId) { | ||
| throw new Error('Proposal not found'); | ||
| } |
There was a problem hiding this comment.
Add expiry validation to prevent race conditions in session proposal approval.
The current implementation doesn't validate if the proposal has expired before approval. Consider:
- Checking proposal expiry status
- Using atomic operations where possible
const pendingProposal = selectPendingProposal(getState());
-if (!pendingProposal || pendingProposal.eventId !== eventId) {
+if (!pendingProposal || pendingProposal.eventId !== eventId || pendingProposal.expired) {
throw new Error('Proposal not found');
}📝 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 pendingProposal = selectPendingProposal(getState()); | |
| if (!pendingProposal || pendingProposal.eventId !== eventId) { | |
| throw new Error('Proposal not found'); | |
| } | |
| const pendingProposal = selectPendingProposal(getState()); | |
| if (!pendingProposal || pendingProposal.eventId !== eventId || pendingProposal.expired) { | |
| throw new Error('Proposal not found'); | |
| } |
| export const walletConnectInitThunk = createThunk( | ||
| `${WALLETCONNECT_MODULE}/walletConnectInitThunk`, | ||
| async (_, { dispatch }) => { | ||
| const core = new Core({ | ||
| projectId: PROJECT_ID, | ||
| }); | ||
|
|
||
| walletKit = await WalletKit.init({ | ||
| core, | ||
| metadata: WALLETCONNECT_METADATA, | ||
| }); | ||
|
|
||
| walletKit.on('session_proposal', event => { | ||
| dispatch( | ||
| walletConnectActions.createSessionProposal({ | ||
| eventId: event.id, | ||
| params: event.params, | ||
| expired: false, | ||
| ...event.verifyContext.verified, | ||
| }), | ||
| ); | ||
| }); | ||
|
|
||
| walletKit.on('proposal_expire', () => { | ||
| dispatch(walletConnectActions.expireSessionProposal()); | ||
| }); | ||
|
|
||
| walletKit.on('session_request', event => { | ||
| dispatch(sessionRequestThunk({ event })); | ||
| }); | ||
|
|
||
| walletKit.on('session_authenticate', event => { | ||
| dispatch(sessionAuthenticateThunk({ event })); | ||
| }); | ||
|
|
||
| walletKit.on('session_delete', event => { | ||
| dispatch(walletConnectActions.removeSession({ topic: event.topic })); | ||
| }); | ||
|
|
||
| const sessions = walletKit.getActiveSessions(); | ||
| for (const topic in sessions) { | ||
| dispatch( | ||
| walletConnectActions.saveSession({ | ||
| ...sessions[topic], | ||
| }), | ||
| ); | ||
| } | ||
| }, | ||
| ); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve robustness of WalletConnect initialization.
The initialization process could be enhanced with:
- Proper cleanup of event listeners
- Error handling for initialization failures
- Session restoration error handling
export const walletConnectInitThunk = createThunk(
`${WALLETCONNECT_MODULE}/walletConnectInitThunk`,
async (_, { dispatch }) => {
+ try {
const core = new Core({
projectId: PROJECT_ID,
});
walletKit = await WalletKit.init({
core,
metadata: WALLETCONNECT_METADATA,
});
+ const cleanup = () => {
+ walletKit.removeAllListeners();
+ walletKit = null;
+ };
+
walletKit.on('session_proposal', event => {
dispatch(
walletConnectActions.createSessionProposal({
eventId: event.id,
params: event.params,
expired: false,
...event.verifyContext.verified,
}),
);
});
// ... other event listeners ...
- const sessions = walletKit.getActiveSessions();
- for (const topic in sessions) {
- dispatch(
- walletConnectActions.saveSession({
- ...sessions[topic],
- }),
- );
+ try {
+ const sessions = walletKit.getActiveSessions();
+ await Promise.all(
+ Object.entries(sessions).map(([_, session]) =>
+ dispatch(walletConnectActions.saveSession(session)),
+ ),
+ );
+ } catch (error) {
+ console.error('Failed to restore sessions:', error);
+ // Continue initialization even if session restoration fails
}
+
+ return cleanup;
+ } catch (error) {
+ console.error('Failed to initialize WalletConnect:', error);
+ throw error;
+ }
},
);📝 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 const walletConnectInitThunk = createThunk( | |
| `${WALLETCONNECT_MODULE}/walletConnectInitThunk`, | |
| async (_, { dispatch }) => { | |
| const core = new Core({ | |
| projectId: PROJECT_ID, | |
| }); | |
| walletKit = await WalletKit.init({ | |
| core, | |
| metadata: WALLETCONNECT_METADATA, | |
| }); | |
| walletKit.on('session_proposal', event => { | |
| dispatch( | |
| walletConnectActions.createSessionProposal({ | |
| eventId: event.id, | |
| params: event.params, | |
| expired: false, | |
| ...event.verifyContext.verified, | |
| }), | |
| ); | |
| }); | |
| walletKit.on('proposal_expire', () => { | |
| dispatch(walletConnectActions.expireSessionProposal()); | |
| }); | |
| walletKit.on('session_request', event => { | |
| dispatch(sessionRequestThunk({ event })); | |
| }); | |
| walletKit.on('session_authenticate', event => { | |
| dispatch(sessionAuthenticateThunk({ event })); | |
| }); | |
| walletKit.on('session_delete', event => { | |
| dispatch(walletConnectActions.removeSession({ topic: event.topic })); | |
| }); | |
| const sessions = walletKit.getActiveSessions(); | |
| for (const topic in sessions) { | |
| dispatch( | |
| walletConnectActions.saveSession({ | |
| ...sessions[topic], | |
| }), | |
| ); | |
| } | |
| }, | |
| ); | |
| export const walletConnectInitThunk = createThunk( | |
| `${WALLETCONNECT_MODULE}/walletConnectInitThunk`, | |
| async (_, { dispatch }) => { | |
| try { | |
| const core = new Core({ | |
| projectId: PROJECT_ID, | |
| }); | |
| walletKit = await WalletKit.init({ | |
| core, | |
| metadata: WALLETCONNECT_METADATA, | |
| }); | |
| const cleanup = () => { | |
| walletKit.removeAllListeners(); | |
| walletKit = null; | |
| }; | |
| walletKit.on('session_proposal', event => { | |
| dispatch( | |
| walletConnectActions.createSessionProposal({ | |
| eventId: event.id, | |
| params: event.params, | |
| expired: false, | |
| ...event.verifyContext.verified, | |
| }), | |
| ); | |
| }); | |
| walletKit.on('proposal_expire', () => { | |
| dispatch(walletConnectActions.expireSessionProposal()); | |
| }); | |
| walletKit.on('session_request', event => { | |
| dispatch(sessionRequestThunk({ event })); | |
| }); | |
| walletKit.on('session_authenticate', event => { | |
| dispatch(sessionAuthenticateThunk({ event })); | |
| }); | |
| walletKit.on('session_delete', event => { | |
| dispatch(walletConnectActions.removeSession({ topic: event.topic })); | |
| }); | |
| try { | |
| const sessions = walletKit.getActiveSessions(); | |
| await Promise.all( | |
| Object.entries(sessions).map(([_, session]) => | |
| dispatch(walletConnectActions.saveSession(session)), | |
| ), | |
| ); | |
| } catch (error) { | |
| console.error('Failed to restore sessions:', error); | |
| // Continue initialization even if session restoration fails | |
| } | |
| return cleanup; | |
| } catch (error) { | |
| console.error('Failed to initialize WalletConnect:', error); | |
| throw error; | |
| } | |
| }, | |
| ); |
martykan
force-pushed
the
feat/walletconnect
branch
from
aa52ef7 to
d063c79
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
packages/suite-desktop-connect-popup/src/connectPopupThunks.ts (1)
109-113:⚠️ Potential issueRace Condition Issue in the 'connect-popup/call' Handler
The event handler still doesn't handle potential race conditions from multiple concurrent calls.
Consider adding request tracking:
+const pendingRequests = new Map(); + desktopApi.on('connect-popup/call', async params => { + if (pendingRequests.has(params.id)) { + console.warn(`Request ${params.id} is already being processed`); + return; + } + pendingRequests.set(params.id, true); const response = await dispatch(connectPopupCallThunk(params)).unwrap(); desktopApi.connectPopupResponse(response); + pendingRequests.delete(params.id); });
🧹 Nitpick comments (5)
packages/suite-desktop-connect-popup/src/connectPopupThunks.ts (3)
82-91: Enhance error logging for better debugging.While the error handling is comprehensive, the error logging could be more detailed.
Consider enhancing the error logging:
- console.error('connectPopupCallThunk', error); + console.error('connectPopupCallThunk failed:', { + method: method, + error, + devicePresent: !!selectSelectedDevice(getState()) + });
95-103: Improve type assertion safety.While the type assertion is necessary, we can make it safer by using a more specific type.
Consider this improvement:
-): AsyncThunkAction< +): AsyncThunkAction< ConnectPopupCallThunkResponse<M>, ConnectPopupCallThunkParams<M>, CustomThunkAPI -> => connectPopupCallThunkInner(params) as any; +> => connectPopupCallThunkInner(params) as AsyncThunkAction< + ConnectPopupCallThunkResponse<M>, + ConnectPopupCallThunkParams<M>, + CustomThunkAPI +>;
110-111: Improve type safety for desktop API params.Instead of using @ts-expect-error, consider defining proper types for the desktop API parameters.
Create an interface for the desktop API parameters:
interface ConnectPopupCallParams<M extends keyof typeof TrezorConnect> { id: number; method: M; payload: Omit<CallMethodParams<M>, 'method'>; processName?: string; origin?: string; }suite-common/walletconnect/src/walletConnectThunks.ts (2)
22-22: Consider safer state management for WalletKit instance.Using a global mutable variable for the WalletKit instance could lead to race conditions and makes it harder to track the initialization state.
Consider one of these alternatives:
- Move the instance to Redux state
- Use a class-based service pattern
- Add null checks and type safety:
-let walletKit: WalletKitClient; +let walletKit: WalletKitClient | null = null;
115-118: Enhance error handling in session requests.The error code is hardcoded and the error message might not provide enough context for debugging.
Consider using standardized error codes and more descriptive messages:
- code: 1, - message: error.message, + code: getSdkError('USER_REJECTED').code, + message: `Request failed: ${error.message}`,
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (33)
packages/components/src/components/Note/Note.tsx(1 hunks)packages/connect/src/events/call.ts(1 hunks)packages/suite-desktop-connect-popup/src/connectPopupThunks.ts(2 hunks)packages/suite-desktop-core/src/config.ts(1 hunks)packages/suite/jest.config.js(1 hunks)packages/suite/package.json(1 hunks)packages/suite/src/actions/suite/__tests__/initAction.test.ts(4 hunks)packages/suite/src/actions/suite/initAction.ts(2 hunks)packages/suite/src/actions/suite/protocolActions.ts(3 hunks)packages/suite/src/components/suite/modals/ReduxModal/UserContextModal/ConnectPopupModal.tsx(1 hunks)packages/suite/src/components/suite/modals/ReduxModal/UserContextModal/UserContextModal.tsx(2 hunks)packages/suite/src/components/suite/modals/ReduxModal/UserContextModal/WalletConnectProposalModal.tsx(1 hunks)packages/suite/src/middlewares/wallet/discoveryMiddleware.ts(2 hunks)packages/suite/src/middlewares/wallet/index.ts(2 hunks)packages/suite/src/reducers/wallet/index.ts(3 hunks)packages/suite/src/support/messages.ts(1 hunks)packages/suite/src/views/settings/SettingsDebug/SettingsDebug.tsx(2 hunks)packages/suite/src/views/settings/SettingsDebug/WalletConnect.tsx(1 hunks)packages/suite/tsconfig.json(1 hunks)packages/urls/src/deeplinks.ts(1 hunks)scripts/list-outdated-dependencies/connect-dependencies.txt(1 hunks)suite-common/suite-types/src/modal.ts(1 hunks)suite-common/walletconnect/package.json(1 hunks)suite-common/walletconnect/src/adapters/ethereum.ts(1 hunks)suite-common/walletconnect/src/adapters/index.ts(1 hunks)suite-common/walletconnect/src/index.ts(1 hunks)suite-common/walletconnect/src/walletConnectActions.ts(1 hunks)suite-common/walletconnect/src/walletConnectConstants.ts(1 hunks)suite-common/walletconnect/src/walletConnectMiddleware.ts(1 hunks)suite-common/walletconnect/src/walletConnectReducer.ts(1 hunks)suite-common/walletconnect/src/walletConnectThunks.ts(1 hunks)suite-common/walletconnect/src/walletConnectTypes.ts(1 hunks)suite-common/walletconnect/tsconfig.json(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (31)
- packages/urls/src/deeplinks.ts
- packages/suite-desktop-core/src/config.ts
- packages/suite/tsconfig.json
- suite-common/walletconnect/tsconfig.json
- scripts/list-outdated-dependencies/connect-dependencies.txt
- packages/suite/package.json
- packages/suite/src/middlewares/wallet/index.ts
- suite-common/walletconnect/src/index.ts
- suite-common/walletconnect/package.json
- packages/suite/src/views/settings/SettingsDebug/WalletConnect.tsx
- packages/connect/src/events/call.ts
- suite-common/suite-types/src/modal.ts
- packages/suite/src/components/suite/modals/ReduxModal/UserContextModal/UserContextModal.tsx
- packages/suite/jest.config.js
- packages/suite/src/reducers/wallet/index.ts
- packages/suite/src/middlewares/wallet/discoveryMiddleware.ts
- packages/suite/src/actions/suite/tests/initAction.test.ts
- packages/suite/src/views/settings/SettingsDebug/SettingsDebug.tsx
- suite-common/walletconnect/src/walletConnectActions.ts
- packages/suite/src/actions/suite/initAction.ts
- suite-common/walletconnect/src/adapters/ethereum.ts
- suite-common/walletconnect/src/walletConnectTypes.ts
- packages/components/src/components/Note/Note.tsx
- suite-common/walletconnect/src/walletConnectReducer.ts
- packages/suite/src/components/suite/modals/ReduxModal/UserContextModal/ConnectPopupModal.tsx
- suite-common/walletconnect/src/walletConnectConstants.ts
- packages/suite/src/components/suite/modals/ReduxModal/UserContextModal/WalletConnectProposalModal.tsx
- packages/suite/src/actions/suite/protocolActions.ts
- suite-common/walletconnect/src/walletConnectMiddleware.ts
- suite-common/walletconnect/src/adapters/index.ts
- packages/suite/src/support/messages.ts
⏰ Context from checks skipped due to timeout of 90000ms (15)
- GitHub Check: build-deploy
- GitHub Check: build-deploy
- GitHub Check: run-desktop-tests (@group=wallet, trezor-user-env-unix bitcoin-regtest)
- GitHub Check: run-desktop-tests (@group=settings, trezor-user-env-unix bitcoin-regtest)
- GitHub Check: EAS Update
- GitHub Check: run-desktop-tests (@group=device-management, trezor-user-env-unix)
- GitHub Check: run-desktop-tests (@group=suite, trezor-user-env-unix)
- GitHub Check: Analyze with CodeQL (javascript)
- GitHub Check: prepare_android_test_app
- GitHub Check: transport-e2e-test
- GitHub Check: build-web
- GitHub Check: Setup and Cache Dependencies
- GitHub Check: build-web
- GitHub Check: test
- GitHub Check: Socket Security: Pull Request Alerts
🔇 Additional comments (4)
packages/suite-desktop-connect-popup/src/connectPopupThunks.ts (2)
1-24: LGTM! Type definitions provide excellent type safety.The generic type definitions with constraints ensure type safety throughout the thunk implementation. This effectively addresses the previous type safety concerns for payload.
35-40: Address the TODO comment about device selection.The current implementation throws an error when no device is found, but there's a TODO comment suggesting we should wait for device selection. This could lead to a poor user experience.
Would you like me to help implement the device selection wait logic? This could involve:
- Adding a timeout mechanism
- Implementing a device selection listener
- Adding retry logic
suite-common/walletconnect/src/walletConnectThunks.ts (2)
133-136: Add expiry validation to prevent race conditions in session proposal approval.The current implementation doesn't validate if the proposal has expired before approval.
229-277: Improve robustness of WalletConnect initialization.The initialization process could be enhanced with proper cleanup and error handling.
| export const walletConnectPairThunk = createThunk<void, { uri: string }>( | ||
| `${WALLETCONNECT_MODULE}/walletConnectPairThunk`, | ||
| async ({ uri }) => { | ||
| await walletKit.pair({ uri }); | ||
| }, | ||
| ); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling for pairing failures.
The pairing thunk doesn't handle potential errors that could occur during the pairing process.
Add error handling:
export const walletConnectPairThunk = createThunk<void, { uri: string }>(
`${WALLETCONNECT_MODULE}/walletConnectPairThunk`,
async ({ uri }) => {
+ try {
await walletKit.pair({ uri });
+ } catch (error) {
+ console.error('Failed to pair:', error.message);
+ throw new Error('Failed to establish WalletConnect pairing');
+ }
},
);📝 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 const walletConnectPairThunk = createThunk<void, { uri: string }>( | |
| `${WALLETCONNECT_MODULE}/walletConnectPairThunk`, | |
| async ({ uri }) => { | |
| await walletKit.pair({ uri }); | |
| }, | |
| ); | |
| export const walletConnectPairThunk = createThunk<void, { uri: string }>( | |
| `${WALLETCONNECT_MODULE}/walletConnectPairThunk`, | |
| async ({ uri }) => { | |
| try { | |
| await walletKit.pair({ uri }); | |
| } catch (error) { | |
| console.error('Failed to pair:', error.message); | |
| throw new Error('Failed to establish WalletConnect pairing'); | |
| } | |
| }, | |
| ); |
| const network = getNetwork(account.symbol); | ||
| const sessions = await walletKit.getActiveSessions(); | ||
| for (const topic in sessions) { | ||
| walletKit.emitSessionEvent({ | ||
| topic, | ||
| event: { | ||
| name: 'chainChanged', | ||
| data: network.chainId, | ||
| }, | ||
| chainId: `eip155:${network.chainId}`, | ||
| }); |
There was a problem hiding this comment.
💡 Verification agent
🛠️ Refactor suggestion
🧩 Analysis chain
Add validation for network and chainId.
The code assumes the network exists and doesn't validate the chainId format.
Add validation:
const network = getNetwork(account.symbol);
+ if (!network?.chainId) {
+ throw new Error(`Unsupported network: ${account.symbol}`);
+ }🏁 Script executed:
#!/bin/bash
# Check for potential undefined networks
rg -A 2 "getNetwork\(" | grep -B 2 "chainId"Length of output: 1338
Add validation for network and chainId in walletConnectThunks.ts
The code in the walletConnectThunks.ts file currently assumes that the result from getNetwork(account.symbol) is always valid and that network.chainId is defined. This assumption can lead to runtime errors if an unsupported account symbol is encountered or if the network object lacks a valid chainId. Other parts of the codebase (e.g., in send form thunks) already have similar validations that prevent using an undefined chainId.
- Ensure that
networkis defined. - Validate that
network.chainIdexists and is in the expected format before using it in any walletKit event emissions.
| } catch (error) { | ||
| console.error(error); | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve error handling in session authentication.
Using console.error directly might leak sensitive information in production.
Consider implementing proper error logging:
- console.error(error);
+ // Log sanitized error information
+ console.error('Session authentication 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.
| } catch (error) { | |
| console.error(error); | |
| } catch (error) { | |
| // Log sanitized error information | |
| console.error('Session authentication failed:', error.message); |
Description
Add support for WalletConnect to Suite.
All the related operations are in a new package (
@suite-common/walletconnect) as Redux thunks.Currently only EVM chains are supported, but there is an extensible interface for adding other chains and methods.
Deeplink scheme is
trezorsuite://walletconnectScreenshots:
Connection request
Method call request
Connection manager (under Debug)
