WalletConnect Mobile #16877
WalletConnect Mobile #16877
Conversation
WalkthroughThis pull request restructures connect popup functionality across the codebase. The previous desktop connect popup module has been removed, with its actions, thunks, and configurations replaced by a new package, @suite-common/connect-popup. New Redux actions, thunks, middleware, and reducers have been introduced for managing connection popups, including deep link processing and wallet connection flows. The changes update related components, navigation routes, TypeScript configurations, and package dependencies to reflect this architectural shift. Additionally, dependencies and configuration references have been updated or removed across desktop, native, and common modules. The refactor includes modifications to feature flags and associated tests, as well as adjustments in wallet and navigation middleware, aligning the project's state management and UI components with the new, centralized connect popup and wallet connect functionality. ✨ 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-mobile
branch
3 times, most recently
from
b3b50c8 to
bf3c833
Compare
martykan
force-pushed
the
feat/walletconnect-mobile
branch
from
bf3c833 to
8ae1d9f
Compare
|
🚀 Expo preview is ready!
|
martykan
force-pushed
the
feat/walletconnect-mobile
branch
2 times, most recently
from
c7dc3aa to
037a898
Compare
martykan
force-pushed
the
feat/walletconnect
branch
from
d063c79 to
f9ee0ec
Compare
martykan
force-pushed
the
feat/walletconnect-mobile
branch
from
037a898 to
33b746f
Compare
martykan
force-pushed
the
feat/walletconnect
branch
3 times, most recently
from
91ffe0b to
2384bfa
Compare
martykan
force-pushed
the
feat/walletconnect-mobile
branch
from
33b746f to
0fe6867
Compare
martykan
force-pushed
the
feat/walletconnect-mobile
branch
2 times, most recently
from
5b61472 to
586433c
Compare
martykan
force-pushed
the
feat/walletconnect-mobile
branch
3 times, most recently
from
6c6ea61 to
cd2c2f2
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (14)
packages/suite/src/components/suite/modals/ReduxModal/UserContextModal/ConnectPopupModal.tsx (2)
9-12: Handle the gray area when 'popupCall' is missing or not in 'request' state.
Returningnullhere provides no user feedback if the popup is not ready or has shifted out of the'request'state. You might consider displaying a fallback view or explanatory message, so the user isn’t left unaware.
19-33: Consider a fallback label and better accessibility for the buttons.
While you already provide a default translation for confirm (TR_CONFIRM) and cancel (TR_CANCEL), adding ARIA labels or descriptive text for assistive technologies may improve accessibility. Also, ifconfirmLabelis missing or empty, ensure the translation fallback is consistently applied.Also applies to: 36-51
suite-native/module-connect-popup/src/screens/ConnectPopupScreen.tsx (2)
60-64: Introduce fallback messaging for missing 'origin'.
IfpopupCall.stateis'request'butoriginis unset (line 62-63), you show an error. This is good, but also consider providing a way for the user to retry or access more context. They may have landed in this state inadvertently.Also applies to: 74-79
99-114: Consolidate error message mapping for 'call-error' state.
The inline functiongetTranslationId()is helpful, but you might centralize such logic in a separate util or constants file for easier maintenance if new error codes appear.suite-common/connect-popup/src/connectPopupThunks.ts (4)
46-55: Validate permissions before dispatching error.
You rely on string matching inmethodInfo.payload.requiredPermissions. If the payload lacks these properties or includes custom strings, it might incorrectly trigger an error. Ensure both list membership and valid permission types are handled.
57-71: Verify deferred confirmation flow.
The flow usescreateDeferred()and then awaitsconfirmation.promise, which can be fragile if other code accidentally re-triggers the same deferred or if the user flow has unexpected steps. Consider clarifying or documenting the user flow so future maintainers understand how it’s expected to proceed.
95-106: Improve error reporting for debugging.
While you log to the console on error, consider grouping or labeling the message to quickly pinpoint which part of the flow failed. This can speed up debugging. For instance, you could log'connectPopupCallThunkInner error'along withserializeError(error).
126-126: Utilize optional chaining to simplify null checks.
Static analysis suggests using optional chaining instead of&&checks to avoid potential negative side effects.Below is an example fix:
- const version = path && path.split('/').slice(-2, -1)[0]; + const version = path?.split('/')?.slice(-2, -1)?.[0];🧰 Tools
🪛 Biome (1.9.4)
[error] 126-126: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
suite-common/connect-popup/src/connectPopupTypes.ts (1)
4-24: LGTM! Well-structured type definition with clear state discrimination.The
ConnectPopupCalltype effectively models different states of a connection popup, with appropriate properties for each state. The use of discriminated unions with thestateproperty makes it type-safe and easy to handle different cases.Consider adding JSDoc comments to document the purpose of each state and its properties, especially for the
requeststate which has several optional properties. This would improve code maintainability and help other developers understand the type's usage.suite-common/connect-popup/src/connectPopupMiddleware.ts (1)
5-21: LGTM! Consider handling finishCall action for modal closure.The middleware effectively manages the modal state based on connect popup actions. However, there's one potential improvement:
Consider adding
finishCallto the list of actions that trigger modal closure:if ( connectPopupActions.approveCall.match(action) || - connectPopupActions.rejectCall.match(action) + connectPopupActions.rejectCall.match(action) || + connectPopupActions.finishCall.match(action) ) { dispatch(extra.actions.onModalCancel()); }suite-common/connect-popup/src/connectPopupReducer.ts (2)
28-36: Consider adding state validation in approve/reject handlers.The current implementation might have a race condition if multiple approve/reject actions are dispatched simultaneously. Consider adding state validation to prevent this:
.addCase(connectPopupActions.approveCall, state => { - if (state.activeCall?.state === 'request') state.activeCall.confirmation.resolve(); + if (state.activeCall?.state === 'request') { + state.activeCall.confirmation.resolve(); + state.activeCall = undefined; + } - state.activeCall = undefined; }) .addCase(connectPopupActions.rejectCall, (state, { payload }) => { - if (state.activeCall?.state === 'request') - state.activeCall.confirmation.reject(payload); - state.activeCall = undefined; + if (state.activeCall?.state === 'request') { + state.activeCall.confirmation.reject(payload); + state.activeCall = undefined; + } });
25-27: Consider adding state validation in finishCall handler.The
finishCallaction should validate the current state before transitioning..addCase(connectPopupActions.finishCall, state => { + if (state.activeCall) { state.activeCall = { state: 'finished' }; + } })suite-native/app/src/initActions.ts (1)
55-57: Consider specific error handling for WalletConnect initialization.The current implementation catches all errors in a general try-catch block. Consider adding specific error handling for WalletConnect initialization:
if (selectIsFeatureFlagEnabled(getState(), FeatureFlag.IsWalletConnectEnabled)) { - dispatch(walletConnectInitThunk()); + try { + await dispatch(walletConnectInitThunk()); + } catch (error) { + console.error('WalletConnect initialization failed:', error); + // Consider showing user-friendly error message or fallback behavior + } }suite-native/module-connect-popup/src/screens/WalletConnectSessionPopupScreen.tsx (1)
54-58: Add fallback for missing icon.The image component should handle cases where the icon URL is invalid or missing.
<Image - source={{ uri: pendingProposal?.params.proposer.metadata.icons[0] }} + source={{ + uri: pendingProposal?.params.proposer.metadata.icons?.[0] ?? + require('../assets/default-dapp-icon.png') + }} width={80} height={80} + onError={({ nativeEvent: { error } }) => { + console.warn('Failed to load dApp icon:', 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 (53)
packages/suite-desktop-connect-popup/src/connectPopupThunks.ts(0 hunks)packages/suite-desktop-connect-popup/src/index.ts(0 hunks)packages/suite-desktop-connect-popup/tsconfig.json(0 hunks)packages/suite/package.json(1 hunks)packages/suite/src/actions/suite/initAction.ts(2 hunks)packages/suite/src/components/suite/modals/ReduxModal/UserContextModal/ConnectPopupModal.tsx(1 hunks)packages/suite/src/components/suite/modals/ReduxModal/UserContextModal/UserContextModal.tsx(1 hunks)packages/suite/src/middlewares/wallet/discoveryMiddleware.ts(1 hunks)packages/suite/src/middlewares/wallet/index.ts(2 hunks)packages/suite/src/reducers/wallet/index.ts(3 hunks)packages/suite/src/types/wallet/index.ts(3 hunks)packages/suite/tsconfig.json(1 hunks)scripts/list-outdated-dependencies/connect-dependencies.txt(1 hunks)scripts/list-outdated-dependencies/mobile-dependencies.txt(1 hunks)suite-common/connect-popup/package.json(2 hunks)suite-common/connect-popup/src/connectPopupActions.ts(1 hunks)suite-common/connect-popup/src/connectPopupDesktopThunks.ts(1 hunks)suite-common/connect-popup/src/connectPopupMiddleware.ts(1 hunks)suite-common/connect-popup/src/connectPopupReducer.ts(1 hunks)suite-common/connect-popup/src/connectPopupThunks.ts(1 hunks)suite-common/connect-popup/src/connectPopupTypes.ts(1 hunks)suite-common/connect-popup/src/index.ts(1 hunks)suite-common/connect-popup/tsconfig.json(1 hunks)suite-common/suite-types/src/modal.ts(0 hunks)suite-common/walletconnect/package.json(1 hunks)suite-common/walletconnect/src/adapters/ethereum.ts(1 hunks)suite-common/walletconnect/tsconfig.json(1 hunks)suite-native/app/package.json(6 hunks)suite-native/app/src/initActions.ts(2 hunks)suite-native/app/src/navigation/RootStackNavigator.tsx(2 hunks)suite-native/app/tsconfig.json(1 hunks)suite-native/feature-flags/src/__tests__/featureFlagsSlice.test.ts(3 hunks)suite-native/feature-flags/src/featureFlagsSlice.ts(3 hunks)suite-native/intl/src/en.ts(3 hunks)suite-native/module-connect-popup/package.json(1 hunks)suite-native/module-connect-popup/redux.d.ts(1 hunks)suite-native/module-connect-popup/src/hooks/useConnectMethod.ts(0 hunks)suite-native/module-connect-popup/src/hooks/useConnectParseParams.ts(0 hunks)suite-native/module-connect-popup/src/hooks/useConnectPopupNavigation.ts(2 hunks)suite-native/module-connect-popup/src/index.ts(1 hunks)suite-native/module-connect-popup/src/screens/ConnectPopupScreen.tsx(5 hunks)suite-native/module-connect-popup/src/screens/WalletConnectPairScreen.tsx(1 hunks)suite-native/module-connect-popup/src/screens/WalletConnectSessionPopupScreen.tsx(1 hunks)suite-native/module-connect-popup/src/utils/callbackURLOrigin.ts(0 hunks)suite-native/module-connect-popup/tsconfig.json(1 hunks)suite-native/module-dev-utils/src/components/FeatureFlags.tsx(1 hunks)suite-native/module-settings/src/components/FeaturesSettings.tsx(2 hunks)suite-native/navigation/package.json(0 hunks)suite-native/navigation/src/navigators.ts(1 hunks)suite-native/navigation/src/routes.ts(1 hunks)suite-native/state/package.json(1 hunks)suite-native/state/src/reducers.ts(4 hunks)suite-native/state/tsconfig.json(1 hunks)
💤 Files with no reviewable changes (8)
- packages/suite-desktop-connect-popup/src/index.ts
- packages/suite-desktop-connect-popup/tsconfig.json
- suite-common/suite-types/src/modal.ts
- suite-native/navigation/package.json
- suite-native/module-connect-popup/src/utils/callbackURLOrigin.ts
- suite-native/module-connect-popup/src/hooks/useConnectMethod.ts
- suite-native/module-connect-popup/src/hooks/useConnectParseParams.ts
- packages/suite-desktop-connect-popup/src/connectPopupThunks.ts
✅ Files skipped from review due to trivial changes (3)
- suite-common/connect-popup/src/index.ts
- packages/suite/src/middlewares/wallet/discoveryMiddleware.ts
- suite-common/connect-popup/tsconfig.json
🧰 Additional context used
🪛 Biome (1.9.4)
suite-common/connect-popup/src/connectPopupThunks.ts
[error] 126-126: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: run_android_e2e_tests
🔇 Additional comments (50)
suite-common/walletconnect/tsconfig.json (1)
6-6: Reference Addition for Connect PopupThe newly added reference
{ "path": "../connect-popup" }correctly integrates the connect popup project into the WalletConnect configuration. This update aligns with the PR objectives by enabling the unified connect popup functionality across platforms. Ensure that the relative path resolves correctly within the repository structure, and consider documenting this reference in your developer notes for future maintainability.suite-common/walletconnect/package.json (1)
15-15: Addition of@suite-common/connect-popupDependencyThe new dependency
"@suite-common/connect-popup": "workspace:*"is correctly added to support the unified connect popup functionality across mobile and desktop. Ensure that the workspace resolution correctly aligns with the monorepo's version management and that its integration is thoroughly tested across both platforms.suite-native/state/package.json (1)
24-24: New WalletConnect Dependency AddedThe addition of
"@suite-common/walletconnect": "workspace:*"is consistent with the PR objectives to implement WalletConnect functionality within the Suite Native application. This change aligns with the overall architecture shift depicted in the PR summary and maintains consistency with other modules that reference the new WalletConnect package.Please verify that all modules consuming WalletConnect functionalities use the correct dependency version as specified.
suite-native/app/tsconfig.json (1)
32-34: New WalletConnect Reference Added:
The newly added reference for"../../suite-common/walletconnect"properly integrates the WalletConnect module into the project. This change aligns with the PR objectives for WalletConnect Mobile, ensuring that the Suite Native application can access wallet connection functionality similar to the desktop suite. Ensure that the module exists at the specified path and that its configuration is updated as needed in downstream changes.suite-native/intl/src/en.ts (3)
310-310: LGTM! Essential UI strings for user control and security.The addition of the 'Cancel' button and the security-focused error message enhance user control and security in the connect popup.
Also applies to: 321-321
326-348: Comprehensive WalletConnect integration strings with strong security focus.The new WalletConnect section provides a complete set of UI strings for:
- Connection flow (title, message, connect/disconnect actions)
- Security status indicators (verified, unknown, dangerous)
- Error handling (request expiration, scam detection, verification issues)
The messaging emphasizes security and trust, which is crucial for a wallet connection feature.
651-654: Clear settings integration for WalletConnect feature.The WalletConnect feature is well-integrated into the settings menu with clear title and description that helps users understand its purpose.
suite-native/module-connect-popup/tsconfig.json (2)
11-13: Added WalletConnect ReferenceThe new reference to
"../../suite-common/walletconnect"has been correctly added. This aligns with the PR objective of integrating WalletConnect functionality into the Suite Native application by ensuring that TypeScript resolves the new dependency.
4-24: Updated References VerificationThe
"references"list now reflects the intended change by including the WalletConnect module and omitting the outdated desktop connect package. This ensures a proper dependency resolution aligning with the unified WalletConnect implementation for mobile. Please confirm that any corresponding adjustments in sibling modules (e.g., desktop and common configurations) are synced accordingly.suite-native/state/tsconfig.json (1)
28-30: Ensure Correct Inclusion of WalletConnect ReferenceThe addition of the
"walletconnect"reference at lines 28–30 aligns with the PR's objective of integrating WalletConnect support in the mobile suite. This change enables the TypeScript compiler to recognize types and definitions from the newsuite-common/walletconnectmodule.packages/suite/tsconfig.json (1)
22-24: Unify Connect Popup Module ReferenceThe new entry for
"../../suite-common/connect-popup"(lines 22–24) correctly replaces the previous desktop-specific reference. This change is key to aligning the mobile suite with the new consolidated connect popup functionality, ensuring uniform behavior across platforms.suite-native/module-dev-utils/src/components/FeatureFlags.tsx (1)
16-16:✅ Verification successful
LGTM! Verify feature flag usage.
The WalletConnect feature flag implementation follows the established pattern and maintains type safety.
Let's verify the feature flag's usage across the codebase:
🏁 Script executed:
#!/bin/bash # Description: Check how the WalletConnect feature flag is used across the codebase # Search for direct usage of the feature flag echo "Direct feature flag usage:" rg "IsWalletConnectEnabled" -A 3 # Search for WalletConnect related conditional logic echo "\nWalletConnect conditional checks:" rg "walletConnect.*enabled|enabled.*walletConnect" -iLength of output: 4707
LGTM – WalletConnect feature flag is correctly implemented and consistently used across the codebase.
- The addition in
suite-native/module-dev-utils/src/components/FeatureFlags.tsxfollows existing patterns and passes type safety with thesatisfiesoperator.- Verification confirms its usage in key locations:
suite-native/app/src/initActions.tsfor conditional initialization.suite-native/feature-flags/src/featureFlagsSlice.tsand corresponding tests.suite-native/module-connect-popup/src/hooks/useConnectPopupNavigation.tsfor navigation logic.suite-native/module-settings/src/components/FeaturesSettings.tsxfor feature toggling.No further action is required.
packages/suite/src/components/suite/modals/ReduxModal/UserContextModal/ConnectPopupModal.tsx (1)
14-17: Ensure error object compatibility with rejection logic.
dispatch(connectPopupActions.rejectCall(ERRORS.TypedError('Method_Cancel')))presumes thatTypedErrorfully matches your action payload shape. Double-check that this error object is interpreted correctly downstream to avoid unexpected breakage or missing error states.suite-native/module-connect-popup/src/hooks/useConnectPopupNavigation.ts (1)
34-35: Verify theURLconstructor availability in React Native.
new URL(url)may not be supported on older target environments without polyfills. Verify that all platforms have the necessary support or handle cases whereURLis undefined.suite-native/module-connect-popup/src/screens/ConnectPopupScreen.tsx (1)
31-35: Prevent inconsistent screen transitions.
WhenpopupCall.statebecomesfinished, you navigate back only ifnavigation.canGoBack()istrue. Consider clarity for scenarios where the user might not have a previous screen or if a deeper nav stack was replaced.suite-common/connect-popup/src/connectPopupThunks.ts (2)
33-44: Ensure error action is dispatched correctly.
The code callsconnectPopupActions.initiateCallbut never dispatches the return value. Consider dispatching the returned action object if you intend to update state. Otherwise, confirm that this is an intentional usage.
146-155: Check for valid numeric parsing of deeplink version.
SinceparseInt(version)can produceNaN, consider validating the result before comparing withDEEPLINK_VERSION. Ifversionis not a number, you may unintentionally skip correct error handling.suite-native/module-connect-popup/redux.d.ts (1)
3-7: Confirm the redux Dispatch extension merges cleanly.
This ambient module extension can conflict with existing type definitions if multiple extensions exist. Verify that your project has only one declaration merging forDispatch.suite-native/module-connect-popup/src/index.ts (1)
2-3: Ensure new screens are tested or used.
These exports are newly added. Confirm that they are referenced in your navigation configuration or tested so that they don't remain unused.suite-common/connect-popup/src/connectPopupActions.ts (1)
5-24: LGTM! Well-structured Redux actions following best practices.The actions are properly namespaced and follow Redux Toolkit's patterns. The use of
as constensures type safety, and the payload creators are appropriately typed.suite-common/connect-popup/src/connectPopupDesktopThunks.ts (1)
10-11: Fix incomplete type definition for desktop API params.The
@ts-expect-errorcomment indicates that the params in desktopApi are not fully typed. This should be addressed to ensure type safety.Could you help improve the type definitions for the desktop API params? This would eliminate the need for the
@ts-expect-errorcomment.packages/suite/src/middlewares/wallet/index.ts (1)
9-10: LGTM! Middleware ordering looks correct.The
prepareConnectPopupMiddlewareis appropriately placed beforeprepareWalletConnectMiddleware, which makes sense as the popup functionality might be needed before WalletConnect operations.Also applies to: 35-36
packages/suite/src/reducers/wallet/index.ts (1)
3-3: LGTM! Reducer integration follows established patterns.The
connectPopupReduceris properly integrated into the wallet reducers, maintaining consistency with other reducers and following the same dependency injection pattern.Also applies to: 35-35, 56-56
suite-native/feature-flags/src/featureFlagsSlice.ts (1)
14-14: LGTM! WalletConnect feature flag integration looks good.The feature flag is properly integrated into the existing structure, following the established patterns for development-only features and state persistence.
Also applies to: 33-33, 44-44
packages/suite/src/types/wallet/index.ts (2)
1-1: LGTM! ConnectPopupAction type is properly defined.The type definition follows the established pattern for Redux action types.
Also applies to: 65-67
88-89: LGTM! ConnectPopupAction is correctly integrated.The action type is properly added to the WalletAction union type.
suite-native/navigation/src/routes.ts (1)
18-19: LGTM! WalletConnect routes are properly integrated.The new routes follow the existing naming convention and are correctly placed in the RootStackRoutes enum.
packages/suite/src/actions/suite/initAction.ts (1)
121-121:❓ Verification inconclusive
LGTM! Verify desktop-specific initialization.
The change to use
connectPopupDesktopInitThunkaligns with the PR's objective of unifying desktop and mobile connect popup logic.
🏁 Script executed:
#!/bin/bash # Verify the initialization sequence and error handling ast-grep --pattern $'dispatch(connectPopupDesktopInitThunk()).catch($_)'Length of output: 73
Ensure Desktop Initialization Error Handling is Verified
The updated initialization using
connectPopupDesktopInitThunksuccessfully aligns with the PR’s goal of unifying desktop and mobile connect popup logic. However, note that our automated check for a chained.catch()on the dispatch call did not return any evidence of additional error handling. If the thunk itself handles errors internally, this implementation is acceptable. Otherwise, please verify manually that errors are managed as needed.
- File:
packages/suite/src/actions/suite/initAction.ts(line 121)- Action: Confirm that error handling is either incorporated within
connectPopupDesktopInitThunkor is managed appropriately elsewhere if a.catch()chain is expected.suite-native/feature-flags/src/__tests__/featureFlagsSlice.test.ts (2)
31-31: LGTM! WalletConnect enabled for debug Android environment.The flag is correctly enabled for debug environment on Android, which aligns with development testing needs.
58-58: LGTM! WalletConnect disabled for production environments.The flag is correctly disabled for production environments on both Android and iOS platforms.
Also applies to: 85-85
suite-native/module-settings/src/components/FeaturesSettings.tsx (2)
25-25: LGTM! Feature flag hook usage.The feature flag is correctly implemented using the
useFeatureFlaghook.
89-99:✅ Verification successful
Verify navigation and translation resources.
The WalletConnect settings UI implementation looks good, following the established pattern.
🏁 Script executed:
#!/bin/bash # Verify translation keys and navigation route exist rg "moduleSettings.items.features.walletConnect" -g '*.json' rg "WalletConnectPair" -g '*Navigator.tsx'Length of output: 406
WalletConnect settings UI verified successfully
The navigation route (
RootStackRoutes.WalletConnectPair) was confirmed insuite-native/app/src/navigation/RootStackNavigator.tsxand the translation keys (moduleSettings.items.features.walletConnect.*) exist as expected. No further changes are necessary.suite-native/module-connect-popup/src/screens/WalletConnectSessionPopupScreen.tsx (2)
64-90: LGTM! Comprehensive security validation UI.The implementation includes clear visual indicators for verified, unknown, and dangerous connections, helping users make informed decisions.
127-129: LGTM! Proper validation checks for confirm button.The button is correctly disabled for expired or scam proposals, preventing potentially dangerous connections.
suite-native/app/src/navigation/RootStackNavigator.tsx (1)
14-18: LGTM! The navigation setup for WalletConnect screens is properly implemented.The changes correctly integrate the WalletConnect screens into the navigation stack, aligning with the PR's objective to implement WalletConnect in the Suite Native app.
Also applies to: 87-94
suite-common/walletconnect/src/adapters/ethereum.ts (1)
3-3: LGTM! The import has been correctly updated to use the new common package.The change aligns with the codebase restructuring to use the new
@suite-common/connect-popuppackage.suite-native/navigation/src/navigators.ts (1)
224-226: LGTM! The route parameters are properly defined for WalletConnect screens.The changes correctly define the route parameters for the new WalletConnect screens and update the
ConnectPopuproute to no longer require theparsedUrlparameter.packages/suite/src/components/suite/modals/ReduxModal/UserContextModal/UserContextModal.tsx (1)
217-217: LGTM! The component has been correctly updated to use Redux state management.The change simplifies the
ConnectPopupModalusage by removing prop passing, aligning with the shift to Redux state management.suite-native/state/src/reducers.ts (2)
68-69: LGTM! Proper integration of new reducers.The connect popup and wallet connect reducers are correctly:
- Initialized with extraDependencies
- Integrated into the walletReducers combineReducer
Also applies to: 91-92
23-25:✅ Verification successful
Monitor the circular dependency workaround.
The direct import from
src/walletConnectReduceris a valid workaround for the circular dependency issue. However, this should be monitored as it might become problematic if the dependency structure changes.
🏁 Script executed:
#!/bin/bash # Check for other potential circular dependencies rg -l "@suite-native/test-utils" | xargs rg "@suite-native/state"Length of output: 434
Monitor the circular dependency workaround.
- The direct import from
@suite-common/walletconnect/src/walletConnectReducerbypasses the circular dependency between@suite-native/test-utilsand@suite-native/state, as indicated by the inline comment and confirmed by our grep output.- No immediate changes are required; however, please keep monitoring this workaround for any future dependency structure changes that might necessitate adjustments.
suite-common/connect-popup/package.json (1)
2-2: LGTM! Well-structured package configuration.The changes effectively:
- Align the package name with the common module pattern
- Make the desktop API optional, enabling cross-platform usage
Also applies to: 26-32
suite-native/module-connect-popup/package.json (1)
14-14: LGTM! Dependencies align with WalletConnect integration.The addition of Redux Toolkit and WalletConnect dependencies properly supports the implementation of WalletConnect in Suite Native.
Also applies to: 17-17
scripts/list-outdated-dependencies/mobile-dependencies.txt (1)
80-84: New Dependency Additions for Mobile Support
The newly added dependencies—@react-native-async-storage/async-storage,@types/fast-text-encoding,@types/react-native-get-random-values,fast-text-encoding, andreact-native-get-random-values—align with the broader integration changes in the mobile application. They support enhanced local storage, proper text encoding, and reliable random value generation, all of which are critical for WalletConnect functionality. Please verify that these versions are consistent with other parts of the codebase.packages/suite/package.json (1)
34-34: Migration to a Unified Connect Popup Module
The addition of"@suite-common/connect-popup": "workspace:*"replaces the older desktop-specific popup dependency. This change supports a more modular and unified approach across platforms. Ensure that all remaining references to the removed"@trezor/suite-desktop-connect-popup"are updated accordingly.suite-native/app/package.json (6)
26-26: Addition of Async Storage Dependency
Introducing"@react-native-async-storage/async-storage": "^2.1.1"strengthens the app's local storage capabilities, which is essential for handling WalletConnect sessions on mobile. Confirm that this version integrates smoothly with your existing storage configuration.
46-46: New WalletConnect Integration Dependency
The addition of"@suite-common/walletconnect": "workspace:*"centralizes WalletConnect functionality within the suite. Ensure that all new WalletConnect flows are properly initialized and that any legacy code referencing previous patterns has been updated.
89-89: Addition of React Native Compatibility for WalletConnect
The introduction of"@walletconnect/react-native-compat": "^2.18.0"addresses potential compatibility gaps in React Native, facilitating seamless WalletConnect behavior on mobile devices. It is recommended to run end-to-end tests on actual devices to validate this integration.
111-111: Integration of Fast Text Encoding Library
Adding"fast-text-encoding": "^1.0.6"should improve text encoding operations required by the WalletConnect flows. Please confirm that its behavior is consistent with expectations without causing any encoding-related issues.
120-120: Enhanced Random Value Generation
The inclusion of"react-native-get-random-values": "^1.11.0"is crucial for cryptographic operations and random value generation within WalletConnect flows. Verify that this dependency works well in concert with other cryptographic routines in your application.
142-142: Type Definitions for New Dependencies
The added devDependencies—"@types/fast-text-encoding": "^1"and"@types/react-native-get-random-values": "^1"—ensure type safety and better development experience when using the corresponding libraries. This practice supports consistent TypeScript integration across your mobile codebase.Also applies to: 145-145
suite-native/module-connect-popup/src/hooks/useConnectPopupNavigation.ts
Outdated
Show resolved
Hide resolved
suite-native/module-connect-popup/src/hooks/useConnectPopupNavigation.ts
Outdated
Show resolved
Hide resolved
suite-native/module-connect-popup/src/screens/WalletConnectPairScreen.tsx
Outdated
Show resolved
Hide resolved
suite-native/module-connect-popup/src/screens/WalletConnectPairScreen.tsx
Show resolved
Hide resolved
| const handlePair = () => { | ||
| dispatch(walletConnectPairThunk({ uri })); | ||
| setUri(''); | ||
| }; |
There was a problem hiding this comment.
Add URI validation and error handling.
The pairing process should validate the URI format and handle potential errors.
Consider implementing validation and error handling:
const handlePair = () => {
+ if (!uri.startsWith('wc:')) {
+ // Show error message
+ return;
+ }
dispatch(walletConnectPairThunk({ uri }));
setUri('');
+ // Handle potential errors from the thunk
};📝 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 handlePair = () => { | |
| dispatch(walletConnectPairThunk({ uri })); | |
| setUri(''); | |
| }; | |
| const handlePair = () => { | |
| if (!uri.startsWith('wc:')) { | |
| // Show error message | |
| return; | |
| } | |
| dispatch(walletConnectPairThunk({ uri })); | |
| setUri(''); | |
| // Handle potential errors from the thunk | |
| }; |
suite-native/module-connect-popup/src/screens/WalletConnectSessionPopupScreen.tsx
Outdated
Show resolved
Hide resolved
suite-native/module-connect-popup/src/hooks/useConnectPopupNavigation.ts
Outdated
Show resolved
Hide resolved
suite-native/module-connect-popup/src/screens/WalletConnectSessionPopupScreen.tsx
Outdated
Show resolved
Hide resolved
suite-native/module-connect-popup/src/screens/WalletConnectSessionPopupScreen.tsx
Outdated
Show resolved
Hide resolved
martykan
force-pushed
the
feat/walletconnect-mobile
branch
from
cd2c2f2 to
d1bac8e
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (9)
suite-native/module-connect-popup/src/screens/ConnectPopupScreen.tsx (2)
31-35: Add null check for popupCall state.While the navigation logic works, it could be more robust by explicitly checking for null/undefined.
useEffect(() => { - if (popupCall?.state == 'finished' && navigation.canGoBack()) { + if (popupCall?.state === 'finished' && navigation.canGoBack()) { navigation.goBack(); } }, [popupCall, navigation]);
99-114: Consider adding error logging for debugging.While the error handling is comprehensive, adding error logging would help with debugging in production.
if (popupCall.state === 'call-error') { + console.error('Connect popup error:', popupCall.callError); const getTranslationId = () => { switch (popupCall.callError.code) {suite-native/intl/src/en.ts (3)
321-321: Consider making the error message more specific.While the message is clear, it could be more helpful to users by specifying which methods are not allowed or providing guidance on allowed methods.
Consider this alternative:
- methodNotAllowed: 'Method not allowed for security reasons.', + methodNotAllowed: 'This operation is not allowed for security reasons. Only approved WalletConnect methods are permitted.',
326-348: Enhance security-related messaging in WalletConnect section.The WalletConnect strings are comprehensive but could benefit from stronger security messaging.
Consider these enhancements:
- message: - 'An external app is trying to connect to your Trezor Suite. Make sure you trust the source!', + message: + 'Warning: An external app is requesting access to your Trezor Suite. Only proceed if you initiated this connection and trust the source!', - serviceStatus: { - verified: 'Verified', - unknown: 'Unknown', - dangerous: 'Dangerous', + serviceStatus: { + verified: 'Verified ✓', + unknown: 'Unverified - Proceed with caution', + dangerous: 'Warning: Known dangerous service',
583-586: Add security context to WalletConnect settings description.The settings description could better inform users about security implications of enabling WalletConnect.
Consider this enhancement:
- subtitle: 'Use external apps using the WalletConnect protocol', + subtitle: 'Securely connect to external apps using the WalletConnect protocol. Only enable for trusted applications.',suite-common/connect-popup/src/connectPopupReducer.ts (1)
28-36: Consider handling potential race conditions in approval/rejection flow.The current implementation might be susceptible to race conditions if multiple approve/reject actions are dispatched in quick succession. Consider adding a check to ensure the state hasn't changed between the confirmation check and state update.
.addCase(connectPopupActions.approveCall, state => { - if (state.activeCall?.state === 'request') state.activeCall.confirmation.resolve(); + const { activeCall } = state; + if (activeCall?.state === 'request') { + activeCall.confirmation.resolve(); + } state.activeCall = undefined; }) .addCase(connectPopupActions.rejectCall, (state, { payload }) => { - if (state.activeCall?.state === 'request') - state.activeCall.confirmation.reject(payload); + const { activeCall } = state; + if (activeCall?.state === 'request') { + activeCall.confirmation.reject(payload); + } state.activeCall = undefined; });suite-common/connect-popup/src/connectPopupThunks.ts (2)
95-102: Enhance error handling with specific error types.The catch block could be more specific about error types and handling.
- } catch (error) { + } catch (error: unknown) { console.error('connectPopupCallThunk', error); + const typedError = error instanceof Error ? error : new Error('Unknown error'); return { success: false, - payload: serializeError(error), + payload: serializeError(typedError), }; }
110-116: Consider improving type safety.The use of
as anytype casting could be replaced with a more specific type assertion.-) => connectPopupCallThunkInner(params) as any; +) => connectPopupCallThunkInner(params) as AsyncThunkAction< + ConnectPopupCallThunkResponse<M>, + ConnectPopupCallThunkParams<M>, + CustomThunkAPI +>;suite-native/state/src/reducers.ts (1)
23-25: Consider resolving circular dependency.The comment indicates a circular dependency issue that should be addressed properly rather than working around it.
Consider restructuring the modules to break the circular dependency between
@suite-native/test-utilsand@suite-native/state.
📜 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 (53)
packages/suite-desktop-connect-popup/src/connectPopupThunks.ts(0 hunks)packages/suite-desktop-connect-popup/src/index.ts(0 hunks)packages/suite-desktop-connect-popup/tsconfig.json(0 hunks)packages/suite/package.json(1 hunks)packages/suite/src/actions/suite/initAction.ts(2 hunks)packages/suite/src/components/suite/modals/ReduxModal/UserContextModal/ConnectPopupModal.tsx(1 hunks)packages/suite/src/components/suite/modals/ReduxModal/UserContextModal/UserContextModal.tsx(1 hunks)packages/suite/src/middlewares/wallet/discoveryMiddleware.ts(1 hunks)packages/suite/src/middlewares/wallet/index.ts(2 hunks)packages/suite/src/reducers/wallet/index.ts(3 hunks)packages/suite/src/types/wallet/index.ts(3 hunks)packages/suite/tsconfig.json(1 hunks)scripts/list-outdated-dependencies/connect-dependencies.txt(1 hunks)scripts/list-outdated-dependencies/mobile-dependencies.txt(1 hunks)suite-common/connect-popup/package.json(2 hunks)suite-common/connect-popup/src/connectPopupActions.ts(1 hunks)suite-common/connect-popup/src/connectPopupDesktopThunks.ts(1 hunks)suite-common/connect-popup/src/connectPopupMiddleware.ts(1 hunks)suite-common/connect-popup/src/connectPopupReducer.ts(1 hunks)suite-common/connect-popup/src/connectPopupThunks.ts(1 hunks)suite-common/connect-popup/src/connectPopupTypes.ts(1 hunks)suite-common/connect-popup/src/index.ts(1 hunks)suite-common/connect-popup/tsconfig.json(1 hunks)suite-common/suite-types/src/modal.ts(0 hunks)suite-common/walletconnect/package.json(1 hunks)suite-common/walletconnect/src/adapters/ethereum.ts(1 hunks)suite-common/walletconnect/tsconfig.json(1 hunks)suite-native/app/package.json(6 hunks)suite-native/app/src/initActions.ts(2 hunks)suite-native/app/src/navigation/RootStackNavigator.tsx(2 hunks)suite-native/app/tsconfig.json(1 hunks)suite-native/feature-flags/src/__tests__/featureFlagsSlice.test.ts(3 hunks)suite-native/feature-flags/src/featureFlagsSlice.ts(3 hunks)suite-native/intl/src/en.ts(3 hunks)suite-native/module-connect-popup/package.json(1 hunks)suite-native/module-connect-popup/redux.d.ts(1 hunks)suite-native/module-connect-popup/src/hooks/useConnectMethod.ts(0 hunks)suite-native/module-connect-popup/src/hooks/useConnectParseParams.ts(0 hunks)suite-native/module-connect-popup/src/hooks/useConnectPopupNavigation.ts(2 hunks)suite-native/module-connect-popup/src/index.ts(1 hunks)suite-native/module-connect-popup/src/screens/ConnectPopupScreen.tsx(5 hunks)suite-native/module-connect-popup/src/screens/WalletConnectPairScreen.tsx(1 hunks)suite-native/module-connect-popup/src/screens/WalletConnectSessionPopupScreen.tsx(1 hunks)suite-native/module-connect-popup/src/utils/callbackURLOrigin.ts(0 hunks)suite-native/module-connect-popup/tsconfig.json(1 hunks)suite-native/module-dev-utils/src/components/FeatureFlags.tsx(1 hunks)suite-native/module-settings/src/components/FeaturesSettings.tsx(2 hunks)suite-native/navigation/package.json(0 hunks)suite-native/navigation/src/navigators.ts(1 hunks)suite-native/navigation/src/routes.ts(1 hunks)suite-native/state/package.json(1 hunks)suite-native/state/src/reducers.ts(4 hunks)suite-native/state/tsconfig.json(1 hunks)
💤 Files with no reviewable changes (8)
- suite-native/navigation/package.json
- packages/suite-desktop-connect-popup/src/index.ts
- packages/suite-desktop-connect-popup/tsconfig.json
- suite-native/module-connect-popup/src/utils/callbackURLOrigin.ts
- suite-common/suite-types/src/modal.ts
- suite-native/module-connect-popup/src/hooks/useConnectParseParams.ts
- suite-native/module-connect-popup/src/hooks/useConnectMethod.ts
- packages/suite-desktop-connect-popup/src/connectPopupThunks.ts
🚧 Files skipped from review as they are similar to previous changes (35)
- packages/suite/src/middlewares/wallet/index.ts
- suite-common/walletconnect/tsconfig.json
- suite-native/app/tsconfig.json
- suite-native/state/tsconfig.json
- packages/suite/src/components/suite/modals/ReduxModal/UserContextModal/UserContextModal.tsx
- suite-native/state/package.json
- packages/suite/src/types/wallet/index.ts
- suite-common/connect-popup/tsconfig.json
- suite-native/navigation/src/routes.ts
- suite-common/connect-popup/src/index.ts
- suite-native/module-connect-popup/redux.d.ts
- suite-common/connect-popup/src/connectPopupMiddleware.ts
- packages/suite/src/actions/suite/initAction.ts
- packages/suite/package.json
- suite-native/module-connect-popup/tsconfig.json
- scripts/list-outdated-dependencies/mobile-dependencies.txt
- packages/suite/src/middlewares/wallet/discoveryMiddleware.ts
- packages/suite/src/reducers/wallet/index.ts
- suite-native/module-settings/src/components/FeaturesSettings.tsx
- suite-common/connect-popup/src/connectPopupTypes.ts
- scripts/list-outdated-dependencies/connect-dependencies.txt
- suite-native/navigation/src/navigators.ts
- suite-common/walletconnect/src/adapters/ethereum.ts
- suite-native/module-dev-utils/src/components/FeatureFlags.tsx
- suite-native/module-connect-popup/src/screens/WalletConnectSessionPopupScreen.tsx
- suite-common/connect-popup/src/connectPopupDesktopThunks.ts
- suite-native/feature-flags/src/tests/featureFlagsSlice.test.ts
- suite-native/feature-flags/src/featureFlagsSlice.ts
- suite-native/module-connect-popup/src/index.ts
- suite-native/module-connect-popup/package.json
- suite-common/walletconnect/package.json
- suite-common/connect-popup/package.json
- packages/suite/tsconfig.json
- suite-native/app/src/navigation/RootStackNavigator.tsx
- suite-native/module-connect-popup/src/screens/WalletConnectPairScreen.tsx
🧰 Additional context used
🪛 Biome (1.9.4)
suite-common/connect-popup/src/connectPopupThunks.ts
[error] 126-126: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
⏰ Context from checks skipped due to timeout of 90000ms (14)
- 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: build-deploy
- GitHub Check: build-deploy
- GitHub Check: EAS Update
- GitHub Check: build-web
- GitHub Check: prepare_android_test_app
- GitHub Check: build-web
- GitHub Check: test
- GitHub Check: Setup and Cache Dependencies
- GitHub Check: Analyze with CodeQL (javascript)
- GitHub Check: transport-e2e-test
- GitHub Check: Socket Security: Pull Request Alerts
🔇 Additional comments (21)
suite-native/module-connect-popup/src/screens/ConnectPopupScreen.tsx (3)
1-20: LGTM! Clean refactor of imports and component signature.The changes effectively centralize the state management by leveraging Redux and removing route-based parameters, which aligns well with the PR's objective to unify desktop and mobile connect popup logic.
37-130: LGTM! Well-structured UI rendering logic.The component effectively handles different states (loading, error, success) with appropriate UI feedback. The useMemo optimization and accessibility considerations (testID attributes) are well implemented.
132-166: LGTM! Clean screen layout implementation.The screen layout is well-structured with appropriate component hierarchy and conditional debug functionality.
suite-native/intl/src/en.ts (1)
310-310: LGTM! Cancel button addition is consistent and necessary.The addition of the cancel button translation follows the established patterns and provides necessary user control for the WalletConnect feature.
suite-native/app/src/initActions.ts (3)
12-12: LGTM!The new imports are well-organized and align with the PR's objective to implement WalletConnect support with proper feature flag control.
Also applies to: 14-14
20-66: LGTM! Well-structured initialization flow.The integration of WalletConnect initialization maintains the existing robust structure:
- Clear initialization sequence
- Proper error handling
- Feature flag control for safe deployment
- Application readiness set only after all initializations complete
54-57:✅ Verification successful
Verify WalletConnect initialization dependencies.
The placement of
walletConnectInitThunkafter device initialization and blockchain setup looks good. However, please verify that WalletConnect doesn't have any dependencies on other services that might be initialized later.Let's verify the dependencies:
🏁 Script executed:
#!/bin/bash # Description: Check for potential dependencies in WalletConnect initialization. # Search for WalletConnect dependencies in the thunk implementation ast-grep --pattern $'export const walletConnectInitThunk = $$$' # Search for any direct usage of other services rg -A 5 'walletConnectInitThunk' | rg -v 'dispatch|getState'Length of output: 6616
WalletConnect initialization dependencies verified.
After reviewing the implementation of
walletConnectInitThunkinsuite-common/walletconnect/src/walletConnectThunks.ts, we confirmed that its internal initialization (including the creation ofCoreandWalletKit.init) is self-contained and guarded by a check onwalletKit. There are no additional dependencies on services that are initialized later. The current placement after device initialization and prior to setting the application as ready is appropriate.
- Verified that the thunk does not depend on any external service initialized later.
- The dependency checks and event handler dispatches are correctly implemented.
suite-native/app/package.json (7)
26-26: Async Storage Dependency Added
The new dependency"@react-native-async-storage/async-storage": "^2.1.1"is appropriately added to support asynchronous storage functionalities needed for the WalletConnect Mobile integration. Please verify its compatibility with other Expo/React Native dependencies in your project.
46-46: WalletConnect Module Integration
Adding"@suite-common/walletconnect": "workspace:*"aligns with the objective of unifying connection logic across platforms. Ensure that the workspace version is correctly configured and integrated with existing modules.
89-89: React Native WalletConnect Compatibility Package
The inclusion of"@walletconnect/react-native-compat": "^2.18.1"is a key dependency ensuring that the WalletConnect functionality works smoothly on mobile. Double-check that this version is fully compatible with your version of React Native (0.76.1) and related packages.
111-111: Fast Text Encoding Dependency Added
The addition of"fast-text-encoding": "^1.0.6"provides a lightweight polyfill for text encoding operations, which can be crucial for wallet connection processes. It is recommended to test its performance within the context of your application.
120-120: Secure Random Values in React Native
Including"react-native-get-random-values": "^1.11.0"ensures secure random number generation, often critical in cryptographic operations and wallet interactions. Please ensure its integration does not conflict with any existing crypto or security libraries.
142-142: Type Definitions for Fast Text Encoding
The added dev dependency"@types/fast-text-encoding": "^1"improves TypeScript support for thefast-text-encodingpackage. This addition enhances developer experience and code safety.
145-145: Type Definitions for React Native Get Random Values
Adding"@types/react-native-get-random-values": "^1"as a dev dependency ensures that type definitions are available for secure random number generation, aiding in type safety and better integration during development.suite-common/connect-popup/src/connectPopupActions.ts (1)
1-24: Well-structured Redux actions implementation!The actions are well-organized, properly typed, and follow Redux best practices. The action prefix is appropriately namespaced, and the actions cover the complete lifecycle of a popup call.
suite-common/connect-popup/src/connectPopupReducer.ts (1)
6-8: Clean state type definition!The state type is well-defined and focused on the essential data needed for the connect popup functionality.
packages/suite/src/components/suite/modals/ReduxModal/UserContextModal/ConnectPopupModal.tsx (1)
9-54: Well-implemented modal with proper state management!The component follows React best practices with:
- Clean Redux integration
- Proper error handling
- Accessible UI with clear button variants
- Internationalization support
suite-native/module-connect-popup/src/hooks/useConnectPopupNavigation.ts (1)
54-63: Guard against repeated deep link invocation.When handling deep links, protect against potential race conditions from multiple rapid invocations.
suite-common/connect-popup/src/connectPopupThunks.ts (2)
1-25: Well-structured type definitions!The types are comprehensive and properly typed for TypeScript, making the code type-safe and maintainable.
33-37: Consider adding method validation.The
@ts-expect-errorcomment suggests potential runtime issues. Consider validating the method existence before calling it.if (!(method in TrezorConnect)) { throw ERRORS.TypedError('Method_NotFound'); }suite-native/state/src/reducers.ts (1)
70-71: LGTM! Clean reducer integration.The new reducers are properly initialized and integrated into the wallet reducers.
Also applies to: 94-95
suite-native/module-connect-popup/src/hooks/useConnectPopupNavigation.ts
Outdated
Show resolved
Hide resolved
suite-native/module-connect-popup/src/hooks/useConnectPopupNavigation.ts
Outdated
Show resolved
Hide resolved
martykan
force-pushed
the
feat/walletconnect-mobile
branch
from
d1bac8e to
17c808d
Compare
martykan
requested review from
mroz22,
karliatto,
szymonlesisz and
marekrjpolak
as code owners
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
suite-common/connect-popup/src/connectPopupThunks.ts (1)
166-179:⚠️ Potential issueAdd domain validation and improve error handling.
The current implementation lacks callback domain validation, which was noted in a previous review. It also combines JSON parsing and URL validation errors into a single error case.
Apply this fix:
let payload, callbackUrl; try { payload = JSON.parse(queryParams.params); + } catch (e) { + dispatch( + connectPopupActions.initiateCall({ + state: 'call-error', + callError: TypedError('Method_InvalidParameter'), + }), + ); + return; + } + + try { callbackUrl = new URL(callback); + + // Add domain validation + const ALLOWED_CALLBACK_DOMAINS = ['trezor.io', 'suite.trezor.io']; // Define allowed domains + if (!ALLOWED_CALLBACK_DOMAINS.some(domain => callbackUrl.host.endsWith(domain))) { + dispatch( + connectPopupActions.initiateCall({ + state: 'call-error', + callError: TypedError('Method_InvalidParameter'), + }), + ); + return; + } } catch { dispatch( connectPopupActions.initiateCall({
🧹 Nitpick comments (13)
packages/connect-mobile/src/index.ts (1)
58-59: Redundant validation ofconnectSrcThe code validates
connectSrctwice - once on line 58 storing it in a local variable, and again on line 62 when setting_settings.connectSrc. This is redundant and could be refactored.public init(settings: InitFullSettings<ConnectSettingsMobile>) { if (!settings.deeplinkOpen) { throw new Error('TrezorConnect native requires "deeplinkOpen" setting.'); } const connectSrc = this.validateConnectSrc(settings.connectSrc); this._settings = { ...parseConnectSettings({ ...this._settings, ...settings }), - connectSrc: this.validateConnectSrc(settings.connectSrc), + connectSrc, deeplinkUrl: `${connectSrc}deeplink/${DEEPLINK_VERSION}/`, deeplinkOpen: settings.deeplinkOpen, deeplinkCallbackUrl: settings.deeplinkCallbackUrl, };Also applies to: 62-63
suite-common/connect-popup/src/connectPopupThunks.ts (3)
77-87: Consider adding documentation for dynamic dispatch and explaining id manipulation.The code contains a ts-expect-error comment without explanation, and it's not clear why the response.id is being set to undefined.
Add comments explaining:
- Why the TypeScript ignore is necessary for dynamic method dispatch
- Why response.id is being set to undefined
134-135: Use optional chaining for path property.The code is trying to access a property on potentially undefined path.
Apply this fix:
- const version = path && path.split('/').slice(-2, -1)[0]; + const version = path?.split('/')?.slice(-2, -1)[0];🧰 Tools
🪛 Biome (1.9.4)
[error] 134-134: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
142-143: Improve null checking and method validation.The code uses
Object.prototype.hasOwnProperty.calldirectly on queryParams, but queryParams might be null.Apply this fix:
- !Object.prototype.hasOwnProperty.call(TrezorConnect, queryParams?.method) + !queryParams?.method || typeof TrezorConnect[queryParams.method as keyof typeof TrezorConnect] !== 'function'suite-common/connect-popup/src/connectPopupReducer.ts (2)
25-27: Consider consistent state handling for finishCall.The
finishCallhandler setsactiveCallto an object with state 'finished', whileapproveCallandrejectCallset it toundefined. Consider making this behavior consistent.This is likely intentional based on the navigation logic in ConnectPopupScreen, but should be documented for clarity.
28-36: Add error handling for confirmation promise.The code resolves/rejects the confirmation promise but doesn't handle potential exceptions that might occur during these operations.
Apply this fix:
.addCase(connectPopupActions.approveCall, state => { - if (state.activeCall?.state === 'request') state.activeCall.confirmation.resolve(); + if (state.activeCall?.state === 'request') { + try { + state.activeCall.confirmation.resolve(); + } catch (error) { + console.error('Error resolving confirmation promise:', error); + } + } state.activeCall = undefined; }) .addCase(connectPopupActions.rejectCall, (state, { payload }) => { - if (state.activeCall?.state === 'request') - state.activeCall.confirmation.reject(payload); + if (state.activeCall?.state === 'request') { + try { + state.activeCall.confirmation.reject(payload); + } catch (error) { + console.error('Error rejecting confirmation promise:', error); + } + } state.activeCall = undefined; });suite-native/module-connect-popup/src/screens/ConnectPopupScreen.tsx (2)
31-35: Use strict equality comparison.The code uses loose equality (
==) which can lead to unexpected behavior when comparing different types.Apply this fix:
useEffect(() => { - if (popupCall?.state == 'finished' && navigation.canGoBack()) { + if (popupCall?.state === 'finished' && navigation.canGoBack()) { navigation.goBack(); } }, [popupCall, navigation]);
99-114: Add handling for deeplink-callback state.The code handles various states but doesn't explicitly handle the 'deeplink-callback' state that's set in the
connectPopupDeeplinkThunk.Add a case for 'deeplink-callback' state which could show an appropriate message about redirection.
suite-native/module-connect-popup/src/screens/WalletConnectPairScreen.tsx (5)
18-19: Add loading state to track pairing progress.The component currently doesn't track when a pairing operation is in progress, which could lead to multiple pairing attempts or a poor user experience.
const [uri, setUri] = useState(''); const [qrVisible, setQrVisible] = useState(false); +const [isPairing, setIsPairing] = useState(false);And update the pairing function:
const handlePair = () => { + setIsPairing(true); dispatch(walletConnectPairThunk({ uri })) + .finally(() => { + setIsPairing(false); + }); setUri(''); };
56-58: Update button state during pairing process.The button should reflect the pairing state to provide better user feedback.
-<Button colorScheme="primary" onPress={handlePair} isDisabled={!uri}> +<Button + colorScheme="primary" + onPress={handlePair} + isDisabled={!uri || isPairing} + isLoading={isPairing} +> <Translation id="moduleConnectPopup.walletConnect.connect" /> </Button>
102-103: Validate QR scan results.When a QR code is scanned, the result should be validated before setting it as the URI.
-onCodeScanned={setUri} +onCodeScanned={(scannedValue) => { + // Validate that the scanned value is a valid WalletConnect URI + if (scannedValue && scannedValue.startsWith('wc:')) { + setUri(scannedValue.trim()); + setQrVisible(false); // Auto-close the scanner on successful scan + } else { + // Show error for invalid QR code + } +}}
48-48: Add translation for placeholder text.For internationalization consistency, the placeholder text should use the translation system.
-placeholder="Connection URL..." +placeholder={translate('moduleConnectPopup.walletConnect.urlPlaceholder')}Make sure to add this key to your translation files.
25-28: Consider adding error feedback to QR scanning.The QR scanning handler doesn't include any error feedback mechanism.
const handleQr = () => { setQrVisible(true); + // Reset any previous error messages if using error state };
📜 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/connect-mobile/src/index.ts(2 hunks)packages/connect/src/data/connectSettings.ts(0 hunks)scripts/list-outdated-dependencies/connect-dependencies.txt(1 hunks)scripts/list-outdated-dependencies/mobile-dependencies.txt(1 hunks)suite-common/connect-popup/src/connectPopupActions.ts(1 hunks)suite-common/connect-popup/src/connectPopupReducer.ts(1 hunks)suite-common/connect-popup/src/connectPopupThunks.ts(1 hunks)suite-common/connect-popup/src/connectPopupTypes.ts(1 hunks)suite-common/walletconnect/package.json(1 hunks)suite-native/app/package.json(6 hunks)suite-native/app/src/initActions.ts(2 hunks)suite-native/app/src/navigation/RootStackNavigator.tsx(2 hunks)suite-native/app/tsconfig.json(1 hunks)suite-native/feature-flags/src/__tests__/featureFlagsSlice.test.ts(3 hunks)suite-native/feature-flags/src/featureFlagsSlice.ts(3 hunks)suite-native/intl/src/en.ts(3 hunks)suite-native/module-connect-popup/package.json(1 hunks)suite-native/module-connect-popup/src/hooks/useConnectPopupNavigation.ts(2 hunks)suite-native/module-connect-popup/src/index.ts(1 hunks)suite-native/module-connect-popup/src/screens/ConnectPopupScreen.tsx(5 hunks)suite-native/module-connect-popup/src/screens/WalletConnectPairScreen.tsx(1 hunks)suite-native/module-connect-popup/src/screens/WalletConnectSessionPopupScreen.tsx(1 hunks)suite-native/module-connect-popup/tsconfig.json(1 hunks)suite-native/module-dev-utils/src/components/FeatureFlags.tsx(1 hunks)suite-native/module-settings/src/components/FeaturesSettings.tsx(2 hunks)suite-native/navigation/src/navigators.ts(1 hunks)suite-native/navigation/src/routes.ts(1 hunks)suite-native/state/package.json(1 hunks)suite-native/state/src/reducers.ts(4 hunks)suite-native/state/tsconfig.json(1 hunks)
💤 Files with no reviewable changes (1)
- packages/connect/src/data/connectSettings.ts
🚧 Files skipped from review as they are similar to previous changes (20)
- suite-native/module-connect-popup/tsconfig.json
- suite-native/state/tsconfig.json
- scripts/list-outdated-dependencies/mobile-dependencies.txt
- scripts/list-outdated-dependencies/connect-dependencies.txt
- suite-native/state/package.json
- suite-native/module-connect-popup/package.json
- suite-native/module-dev-utils/src/components/FeatureFlags.tsx
- suite-native/module-settings/src/components/FeaturesSettings.tsx
- suite-native/feature-flags/src/featureFlagsSlice.ts
- suite-native/navigation/src/navigators.ts
- suite-native/navigation/src/routes.ts
- suite-native/feature-flags/src/tests/featureFlagsSlice.test.ts
- suite-native/app/src/initActions.ts
- suite-native/module-connect-popup/src/screens/WalletConnectSessionPopupScreen.tsx
- suite-common/connect-popup/src/connectPopupTypes.ts
- suite-native/module-connect-popup/src/hooks/useConnectPopupNavigation.ts
- suite-native/module-connect-popup/src/index.ts
- suite-native/app/src/navigation/RootStackNavigator.tsx
- suite-native/app/tsconfig.json
- suite-common/walletconnect/package.json
🧰 Additional context used
🪛 Biome (1.9.4)
suite-common/connect-popup/src/connectPopupThunks.ts
[error] 134-134: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
⏰ Context from checks skipped due to timeout of 90000ms (13)
- GitHub Check: PR-check / node-authorizeCoinjoin cancelCoinjoinAuthorization passphrase unlockPath setBusy checkFirmwareAuthenticity keepSession cancel.test info.test resetDevice-api
- GitHub Check: PR-check / web-override init-api-flaky
- GitHub Check: PR-check / node-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: EAS Update
- GitHub Check: transport-e2e-test
- GitHub Check: prepare_android_test_app
- GitHub Check: Setup and Cache Dependencies
- GitHub Check: build-web
- GitHub Check: Analyze with CodeQL (javascript)
- GitHub Check: test
🔇 Additional comments (26)
suite-native/intl/src/en.ts (4)
310-310: Good addition of the cancel button text.The addition of the "Cancel" option in the
moduleConnectPopupsection provides better user control when connecting with external applications.
321-321: Security-focused error message addition.Adding the
methodNotAllowederror message provides clear feedback to users when an operation is restricted for security reasons.
326-348: Comprehensive WalletConnect section added.The WalletConnect section includes a complete set of user messages for the connection flow:
- Connection prompts and warnings
- Verification status indicators
- Error handling for expired requests, scams, and verification issues
- Active connection management
All messages appear properly formatted and aligned with the project's terminology standards.
583-586: Settings integration for WalletConnect.Good addition of WalletConnect settings in the features section with clear title and descriptive subtitle that explains the functionality to users.
suite-native/state/src/reducers.ts (4)
4-4: Good addition of connect-popup reducerThis new import aligns with the PR objective to unify logic between desktop and mobile Connect Popup functionality.
23-25: Good approach to resolving circular dependenciesThe comment clearly explains the circular dependency issue and provides the rationale for importing the WalletConnect reducer directly. This is a reasonable solution to prevent type handling problems while maintaining functionality.
70-71: Consistent initialization of new reducersThe initialization pattern follows the existing code structure, ensuring the new reducers receive the same
extraDependenciesas other reducers in the file.
101-102: Clear integration of new reducersThe new reducers are correctly added to the walletReducers object, following the same pattern as existing reducers. The descriptive key names clearly indicate their purpose and align with the PR's goal of implementing WalletConnect support.
suite-native/app/package.json (7)
26-26: Add Async Storage Dependency
The addition of@react-native-async-storage/async-storageat version^2.1.1is appropriate for managing persistent storage in React Native. Ensure that its API usage across the app aligns with this version.
46-46: Include WalletConnect Common Package
Adding@suite-common/walletconnectas a workspace dependency supports the new WalletConnect integration for mobile. This aligns with the PR's objective to unify wallet connection functionality across platforms.
90-90: Integrate React Native Compatibility for WalletConnect
The inclusion of@walletconnect/react-native-compatat version^2.18.1is a key step to ensure WalletConnect works correctly in the React Native environment. Verify that this version is compatible with other wallet connect components in the project.
112-112: Add Fast Text Encoding Dependency
The newly addedfast-text-encodingpackage at version^1.0.6should help with text encoding requirements. Confirm that its functionality is required and well integrated, especially as similar setups might be expected to handle edge cases in encoding.
121-121: Include getRandomValues Polyfill for React Native
react-native-get-random-valuesat version^1.11.0ensures that cryptographic operations dependent on secure random values work in the React Native environment. This is essential for security-sensitive features and aligns with the overall mobile support goals.
143-143: Add Type Definitions for Fast Text Encoding
Adding@types/fast-text-encodingas a dev dependency is a good move for TypeScript support. Make sure that its version (^1) aligns with the fast-text-encoding package to avoid any type inconsistencies.
146-146: Add Type Definitions for React Native getRandomValues
Including@types/react-native-get-random-valuesis important to ensure proper TypeScript integration for the random values polyfill in React Native. This improves development experience by providing correct typings.packages/connect-mobile/src/index.ts (4)
4-5: New imports support WalletConnect functionalityGood addition of the
corsValidatorandDEEPLINK_VERSIONimports which are necessary for the new deeplink validation logic.
48-52: Good implementation of the connection source validation logicThe
validateConnectSrcmethod properly handles the special case for the WalletConnect deeplink schemetrezorsuitelite://connectwhile delegating to the existingcorsValidatorfor all other cases, which is a clean approach.
63-63: Proper implementation of the deeplink URL constructionUsing the
DEEPLINK_VERSIONin the URL construction ensures that the application can handle versioned deeplinks correctly. This is aligned with the PR objective to implement WalletConnect support.
49-49:❓ Verification inconclusive
Verify the hardcoded deeplink scheme
The hardcoded value
trezorsuitelite://connectshould match the scheme mentioned in the PR objectives (trezorsuitelite://walletconnect). Please check if this is intentional or if one of them is incorrect.
🏁 Script executed:
#!/bin/bash # Search for references to the deeplink schemes to check for consistency echo "Searching for trezorsuitelite://connect references:" rg "trezorsuitelite://connect" -A 1 -B 1 echo "Searching for trezorsuitelite://walletconnect references:" rg "trezorsuitelite://walletconnect" -A 1 -B 1Length of output: 2383
Action: Confirm Deeplink Scheme Consistency
After verifying the codebase, we’ve observed that:
trezorsuitelite://connectis used extensively in several modules (e.g., in packages/connect-mobile/src/index.ts, packages/connect-examples/mobile-expo/App.tsx, and packages/connect-explorer), which suggests it is the established default/development scheme.- In contrast,
trezorsuitelite://walletconnectappears in suite-native/module-connect-popup/src/hooks/useConnectPopupNavigation.ts, implying it may be intended for a specific WalletConnect flow.Please confirm whether this dual usage is intentional. If the PR objective is to standardize on
trezorsuitelite://walletconnectacross the application, then the hardcoded value in packages/connect-mobile/src/index.ts (and potentially other references) should be updated accordingly. Otherwise, please document the rationale behind maintaining different schemes to clarify the intended design.suite-common/connect-popup/src/connectPopupActions.ts (1)
1-24: LGTM - Well structured Redux actions!The Redux actions are well-organized and follow best practices with appropriate typing.
suite-native/module-connect-popup/src/screens/ConnectPopupScreen.tsx (1)
20-36: Well-structured component with improved Redux integration.The component has been successfully refactored to use the centralized Redux state management, which aligns with the PR objective of unifying logic between desktop and mobile Connect Popup. The use of the useEffect hook to handle navigation is clean and effective.
suite-native/module-connect-popup/src/screens/WalletConnectPairScreen.tsx (5)
21-24: Add URI validation and error handling.The current implementation lacks validation for the WalletConnect URI format and doesn't handle potential pairing errors.
Consider implementing validation and error handling:
const handlePair = () => { + if (!uri.startsWith('wc:')) { + // Show error message using your app's notification system + return; + } dispatch(walletConnectPairThunk({ uri })); setUri(''); + // Consider adding error handling for the thunk response };
44-55: Add loading state indication and improve input security.The input field should show loading state during pairing and sanitize input by trimming whitespace.
<Input value={uri} - onChangeText={setUri} + onChangeText={text => setUri(text.trim())} placeholder="Connection URL..." + isDisabled={isPairing} rightIcon={ <IconButton iconName="qrCode" onPress={handleQr} colorScheme="tertiaryElevation0" + isDisabled={isPairing} /> } />
72-74: Enhance session metadata display security.The session topic might contain sensitive information. Consider truncating it for better security.
<Text color="textSubdued" numberOfLines={1}> - {session.topic} + {session.topic.substring(0, 8)}... </Text>
1-13: LGTM on imports and component structure.The imports are well organized, and the component structure follows project conventions.
91-95: Good UX for empty state.Providing an explicit message for when no connections are active is a good user experience practice.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
suite-common/connect-popup/src/connectPopupThunks.ts (2)
58-59:⚠️ Potential issueFunction returns undefined instead of a proper response object.
This function returns
undefinedwhen permission checks fail, but it should return a response object of typeConnectPopupCallThunkResponse. This inconsistency could lead to unexpected behavior for consumers of this function.Apply this fix:
- return; + return { + success: false, + payload: ERRORS.TypedError('Method_NotAllowed'), + };
171-183:⚠️ Potential issueAdd callback domain validation for security.
The current implementation validates the callback URL format but does not restrict which domains are allowed as callbacks. This could pose a security risk by allowing arbitrary callback domains.
Consider adding domain validation:
let payload, callbackUrl; try { payload = JSON.parse(queryParams.params); callbackUrl = new URL(callback); + + // Define allowed domains + const ALLOWED_CALLBACK_DOMAINS = ['trezor.io', 'suite.trezor.io']; + + // Validate callback domain + if (!ALLOWED_CALLBACK_DOMAINS.some(domain => callbackUrl.host === domain || callbackUrl.host.endsWith(`.${domain}`))) { + throw new Error('Invalid callback domain'); + } } catch { dispatch( connectPopupActions.initiateCall({ state: 'call-error', - callError: TypedError('Method_InvalidParameter'), + callError: ERRORS.TypedError('Method_InvalidParameter'), }), ); return; }#!/bin/bash # Description: Check if there are any callback domain validation mechanisms elsewhere in the codebase # Look for callback domain validation logic rg -A 3 -B 3 "callback.*domain|domain.*validation" --glob "*.ts" --glob "*.tsx" # Check for URL allowlists or domain validation utilities rg -A 3 "ALLOWED_.*DOMAINS|allowedDomains" --glob "*.ts"
🧹 Nitpick comments (4)
suite-common/connect-popup/src/connectPopupThunks.ts (4)
138-138: Use optional chaining for safer property access.The
pathvariable could be undefined, which makes the split operation unsafe. Using optional chaining would make this code more robust.- const version = path && path.split('/').slice(-2, -1)[0]; + const version = path?.split('/').slice(-2, -1)[0];🧰 Tools
🪛 Biome (1.9.4)
[error] 138-138: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
151-152: Inconsistent TypedError usage.The
TypedErrorfunction is being called directly without theERRORS.prefix, unlike in other places in the code (e.g., lines 42, 55). This inconsistency should be fixed for better maintainability.- callError: TypedError('Method_InvalidParameter'), + callError: ERRORS.TypedError('Method_InvalidParameter'),
162-163: Inconsistent TypedError usage.The
TypedErrorfunction is being called directly without theERRORS.prefix, unlike in other places in the code.- callError: TypedError('Deeplink_VersionMismatch'), + callError: ERRORS.TypedError('Deeplink_VersionMismatch'),
178-179: Inconsistent TypedError usage.The
TypedErrorfunction is being called directly without theERRORS.prefix, unlike in other places in the code.- callError: TypedError('Method_InvalidParameter'), + callError: ERRORS.TypedError('Method_InvalidParameter'),
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
suite-common/connect-popup/src/connectPopupThunks.ts(1 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
suite-common/connect-popup/src/connectPopupThunks.ts
[error] 138-138: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
martykan
force-pushed
the
feat/walletconnect-mobile
branch
from
a1bcb0f to
8b7a734
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
suite-common/connect-popup/src/connectPopupThunks.ts (2)
169-183:⚠️ Potential issueAdd more robust JSON parsing and URL validation with domain restrictions.
The JSON parsing and URL validation could be more robust by adding proper error handling and domain validation.
- const { method, callback } = queryParams; - let payload, callbackUrl; - try { - payload = JSON.parse(queryParams.params); - callbackUrl = new URL(callback); - } catch { + const { method, callback } = queryParams; + let payload, callbackUrl; + try { + payload = JSON.parse(queryParams.params); + } catch { + dispatch( + connectPopupActions.initiateCall({ + state: 'call-error', + callError: TypedError('Method_InvalidParameter'), + }), + ); + + return; + } + + try { + callbackUrl = new URL(callback); + // Define allowed domains for callbacks + const ALLOWED_CALLBACK_DOMAINS = ['connect.trezor.io', 'trezor.io', 'suite.trezor.io']; + if (!ALLOWED_CALLBACK_DOMAINS.some(domain => callbackUrl.host.endsWith(domain))) { + throw new Error('Invalid callback domain'); + } + } catch { dispatch( connectPopupActions.initiateCall({ state: 'call-error', - callError: TypedError('Method_InvalidParameter'), + callError: TypedError('Method_InvalidParameter', 'Invalid callback URL or domain'), }), ); return; }
58-59:⚠️ Potential issueReturn type error in method protection check.
The function returns
undefinedwhen it should return a properly typed response object, inconsistent with the return type definition and the error handling in the catch block.- return; + return { + success: false, + payload: ERRORS.TypedError('Method_NotAllowed'), + };
🧹 Nitpick comments (7)
packages/connect-mobile/src/index.ts (2)
48-51: Guard against undefined or emptyconnectSrc.
Currently, ifconnectSrcis undefined or empty (not matching'trezorsuitelite://connect'),corsValidatoris invoked. Verify thatcorsValidatorhandles undefined or empty strings gracefully. If not, consider adding a fallback or throwing a user-friendly error to help with debugging.
58-58: Avoid multiple validation calls and confirm URL path structure.
The code callsvalidateConnectSrc(settings.connectSrc)twice within the same function. This can be simplified by reusing the local variableconnectSrc:-const connectSrc = this.validateConnectSrc(settings.connectSrc); ... connectSrc: this.validateConnectSrc(settings.connectSrc), deeplinkUrl: `${connectSrc}deeplink/${DEEPLINK_VERSION}/`, +const validatedSrc = this.validateConnectSrc(settings.connectSrc); ... connectSrc: validatedSrc, deeplinkUrl: `${validatedSrc}deeplink/${DEEPLINK_VERSION}/`,Additionally, confirm whether a trailing slash is indeed intended (e.g.,
trezorsuitelite://connectdeeplink/...) or if a separator is needed beforedeeplink/.Also applies to: 62-63
suite-common/connect-popup/src/connectPopupThunks.ts (2)
138-138: Optimize optional chain usage.The static analysis suggests using optional chain for better readability.
- const version = path && path.split('/').slice(-2, -1)[0]; + const version = path?.split('/')?.slice(-2, -1)[0];🧰 Tools
🪛 Biome (1.9.4)
[error] 138-138: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
31-110: Implement comprehensive request/response logging.Consider adding comprehensive debug logging for the request/response cycle to help with troubleshooting, especially for mobile environments where debugging can be more challenging.
export const connectPopupCallThunkInner = createThunk< ConnectPopupCallThunkResponse<keyof typeof TrezorConnect>, ConnectPopupCallThunkParams<keyof typeof TrezorConnect> >( `${CONNECT_POPUP_MODULE}/callThunk`, async ({ method, payload, processName, origin }, { dispatch, getState, extra }) => { + console.debug(`[ConnectPopup] Call initiated: ${method}`, { processName, origin }); try { // @ts-expect-error: method is dynamic const methodInfo = await TrezorConnect[method]({ ...payload, __info: true, }); + console.debug(`[ConnectPopup] Method info received for: ${method}`, { success: methodInfo.success }); if (!methodInfo.success) { + console.debug(`[ConnectPopup] Method info error: ${methodInfo.payload.code}`); dispatch( connectPopupActions.initiateCall({ state: 'call-error', callError: ERRORS.TypedError(methodInfo.payload.code), }), ); throw methodInfo; }suite-native/module-connect-popup/src/screens/ConnectPopupScreen.tsx (3)
31-35: Add error handling for navigation failures.The navigation effect should include error handling for cases where navigation might fail.
useEffect(() => { - if (popupCall?.state == 'finished' && navigation.canGoBack()) { - navigation.goBack(); + if (popupCall?.state === 'finished') { + try { + if (navigation.canGoBack()) { + navigation.goBack(); + } else { + console.warn('[ConnectPopup] Could not go back, no screens in history'); + } + } catch (error) { + console.error('[ConnectPopup] Navigation error:', error); + } } }, [popupCall, navigation]);
61-97: Add loading state for device communication.The UI should provide feedback when communication with the device is in progress, especially after the user confirms the operation.
if (popupCall.state === 'request') { if (!popupCall.origin) { return ( <ErrorMessage errorMessage={ <Translation id="moduleConnectPopup.errors.invalidCallback" /> } /> ); } return ( <VStack testID="@popup/deeplink-info" spacing="sp8" alignItems="center"> <Text variant="titleSmall">{popupCall.methodTitle}</Text> <Text> <Translation id="moduleConnectPopup.callback" /> {': '} <Text color="textAlertBlue">{popupCall.origin}</Text> </Text> <Text style={{ textAlign: 'center', padding: 20, }} color="textSubdued" > <Translation id="moduleConnectPopup.areYouSureMessage" /> </Text> <Button testID="@popup/call-device" onPress={onConfirm}> {popupCall.confirmLabel || ( <Translation id="moduleConnectPopup.confirm" /> )} </Button> + {popupCall.inProgress && ( + <Box marginTop="sp4"> + <Loader size="small" /> + <Text color="textSubdued" marginTop="sp2"> + <Translation id="moduleConnectPopup.communicatingWithDevice" /> + </Text> + </Box> + )} </VStack> ); }
99-114: Improve error message presentation with codes and debugging info.The error presentation should include error codes and more detailed information, especially in development environments.
if (popupCall.state === 'call-error') { const getTranslationId = () => { switch (popupCall.callError.code) { case 'Deeplink_VersionMismatch': return 'moduleConnectPopup.errors.versionUnsupported'; case 'Method_NotAllowed': return 'moduleConnectPopup.errors.methodNotAllowed'; case 'Device_NotFound': return 'moduleConnectPopup.errors.deviceNotConnected'; default: return 'moduleConnectPopup.errors.invalidParams'; } }; - return <ErrorMessage errorMessage={<Translation id={getTranslationId()} />} />; + return ( + <VStack spacing="sp4" alignItems="center"> + <ErrorMessage errorMessage={<Translation id={getTranslationId()} />} /> + {isDevelopOrDebugEnv() && showDebug && ( + <Box marginTop="sp4" padding="sp4" backgroundColor="backgroundSurfaceElevation1" borderRadius="md"> + <Text variant="hint" color="textSubdued">Error code: {popupCall.callError.code}</Text> + <Text variant="hint" color="textSubdued">Message: {popupCall.callError.message}</Text> + </Box> + )} + </VStack> + ); }
📜 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/connect-mobile/src/index.ts(2 hunks)packages/connect/src/data/connectSettings.ts(0 hunks)scripts/list-outdated-dependencies/connect-dependencies.txt(1 hunks)scripts/list-outdated-dependencies/mobile-dependencies.txt(1 hunks)suite-common/connect-popup/src/connectPopupActions.ts(1 hunks)suite-common/connect-popup/src/connectPopupReducer.ts(1 hunks)suite-common/connect-popup/src/connectPopupThunks.ts(1 hunks)suite-common/connect-popup/src/connectPopupTypes.ts(1 hunks)suite-common/walletconnect/package.json(1 hunks)suite-native/app/package.json(6 hunks)suite-native/app/src/initActions.ts(2 hunks)suite-native/app/src/navigation/RootStackNavigator.tsx(2 hunks)suite-native/app/tsconfig.json(1 hunks)suite-native/feature-flags/src/__tests__/featureFlagsSlice.test.ts(3 hunks)suite-native/feature-flags/src/featureFlagsSlice.ts(3 hunks)suite-native/intl/src/en.ts(3 hunks)suite-native/module-connect-popup/package.json(1 hunks)suite-native/module-connect-popup/src/hooks/useConnectPopupNavigation.ts(2 hunks)suite-native/module-connect-popup/src/index.ts(1 hunks)suite-native/module-connect-popup/src/screens/ConnectPopupScreen.tsx(5 hunks)suite-native/module-connect-popup/src/screens/WalletConnectPairScreen.tsx(1 hunks)suite-native/module-connect-popup/src/screens/WalletConnectSessionPopupScreen.tsx(1 hunks)suite-native/module-connect-popup/tsconfig.json(1 hunks)suite-native/module-dev-utils/src/components/FeatureFlags.tsx(1 hunks)suite-native/module-settings/src/components/FeaturesSettings.tsx(2 hunks)suite-native/navigation/src/navigators.ts(1 hunks)suite-native/navigation/src/routes.ts(1 hunks)suite-native/state/package.json(1 hunks)suite-native/state/src/reducers.ts(4 hunks)suite-native/state/tsconfig.json(1 hunks)
💤 Files with no reviewable changes (1)
- packages/connect/src/data/connectSettings.ts
🚧 Files skipped from review as they are similar to previous changes (23)
- suite-native/state/package.json
- suite-native/module-dev-utils/src/components/FeatureFlags.tsx
- suite-native/app/tsconfig.json
- suite-native/state/tsconfig.json
- scripts/list-outdated-dependencies/connect-dependencies.txt
- suite-native/feature-flags/src/tests/featureFlagsSlice.test.ts
- suite-native/module-connect-popup/tsconfig.json
- suite-native/app/src/initActions.ts
- suite-native/module-settings/src/components/FeaturesSettings.tsx
- suite-native/module-connect-popup/src/index.ts
- suite-native/navigation/src/navigators.ts
- suite-native/module-connect-popup/package.json
- scripts/list-outdated-dependencies/mobile-dependencies.txt
- suite-native/feature-flags/src/featureFlagsSlice.ts
- suite-common/connect-popup/src/connectPopupTypes.ts
- suite-common/connect-popup/src/connectPopupReducer.ts
- suite-native/navigation/src/routes.ts
- suite-native/module-connect-popup/src/hooks/useConnectPopupNavigation.ts
- suite-native/module-connect-popup/src/screens/WalletConnectSessionPopupScreen.tsx
- suite-native/module-connect-popup/src/screens/WalletConnectPairScreen.tsx
- suite-common/walletconnect/package.json
- suite-native/app/package.json
- suite-common/connect-popup/src/connectPopupActions.ts
🧰 Additional context used
🪛 Biome (1.9.4)
suite-common/connect-popup/src/connectPopupThunks.ts
[error] 138-138: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
🔇 Additional comments (11)
suite-native/app/src/navigation/RootStackNavigator.tsx (2)
14-18: Clean import restructuring for WalletConnect integration.The imports have been properly organized to include the new WalletConnect-related components alongside the existing ConnectPopupScreen, maintaining a clear and readable structure.
87-94: WalletConnect navigation screens appropriately integrated.The two new screens for WalletConnect functionality have been correctly added to the navigation stack, following the established pattern in the codebase. These additions align perfectly with the PR objective of implementing WalletConnect support for the Suite Native application, matching the functionality available in the Desktop version.
The navigation structure accommodates both the session popup and the pairing screen, which will be necessary for the complete WalletConnect flow, including the deep linking scheme mentioned in the PR description.
suite-native/intl/src/en.ts (4)
310-310: Added a cancel button for user action termination.This addition provides users with a clear way to cancel operations in the connect popup, improving the user experience and aligning with standard UI patterns.
321-321: Added security-related error message for restricted methods.The new error message clarifies when operations are blocked for security reasons, providing better user feedback during connection attempts.
326-348: Complete WalletConnect section added with comprehensive messaging.The implementation includes all necessary UI strings for the WalletConnect functionality:
- Clear title and connection messages
- Options for connecting via pairing URL or QR code
- Active connection management
- Security status indicators (verified, unknown, dangerous)
- Appropriate error messaging for expired requests, scam detection, and verification issues
This section properly supports the WalletConnect implementation described in the PR objectives, providing a complete user interface solution for managing wallet connections.
583-586: Added WalletConnect in device settings menu.This addition creates the necessary menu entry in the features section, making the WalletConnect functionality discoverable by users. The subtitle clearly explains the purpose of this feature.
The implementation aligns with the PR objective of adding WalletConnect support to the Suite Native application.
packages/connect-mobile/src/index.ts (1)
4-5: Leverage corsValidator for consistent validation across the codebase.
UsingcorsValidatorin conjunction withparseConnectSettingsaligns well with the general practice of validating the origin. It's good to see this being handled specifically in the mobile environment to help avoid invalid or malicious connect sources.suite-native/state/src/reducers.ts (4)
4-4: New import for WalletConnect mobile supportThis import brings in the
prepareConnectPopupReducerfrom the common module, which is consistent with the PR objective of aligning the mobile and desktop connect popup functionality.
23-25: Good handling of circular dependencyThe direct import from the source path rather than the module is a reasonable solution to avoid circular dependencies affecting types. The comments clearly explain the issue and justify this approach.
Consider monitoring this solution over time. While it works now, direct imports from internal module paths can be fragile if the source code structure changes. If this becomes a recurring pattern, a more structural refactoring of the module dependencies might be worth considering.
70-71: Proper instantiation of new reducersThe reducers are correctly instantiated with
extraDependenciesfollowing the same pattern as other reducers in this file.
101-102:❓ Verification inconclusive
Integration of WalletConnect into Redux state
The new reducers are properly integrated into the wallet state. This integration follows the existing patterns and will make the WalletConnect state available in the Redux store.
Make sure that any components dependent on these new state slices are updated to handle potential initialization states correctly.
🏁 Script executed:
#!/bin/bash # Check for component usage of the new state slices echo "Checking for components accessing connectPopup state:" rg -l "connectPopup" --type tsx echo -e "\nChecking for components accessing walletConnect state:" rg -l "walletConnect" --type tsxLength of output: 362
WalletConnect Redux Integration Verification
The reducers for
connectPopupandwalletConnecthave been integrated following our established patterns, exposing the new state slices correctly to the Redux store:connectPopup: connectPopupReducer, walletConnect: walletConnectReducer,Action Required:
Please manually verify that all components depending on these state slices (typically in TSX files) correctly handle scenarios where the state might not yet be initialized. Note that our initial automated search encountered issues with the specified TSX file type filter. It is recommended to re-run the search using appropriate glob patterns or review the TSX files manually to ensure proper handling.
martykan
force-pushed
the
feat/walletconnect-mobile
branch
from
8b7a734 to
24c0203
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (5)
suite-common/connect-popup/src/connectPopupThunks.ts (5)
58-59:⚠️ Potential issueMissing typed return in early exit path.
The function returns without a proper return value on line 58, which conflicts with the declared return type
ConnectPopupCallThunkResponse<keyof typeof TrezorConnect>.Apply this fix:
- return; + return { + success: false, + payload: ERRORS.TypedError('Method_NotAllowed'), + };
126-132: 🛠️ Refactor suggestionAdd URL validation for WalletConnect scheme.
This function should validate that the URL matches the expected scheme for WalletConnect deeplinks, such as "trezorsuitelite://walletconnect".
40-46:⚠️ Potential issueFix dispatched action with proper syntax.
The action creator needs to be dispatched properly.
169-183:⚠️ Potential issueImplement callback domain validation for security.
The current implementation validates the URL format but doesn't check if the callback domain is trusted. This is a potential security vulnerability.
+ // List of allowed callback domains + const ALLOWED_CALLBACK_DOMAINS = ['trezor.io', 'wallet.trezor.io']; // Update with actual allowed domains + const { method, callback } = queryParams; let payload, callbackUrl; try { payload = JSON.parse(queryParams.params); callbackUrl = new URL(callback); + // Validate callback domain + if (!ALLOWED_CALLBACK_DOMAINS.includes(callbackUrl.host)) { + throw new Error('Invalid callback domain'); + } } catch {
172-173: 🛠️ Refactor suggestionImprove error handling for JSON parsing and URL creation.
The current implementation catches all errors in a generic catch block. It would be better to separate the error handling for JSON parsing and URL creation.
try { - payload = JSON.parse(queryParams.params); - callbackUrl = new URL(callback); + try { + payload = JSON.parse(queryParams.params); + } catch (error) { + console.error('Invalid JSON payload', error); + throw new Error('Invalid JSON payload'); + } + + try { + callbackUrl = new URL(callback); + } catch (error) { + console.error('Invalid callback URL', error); + throw new Error('Invalid callback URL'); + } } catch {
🧹 Nitpick comments (2)
suite-common/connect-popup/src/connectPopupThunks.ts (2)
137-147: Use optional chaining for path parsing.The path splitting and array access could be simplified with optional chaining.
- const version = path && path.split('/').slice(-2, -1)[0]; + const version = path?.split('/')?.slice(-2, -1)?.[0];🧰 Tools
🪛 Biome (1.9.4)
[error] 138-138: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
31-46: Improve dynamic method call type safety.The code uses
@ts-expect-errorfor dynamic method calls. Consider improving type safety by using a more specific type assertion.- // @ts-expect-error: method is dynamic - const methodInfo = await TrezorConnect[method]({ + // Type-safe dynamic method call + const trezorMethod = TrezorConnect[method] as ( + params: CallMethodParams<typeof method> + ) => Promise<CallMethodResponse<typeof method>>; + const methodInfo = await trezorMethod({
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (31)
packages/connect-mobile/src/index.ts(2 hunks)packages/connect/src/data/connectSettings.ts(0 hunks)packages/suite-desktop-connect-popup/src/connectPopupThunks.ts(0 hunks)packages/suite-desktop-connect-popup/src/index.ts(0 hunks)packages/suite-desktop-connect-popup/tsconfig.json(0 hunks)packages/suite/package.json(1 hunks)packages/suite/src/actions/suite/initAction.ts(2 hunks)packages/suite/src/components/suite/modals/ReduxModal/UserContextModal/ConnectPopupModal.tsx(1 hunks)packages/suite/src/components/suite/modals/ReduxModal/UserContextModal/UserContextModal.tsx(1 hunks)packages/suite/src/middlewares/wallet/discoveryMiddleware.ts(1 hunks)packages/suite/src/middlewares/wallet/index.ts(2 hunks)packages/suite/src/reducers/wallet/index.ts(3 hunks)packages/suite/src/types/wallet/index.ts(3 hunks)packages/suite/tsconfig.json(1 hunks)scripts/list-outdated-dependencies/connect-dependencies.txt(1 hunks)scripts/list-outdated-dependencies/mobile-dependencies.txt(1 hunks)suite-common/connect-popup/package.json(2 hunks)suite-common/connect-popup/src/connectPopupActions.ts(1 hunks)suite-common/connect-popup/src/connectPopupDesktopThunks.ts(1 hunks)suite-common/connect-popup/src/connectPopupMiddleware.ts(1 hunks)suite-common/connect-popup/src/connectPopupReducer.ts(1 hunks)suite-common/connect-popup/src/connectPopupThunks.ts(1 hunks)suite-common/connect-popup/src/connectPopupTypes.ts(1 hunks)suite-common/connect-popup/src/index.ts(1 hunks)suite-common/connect-popup/tsconfig.json(1 hunks)suite-common/suite-types/src/modal.ts(0 hunks)suite-common/walletconnect/package.json(1 hunks)suite-common/walletconnect/src/adapters/ethereum.ts(1 hunks)suite-common/walletconnect/tsconfig.json(1 hunks)suite-native/app/package.json(6 hunks)suite-native/app/src/initActions.ts(2 hunks)
💤 Files with no reviewable changes (5)
- packages/suite-desktop-connect-popup/src/index.ts
- packages/suite-desktop-connect-popup/tsconfig.json
- packages/connect/src/data/connectSettings.ts
- packages/suite-desktop-connect-popup/src/connectPopupThunks.ts
- suite-common/suite-types/src/modal.ts
🚧 Files skipped from review as they are similar to previous changes (22)
- suite-common/walletconnect/tsconfig.json
- scripts/list-outdated-dependencies/mobile-dependencies.txt
- suite-common/connect-popup/tsconfig.json
- suite-common/connect-popup/src/index.ts
- packages/suite/tsconfig.json
- packages/suite/src/components/suite/modals/ReduxModal/UserContextModal/UserContextModal.tsx
- suite-native/app/src/initActions.ts
- scripts/list-outdated-dependencies/connect-dependencies.txt
- suite-common/connect-popup/src/connectPopupMiddleware.ts
- packages/suite/src/reducers/wallet/index.ts
- packages/suite/src/middlewares/wallet/index.ts
- suite-common/connect-popup/src/connectPopupDesktopThunks.ts
- packages/suite/src/types/wallet/index.ts
- packages/connect-mobile/src/index.ts
- suite-common/walletconnect/src/adapters/ethereum.ts
- packages/suite/package.json
- suite-common/connect-popup/package.json
- suite-common/connect-popup/src/connectPopupReducer.ts
- suite-common/walletconnect/package.json
- suite-common/connect-popup/src/connectPopupTypes.ts
- packages/suite/src/actions/suite/initAction.ts
- packages/suite/src/middlewares/wallet/discoveryMiddleware.ts
🧰 Additional context used
🪛 Biome (1.9.4)
suite-common/connect-popup/src/connectPopupThunks.ts
[error] 138-138: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
⏰ Context from checks skipped due to timeout of 90000ms (14)
- 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: EAS Update
- GitHub Check: Socket Security: Pull Request Alerts
- GitHub Check: Analyze with CodeQL (javascript)
- GitHub Check: test
- GitHub Check: build-web
- GitHub Check: prepare_android_test_app
- GitHub Check: transport-e2e-test
- GitHub Check: Setup and Cache Dependencies
🔇 Additional comments (11)
suite-native/app/package.json (7)
26-26: Dependency Addition: Async StorageThe dependency
"@react-native-async-storage/async-storage": "^2.1.1"has been added to support React Native’s asynchronous local storage needed for state persistence and caching. Confirm that this version is compatible with other modules in the project.
46-46: Dependency Addition: Suite Common WalletConnectThe addition of
"@suite-common/walletconnect": "workspace:*"integrates the WalletConnect functionality from the suite-common package. Ensure that this workspace dependency is properly linked and that its interface aligns with both mobile and desktop implementations.
90-90: Dependency Addition: React Native WalletConnect CompatThe package
"@walletconnect/react-native-compat": "^2.18.1"has been introduced to improve WalletConnect support on React Native platforms. Verify that its native compatibility layer works seamlessly with other wallet connection flows and the deep link integration (trezorsuitelite://walletconnect).
112-112: Dependency Addition: Fast Text EncodingThe dependency
"fast-text-encoding": "^1.0.6"is added likely for improved performance in text encoding/decoding processes, which can be critical in crypto-related operations. Check that it integrates correctly with the rest of the code, particularly if used in WalletConnect flows.
121-121: Dependency Addition: React Native Get Random ValuesThe inclusion of
"react-native-get-random-values": "^1.11.0"ensures the availability of secure random numbers—an essential requirement for cryptographic operations within WalletConnect sessions. Make sure it is initialized early in the app’s lifecycle to avoid potential issues.
143-143: Type Definitions Addition: Fast Text EncodingThe dev dependency
"@types/fast-text-encoding": "^1"improves TypeScript type checking for the fast-text-encoding package. This addition enhances developer productivity and code safety, so it’s a useful inclusion.
146-146: Type Definitions Addition: React Native Get Random ValuesAdding
"@types/react-native-get-random-values": "^1"ensures proper TypeScript support for the corresponding dependency. This should help catch potential type mismatches early during development.suite-common/connect-popup/src/connectPopupActions.ts (1)
1-24: Well-structured Redux action definitions.This file establishes a clean set of Redux actions for the connect popup functionality using Redux Toolkit's
createAction. The actions follow a consistent pattern with proper typing and a clear namespace.packages/suite/src/components/suite/modals/ReduxModal/UserContextModal/ConnectPopupModal.tsx (3)
1-13: Good refactoring to Redux-based state management.The component has been successfully migrated from prop-based to Redux-based state management. The conditional rendering is well implemented to only show the modal when there's an active request.
14-18: Clean implementation of component handlers.The extraction of data from the popupCall state and the implementation of action dispatchers are straightforward and follow Redux best practices.
19-54:❓ Verification inconclusive
Verify modal content rendering for all possible states.
The modal rendering looks good, with appropriate conditional rendering for optional fields like
processNameandorigin. The component also intelligently falls back to a default translation whenconfirmLabelis not provided.
🏁 Script executed:
#!/bin/bash # Check for other usages of ConnectPopupModal to ensure all transition paths # have been updated to work with the new Redux implementation # Check for any remaining props being passed to ConnectPopupModal rg "ConnectPopupModal\s+\w+={\w+}" --type ts --type tsxLength of output: 194
Please verify the updated prop usage for ConnectPopupModal.
The modal’s conditional rendering for optional fields (i.e.
processNameandorigin) and the default fallback forconfirmLabelappear correct. However, the intended automated check for extra props on ConnectPopupModal failed due to a regex error. I recommend either manually reviewing the usages of<ConnectPopupModal …>to ensure that all props are updated for the new Redux implementation or re-running the check with an adjusted regex (e.g., escaping curly braces). Once this verification is complete, the changes seem in line with the intended updates.
martykan
force-pushed
the
feat/walletconnect-mobile
branch
from
24c0203 to
2c80ff6
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (3)
suite-common/connect-popup/src/connectPopupActions.ts (1)
17-24:⚠️ Potential issueFix the export declaration structure.
There appears to be a merge conflict or incomplete edit in the export statement. Lines 22-24 look like duplicated action exports but don't align with the actual actions defined above.
export const connectPopupActions = { initiateCall, approveCall, rejectCall, } as const; - finishCall, - rejectCall, -} as const; -suite-native/state/src/reducers.ts (1)
100-103:⚠️ Potential issueFix the walletPersistedReducer duplication.
There appears to be a duplication of
connectPopupandwalletConnectproperties before the actualwalletPersistedReducerdeclaration. These lines should be removed as they're likely part of a merge conflict.- const walletPersistedReducer = await preparePersistReducer({ - connectPopup: connectPopupReducer, - walletConnect: walletConnectReducer, - });suite-common/connect-popup/src/connectPopupThunks.ts (1)
180-202:⚠️ Potential issueRemove duplicated code.
Lines 180-202 appear to be duplicated code from the implementation above. This is likely a merge conflict that wasn't resolved properly.
- ); - - return; - } - - const response = await dispatch( - connectPopupCallThunk({ - processName: 'deeplink', - origin: `${callbackUrl.protocol}//${callbackUrl.host}`, - // @ts-expect-error: method is dynamic - method, - payload, - }), - ).unwrap(); - callbackUrl.searchParams.set('response', JSON.stringify(response)); - dispatch( - connectPopupActions.initiateCall({ - state: 'deeplink-callback', - callbackUrl: callbackUrl.toString(), - }), - ); - }, -);
♻️ Duplicate comments (5)
suite-common/connect-popup/src/connectPopupThunks.ts (3)
39-42:⚠️ Potential issueAction creator called without dispatch.
The
connectPopupActions.initiateCallis being called directly instead of being dispatched, which won't update the Redux store state.- connectPopupActions.initiateCall({ + dispatch(connectPopupActions.initiateCall({ state: 'call-error', callError: ERRORS.TypedError(methodInfo.payload.code), - }); + }));
49-55:⚠️ Potential issueAction creator called without dispatch and missing return type.
There are two issues in this code segment:
- The action creator is called without dispatch
- The function returns
undefinedwhen it should return a response object of typeConnectPopupCallThunkResponse- connectPopupActions.initiateCall({ + dispatch(connectPopupActions.initiateCall({ state: 'call-error', callError: ERRORS.TypedError('Method_NotAllowed'), - }); + })); - return; + return { + success: false, + payload: ERRORS.TypedError('Method_NotAllowed'), + };
155-157: 🛠️ Refactor suggestionAdd JSON parsing error handling and URL validation.
The JSON parsing and URL validation could be more robust.
- const payload = JSON.parse(queryParams.params); - const { method, callback } = queryParams; - const callbackUrl = new URL(callback); + let payload; + try { + payload = JSON.parse(queryParams.params); + } catch (e) { + throw new Error('Invalid params JSON'); + } + + const { method, callback } = queryParams; + let callbackUrl; + try { + callbackUrl = new URL(callback); + // Validate allowed callback domains + const ALLOWED_CALLBACK_DOMAINS = ['trezor.io', 'suite.trezor.io']; // Add your allowed domains + if (!ALLOWED_CALLBACK_DOMAINS.includes(callbackUrl.host)) { + throw new Error('Invalid callback domain'); + } + } catch (e) { + throw new Error('Invalid callback URL'); + }suite-native/module-connect-popup/src/screens/WalletConnectPairScreen.tsx (2)
44-55: 🛠️ Refactor suggestionAdd loading state and improve input security
The input field should display loading state during pairing and sanitize input.
Consider these improvements:
<Input value={uri} - onChangeText={setUri} + onChangeText={text => setUri(text.trim())} placeholder="Connection URL..." + isDisabled={isPairing} rightIcon={ <IconButton iconName="qrCode" onPress={handleQr} colorScheme="tertiaryElevation0" + isDisabled={isPairing} /> } />This requires adding a loading state:
+const [isPairing, setIsPairing] = useState(false); const handlePair = () => { + setIsPairing(true); dispatch(walletConnectPairThunk({ uri })) + .finally(() => setIsPairing(false)); setUri(''); };
21-24: 💡 Verification agent🧩 Analysis chain
Add URI validation and error handling
The current implementation does not validate the URI format before dispatching the pairing action. This could lead to errors or unexpected behavior if an invalid URI is provided.
Implement URI validation as suggested in the previous review:
const handlePair = () => { + if (!uri.trim().startsWith('wc:')) { + // Show error toast/alert + return; + } dispatch(walletConnectPairThunk({ uri })); setUri(''); };
🏁 Script executed:
#!/bin/bash # Check if there's error handling in the walletConnectPairThunk rg -A 10 "walletConnectPairThunk" --glob "*.ts" --glob "*.tsx"Length of output: 9344
Action: Add Client-Side URI Validation Prior to Dispatching the Pairing Action
After reviewing the code and verifying error handling exists only within the thunk, it’s clear that the UI does not perform any validation of the URI before dispatching the pairing request. Relying solely on the catch block in
walletConnectPairThunkmeans that users might experience delayed and less informative error feedback. We recommend adding a client-side check (e.g., ensuring the URI starts with"wc:") and displaying an immediate error alert or toast if the validation fails.
- Location:
suite-native/module-connect-popup/src/screens/WalletConnectPairScreen.tsx(lines 21–24)- Suggested diff:
const handlePair = () => { + if (!uri.trim().startsWith('wc:')) { + // Display an error toast/alert indicating that the URI format is invalid + return; + } dispatch(walletConnectPairThunk({ uri })); setUri(''); };This change will provide immediate user feedback and help prevent dispatching invalid URIs.
🧹 Nitpick comments (3)
suite-common/connect-popup/src/connectPopupThunks.ts (1)
138-138: Use optional chaining for improved code quality.Consider using optional chaining for a more modern and cleaner approach to property access.
- !Object.prototype.hasOwnProperty.call(TrezorConnect, queryParams?.method) + !Object.prototype.hasOwnProperty?.call(TrezorConnect, queryParams?.method)🧰 Tools
🪛 Biome (1.9.4)
[error] 138-138: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
suite-native/module-connect-popup/src/screens/ConnectPopupScreen.tsx (1)
32-39: Good improvement on deeplink callback handling!The implementation now correctly handles the
deeplink-callbackstate by opening the external URL and navigating back, addressing the previous review comment.Consider modifying the implementation to ensure the URL is successfully opened before navigating back:
useEffect(() => { if (popupCall?.state === 'deeplink-callback') { - Linking.openURL(popupCall.callbackUrl); - if (navigation.canGoBack()) { - navigation.goBack(); - } + Linking.openURL(popupCall.callbackUrl) + .then(() => { + if (navigation.canGoBack()) { + navigation.goBack(); + } + }) + .catch(error => console.error('Failed to open URL:', error)); } }, [popupCall, navigation]);This handles any potential errors when opening the URL and ensures navigation happens after the URL is opened.
suite-native/module-connect-popup/src/screens/WalletConnectPairScreen.tsx (1)
97-104: Consider adding feedback after QR scanWhen a QR code is scanned, the user doesn't receive confirmation that the scan was successful.
<ScanQRBottomSheet title={<Translation id="moduleConnectPopup.walletConnect.title" />} isVisible={qrVisible} - onCodeScanned={setUri} + onCodeScanned={(code) => { + setUri(code); + setQrVisible(false); // Close bottom sheet after successful scan + }} onClose={() => setQrVisible(false)} />
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (3)
yarn.lockis excluded by!**/yarn.lock,!**/*.lockyarn.lockis excluded by!**/yarn.lock,!**/*.lockyarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (74)
packages/suite-desktop-connect-popup/tsconfig.json(0 hunks)packages/suite/package.json(1 hunks)packages/suite/src/actions/suite/initAction.ts(2 hunks)packages/suite/src/middlewares/wallet/discoveryMiddleware.ts(1 hunks)packages/suite/tsconfig.json(1 hunks)suite-common/connect-popup/package.json(2 hunks)suite-common/connect-popup/src/connectPopupDesktopThunks.ts(1 hunks)suite-common/connect-popup/src/connectPopupThunks.ts(0 hunks)suite-common/connect-popup/tsconfig.json(1 hunks)suite-common/walletconnect/package.json(1 hunks)suite-common/walletconnect/src/adapters/ethereum.ts(1 hunks)suite-common/walletconnect/tsconfig.json(1 hunks)packages/suite/src/components/suite/modals/ReduxModal/UserContextModal/ConnectPopupModal.tsx(1 hunks)packages/suite/src/components/suite/modals/ReduxModal/UserContextModal/UserContextModal.tsx(1 hunks)packages/suite/src/middlewares/wallet/index.ts(2 hunks)packages/suite/src/reducers/wallet/index.ts(3 hunks)packages/suite/src/types/wallet/index.ts(3 hunks)suite-common/connect-popup/src/connectPopupActions.ts(1 hunks)suite-common/connect-popup/src/connectPopupMiddleware.ts(1 hunks)suite-common/connect-popup/src/connectPopupReducer.ts(1 hunks)suite-common/connect-popup/src/connectPopupThunks.ts(3 hunks)suite-common/connect-popup/src/connectPopupTypes.ts(1 hunks)suite-common/connect-popup/src/index.ts(1 hunks)suite-common/suite-types/src/modal.ts(0 hunks)packages/suite/src/components/suite/modals/ReduxModal/UserContextModal/ConnectPopupModal.tsx(2 hunks)suite-common/connect-popup/src/connectPopupMiddleware.ts(1 hunks)suite-common/connect-popup/src/connectPopupReducer.ts(1 hunks)suite-common/connect-popup/src/connectPopupThunks.ts(4 hunks)suite-common/connect-popup/src/connectPopupTypes.ts(1 hunks)suite-common/walletconnect/src/adapters/ethereum.ts(0 hunks)suite-native/intl/src/en.ts(1 hunks)suite-native/module-connect-popup/package.json(1 hunks)suite-native/module-connect-popup/redux.d.ts(1 hunks)suite-native/module-connect-popup/src/hooks/useConnectMethod.ts(0 hunks)suite-native/module-connect-popup/src/hooks/useConnectParseParams.ts(0 hunks)suite-native/module-connect-popup/src/hooks/useConnectPopupNavigation.ts(2 hunks)suite-native/module-connect-popup/src/screens/ConnectPopupScreen.tsx(5 hunks)suite-native/module-connect-popup/src/utils/callbackURLOrigin.ts(0 hunks)suite-native/module-connect-popup/tsconfig.json(0 hunks)suite-native/navigation/package.json(0 hunks)suite-native/navigation/src/navigators.ts(1 hunks)suite-native/state/src/reducers.ts(3 hunks)scripts/list-outdated-dependencies/connect-dependencies.txt(1 hunks)scripts/list-outdated-dependencies/mobile-dependencies.txt(1 hunks)suite-common/connect-popup/src/connectPopupActions.ts(1 hunks)suite-common/connect-popup/src/connectPopupReducer.ts(1 hunks)suite-common/connect-popup/src/connectPopupThunks.ts(3 hunks)suite-common/connect-popup/src/connectPopupTypes.ts(1 hunks)suite-common/walletconnect/package.json(1 hunks)suite-native/app/package.json(6 hunks)suite-native/app/src/initActions.ts(2 hunks)suite-native/app/src/navigation/RootStackNavigator.tsx(2 hunks)suite-native/app/tsconfig.json(1 hunks)suite-native/feature-flags/src/__tests__/featureFlagsSlice.test.ts(3 hunks)suite-native/feature-flags/src/featureFlagsSlice.ts(3 hunks)suite-native/intl/src/en.ts(3 hunks)suite-native/module-connect-popup/package.json(1 hunks)suite-native/module-connect-popup/src/hooks/useConnectPopupNavigation.ts(2 hunks)suite-native/module-connect-popup/src/index.ts(1 hunks)suite-native/module-connect-popup/src/screens/ConnectPopupScreen.tsx(1 hunks)suite-native/module-connect-popup/src/screens/WalletConnectPairScreen.tsx(1 hunks)suite-native/module-connect-popup/src/screens/WalletConnectSessionPopupScreen.tsx(1 hunks)suite-native/module-connect-popup/tsconfig.json(1 hunks)suite-native/module-dev-utils/src/components/FeatureFlags.tsx(1 hunks)suite-native/module-settings/src/components/FeaturesSettings.tsx(2 hunks)suite-native/navigation/src/navigators.ts(1 hunks)suite-native/navigation/src/routes.ts(1 hunks)suite-native/state/package.json(1 hunks)suite-native/state/src/reducers.ts(3 hunks)suite-native/state/tsconfig.json(1 hunks)packages/connect-mobile/src/index.ts(2 hunks)packages/connect/src/data/connectSettings.ts(0 hunks)packages/suite-desktop-connect-popup/src/connectPopupThunks.ts(0 hunks)packages/suite-desktop-connect-popup/src/index.ts(0 hunks)
💤 Files with no reviewable changes (9)
- suite-native/navigation/package.json
- packages/suite-desktop-connect-popup/src/index.ts
- packages/suite-desktop-connect-popup/tsconfig.json
- suite-common/suite-types/src/modal.ts
- suite-native/module-connect-popup/src/utils/callbackURLOrigin.ts
- packages/connect/src/data/connectSettings.ts
- suite-native/module-connect-popup/src/hooks/useConnectMethod.ts
- packages/suite-desktop-connect-popup/src/connectPopupThunks.ts
- suite-native/module-connect-popup/src/hooks/useConnectParseParams.ts
🚧 Files skipped from review as they are similar to previous changes (51)
- suite-native/app/tsconfig.json
- suite-common/walletconnect/src/adapters/ethereum.ts
- scripts/list-outdated-dependencies/mobile-dependencies.txt
- suite-native/state/package.json
- suite-common/walletconnect/tsconfig.json
- suite-native/module-connect-popup/tsconfig.json
- suite-common/connect-popup/src/connectPopupThunks.ts
- suite-native/module-dev-utils/src/components/FeatureFlags.tsx
- suite-native/module-connect-popup/redux.d.ts
- suite-native/intl/src/en.ts
- packages/suite/package.json
- scripts/list-outdated-dependencies/connect-dependencies.txt
- suite-common/connect-popup/tsconfig.json
- suite-native/navigation/src/routes.ts
- packages/suite/src/middlewares/wallet/index.ts
- suite-common/connect-popup/src/connectPopupReducer.ts
- packages/suite/tsconfig.json
- suite-native/feature-flags/src/tests/featureFlagsSlice.test.ts
- packages/suite/src/reducers/wallet/index.ts
- suite-native/navigation/src/navigators.ts
- suite-common/connect-popup/src/connectPopupMiddleware.ts
- suite-common/connect-popup/src/connectPopupTypes.ts
- suite-common/connect-popup/src/connectPopupMiddleware.ts
- suite-native/state/tsconfig.json
- suite-native/module-connect-popup/tsconfig.json
- packages/suite/src/middlewares/wallet/discoveryMiddleware.ts
- suite-native/app/src/initActions.ts
- suite-common/connect-popup/src/connectPopupThunks.ts
- suite-native/module-connect-popup/package.json
- suite-common/connect-popup/src/connectPopupTypes.ts
- suite-common/connect-popup/src/connectPopupDesktopThunks.ts
- packages/suite/src/actions/suite/initAction.ts
- suite-native/module-settings/src/components/FeaturesSettings.tsx
- suite-common/connect-popup/src/connectPopupActions.ts
- suite-native/navigation/src/navigators.ts
- suite-native/module-connect-popup/src/index.ts
- suite-common/connect-popup/src/connectPopupReducer.ts
- suite-native/module-connect-popup/src/hooks/useConnectPopupNavigation.ts
- suite-common/walletconnect/src/adapters/ethereum.ts
- suite-native/feature-flags/src/featureFlagsSlice.ts
- packages/suite/src/components/suite/modals/ReduxModal/UserContextModal/UserContextModal.tsx
- suite-native/module-connect-popup/src/screens/WalletConnectSessionPopupScreen.tsx
- suite-common/connect-popup/src/connectPopupThunks.ts
- suite-native/module-connect-popup/package.json
- suite-common/walletconnect/package.json
- suite-native/state/src/reducers.ts
- packages/suite/src/types/wallet/index.ts
- suite-common/connect-popup/src/connectPopupReducer.ts
- suite-common/connect-popup/package.json
- packages/connect-mobile/src/index.ts
- suite-native/intl/src/en.ts
🧰 Additional context used
🪛 Biome (1.9.4)
suite-common/connect-popup/src/connectPopupThunks.ts
[error] 138-138: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
⏰ Context from checks skipped due to timeout of 90000ms (14)
- GitHub Check: PR-check / node-authorizeCoinjoin cancelCoinjoinAuthorization passphrase unlockPath setBusy checkFirmwareAuthenticity keepSession cancel.test info.test resetDevice-api
- GitHub Check: PR-check / web-override init-api-flaky
- GitHub Check: PR-check / node-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: build-web
- GitHub Check: EAS Update
- GitHub Check: Setup and Cache Dependencies
- GitHub Check: transport-e2e-test
- GitHub Check: Analyze with CodeQL (javascript)
- GitHub Check: test
- GitHub Check: prepare_android_test_app
- GitHub Check: Socket Security: Pull Request Alerts
🔇 Additional comments (16)
suite-common/walletconnect/package.json (1)
15-15: Addition of new dependency for unified connect popup functionality
The new dependency"@suite-common/connect-popup": "workspace:*"has been introduced to allow the WalletConnect package to leverage the shared connect popup logic. This change aligns with the project’s overall move towards consolidating connect popup functionality across both desktop and mobile platforms. Please ensure that all usages in WalletConnect correctly reference the new package, and verify that the dependency’s workspace resolution is properly configured in your build/system.suite-common/connect-popup/src/connectPopupThunks.ts (1)
126-133: Add URL validation for WalletConnect scheme.This function should validate that the URL matches the expected scheme for WalletConnect deeplinks, such as "trezorsuitelite://walletconnect".
let parsedUrl; try { parsedUrl = new URL(url); + // Validate URL scheme for WalletConnect deeplinks + const validSchemes = ['trezorsuitelite']; + if (!validSchemes.includes(parsedUrl.protocol.replace(':', ''))) { + throw new Error('Invalid URL scheme'); + } } catch (error) { console.warn('Invalid deeplink URL', { error, url }); return; }suite-native/app/package.json (1)
26-26: LGTM: Dependencies look appropriate for WalletConnect integration.The added dependencies align well with the PR objective of implementing WalletConnect support for the Suite Native application. The packages for async storage, randomness, and text encoding are commonly needed for wallet connection functionality.
Also applies to: 46-46, 90-90, 112-112, 121-121, 143-143, 146-146
suite-common/connect-popup/src/index.ts (1)
1-1: LGTM! The new exports enhance the package API.Adding these exports makes it easier for other parts of the application to access the connect popup functionality, supporting the PR's goal of unifying desktop and mobile implementations.
Also applies to: 3-4
suite-native/app/src/navigation/RootStackNavigator.tsx (2)
14-18: LGTM! Import structure follows established patterns.The import structure cleanly imports all related screens from the module-connect-popup package.
87-94: LGTM! New navigation routes added correctly.The new screens for WalletConnect functionality are properly integrated into the navigation stack, following the established patterns used for other screens in the navigator.
suite-common/connect-popup/src/connectPopupTypes.ts (1)
4-24: Well-structured union type improves type safety.The refactored ConnectPopupCall type as a union of different states enhances type safety and makes the state transitions more explicit. This approach will help prevent potential runtime errors by catching incorrect state access at compile time.
suite-native/module-connect-popup/src/hooks/useConnectPopupNavigation.ts (4)
34-35: LGTM! Clear URL validation helper.The
isWalletConnectUrlfunction clearly identifies URLs that should be handled by the WalletConnect functionality.
51-65: LGTM! Clean implementation with proper error handling.The deeplink handling code is well-structured with appropriate conditional checks and error handling for malformed URLs.
67-72: Add error handling for URL opening.When opening external URLs, it's important to handle potential failures gracefully.
- Linking.openURL(connectPopupCall.callbackUrl); + Linking.openURL(connectPopupCall.callbackUrl).catch(error => { + console.error('Failed to open URL:', error); + // Consider showing a user-facing error message if appropriate + });
76-85: Guard against navigation errors for WalletConnect.Similar to the connect popup navigation, add error handling for the WalletConnect session popup navigation.
useEffect(() => { if ( walletConnectProposal && !walletConnectProposal.expired && walletConnectProposal.eventId !== lastProposalId.current ) { lastProposalId.current = walletConnectProposal.eventId; - navigation.navigate(RootStackRoutes.WalletConnectSessionPopup); + navigation.navigate(RootStackRoutes.WalletConnectSessionPopup).catch(error => { + console.error('Failed to navigate to WalletConnect session popup:', error); + }); } }, [walletConnectProposal, navigation]);suite-native/module-connect-popup/src/screens/ConnectPopupScreen.tsx (1)
103-118: Well-structured error handling!The switch statement provides clear error handling based on specific error codes, improving the user experience with targeted error messages.
suite-native/module-connect-popup/src/screens/WalletConnectPairScreen.tsx (1)
67-90: Enhance session metadata display securityThe current implementation shows the full session topic, which might expose sensitive information.
Consider truncating sensitive data:
-<Text color="textSubdued" numberOfLines={1}> - {session.topic} -</Text> +<Text color="textSubdued" numberOfLines={1}> + {session.topic.substring(0, 10)}...{session.topic.slice(-10)} +</Text>This displays only the first and last 10 characters of the topic for better security while still providing enough context.
packages/suite/src/components/suite/modals/ReduxModal/UserContextModal/ConnectPopupModal.tsx (3)
12-12: Good defensive programming!The early return pattern prevents rendering the modal when there's no popup call or when it's not in the request state, which avoids potential errors and improves performance.
14-14: Updated property destructuringThe component now uses
methodTitleinstead ofmethodand addsconfirmLabel, which aligns with the changes in the Redux state structure.
30-30: Better UI customizationUsing
confirmLabelwith a fallback to the translation provides flexibility while maintaining a good default experience.
suite-native/module-connect-popup/src/hooks/useConnectPopupNavigation.ts
Show resolved
Hide resolved
suite-native/module-connect-popup/src/screens/WalletConnectPairScreen.tsx
Outdated
Show resolved
Hide resolved
martykan
force-pushed
the
feat/walletconnect-mobile
branch
2 times, most recently
from
d22e59c to
887e837
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (4)
suite-native/module-connect-popup/src/screens/WalletConnectPairScreen.tsx (2)
78-80: 🛠️ Refactor suggestionEnhance session metadata display security.
Displaying the full session topic might expose sensitive information. Consider truncating it.
<Text color="textSubdued" numberOfLines={1}> - {session.topic} + {session.topic.substring(0, 8)}... </Text>
22-28: 🛠️ Refactor suggestionAdd URI validation and proper error handling.
The current implementation lacks URI validation before pairing and doesn't handle potential errors that might occur during the pairing process.
const handlePair = () => { + // Validate WalletConnect URI format + if (!uri.trim().startsWith('wc:')) { + // Show error notification or alert + return; + } setIsPairing(true); dispatch(walletConnectPairThunk({ uri })).finally(() => { setIsPairing(false); setUri(''); + }).catch(error => { + // Handle/display the error to the user + console.error('WalletConnect pairing failed:', error); + }); };suite-native/module-connect-popup/src/screens/ConnectPopupScreen.tsx (1)
31-35:⚠️ Potential issueMissing deeplink callback handling in navigation logic
The
useEffecthook only handles the case wherepopupCall?.state === 'finished', but it doesn't handle the 'deeplink-callback' state.When a deeplink callback is provided, the component should navigate to external URLs. Update the effect to handle this case:
useEffect(() => { + if (popupCall?.state === 'deeplink-callback') { + Linking.openURL(popupCall.callbackUrl); + } else if (popupCall?.state == 'finished' && navigation.canGoBack()) { navigation.goBack(); } }, [popupCall, navigation]);Also, add the Linking import back at the top of the file:
+import { Linking } from 'react-native';suite-common/connect-popup/src/connectPopupThunks.ts (1)
47-59:⚠️ Potential issueMissing return value for NotAllowed error case
When a method requires 'management' or 'push_tx' permissions, the code dispatches an error action but returns
undefinedinstead of a proper response object.dispatch( connectPopupActions.initiateCall({ state: 'call-error', callError: ERRORS.TypedError('Method_NotAllowed'), }), ); -return; +return { + success: false, + payload: ERRORS.TypedError('Method_NotAllowed'), +};
🧹 Nitpick comments (7)
suite-native/module-connect-popup/src/screens/WalletConnectPairScreen.tsx (4)
62-64: Add a loading indicator during pairing process.The button should indicate when a pairing operation is in progress to improve user feedback.
-<Button colorScheme="primary" onPress={handlePair} isDisabled={!uri}> +<Button + colorScheme="primary" + onPress={handlePair} + isDisabled={!uri || isPairing} + isLoading={isPairing} +> <Translation id="moduleConnectPopup.walletConnect.connect" /> </Button>
83-93: Add confirmation before disconnecting sessions.Consider adding a confirmation dialog before disconnecting active sessions to prevent accidental disconnections.
<Button onPress={() => { + // Add confirmation dialog before disconnecting + // Example implementation depends on your UI components + const confirmDisconnect = window.confirm( + 'Are you sure you want to disconnect this session?' + ); + if (confirmDisconnect) { dispatch( walletConnectDisconnectThunk({ topic: session.topic }), ); + } }} size="small" colorScheme="redElevation0" > <Translation id="moduleConnectPopup.walletConnect.disconnect" /> </Button>
105-110: Validate scanned QR codes.Add validation to ensure scanned QR codes follow the WalletConnect URI format before setting them.
<ScanQRBottomSheet title={<Translation id="moduleConnectPopup.walletConnect.title" />} isVisible={qrVisible} - onCodeScanned={setUri} + onCodeScanned={(code) => { + if (code.startsWith('wc:')) { + setUri(code); + setQrVisible(false); // Close the sheet automatically on successful scan + } else { + // Show error about invalid QR code + console.error('Invalid WalletConnect QR code'); + } + }} onClose={() => setQrVisible(false)} />
1-20: Add feedback mechanism for connection status.The component should provide visual feedback about connection status to improve user experience.
import { useState } from 'react'; import { useDispatch, useSelector } from 'react-redux'; import { selectSessions, walletConnectDisconnectThunk, walletConnectPairThunk, } from '@suite-common/walletconnect'; import { Button, Divider, HStack, IconButton, Input, Text, VStack } from '@suite-native/atoms'; import { Translation } from '@suite-native/intl'; import { Screen, ScreenHeader } from '@suite-native/navigation'; import { ScanQRBottomSheet } from '@suite-native/qr-code'; export const WalletConnectPairScreen = () => { const dispatch = useDispatch(); const sessions = useSelector(selectSessions); const [uri, setUri] = useState(''); const [qrVisible, setQrVisible] = useState(false); const [isPairing, setIsPairing] = useState(false); + const [connectionStatus, setConnectionStatus] = useState<'idle' | 'connecting' | 'success' | 'error'>('idle');Then update the
handlePairfunction to use this state:const handlePair = () => { if (!uri.trim().startsWith('wc:')) { + setConnectionStatus('error'); return; } setIsPairing(true); + setConnectionStatus('connecting'); dispatch(walletConnectPairThunk({ uri })) + .then(() => { + setConnectionStatus('success'); + // Reset status after a delay + setTimeout(() => setConnectionStatus('idle'), 2000); + }) + .catch(() => { + setConnectionStatus('error'); + }) .finally(() => { setIsPairing(false); setUri(''); }); };And display status feedback in the UI.
suite-common/connect-popup/src/connectPopupReducer.ts (1)
25-27: Consider preserving additional context in finishCall reducerWhen finishing a call, the reducer only sets
state: 'finished'but loses all other properties from the active call. Consider preserving additional context that might be useful for analytics or debugging..addCase(connectPopupActions.finishCall, state => { - state.activeCall = { state: 'finished' }; + if (state.activeCall) { + state.activeCall = { + ...state.activeCall, + state: 'finished' + }; + } else { + state.activeCall = { state: 'finished' }; + } })suite-common/connect-popup/src/connectPopupThunks.ts (1)
171-183: Improve error handling for JSON parsing and URL validationThe current try-catch block catches all errors without distinguishing between JSON parsing errors and URL creation errors. This makes it harder to debug and provide specific error messages.
-let payload, callbackUrl; -try { - payload = JSON.parse(queryParams.params); - callbackUrl = new URL(callback); -} catch { +let payload; +try { + payload = JSON.parse(queryParams.params); +} catch (error) { + console.warn('Failed to parse JSON params', error); dispatch( connectPopupActions.initiateCall({ state: 'call-error', - callError: TypedError('Method_InvalidParameter'), + callError: TypedError('Method_InvalidParameter', 'Invalid JSON parameters'), }), ); return; +} + +let callbackUrl; +try { + callbackUrl = new URL(callback); +} catch (error) { + console.warn('Invalid callback URL', error); + dispatch( + connectPopupActions.initiateCall({ + state: 'call-error', + callError: TypedError('Method_InvalidParameter', 'Invalid callback URL'), + }), + ); + return; }suite-native/module-connect-popup/src/hooks/useConnectPopupNavigation.ts (1)
35-36: EnsureisWalletConnectUrlhandles all valid schemes if needed.
If additional patterns or sub-schemes become relevant in the future, this helper may need updating.
📜 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 (55)
packages/connect-mobile/src/index.ts(2 hunks)packages/connect/src/data/connectSettings.ts(0 hunks)packages/suite-desktop-connect-popup/src/connectPopupThunks.ts(0 hunks)packages/suite-desktop-connect-popup/src/index.ts(0 hunks)packages/suite-desktop-connect-popup/tsconfig.json(0 hunks)packages/suite/package.json(1 hunks)packages/suite/src/actions/suite/initAction.ts(2 hunks)packages/suite/src/components/suite/modals/ReduxModal/UserContextModal/ConnectPopupModal.tsx(1 hunks)packages/suite/src/components/suite/modals/ReduxModal/UserContextModal/UserContextModal.tsx(1 hunks)packages/suite/src/middlewares/wallet/discoveryMiddleware.ts(1 hunks)packages/suite/src/middlewares/wallet/index.ts(2 hunks)packages/suite/src/reducers/wallet/index.ts(3 hunks)packages/suite/src/types/wallet/index.ts(3 hunks)packages/suite/tsconfig.json(1 hunks)scripts/list-outdated-dependencies/connect-dependencies.txt(1 hunks)scripts/list-outdated-dependencies/mobile-dependencies.txt(1 hunks)suite-common/connect-popup/package.json(2 hunks)suite-common/connect-popup/src/connectPopupActions.ts(1 hunks)suite-common/connect-popup/src/connectPopupDesktopThunks.ts(1 hunks)suite-common/connect-popup/src/connectPopupMiddleware.ts(1 hunks)suite-common/connect-popup/src/connectPopupReducer.ts(1 hunks)suite-common/connect-popup/src/connectPopupThunks.ts(1 hunks)suite-common/connect-popup/src/connectPopupTypes.ts(1 hunks)suite-common/connect-popup/src/index.ts(1 hunks)suite-common/connect-popup/tsconfig.json(1 hunks)suite-common/suite-types/src/modal.ts(0 hunks)suite-common/walletconnect/package.json(1 hunks)suite-common/walletconnect/src/adapters/ethereum.ts(1 hunks)suite-common/walletconnect/tsconfig.json(1 hunks)suite-native/app/package.json(1 hunks)suite-native/app/src/initActions.ts(2 hunks)suite-native/app/src/navigation/RootStackNavigator.tsx(2 hunks)suite-native/app/tsconfig.json(1 hunks)suite-native/feature-flags/src/__tests__/featureFlagsSlice.test.ts(3 hunks)suite-native/feature-flags/src/featureFlagsSlice.ts(3 hunks)suite-native/intl/src/en.ts(3 hunks)suite-native/module-connect-popup/package.json(1 hunks)suite-native/module-connect-popup/redux.d.ts(1 hunks)suite-native/module-connect-popup/src/hooks/useConnectMethod.ts(0 hunks)suite-native/module-connect-popup/src/hooks/useConnectParseParams.ts(0 hunks)suite-native/module-connect-popup/src/hooks/useConnectPopupNavigation.ts(2 hunks)suite-native/module-connect-popup/src/index.ts(1 hunks)suite-native/module-connect-popup/src/screens/ConnectPopupScreen.tsx(5 hunks)suite-native/module-connect-popup/src/screens/WalletConnectPairScreen.tsx(1 hunks)suite-native/module-connect-popup/src/screens/WalletConnectSessionPopupScreen.tsx(1 hunks)suite-native/module-connect-popup/src/utils/callbackURLOrigin.ts(0 hunks)suite-native/module-connect-popup/tsconfig.json(1 hunks)suite-native/module-dev-utils/src/components/FeatureFlags.tsx(1 hunks)suite-native/module-settings/src/components/FeaturesSettings.tsx(2 hunks)suite-native/navigation/package.json(0 hunks)suite-native/navigation/src/navigators.ts(1 hunks)suite-native/navigation/src/routes.ts(1 hunks)suite-native/state/package.json(1 hunks)suite-native/state/src/reducers.ts(4 hunks)suite-native/state/tsconfig.json(1 hunks)
💤 Files with no reviewable changes (9)
- suite-native/navigation/package.json
- packages/suite-desktop-connect-popup/src/index.ts
- suite-common/suite-types/src/modal.ts
- suite-native/module-connect-popup/src/utils/callbackURLOrigin.ts
- packages/connect/src/data/connectSettings.ts
- packages/suite-desktop-connect-popup/tsconfig.json
- packages/suite-desktop-connect-popup/src/connectPopupThunks.ts
- suite-native/module-connect-popup/src/hooks/useConnectMethod.ts
- suite-native/module-connect-popup/src/hooks/useConnectParseParams.ts
🚧 Files skipped from review as they are similar to previous changes (34)
- suite-native/app/package.json
- suite-native/app/tsconfig.json
- scripts/list-outdated-dependencies/connect-dependencies.txt
- suite-common/walletconnect/tsconfig.json
- suite-native/module-connect-popup/redux.d.ts
- suite-native/state/package.json
- suite-native/module-dev-utils/src/components/FeatureFlags.tsx
- suite-native/module-connect-popup/tsconfig.json
- suite-native/navigation/src/routes.ts
- suite-common/connect-popup/src/connectPopupDesktopThunks.ts
- suite-native/module-connect-popup/src/index.ts
- packages/suite/src/components/suite/modals/ReduxModal/UserContextModal/UserContextModal.tsx
- suite-native/state/tsconfig.json
- suite-native/module-settings/src/components/FeaturesSettings.tsx
- suite-native/feature-flags/src/tests/featureFlagsSlice.test.ts
- suite-common/walletconnect/src/adapters/ethereum.ts
- suite-native/feature-flags/src/featureFlagsSlice.ts
- suite-common/connect-popup/tsconfig.json
- packages/suite/package.json
- packages/suite/src/actions/suite/initAction.ts
- suite-native/navigation/src/navigators.ts
- suite-common/walletconnect/package.json
- suite-common/connect-popup/src/connectPopupTypes.ts
- packages/suite/src/types/wallet/index.ts
- packages/suite/src/middlewares/wallet/index.ts
- suite-native/module-connect-popup/src/screens/WalletConnectSessionPopupScreen.tsx
- suite-common/connect-popup/package.json
- suite-common/connect-popup/src/connectPopupActions.ts
- packages/suite/src/reducers/wallet/index.ts
- packages/suite/src/middlewares/wallet/discoveryMiddleware.ts
- packages/connect-mobile/src/index.ts
- suite-native/app/src/initActions.ts
- suite-native/app/src/navigation/RootStackNavigator.tsx
- packages/suite/tsconfig.json
🧰 Additional context used
🪛 Biome (1.9.4)
suite-common/connect-popup/src/connectPopupThunks.ts
[error] 138-138: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
🪛 LanguageTool
scripts/list-outdated-dependencies/mobile-dependencies.txt
[duplication] ~81-~81: Possible typo: you repeated a word.
Context: ...tive-async-storage/async-storage @types/fast-text-encoding fast-text-encoding
(ENGLISH_WORD_REPEAT_RULE)
⏰ Context from checks skipped due to timeout of 90000ms (18)
- GitHub Check: Unit Tests
- GitHub Check: Releases revision Checks
- GitHub Check: Type Checking
- GitHub Check: Other Checks
- GitHub Check: Linting and formatting
- GitHub Check: Build libs for publishing
- 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: transport-e2e-test
- GitHub Check: prepare_android_test_app
- GitHub Check: EAS Update
- GitHub Check: test
- GitHub Check: Analyze with CodeQL (javascript)
- GitHub Check: build-web
🔇 Additional comments (28)
suite-native/module-connect-popup/src/screens/WalletConnectPairScreen.tsx (1)
48-61: Add input sanitization to improve security.The input field for the connection URL should sanitize the input to prevent potential security issues.
<Input value={uri} - onChangeText={setUri} + onChangeText={text => setUri(text.trim())} placeholder="Connection URL..." rightIcon={ <IconButton iconName="qrCode" onPress={handleQr} colorScheme="tertiaryElevation0" isDisabled={isPairing} /> } editable={!isPairing} />scripts/list-outdated-dependencies/mobile-dependencies.txt (1)
80-82: New dependencies added for WalletConnect mobile supportThese new dependencies (
@react-native-async-storage/async-storage,@types/fast-text-encoding, andfast-text-encoding) are required for implementing WalletConnect functionality in the mobile application. They align with the PR's objective to add WalletConnect support to Suite Native.🧰 Tools
🪛 LanguageTool
[duplication] ~81-~81: Possible typo: you repeated a word.
Context: ...tive-async-storage/async-storage @types/fast-text-encoding fast-text-encoding(ENGLISH_WORD_REPEAT_RULE)
suite-native/state/src/reducers.ts (4)
4-4: LGTM - Importing the connect popup reducerProper import of the connect popup reducer from the new common package.
23-25: Good approach to handling circular dependencyThe direct import of the WalletConnect reducer with a clear comment explaining the circular dependency issue is a good practice. This approach prevents type handling problems while maintaining code functionality.
70-71: Proper initialization of new reducersThe connect popup and wallet connect reducers are correctly initialized with the extra dependencies, consistent with other reducers in the file.
101-102: Successfully integrated new reducers into wallet stateThe connect popup and wallet connect reducers are properly added to the wallet reducers object, making them available throughout the application.
suite-native/module-connect-popup/package.json (1)
13-13:✅ Verification successful
Dependencies updated for WalletConnect mobile support
The dependencies have been properly updated to support WalletConnect functionality in the mobile application:
- Added async storage for persistent data
- Updated Redux Toolkit to a compatible version
- Added WalletConnect-related dependencies for native compatibility
- Added text encoding libraries required by WalletConnect
These changes align with the PR objective to implement WalletConnect in Suite Native.
Verify compatibility of AsyncStorage version
The AsyncStorage version (
^2.1.1) is relatively new. Please verify it's compatible with the other React Native dependencies in the project.Also applies to: 15-15, 18-18, 27-29, 34-35
🏁 Script executed:
#!/bin/bash # Check for potential compatibility issues with AsyncStorage version npm view @react-native-async-storage/async-storage versions --json | jq 'map(select(startswith("2.")))' npm view @react-native-async-storage/async-storage peerDependenciesLength of output: 268
WalletConnect Dependencies Update Confirmed
The dependency updates for WalletConnect mobile support have been verified:
- AsyncStorage Version: The version
^2.1.1is within the available 2.x releases (2.0.0, 2.1.0, 2.1.1, 2.1.2) and its peer dependency requiresreact-nativeversion>=0.65 <1.0, which aligns with our project’s expected range.- The updates to Redux Toolkit, WalletConnect-related libraries, and text encoding dependencies also align with the PR’s objective to support WalletConnect in Suite Native.
No compatibility issues were detected based on the verification output.
suite-common/connect-popup/src/index.ts (1)
1-4: Well-structured exports for the new common connect popup packageThis index file properly exports all necessary modules for the connect popup functionality, creating a clean public API for this package. This structure aligns with the PR's goal of unifying the logic between desktop and mobile connect popups.
suite-common/connect-popup/src/connectPopupMiddleware.ts (1)
5-21: LGTM: Middleware implementation is clean and effectiveThe middleware implementation correctly handles connect popup actions by dispatching appropriate modal actions based on the state of the connect popup call. It properly passes actions through the middleware chain first with
await next(action)before performing any side effects.suite-native/module-connect-popup/src/screens/ConnectPopupScreen.tsx (2)
72-79: LGTM: Information display for originThe component correctly displays the origin of the call, which helps users understand where the request is coming from.
99-113: Well-structured error handling with appropriate translationsThe error handling implementation uses a clean switch statement to map error codes to appropriate translation IDs, making it easy to maintain and extend.
suite-common/connect-popup/src/connectPopupReducer.ts (1)
22-36: Clean reducers with proper state transitionsThe reducer properly handles the different states of the connect popup flow, with clear action handlers for initiating, approving, rejecting, and finishing calls.
suite-common/connect-popup/src/connectPopupThunks.ts (2)
38-46: Error handling looks good for invalid method infoThe code correctly handles unsuccessful method info responses by dispatching an error action and throwing the method info for further processing.
122-132: Add URL scheme validation for WalletConnect deeplinksThe function should validate that the URL matches the expected scheme for WalletConnect deeplinks, such as "trezorsuitelite://walletconnect".
try { parsedUrl = new URL(url); + // Validate URL scheme for WalletConnect deeplinks + const validSchemes = ['trezorsuitelite']; + if (!validSchemes.includes(parsedUrl.protocol.replace(':', ''))) { + throw new Error('Invalid URL scheme'); + } } catch (error) { console.warn('Invalid deeplink URL', { error, url }); return; }suite-native/intl/src/en.ts (4)
311-311: No issues spotted on the new ‘cancel’ key.
322-322: Straightforward addition for 'methodNotAllowed'.
327-349: New WalletConnect translations look consistent.
They provide clear usage instructions and error handling messages for WalletConnect functionality.
584-587: Added WalletConnect settings section appears aligned with naming conventions.
No issues with the new fields.suite-native/module-connect-popup/src/hooks/useConnectPopupNavigation.ts (6)
1-2: Imports and Redux usage look correct.
All newly imported symbols and hooks appear valid and consistent with the Redux toolkit approach.Also applies to: 7-8, 41-46
49-49: Deeplink usage viaLinking.useURL().
No issues; the approach is standard for retrieving URLs in Expo.
52-52: Early return avoids unnecessary processing.
This condition is clean and prevents overhead.
54-65: Dispatch logic for connect popup or wallet connect flows is clear.
Exceptions for malformed URLs are handled gracefully in a catch block.
68-75: Handling the 'deeplink-callback' scenario.
Linking out to the callback URL is expected. No concerns here.
77-85: Correctly managing the new WalletConnect proposal event flow.
Tracking the last proposal ID prevents duplicated popups.packages/suite/src/components/suite/modals/ReduxModal/UserContextModal/ConnectPopupModal.tsx (4)
1-3: Refactor to Redux hooks is consistent with the rest of the codebase.Also applies to: 7-7
9-12: Conditional rendering of the modal based onpopupCallstate is straightforward.
Prevents unnecessary UI if no request is in progress.
14-17: Use descriptive keys for method-related data.
ExtractingmethodTitle,confirmLabel,processName, andoriginclarifies the intended usage.
19-53: Overall modal implementation looks good.
Confirmation and cancellation dispatch actions are well-handled, and translation strings are properly integrated.
martykan
force-pushed
the
feat/walletconnect-mobile
branch
from
887e837 to
41825e4
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
suite-native/module-connect-popup/src/screens/WalletConnectPairScreen.tsx (2)
48-61: 🛠️ Refactor suggestionAdd URI validation for more secure input handling.
The input field should validate that the URI follows the WalletConnect format before enabling the connect button.
<Input value={uri} - onChangeText={setUri} + onChangeText={text => setUri(text.trim())} placeholder="Connection URL..." rightIcon={ <IconButton iconName="qrCode" onPress={handleQr} colorScheme="tertiaryElevation0" isDisabled={isPairing} /> } editable={!isPairing} /> -<Button colorScheme="primary" onPress={handlePair} isDisabled={!uri}> +<Button + colorScheme="primary" + onPress={handlePair} + isDisabled={!uri || !uri.toLowerCase().startsWith('wc:')} +> <Translation id="moduleConnectPopup.walletConnect.connect" /> </Button>
73-96: 🛠️ Refactor suggestionEnhance session metadata display security.
The current implementation shows the full session topic, which might expose sensitive information.
<Text color="textSubdued" numberOfLines={1}> - {session.topic} + {`${session.topic.substring(0, 8)}...`} </Text>
🧹 Nitpick comments (15)
scripts/list-outdated-dependencies/mobile-dependencies.txt (1)
80-82: Clarify Dependency Additions and Duplication WarningThe newly added dependencies:
- @react-native-async-storage/async-storage (for asynchronous storage support),
- @types/fast-text-encoding (TypeScript definitions for text encoding), and
- fast-text-encoding (the actual text encoding implementation)
are appropriate for supporting the mobile WalletConnect enhancements. Note that while the static analysis tool flagged a potential duplication (a repeated word) around the fast-text-encoding entries, this is intentional since both the type definitions and the runtime package are needed.
Please confirm that both the type definitions and the module itself are required as per the project’s needs.
🧰 Tools
🪛 LanguageTool
[duplication] ~81-~81: Possible typo: you repeated a word.
Context: ...tive-async-storage/async-storage @types/fast-text-encoding fast-text-encoding(ENGLISH_WORD_REPEAT_RULE)
suite-native/module-connect-popup/src/screens/ConnectPopupScreen.tsx (5)
31-31: Use triple equals for strict comparison.
ReplacepopupCall?.state == 'finished'with strict equality (===) for consistency and to avoid subtle type coercion issues.- if (popupCall?.state == 'finished' && navigation.canGoBack()) { + if (popupCall?.state === 'finished' && navigation.canGoBack()) {
38-39: Optional: Add error handling or state feedback.
While this dispatch call is straightforward, consider surfacing if it fails or providing a loading indicator after triggeringapproveCall().
61-62: Validation for request origin.
This conditional block is necessary to guard against invalid or missing origins. Consider returning a more descriptive error in logs ifpopupCall.originis missing.
99-111: Comprehensive error handling.
Mapping error codes to translations is a solid approach. You might consider logging the raw error code or message for debugging or telemetry.
130-130: Dependency array consideration.
onConfirmis declared inside thisuseMemoblock (line 38) but is not part of the dependencies. IfapproveCallcan change, consider extracting or memoizingonConfirmto avoid stale references.suite-native/module-connect-popup/src/screens/WalletConnectPairScreen.tsx (3)
105-110: Consider adding filtering to the QR code scanner.The QR scanner accepts any scanned value without validation. Consider filtering for WalletConnect-specific QR codes.
<ScanQRBottomSheet title={<Translation id="moduleConnectPopup.walletConnect.title" />} isVisible={qrVisible} - onCodeScanned={setUri} + onCodeScanned={(code) => { + // Only accept WalletConnect URIs + if (code.toLowerCase().startsWith('wc:')) { + setUri(code); + setQrVisible(false); // Automatically close after valid scan + } else { + // Could show an error toast here + } + }} onClose={() => setQrVisible(false)} />
1-113: Add loading indicator during pairing process.While interaction is properly disabled during pairing, there's no visual feedback to the user that the process is ongoing.
Consider adding a loading state to the Button component:
<Button colorScheme="primary" onPress={handlePair} isDisabled={!uri} + isLoading={isPairing} > <Translation id="moduleConnectPopup.walletConnect.connect" /> </Button>
67-102: Consider adding a scroll container for session list.If many active connections accumulate, the current implementation doesn't handle overflow properly. Consider wrapping the sessions list in a ScrollView with max height.
<Divider /> <VStack> <Text variant="highlight"> <Translation id="moduleConnectPopup.walletConnect.activeConnections" />: </Text> + <ScrollView style={{ maxHeight: 300 }}> {sessions.map(session => ( <HStack key={session.topic} spacing="sp12"> {/* ... existing code ... */} </HStack> ))} {sessions.length === 0 && ( <Text> <Translation id="moduleConnectPopup.walletConnect.noActiveConnections" /> </Text> )} + </ScrollView> </VStack>suite-common/connect-popup/src/connectPopupThunks.ts (3)
60-76: Validate the selected device before waiting for user confirmation.While you do check for a selected device later (line 77), consider checking it right before creating the deferred confirmation to ensure the device context is valid beforehand. This can help avoid a stalled user flow.
91-91: Clarify settingresponse.id = undefined.This manipulation removes the
idproperty from the response. If this is intentional for privacy or tracking reasons, add a short comment explaining the rationale for future maintainers.
122-146: Optional chain usage could simplify version extraction.The current code ensures safety by checking
path && path.split(...). Switching to optional chaining offers a cleaner expression:- const version = path && path.split('/').slice(-2, -1)[0]; + const version = path?.split('/')?.slice(-2, -1)?.[0];🧰 Tools
🪛 Biome (1.9.4)
[error] 138-138: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
suite-common/connect-popup/src/connectPopupTypes.ts (1)
4-24: Add descriptive comments for each state inConnectPopupCall.Though self-explanatory, brief comments or JSDoc describing each union variant's purpose will simplify onboarding and reduce guesswork for future contributors.
suite-native/module-connect-popup/src/hooks/useConnectPopupNavigation.ts (1)
57-65: Consider debouncing repeated wallet connect requests.Repeatedly opening the same malformed or valid hash of a WalletConnect URL could cause race conditions. A short debouncing or a check for in-flight requests can mitigate unexpected states.
suite-native/state/src/reducers.ts (1)
23-25: Consider addressing the circular dependency for WalletConnectWhile the direct import of the WalletConnectReducer is a workable solution for now, circular dependencies often indicate architectural issues that should be addressed more thoroughly in the future. This workaround may make the codebase harder to maintain long-term.
Consider refactoring the dependency graph between
@suite-native/test-utilsand related modules to eliminate the circular dependency. This might involve:
- Extracting shared types to a separate package
- Restructuring the test utilities to avoid depending on state
- Creating interface-only packages for type definitions
Also applies to: 71-71, 102-102
📜 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/connect-mobile/src/index.ts(2 hunks)packages/connect/src/data/connectSettings.ts(0 hunks)scripts/list-outdated-dependencies/connect-dependencies.txt(1 hunks)scripts/list-outdated-dependencies/mobile-dependencies.txt(1 hunks)suite-common/connect-popup/src/connectPopupActions.ts(1 hunks)suite-common/connect-popup/src/connectPopupReducer.ts(1 hunks)suite-common/connect-popup/src/connectPopupThunks.ts(1 hunks)suite-common/connect-popup/src/connectPopupTypes.ts(1 hunks)suite-common/walletconnect/package.json(1 hunks)suite-native/app/package.json(5 hunks)suite-native/app/src/initActions.ts(2 hunks)suite-native/app/src/navigation/RootStackNavigator.tsx(2 hunks)suite-native/app/tsconfig.json(1 hunks)suite-native/feature-flags/src/__tests__/featureFlagsSlice.test.ts(3 hunks)suite-native/feature-flags/src/featureFlagsSlice.ts(3 hunks)suite-native/intl/src/en.ts(3 hunks)suite-native/module-connect-popup/package.json(1 hunks)suite-native/module-connect-popup/src/hooks/useConnectPopupNavigation.ts(2 hunks)suite-native/module-connect-popup/src/index.ts(1 hunks)suite-native/module-connect-popup/src/screens/ConnectPopupScreen.tsx(5 hunks)suite-native/module-connect-popup/src/screens/WalletConnectPairScreen.tsx(1 hunks)suite-native/module-connect-popup/src/screens/WalletConnectSessionPopupScreen.tsx(1 hunks)suite-native/module-connect-popup/tsconfig.json(1 hunks)suite-native/module-dev-utils/src/components/FeatureFlags.tsx(1 hunks)suite-native/module-settings/src/components/FeaturesSettings.tsx(2 hunks)suite-native/navigation/src/navigators.ts(1 hunks)suite-native/navigation/src/routes.ts(1 hunks)suite-native/state/package.json(1 hunks)suite-native/state/src/reducers.ts(4 hunks)suite-native/state/tsconfig.json(1 hunks)
💤 Files with no reviewable changes (1)
- packages/connect/src/data/connectSettings.ts
🚧 Files skipped from review as they are similar to previous changes (20)
- suite-native/app/tsconfig.json
- suite-native/module-dev-utils/src/components/FeatureFlags.tsx
- suite-native/module-connect-popup/tsconfig.json
- suite-native/navigation/src/navigators.ts
- suite-native/state/package.json
- suite-native/navigation/src/routes.ts
- suite-native/feature-flags/src/tests/featureFlagsSlice.test.ts
- scripts/list-outdated-dependencies/connect-dependencies.txt
- suite-native/app/src/initActions.ts
- suite-native/state/tsconfig.json
- suite-native/module-settings/src/components/FeaturesSettings.tsx
- suite-native/module-connect-popup/src/screens/WalletConnectSessionPopupScreen.tsx
- suite-native/module-connect-popup/package.json
- suite-common/connect-popup/src/connectPopupReducer.ts
- suite-common/connect-popup/src/connectPopupActions.ts
- suite-native/module-connect-popup/src/index.ts
- suite-common/walletconnect/package.json
- packages/connect-mobile/src/index.ts
- suite-native/feature-flags/src/featureFlagsSlice.ts
- suite-native/app/package.json
🧰 Additional context used
🪛 Biome (1.9.4)
suite-common/connect-popup/src/connectPopupThunks.ts
[error] 138-138: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
🪛 LanguageTool
scripts/list-outdated-dependencies/mobile-dependencies.txt
[duplication] ~81-~81: Possible typo: you repeated a word.
Context: ...tive-async-storage/async-storage @types/fast-text-encoding fast-text-encoding
(ENGLISH_WORD_REPEAT_RULE)
⏰ Context from checks skipped due to timeout of 90000ms (14)
- GitHub Check: PR-check / node-authorizeCoinjoin cancelCoinjoinAuthorization passphrase unlockPath setBusy checkFirmwareAuthenticity keepSession cancel.test info.test resetDevice-api
- 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: PR-check / node-override init-api-flaky
- GitHub Check: build-deploy
- GitHub Check: build-deploy
- GitHub Check: prepare_android_test_app
- GitHub Check: EAS Update
- GitHub Check: transport-e2e-test
- GitHub Check: build-web
- GitHub Check: test
- GitHub Check: Setup and Cache Dependencies
- GitHub Check: Analyze with CodeQL (javascript)
- GitHub Check: Socket Security: Pull Request Alerts
🔇 Additional comments (21)
suite-native/intl/src/en.ts (4)
327-349: Well-designed security messaging for WalletConnect integrationThe WalletConnect section includes comprehensive security-focused messaging that properly warns users about potential risks when connecting external applications. The status indicators (verified/unknown/dangerous) and clear error messages for scam detection, expired requests, and unverified connections provide users with the information needed to make informed decisions.
311-311: Good addition of cancel optionAdding a cancel button to the connect popup provides users with a clear way to abort the connection process, which is important for user control during security-sensitive operations.
322-322: Appropriate security error messageThe "methodNotAllowed" error message clearly communicates to users when an operation has been rejected due to security constraints, which aligns with the PR objective of implementing secure WalletConnect functionality.
584-587: Good integration in settings menuAdding WalletConnect to the settings menu with a clear title and descriptive subtitle makes the feature discoverable and helps users understand what the feature does without technical jargon.
suite-native/module-connect-popup/src/screens/ConnectPopupScreen.tsx (9)
1-1: No issues with the added import.
The addition ofuseStatefromreacthere appears correct and aligns with usage in this file.
6-6: Import usage looks good.
ImportingconnectPopupActionsandselectConnectPopupCallis consistent with this screen’s usage.
16-16: Navigation imports are appropriate.
Bringing inScreenandScreenHeaderfrom@suite-native/navigationmatches their usage below.
20-20: Component declaration is straightforward.
This functional component definition is consistent with typical patterns in React/React Native.
28-28: Popup call selector usage is valid.
Selecting the connect popup call from Redux is the correct approach for retrieving the call state.
30-35: Unaddressed deeplink-callback scenario.
Despite previously noted feedback, the snippet only handles'finished'state. It doesn’t handle'deeplink-callback'state to open external URLs. Integrate logic to handle'deeplink-callback'or use the dedicated hook for consistent handling.
74-78: Sanitize or validate user content before rendering.
IfpopupCall.originorpopupCall.methodTitlecan be user-supplied or untrusted, ensure to validate it. In React Native, XSS is less typical than on the web, but it’s prudent to guard against any malicious strings.
90-91: Confirmed fallback label is user-friendly.
Providing a fallback i18n translation ifpopupCall.confirmLabelis absent ensures a consistent user experience.
113-113: Localized error message returned.
Returning a localized error message fromgetTranslationId()effectively communicates specific error conditions to the user.suite-native/app/src/navigation/RootStackNavigator.tsx (2)
14-18: Clean import structure for the Connect Popup components.The import has been neatly organized to include the new WalletConnect-related screens along with the existing ConnectPopupScreen, maintaining code consistency.
87-94: Well-integrated navigation screens for WalletConnect functionality.The new WalletConnect screens have been properly added to the navigation stack, following the existing pattern of screen registration. This implementation aligns with the PR objective of implementing WalletConnect support for Suite Native.
suite-native/module-connect-popup/src/screens/WalletConnectPairScreen.tsx (3)
18-20: Good state management implementation.The component correctly manages three state variables for the URI, QR visibility, and pairing status. The isPairing state is used to disable interaction during the pairing process.
97-101: Clear handling of the empty state.Good implementation of a conditional message when no active connections are present, providing clear feedback to the user.
14-113: Verify deep link handling through testing.The implementation aligns with the PR objective to support the
trezorsuitelite://walletconnectdeeplink scheme, but ensure it's thoroughly tested.Test the deeplink scheme on both iOS and Android to verify the correct handling and navigation flow. Make sure the app can properly receive and parse WalletConnect URIs from external sources.
suite-native/module-connect-popup/src/hooks/useConnectPopupNavigation.ts (1)
67-72: Catch possible exceptions fromLinking.openURL.If
callbackUrlis invalid or fails to open, the exception might bubble up. Adding a.catch(...)helps handle or log failures. This has been previously suggested.suite-native/state/src/reducers.ts (2)
4-4: Proper integration of ConnectPopup reducerThe new ConnectPopup reducer has been correctly integrated following the same pattern as other reducers in the file. The imported preparer is initialized with extraDependencies like other reducers, and properly added to the walletReducers combiner.
Also applies to: 70-70, 101-101
105-136:❓ Verification inconclusive
Verify persistence behavior for new reducers
The new reducers (
connectPopupandwalletConnect) are added to the walletReducers but aren't included in the persisted keys list. This appears intentional, but it's worth confirming that these states should indeed be ephemeral and not persisted between sessions.
🏁 Script executed:
#!/bin/bash # Check if there are any references to persisting these reducers elsewhere rg -A 3 -B 3 "persistedKeys.*(\bconnectPopup\b|\bwalletConnect\b)" --type tsLength of output: 78
Below is an additional verification step to ensure we’re accurately handling the persistence behavior for the new reducers:
Once you review the output from the above script, please verify that the
connectPopupandwalletConnectreducers are indeed designed to be ephemeral (i.e. they are intentionally excluded from the persisted keys). If they should be persisted between sessions, thepersistedKeyslist will need updating. If their state is meant to remain transient, then no change is required.
Action: Verify Persistence Behavior for New Reducers
The
walletReducersnow includeconnectPopupandwalletConnect, but these keys aren’t part of the persisted state. This appears intentional if their state is meant to be ephemeral. However, please confirm that:
- Excluding these reducers from persistence is the intended behavior.
- No unintended side effects arise from their state not being retained between sessions.
Based on your review of the additional script output and the intended design, adjust the persistence configuration if necessary.
🏁 Script executed:
#!/bin/bash echo "Searching for walletReducers definition in suite-native/state/src/reducers.ts:" rg -n "const walletReducers" suite-native/state/src/reducers.ts echo "Searching for references to 'connectPopup' in suite-native/state/src/reducers.ts:" rg -n "connectPopup" suite-native/state/src/reducers.ts echo "Searching for references to 'walletConnect' in suite-native/state/src/reducers.ts:" rg -n "walletConnect" suite-native/state/src/reducers.tsLength of output: 1110
Action: Confirm Persistence Behavior for New Reducers
The verification shows that the
walletReducerscombineconnectPopupandwalletConnect, but these reducers are not included in thepersistedKeysarray—which currently persists only'accounts'and'transactions'. This appears intentional if the states forconnectPopupandwalletConnectare intended to be ephemeral. Please verify that:
- Ephemeral design is intentional: Confirm that
connectPopupandwalletConnectshould not persist between sessions.- Persisted state requirement: If these reducers are later required to retain state, update the
persistedKeyslist accordingly.Relevant locations in the codebase:
- File:
suite-native/state/src/reducers.ts
- connectPopup declaration: Line 70 & usage at Line 101.
- walletConnect declaration: Line 25 (import), Line 71 (declaration) & usage at Line 102.
If the ephemeral behavior is as designed, no change is necessary; otherwise, adjust the persistence configuration.
suite-native/module-connect-popup/src/screens/WalletConnectPairScreen.tsx
Show resolved
Hide resolved
| export const connectPopupCallThunkInner = createThunk< | ||
| ConnectPopupCallThunkResponse<keyof typeof TrezorConnect>, | ||
| ConnectPopupCallThunkParams<keyof typeof TrezorConnect> | ||
| >( | ||
| `${CONNECT_POPUP_MODULE}/callThunk`, | ||
| async ({ method, payload, processName, origin }, { dispatch, getState, extra }) => { | ||
| try { | ||
| // @ts-expect-error: method is dynamic | ||
| const methodInfo = await TrezorConnect[method]({ | ||
| ...payload, | ||
| __info: true, | ||
| }); | ||
| if (!methodInfo.success) { | ||
| dispatch( | ||
| connectPopupActions.initiateCall({ | ||
| state: 'call-error', | ||
| callError: ERRORS.TypedError(methodInfo.payload.code), | ||
| }), | ||
| ); | ||
| throw methodInfo; | ||
| } | ||
| if ( | ||
| methodInfo.payload.requiredPermissions.includes('management') || | ||
| methodInfo.payload.requiredPermissions.includes('push_tx') | ||
| ) { | ||
| dispatch( | ||
| connectPopupActions.initiateCall({ | ||
| state: 'call-error', | ||
| callError: ERRORS.TypedError('Method_NotAllowed'), | ||
| }), | ||
| ); | ||
|
|
||
| return; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider returning a consistent response type in permission-denied branches.
When required permissions include 'management' or 'push_tx', the code dispatches an error action and then return; on line 58. Since the function’s return type promises a response object, returning undefined could introduce type uncertainty downstream.
- return;
+ return {
+ success: false,
+ payload: ERRORS.TypedError('Method_NotAllowed'),
+ };📝 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 connectPopupCallThunkInner = createThunk< | |
| ConnectPopupCallThunkResponse<keyof typeof TrezorConnect>, | |
| ConnectPopupCallThunkParams<keyof typeof TrezorConnect> | |
| >( | |
| `${CONNECT_POPUP_MODULE}/callThunk`, | |
| async ({ method, payload, processName, origin }, { dispatch, getState, extra }) => { | |
| try { | |
| // @ts-expect-error: method is dynamic | |
| const methodInfo = await TrezorConnect[method]({ | |
| ...payload, | |
| __info: true, | |
| }); | |
| if (!methodInfo.success) { | |
| dispatch( | |
| connectPopupActions.initiateCall({ | |
| state: 'call-error', | |
| callError: ERRORS.TypedError(methodInfo.payload.code), | |
| }), | |
| ); | |
| throw methodInfo; | |
| } | |
| if ( | |
| methodInfo.payload.requiredPermissions.includes('management') || | |
| methodInfo.payload.requiredPermissions.includes('push_tx') | |
| ) { | |
| dispatch( | |
| connectPopupActions.initiateCall({ | |
| state: 'call-error', | |
| callError: ERRORS.TypedError('Method_NotAllowed'), | |
| }), | |
| ); | |
| return; | |
| export const connectPopupCallThunkInner = createThunk< | |
| ConnectPopupCallThunkResponse<keyof typeof TrezorConnect>, | |
| ConnectPopupCallThunkParams<keyof typeof TrezorConnect> | |
| >( | |
| `${CONNECT_POPUP_MODULE}/callThunk`, | |
| async ({ method, payload, processName, origin }, { dispatch, getState, extra }) => { | |
| try { | |
| // @ts-expect-error: method is dynamic | |
| const methodInfo = await TrezorConnect[method]({ | |
| ...payload, | |
| __info: true, | |
| }); | |
| if (!methodInfo.success) { | |
| dispatch( | |
| connectPopupActions.initiateCall({ | |
| state: 'call-error', | |
| callError: ERRORS.TypedError(methodInfo.payload.code), | |
| }), | |
| ); | |
| throw methodInfo; | |
| } | |
| if ( | |
| methodInfo.payload.requiredPermissions.includes('management') || | |
| methodInfo.payload.requiredPermissions.includes('push_tx') | |
| ) { | |
| dispatch( | |
| connectPopupActions.initiateCall({ | |
| state: 'call-error', | |
| callError: ERRORS.TypedError('Method_NotAllowed'), | |
| }), | |
| ); | |
| return { | |
| success: false, | |
| payload: ERRORS.TypedError('Method_NotAllowed'), | |
| }; | |
| } | |
| // Rest of the implementation... | |
| } catch (error) { | |
| // Error handling logic... | |
| } | |
| } | |
| ); |
martykan
force-pushed
the
feat/walletconnect-mobile
branch
from
41825e4 to
7b8fbf4
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
suite-native/module-connect-popup/src/screens/ConnectPopupScreen.tsx (1)
31-35:⚠️ Potential issueMissing deeplink callback handling.
The current implementation only handles navigation when
popupCall?.stateis 'finished', but doesn't handle the 'deeplink-callback' state. This means external URL navigation via deeplinks won't work properly.Apply this fix:
useEffect(() => { - if (popupCall?.state == 'finished' && navigation.canGoBack()) { - navigation.goBack(); - } + if (popupCall?.state === 'deeplink-callback') { + Linking.openURL(popupCall.callbackUrl); + } else if (popupCall?.state === 'finished' && navigation.canGoBack()) { + navigation.goBack(); + } }, [popupCall, navigation]);Note: You'll need to re-import the
Linkingmodule from 'react-native' that was previously removed.suite-common/connect-popup/src/connectPopupThunks.ts (1)
26-59: 🛠️ Refactor suggestionMissing return type in permission-denied branch.
When required permissions include 'management' or 'push_tx', the function returns undefined instead of a properly typed response object.
- return; + return { + success: false, + payload: ERRORS.TypedError('Method_NotAllowed'), + };
🧹 Nitpick comments (2)
suite-common/connect-popup/src/connectPopupThunks.ts (2)
58-66: Redundant connectSrc validation.The
connectSrcis being validated twice - once on line 58 and again on line 63, which is unnecessary.const connectSrc = this.validateConnectSrc(settings.connectSrc); this._settings = { ...parseConnectSettings({ ...this._settings, ...settings }), - connectSrc: this.validateConnectSrc(settings.connectSrc), + connectSrc, deeplinkUrl: `${connectSrc}deeplink/${DEEPLINK_VERSION}/`, deeplinkOpen: settings.deeplinkOpen, deeplinkCallbackUrl: settings.deeplinkCallbackUrl, };
138-138: Use optional chaining for path access.The code could use optional chaining to simplify the path access.
- const version = path && path.split('/').slice(-2, -1)[0]; + const version = path?.split('/')?.slice(-2, -1)?.[0];🧰 Tools
🪛 Biome (1.9.4)
[error] 138-138: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
📜 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 (56)
packages/connect-mobile/src/index.ts(2 hunks)packages/connect/src/data/connectSettings.ts(0 hunks)packages/suite-desktop-connect-popup/src/connectPopupThunks.ts(0 hunks)packages/suite-desktop-connect-popup/src/index.ts(0 hunks)packages/suite-desktop-connect-popup/tsconfig.json(0 hunks)packages/suite/package.json(1 hunks)packages/suite/src/actions/suite/initAction.ts(2 hunks)packages/suite/src/components/suite/modals/ReduxModal/UserContextModal/ConnectPopupModal.tsx(1 hunks)packages/suite/src/components/suite/modals/ReduxModal/UserContextModal/UserContextModal.tsx(1 hunks)packages/suite/src/middlewares/wallet/discoveryMiddleware.ts(1 hunks)packages/suite/src/middlewares/wallet/index.ts(2 hunks)packages/suite/src/reducers/wallet/index.ts(3 hunks)packages/suite/src/types/wallet/index.ts(3 hunks)packages/suite/tsconfig.json(1 hunks)scripts/list-outdated-dependencies/connect-dependencies.txt(1 hunks)scripts/list-outdated-dependencies/mobile-dependencies.txt(1 hunks)suite-common/connect-popup/package.json(2 hunks)suite-common/connect-popup/src/connectPopupActions.ts(1 hunks)suite-common/connect-popup/src/connectPopupDesktopThunks.ts(1 hunks)suite-common/connect-popup/src/connectPopupMiddleware.ts(1 hunks)suite-common/connect-popup/src/connectPopupReducer.ts(1 hunks)suite-common/connect-popup/src/connectPopupThunks.ts(1 hunks)suite-common/connect-popup/src/connectPopupTypes.ts(1 hunks)suite-common/connect-popup/src/index.ts(1 hunks)suite-common/connect-popup/tsconfig.json(1 hunks)suite-common/suite-types/src/modal.ts(0 hunks)suite-common/walletconnect/package.json(1 hunks)suite-common/walletconnect/src/adapters/ethereum.ts(1 hunks)suite-common/walletconnect/tsconfig.json(1 hunks)suite-native/app/e2e/tests/deeplinkPopup.test.ts(1 hunks)suite-native/app/package.json(5 hunks)suite-native/app/src/initActions.ts(2 hunks)suite-native/app/src/navigation/RootStackNavigator.tsx(2 hunks)suite-native/app/tsconfig.json(1 hunks)suite-native/feature-flags/src/__tests__/featureFlagsSlice.test.ts(3 hunks)suite-native/feature-flags/src/featureFlagsSlice.ts(3 hunks)suite-native/intl/src/en.ts(3 hunks)suite-native/module-connect-popup/package.json(1 hunks)suite-native/module-connect-popup/redux.d.ts(1 hunks)suite-native/module-connect-popup/src/hooks/useConnectMethod.ts(0 hunks)suite-native/module-connect-popup/src/hooks/useConnectParseParams.ts(0 hunks)suite-native/module-connect-popup/src/hooks/useConnectPopupNavigation.ts(2 hunks)suite-native/module-connect-popup/src/index.ts(1 hunks)suite-native/module-connect-popup/src/screens/ConnectPopupScreen.tsx(5 hunks)suite-native/module-connect-popup/src/screens/WalletConnectPairScreen.tsx(1 hunks)suite-native/module-connect-popup/src/screens/WalletConnectSessionPopupScreen.tsx(1 hunks)suite-native/module-connect-popup/src/utils/callbackURLOrigin.ts(0 hunks)suite-native/module-connect-popup/tsconfig.json(1 hunks)suite-native/module-dev-utils/src/components/FeatureFlags.tsx(1 hunks)suite-native/module-settings/src/components/FeaturesSettings.tsx(2 hunks)suite-native/navigation/package.json(0 hunks)suite-native/navigation/src/navigators.ts(1 hunks)suite-native/navigation/src/routes.ts(1 hunks)suite-native/state/package.json(1 hunks)suite-native/state/src/reducers.ts(4 hunks)suite-native/state/tsconfig.json(1 hunks)
💤 Files with no reviewable changes (9)
- packages/suite-desktop-connect-popup/src/index.ts
- packages/connect/src/data/connectSettings.ts
- suite-native/navigation/package.json
- suite-common/suite-types/src/modal.ts
- packages/suite-desktop-connect-popup/tsconfig.json
- suite-native/module-connect-popup/src/utils/callbackURLOrigin.ts
- suite-native/module-connect-popup/src/hooks/useConnectMethod.ts
- packages/suite-desktop-connect-popup/src/connectPopupThunks.ts
- suite-native/module-connect-popup/src/hooks/useConnectParseParams.ts
🚧 Files skipped from review as they are similar to previous changes (37)
- suite-native/app/tsconfig.json
- suite-native/state/package.json
- packages/suite/tsconfig.json
- suite-native/feature-flags/src/tests/featureFlagsSlice.test.ts
- suite-common/connect-popup/src/index.ts
- suite-common/connect-popup/src/connectPopupTypes.ts
- suite-common/connect-popup/src/connectPopupMiddleware.ts
- packages/suite/src/middlewares/wallet/index.ts
- suite-native/module-dev-utils/src/components/FeatureFlags.tsx
- scripts/list-outdated-dependencies/connect-dependencies.txt
- packages/suite/src/components/suite/modals/ReduxModal/UserContextModal/UserContextModal.tsx
- suite-native/module-connect-popup/tsconfig.json
- suite-common/walletconnect/src/adapters/ethereum.ts
- suite-common/connect-popup/tsconfig.json
- suite-common/connect-popup/src/connectPopupDesktopThunks.ts
- suite-native/module-connect-popup/src/index.ts
- suite-common/walletconnect/tsconfig.json
- packages/suite/src/middlewares/wallet/discoveryMiddleware.ts
- packages/suite/package.json
- suite-common/connect-popup/src/connectPopupActions.ts
- suite-native/module-settings/src/components/FeaturesSettings.tsx
- packages/suite/src/actions/suite/initAction.ts
- suite-common/connect-popup/src/connectPopupReducer.ts
- packages/suite/src/types/wallet/index.ts
- suite-native/app/src/initActions.ts
- suite-native/feature-flags/src/featureFlagsSlice.ts
- suite-native/module-connect-popup/src/hooks/useConnectPopupNavigation.ts
- suite-native/module-connect-popup/redux.d.ts
- suite-native/navigation/src/navigators.ts
- suite-common/walletconnect/package.json
- suite-native/module-connect-popup/src/screens/WalletConnectSessionPopupScreen.tsx
- suite-common/connect-popup/package.json
- suite-native/module-connect-popup/package.json
- suite-native/module-connect-popup/src/screens/WalletConnectPairScreen.tsx
- suite-native/navigation/src/routes.ts
- suite-native/state/tsconfig.json
- packages/suite/src/reducers/wallet/index.ts
🧰 Additional context used
🪛 Biome (1.9.4)
suite-common/connect-popup/src/connectPopupThunks.ts
[error] 138-138: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
🪛 LanguageTool
scripts/list-outdated-dependencies/mobile-dependencies.txt
[duplication] ~81-~81: Possible typo: you repeated a word.
Context: ...tive-async-storage/async-storage @types/fast-text-encoding fast-text-encoding
(ENGLISH_WORD_REPEAT_RULE)
⏰ Context from checks skipped due to timeout of 90000ms (20)
- 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-authorizeCoinjoin cancelCoinjoinAuthorization passphrase unlockPath setBusy checkFirmwareAuthenticity keepSession cancel.test info.test resetDevice-api
- GitHub Check: PR-check / web-override init-api-flaky
- GitHub Check: run-e2e-suite-desktop-tests (@group=wallet, trezor-user-env-unix bitcoin-regtest)
- GitHub Check: run-e2e-suite-desktop-tests (@group=other, trezor-user-env-unix)
- GitHub Check: run-e2e-suite-desktop-tests (@group=passphrase, trezor-user-env-unix)
- GitHub Check: run-e2e-suite-desktop-tests (@group=settings, trezor-user-env-unix bitcoin-regtest)
- GitHub Check: run-e2e-suite-desktop-tests (@group=device-management, trezor-user-env-unix)
- GitHub Check: run-e2e-suite-desktop-tests (@group=suite, trezor-user-env-unix)
- GitHub Check: build-deploy
- GitHub Check: build-deploy
- GitHub Check: EAS Update
- GitHub Check: transport-e2e-test
- GitHub Check: Analyze with CodeQL (javascript)
- GitHub Check: prepare_android_test_app
- GitHub Check: Setup and Cache Dependencies
- GitHub Check: build-web
- GitHub Check: test
- GitHub Check: Socket Security: Pull Request Alerts
🔇 Additional comments (21)
suite-native/app/src/navigation/RootStackNavigator.tsx (2)
14-18: New WalletConnect screens imported correctly.The import statement has been updated to include the new
WalletConnectPairScreenandWalletConnectSessionPopupScreencomponents from the connect-popup module, properly supporting the PR's objective of implementing WalletConnect functionality.
87-94: Navigation routes added for WalletConnect screens.The addition of these two new screens in the RootStack.Navigator matches the PR objective of implementing WalletConnect support in the Suite Native application. The navigation structure is appropriately set up to handle the new WalletConnect pairing and session flows.
suite-native/app/package.json (3)
26-26: WalletConnect dependency for async storage.This dependency is required by WalletConnect for persistent storage. While we typically use MMKV for persistent storage in the app, WalletConnect uses AsyncStorage directly as mentioned in previous discussions.
46-46: WalletConnect core dependencies added correctly.The addition of
@suite-common/walletconnectand@walletconnect/react-native-compatdependencies is essential for implementing the WalletConnect functionality in the mobile app, aligning with the PR objectives.Also applies to: 90-90
112-112: Text encoding dependencies for WalletConnect.The
fast-text-encodingpackage and its type definitions are required for WalletConnect functionality in the React Native environment to ensure proper text encoding/decoding.Also applies to: 142-142
scripts/list-outdated-dependencies/mobile-dependencies.txt (1)
80-82: Mobile dependencies list updated.The list has been correctly updated to include the new dependencies needed for WalletConnect implementation: AsyncStorage, text encoding library, and its type definitions.
🧰 Tools
🪛 LanguageTool
[duplication] ~81-~81: Possible typo: you repeated a word.
Context: ...tive-async-storage/async-storage @types/fast-text-encoding fast-text-encoding(ENGLISH_WORD_REPEAT_RULE)
suite-native/app/e2e/tests/deeplinkPopup.test.ts (1)
27-27: Improved URL handling in tests.The command has been enhanced in two ways:
- Single quotes are now properly encoded as
%27instead of being removed- The
--rawflag has been added to the commandThis improves the reliability of URI scheme tests, especially when handling URLs that might contain special characters.
suite-native/module-connect-popup/src/screens/ConnectPopupScreen.tsx (1)
99-114: Well-structured error handling.The error handling has been nicely improved with a clear switch statement that maps error codes to appropriate translation IDs.
suite-common/connect-popup/src/connectPopupThunks.ts (1)
48-52: Good implementation of connectSrc validation.The new validation method correctly checks for the Trezor Suite Lite scheme and falls back to the standard CORS validator.
suite-native/intl/src/en.ts (2)
327-349: Comprehensive WalletConnect strings added.The new WalletConnect section provides a complete set of strings for the UI, error handling, and status indicators, which aligns well with the PR objective of implementing WalletConnect support.
584-587: WalletConnect settings menu entry added.The addition of WalletConnect to the settings menu is consistent with the PR objective of adding WalletConnect support to the Suite Native application.
packages/connect-mobile/src/index.ts (2)
4-5: Appropriate imports for deeplink functionality.The addition of
corsValidatorandDEEPLINK_VERSIONimports supports the implementation of WalletConnect deeplink functionality.
48-52: Custom scheme validation for Trezor Suite Lite.The
validateConnectSrcmethod correctly prioritizes the Trezor Suite Lite scheme before falling back to standard CORS validation, which is necessary for the mobile WalletConnect implementation.packages/suite/src/components/suite/modals/ReduxModal/UserContextModal/ConnectPopupModal.tsx (5)
1-7: Imports updated to support Redux-based state management.The imports have been updated to support the transition from props-based to Redux-based state management. The component now imports actions and selectors from
@suite-common/connect-popup, which aligns with the PR's objective of centralizing connect popup functionality.
9-12: Component refactored to use Redux hooks instead of props.The component has been successfully refactored to use Redux hooks (
useDispatchanduseSelector) instead of accepting props. The conditional rendering logic is appropriate, ensuring the modal only renders when there's an active popup call in the 'request' state.
14-17: Event handlers now dispatch Redux actions.Event handlers have been properly implemented to dispatch Redux actions instead of calling prop functions. This is a good pattern that centralizes the state management logic in the Redux store rather than passing it through props.
19-37: Modal UI updated with refactored properties.The modal UI has been updated to use the new property names from the Redux store. The
methodTitleis now used instead ofmethod, and the confirm button now correctly uses theconfirmLabelproperty with a fallback to the generic "Confirm" translation.
38-54: Conditional rendering for process and origin information.The conditional rendering for process name and origin information has been maintained, ensuring these details are only shown when available. The paragraph styling is consistent with the design system.
suite-native/state/src/reducers.ts (3)
4-4: Clean integration of WalletConnect and connect popup dependenciesThe imports for WalletConnect functionality are properly structured. I particularly appreciate the detailed comment explaining the circular dependency issue and the solution employed (direct import of the reducer instead of the whole module).
Also applies to: 23-25
70-71: Well-integrated reducer initializationThe reducer initialization follows the established pattern in the codebase, using the extraDependencies pattern consistently with other reducers. This helps maintain code consistency.
101-102:❓ Verification inconclusive
Proper integration into wallet state tree
The new reducers are correctly integrated into the wallet state tree, which will make the WalletConnect and connect popup state accessible throughout the application.
One thing to verify is that these reducers are properly typed in any consuming components:
🏁 Script executed:
#!/bin/bash # Check how these new reducers are accessed in components rg "connectPopup|walletConnect" --type tsx --type ts -A 3 -B 3Length of output: 92
Below is an updated shell script that adds proper TSX file type recognition. Please re-run this script to verify how the new reducers are accessed in components:
#!/bin/bash # Updated command to search for 'connectPopup' and 'walletConnect' references with TSX file type support rg "connectPopup|walletConnect" --type-add 'tsx:*.tsx' --type tsx --type ts -A 3 -B 3Once you run this, please confirm if the results show the expected references.
martykan
force-pushed
the
feat/walletconnect-mobile
branch
from
7b8fbf4 to
9d1158e
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
suite-native/module-connect-popup/src/screens/ConnectPopupScreen.tsx (1)
31-35:⚠️ Potential issueDeeplink Callback Navigation Handling – Action Required
Currently, the
ConnectPopupScreencomponent only handles the case wherepopupCall?.stateis'finished'. However, when a deeplink callback is provided (i.e. whenpopupCall?.stateis'deeplink-callback'), there is no handling in this screen.Apply this diff to handle the deeplink callback state:
useEffect(() => { + if (popupCall?.state === 'deeplink-callback' && popupCall.callbackUrl) { + Linking.openURL(popupCall.callbackUrl); + } else if (popupCall?.state == 'finished' && navigation.canGoBack()) { navigation.goBack(); } }, [popupCall, navigation]);Note: You'll need to re-import the
Linkingmodule from 'react-native' to handle external URL navigation.suite-common/connect-popup/src/connectPopupThunks.ts (1)
26-59: 🛠️ Refactor suggestionConsider returning a consistent response type in permission-denied branches.
When required permissions include
'management'or'push_tx', the code dispatches an error action and then uses a barereturn;on line 58. Since the function's return type promises a response object, returningundefinedcould introduce type uncertainty downstream.- return; + return { + success: false, + payload: ERRORS.TypedError('Method_NotAllowed'), + };
🧹 Nitpick comments (2)
suite-common/connect-popup/src/connectPopupThunks.ts (2)
138-138: Use optional chaining for safer property accessThe static analysis tool suggests using optional chaining here for safer access to potentially undefined properties.
- const version = path && path.split('/').slice(-2, -1)[0]; + const version = path?.split('/').slice(-2, -1)[0];🧰 Tools
🪛 Biome (1.9.4)
[error] 138-138: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
169-183: Consider adding error type differentiationThe try-catch block currently doesn't distinguish between different types of errors (JSON parse errors vs URL construction errors). This makes debugging more difficult.
try { payload = JSON.parse(queryParams.params); callbackUrl = new URL(callback); } catch (error) { + console.warn('Failed to parse params or callback URL', { error }); dispatch( connectPopupActions.initiateCall({ state: 'call-error', - callError: TypedError('Method_InvalidParameter'), + callError: error instanceof SyntaxError + ? TypedError('Method_InvalidParameter', 'Invalid JSON') + : TypedError('Method_InvalidParameter', 'Invalid callback URL'), }), ); return; }
📜 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 (56)
packages/connect-mobile/src/index.ts(2 hunks)packages/connect/src/data/connectSettings.ts(0 hunks)packages/suite-desktop-connect-popup/src/connectPopupThunks.ts(0 hunks)packages/suite-desktop-connect-popup/src/index.ts(0 hunks)packages/suite-desktop-connect-popup/tsconfig.json(0 hunks)packages/suite/package.json(1 hunks)packages/suite/src/actions/suite/initAction.ts(2 hunks)packages/suite/src/components/suite/modals/ReduxModal/UserContextModal/ConnectPopupModal.tsx(1 hunks)packages/suite/src/components/suite/modals/ReduxModal/UserContextModal/UserContextModal.tsx(1 hunks)packages/suite/src/middlewares/wallet/discoveryMiddleware.ts(1 hunks)packages/suite/src/middlewares/wallet/index.ts(2 hunks)packages/suite/src/reducers/wallet/index.ts(3 hunks)packages/suite/src/types/wallet/index.ts(3 hunks)packages/suite/tsconfig.json(1 hunks)scripts/list-outdated-dependencies/connect-dependencies.txt(1 hunks)scripts/list-outdated-dependencies/mobile-dependencies.txt(1 hunks)suite-common/connect-popup/package.json(2 hunks)suite-common/connect-popup/src/connectPopupActions.ts(1 hunks)suite-common/connect-popup/src/connectPopupDesktopThunks.ts(1 hunks)suite-common/connect-popup/src/connectPopupMiddleware.ts(1 hunks)suite-common/connect-popup/src/connectPopupReducer.ts(1 hunks)suite-common/connect-popup/src/connectPopupThunks.ts(1 hunks)suite-common/connect-popup/src/connectPopupTypes.ts(1 hunks)suite-common/connect-popup/src/index.ts(1 hunks)suite-common/connect-popup/tsconfig.json(1 hunks)suite-common/suite-types/src/modal.ts(0 hunks)suite-common/walletconnect/package.json(1 hunks)suite-common/walletconnect/src/adapters/ethereum.ts(1 hunks)suite-common/walletconnect/tsconfig.json(1 hunks)suite-native/app/e2e/tests/deeplinkPopup.test.ts(4 hunks)suite-native/app/package.json(5 hunks)suite-native/app/src/initActions.ts(2 hunks)suite-native/app/src/navigation/RootStackNavigator.tsx(2 hunks)suite-native/app/tsconfig.json(1 hunks)suite-native/feature-flags/src/__tests__/featureFlagsSlice.test.ts(3 hunks)suite-native/feature-flags/src/featureFlagsSlice.ts(3 hunks)suite-native/intl/src/en.ts(3 hunks)suite-native/module-connect-popup/package.json(1 hunks)suite-native/module-connect-popup/redux.d.ts(1 hunks)suite-native/module-connect-popup/src/hooks/useConnectMethod.ts(0 hunks)suite-native/module-connect-popup/src/hooks/useConnectParseParams.ts(0 hunks)suite-native/module-connect-popup/src/hooks/useConnectPopupNavigation.ts(2 hunks)suite-native/module-connect-popup/src/index.ts(1 hunks)suite-native/module-connect-popup/src/screens/ConnectPopupScreen.tsx(5 hunks)suite-native/module-connect-popup/src/screens/WalletConnectPairScreen.tsx(1 hunks)suite-native/module-connect-popup/src/screens/WalletConnectSessionPopupScreen.tsx(1 hunks)suite-native/module-connect-popup/src/utils/callbackURLOrigin.ts(0 hunks)suite-native/module-connect-popup/tsconfig.json(1 hunks)suite-native/module-dev-utils/src/components/FeatureFlags.tsx(1 hunks)suite-native/module-settings/src/components/FeaturesSettings.tsx(2 hunks)suite-native/navigation/package.json(0 hunks)suite-native/navigation/src/navigators.ts(1 hunks)suite-native/navigation/src/routes.ts(1 hunks)suite-native/state/package.json(1 hunks)suite-native/state/src/reducers.ts(4 hunks)suite-native/state/tsconfig.json(1 hunks)
💤 Files with no reviewable changes (9)
- packages/suite-desktop-connect-popup/src/index.ts
- packages/suite-desktop-connect-popup/tsconfig.json
- suite-native/navigation/package.json
- suite-native/module-connect-popup/src/utils/callbackURLOrigin.ts
- suite-common/suite-types/src/modal.ts
- packages/connect/src/data/connectSettings.ts
- suite-native/module-connect-popup/src/hooks/useConnectParseParams.ts
- suite-native/module-connect-popup/src/hooks/useConnectMethod.ts
- packages/suite-desktop-connect-popup/src/connectPopupThunks.ts
🚧 Files skipped from review as they are similar to previous changes (39)
- suite-native/module-connect-popup/redux.d.ts
- suite-native/state/package.json
- suite-native/app/tsconfig.json
- suite-common/walletconnect/tsconfig.json
- suite-native/navigation/src/routes.ts
- suite-native/module-connect-popup/src/index.ts
- packages/suite/src/components/suite/modals/ReduxModal/UserContextModal/UserContextModal.tsx
- scripts/list-outdated-dependencies/connect-dependencies.txt
- suite-native/module-dev-utils/src/components/FeatureFlags.tsx
- suite-native/module-connect-popup/tsconfig.json
- suite-native/navigation/src/navigators.ts
- packages/suite/src/reducers/wallet/index.ts
- packages/suite/src/actions/suite/initAction.ts
- suite-native/module-settings/src/components/FeaturesSettings.tsx
- packages/suite/src/middlewares/wallet/index.ts
- packages/suite/package.json
- suite-common/walletconnect/src/adapters/ethereum.ts
- suite-native/state/tsconfig.json
- packages/suite/tsconfig.json
- packages/suite/src/types/wallet/index.ts
- suite-common/connect-popup/src/connectPopupMiddleware.ts
- suite-common/connect-popup/tsconfig.json
- packages/suite/src/middlewares/wallet/discoveryMiddleware.ts
- suite-common/connect-popup/src/index.ts
- suite-common/connect-popup/src/connectPopupActions.ts
- suite-native/feature-flags/src/featureFlagsSlice.ts
- suite-native/state/src/reducers.ts
- suite-native/module-connect-popup/src/screens/WalletConnectSessionPopupScreen.tsx
- suite-common/connect-popup/src/connectPopupDesktopThunks.ts
- suite-native/app/e2e/tests/deeplinkPopup.test.ts
- suite-native/feature-flags/src/tests/featureFlagsSlice.test.ts
- suite-common/connect-popup/package.json
- suite-common/connect-popup/src/connectPopupTypes.ts
- suite-common/walletconnect/package.json
- suite-native/module-connect-popup/src/hooks/useConnectPopupNavigation.ts
- suite-native/module-connect-popup/src/screens/WalletConnectPairScreen.tsx
- suite-common/connect-popup/src/connectPopupReducer.ts
- packages/connect-mobile/src/index.ts
- suite-native/module-connect-popup/package.json
🧰 Additional context used
🪛 Biome (1.9.4)
suite-common/connect-popup/src/connectPopupThunks.ts
[error] 138-138: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
🪛 LanguageTool
scripts/list-outdated-dependencies/mobile-dependencies.txt
[duplication] ~81-~81: Possible typo: you repeated a word.
Context: ...tive-async-storage/async-storage @types/fast-text-encoding fast-text-encoding
(ENGLISH_WORD_REPEAT_RULE)
⏰ Context from checks skipped due to timeout of 90000ms (19)
- 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-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-e2e-suite-desktop-tests (@group=wallet, trezor-user-env-unix bitcoin-regtest)
- GitHub Check: run-e2e-suite-desktop-tests (@group=other, trezor-user-env-unix)
- GitHub Check: run-e2e-suite-desktop-tests (@group=passphrase, trezor-user-env-unix)
- GitHub Check: run-e2e-suite-desktop-tests (@group=settings, trezor-user-env-unix bitcoin-regtest)
- GitHub Check: run-e2e-suite-desktop-tests (@group=device-management, trezor-user-env-unix)
- GitHub Check: run-e2e-suite-desktop-tests (@group=suite, trezor-user-env-unix)
- GitHub Check: build-deploy
- GitHub Check: build-deploy
- GitHub Check: EAS Update
- GitHub Check: transport-e2e-test
- GitHub Check: Setup and Cache Dependencies
- GitHub Check: Analyze with CodeQL (javascript)
- GitHub Check: prepare_android_test_app
- GitHub Check: test
- GitHub Check: build-web
🔇 Additional comments (18)
scripts/list-outdated-dependencies/mobile-dependencies.txt (1)
80-82: Added dependencies align with WalletConnect functionalityThe new dependencies added to the mobile dependencies list are necessary for the WalletConnect implementation. These align with the package.json additions and support the PR objective of implementing WalletConnect in Suite Native.
🧰 Tools
🪛 LanguageTool
[duplication] ~81-~81: Possible typo: you repeated a word.
Context: ...tive-async-storage/async-storage @types/fast-text-encoding fast-text-encoding(ENGLISH_WORD_REPEAT_RULE)
suite-native/app/src/navigation/RootStackNavigator.tsx (2)
14-18: LGTM: Clean import structuringThe import structure follows the project's pattern by grouping related components from the connect-popup module.
87-94: WalletConnect navigation routes properly integratedThe new screens for WalletConnect are added to the navigation stack following existing patterns. This implementation aligns well with the PR objective of supporting WalletConnect in Suite Native.
suite-native/app/src/initActions.ts (2)
12-14: LGTM: Necessary imports for WalletConnect initializationAdded imports to support the WalletConnect initialization with feature flag control.
54-57: Feature-flagged WalletConnect initialization properly implementedGood implementation of the conditional WalletConnect initialization based on feature flag status. The initialization is placed at the appropriate point in the application startup flow, after device initialization and before marking the app as ready.
suite-native/app/package.json (4)
26-26: AsyncStorage dependency for WalletConnectAdding this dependency is necessary as WalletConnect uses it directly. Previous discussions acknowledged that while the team prefers react-native-mmkv for persistent storage (with encryption support), this dependency is required by WalletConnect.
46-46: Added WalletConnect workspace dependencyCorrectly integrates the common WalletConnect package from workspace, following the project's internal dependency pattern.
90-90: Added WalletConnect React Native compatibility packageThe dependency is correctly added to support WalletConnect in the React Native environment.
112-112: Text encoding dependencies for WalletConnectThese dependencies provide necessary text encoding functionality and type definitions required by the WalletConnect implementation.
Also applies to: 142-142
suite-native/module-connect-popup/src/screens/ConnectPopupScreen.tsx (2)
39-97: Approval of connection UI and workflowThe connection approval workflow is properly implemented with clear UI indications, showing method information, origin details, and a confirmation button.
99-114: Well structured error handlingThe error handling implementation using a switch statement based on error codes improves the user experience by providing specific error messages for different scenarios.
suite-common/connect-popup/src/connectPopupThunks.ts (2)
122-132: Add URL validation for WalletConnect scheme.This function should validate that the URL matches the expected scheme for WalletConnect deeplinks, such as "trezorsuitelite://walletconnect".
let parsedUrl; try { parsedUrl = new URL(url); + // Validate URL scheme for WalletConnect deeplinks + const validSchemes = ['trezorsuitelite']; + if (!validSchemes.includes(parsedUrl.protocol.replace(':', ''))) { + throw new Error('Invalid URL scheme'); + } } catch (error) { console.warn('Invalid deeplink URL', { error, url }); return; }
185-202: Robust implementation for WalletConnect deeplink handlingThe implementation correctly handles the flow for deeplink callback processing - dispatching the connect popup call thunk with the appropriate parameters and updating the callback URL with the response.
suite-native/intl/src/en.ts (4)
311-311: Cancel button additionAdding a cancel option provides better user control in the WalletConnect flow.
322-322: Clear method error messageThe error message for disallowed methods clearly informs users about security restrictions.
327-349: Comprehensive WalletConnect messagingThe translation strings added for WalletConnect provide a complete set of user interface messages, including connection flow, status indicators, and error scenarios. This implementation aligns well with the PR objective of implementing WalletConnect support.
584-587: Added WalletConnect to settings menuAdding WalletConnect to the device settings menu provides users with easy access to configure and manage external app connections.
packages/suite/src/components/suite/modals/ReduxModal/UserContextModal/ConnectPopupModal.tsx (1)
1-54: Improved Redux integration for ConnectPopupModalThe complete refactoring of this component improves its integration with Redux by:
- Using Redux hooks to access state directly
- Dispatching actions for confirm and cancel operations
- Simplifying the component's props and structure
This change aligns well with the PR objective of implementing WalletConnect and unifying the logic between desktop and mobile Connect Popup.
| @@ -1,81 +1,43 @@ | |||
| import { useCallback, useMemo, useState } from 'react'; | |||
| import { useEffect, useMemo, useState } from 'react'; | |||
There was a problem hiding this comment.
Missing import for Linking module
The Linking import has been removed but is needed for handling deeplink callbacks.
-import { useEffect, useMemo, useState } from 'react';
+import { useEffect, useMemo, useState } from 'react';
+import { Linking } from 'react-native';📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| import { useEffect, useMemo, useState } from 'react'; | |
| import { useEffect, useMemo, useState } from 'react'; | |
| import { Linking } from 'react-native'; |
Description
Support WalletConnect in Suite Native, the same way as Desktop suite.
This PR unifies logic between desktop and mobile Connect Popup and builds on top of that.
Deeplink scheme is
trezorsuitelite://walletconnect.Screenshots: