Wallet Connect external browser redirect#918
Conversation
PR SummaryAdded support for handling WalletConnect deep linking with external browser redirection. Improved connection state management and user feedback during the WalletConnect pairing process. Added fallback mechanism with toast notifications when redirect URL is missing. Enhanced error handling and logging throughout the WalletConnect flow. Changes
autogenerated by presubmit.ai |
lealobanov
changed the title
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
🚨 Pull request needs attention.
Review Summary
Commits Considered (1)
- 79de9db: WIP - integration
Files Processed (7)
- app/src/main/java/com/flowfoundation/wallet/manager/walletconnect/WalletConnect.kt (3 hunks)
- app/src/main/java/com/flowfoundation/wallet/manager/walletconnect/WalletConnectDelegate.kt (6 hunks)
- app/src/main/java/com/flowfoundation/wallet/manager/walletconnect/WalletConnectRequestDispatcher.kt (1 hunk)
- app/src/main/java/com/flowfoundation/wallet/page/component/deeplinking/DeepLinkingActivity.kt (2 hunks)
- app/src/main/java/com/flowfoundation/wallet/page/component/deeplinking/PendingActionHelper.kt (2 hunks)
- app/src/main/java/com/flowfoundation/wallet/page/component/deeplinking/Utils.kt (4 hunks)
- app/src/main/java/com/flowfoundation/wallet/widgets/webview/fcl/dialog/FclAuthnDialog.kt (2 hunks)
Actionable Comments (1)
-
app/src/main/java/com/flowfoundation/wallet/manager/walletconnect/WalletConnectDelegate.kt [134-134]
possible issue: "Potential race condition in activity state check"
Skipped Comments (3)
-
app/src/main/java/com/flowfoundation/wallet/manager/walletconnect/WalletConnectDelegate.kt [62-63]
best practice: "Improve error handling for connection state changes"
-
app/src/main/java/com/flowfoundation/wallet/manager/walletconnect/WalletConnect.kt [61-62]
possible bug: "Potential memory leak in session management"
-
app/src/main/java/com/flowfoundation/wallet/page/component/deeplinking/DeepLinkingActivity.kt [32-42]
enhancement: "Improve deep linking error recovery"
| var attempts = 0 | ||
| val maxAttempts = 5 | ||
| var activity: BaseActivity? = null | ||
|
|
There was a problem hiding this comment.
The activity state check is done in a loop with a fixed number of attempts, but there's no guarantee the activity will be ready even after the maximum attempts. Consider implementing a more robust activity lifecycle-aware solution or handling the null activity case more gracefully.
|
|
||
| logd(TAG, "Attempting to redirect to: $redirectUrl") | ||
| try { | ||
| redirectUrl.openInSystemBrowser(activity, true) |
There was a problem hiding this comment.
The user might not really coming form system browser, for example they could deeplink from telegram to flow wallet. If we navigate them to browser, it's wired.
| sessionProposal.redirect | ||
| } else { | ||
| logd(TAG, "No redirect provided, using DApp URL: ${sessionProposal.url}") | ||
| sessionProposal.url |
There was a problem hiding this comment.
We need double check on this, for example:
if user comes from flow port A, now if we navigate user back with flow port url.
Will it open flow port B ? But our app was just authn the flow port A.
Screen.Recording.2025-05-01.at.8.40.44.PM.mov |
There was a problem hiding this comment.
🚨 Pull request needs attention.
Review Summary
Commits Considered (1)
- 9e26de8: WC redirect fallback
Files Processed (3)
- app/src/main/java/com/flowfoundation/wallet/manager/walletconnect/WalletConnect.kt (5 hunks)
- app/src/main/java/com/flowfoundation/wallet/manager/walletconnect/WalletConnectDelegate.kt (5 hunks)
- app/src/main/res/values/strings.xml (2 hunks)
Actionable Comments (1)
-
app/src/main/java/com/flowfoundation/wallet/manager/walletconnect/WalletConnectDelegate.kt [284-292]
best practice: "Missing URL validation before redirect"
Skipped Comments (2)
-
app/src/main/java/com/flowfoundation/wallet/manager/walletconnect/WalletConnectDelegate.kt [137-143]
performance: "Potential performance bottleneck in activity retrieval"
-
app/src/main/java/com/flowfoundation/wallet/manager/walletconnect/WalletConnect.kt [67-82]
maintainability: "Code duplication in connection handling"
| try { | ||
| redirectUrl.openInSystemBrowser(activity, true) | ||
| logd(TAG, "Successfully opened URL in system browser") | ||
| pendingRedirectUrl = null | ||
| isRedirecting = false | ||
| } catch (e: Exception) { | ||
| loge(TAG, "Failed to open URL in system browser: ${e.message}") | ||
| loge(e) | ||
| } |
There was a problem hiding this comment.
The redirect URL is opened without any validation. Consider adding URL validation to prevent potential security issues with malicious URLs. Also, add a try-catch block specifically for the openInSystemBrowser call to handle potential ActivityNotFoundException.
lealobanov
changed the title
| <string name="blocked_tip">This app has been flagged as dangerous.</string> | ||
| <string name="switch_to">Switch to %1$s</string> | ||
| <string name="deeplink_login_failed">Failed to open, please create a wallet first.</string> | ||
| <string name="return_to_browser_to_continue">Please return to the browser to continue your session.</string> |
There was a problem hiding this comment.
To-do: add translations
There was a problem hiding this comment.
🚨 Pull request needs attention.
Review Summary
Commits Considered (1)
- 3b0743c: WC redirect fallback
Files Processed (1)
- app/src/main/java/com/flowfoundation/wallet/page/component/deeplinking/Utils.kt (4 hunks)
Actionable Comments (1)
-
app/src/main/java/com/flowfoundation/wallet/page/component/deeplinking/Utils.kt [124-133]
possible issue: "Potential timeout issue in initialization retry logic"
Skipped Comments (2)
-
app/src/main/java/com/flowfoundation/wallet/page/component/deeplinking/Utils.kt [57-64]
enhancement: "Missing user feedback for failed connection attempts"
-
app/src/main/java/com/flowfoundation/wallet/page/component/deeplinking/Utils.kt [189-198]
possible bug: "Potential URI decoding edge case"
| val initResult = withTimeoutOrNull(3000) { | ||
| var waitTime = 50L | ||
| var attempts = 0 | ||
| val maxAttempts = 15 | ||
|
|
||
| while (!WalletConnect.isInitialized() && attempts < maxAttempts) { | ||
| logd(TAG, "Waiting for WalletConnect initialization (attempt $attempts)") | ||
| delay(waitTime) | ||
| attempts++ | ||
| waitTime = minOf(waitTime * 2, 500) |
There was a problem hiding this comment.
The exponential backoff implementation could potentially exceed the 3000ms timeout due to the delay accumulation. Consider adding a check to ensure the total elapsed time doesn't exceed the timeout value, or adjust the delay calculation to account for the total time limit.
| } | ||
| return@runCatching true | ||
| result | ||
| } else { |
There was a problem hiding this comment.
The result variable is set inside the ioScope block but returned outside of it. Since ioScope is asynchronous, the result value might not be set when it's returned. Consider restructuring the code to properly handle the asynchronous result, possibly by making the entire initialization block synchronous or by using suspendCoroutine.
| val maxAttempts = 15 | ||
| val maxAttempts = 5 | ||
|
|
||
| while (!WalletConnect.isInitialized() && attempts < maxAttempts) { |
There was a problem hiding this comment.
The while loop condition checks attempts but doesn't increment the counter, which could lead to an infinite loop. Add attempts++ inside the loop body.
There was a problem hiding this comment.
✅ LGTM!
Review Summary
Commits Considered (1)
- a179ec4: WC redirect fallback
Files Processed (1)
- app/src/main/java/com/flowfoundation/wallet/widgets/webview/fcl/dialog/FclAuthnDialog.kt (1 hunk)
Actionable Comments (0)
Skipped Comments (0)
There was a problem hiding this comment.
🚨 Pull request needs attention.
Review Summary
Commits Considered (1)
- 9ec01b4: Clean up
Files Processed (3)
- app/src/main/java/com/flowfoundation/wallet/manager/walletconnect/WalletConnectDelegate.kt (5 hunks)
- app/src/main/java/com/flowfoundation/wallet/page/component/deeplinking/Utils.kt (6 hunks)
- app/src/main/java/com/flowfoundation/wallet/widgets/webview/fcl/dialog/FclAuthnDialog.kt (1 hunk)
Actionable Comments (1)
-
app/src/main/java/com/flowfoundation/wallet/page/component/deeplinking/Utils.kt [171-178]
possible issue: "Improve URL decoding robustness"
Skipped Comments (2)
-
app/src/main/java/com/flowfoundation/wallet/manager/walletconnect/WalletConnectDelegate.kt [73-74]
best practice: "Improve error handling for browser intent"
-
app/src/main/java/com/flowfoundation/wallet/manager/walletconnect/WalletConnectDelegate.kt [136-141]
best practice: "Implement exponential backoff for reconnection attempts"
| wcUriEncoded?.let { | ||
| if (it.contains("%")) { | ||
| URLDecoder.decode(it, StandardCharsets.UTF_8.name()) | ||
| val decoded = URLDecoder.decode(it, StandardCharsets.UTF_8.name()) | ||
| decoded | ||
| } else { | ||
| it | ||
| } | ||
| } |
There was a problem hiding this comment.
The URL decoding is only performed when the URI contains '%', but there might be other URL-encoded characters that don't use percent encoding. Consider always decoding the URI to ensure all encoded characters are properly handled. Also, the error handling for decoding should be more specific to catch UnsupportedEncodingException and IllegalArgumentException.
There was a problem hiding this comment.
🚨 Pull request needs attention.
Review Summary
Commits Considered (1)
- 010aa2d: Hide toast when on inapp browser auth
Files Processed (1)
- app/src/main/java/com/flowfoundation/wallet/manager/walletconnect/WalletConnectDelegate.kt (5 hunks)
Actionable Comments (1)
-
app/src/main/java/com/flowfoundation/wallet/manager/walletconnect/WalletConnectDelegate.kt [45-58]
performance: "Potential infinite reconnection loop"
Skipped Comments (2)
-
app/src/main/java/com/flowfoundation/wallet/manager/walletconnect/WalletConnectDelegate.kt [127-133]
possible issue: "Potential race condition in activity retrieval loop"
-
app/src/main/java/com/flowfoundation/wallet/manager/walletconnect/WalletConnectDelegate.kt [180-191]
possible bug: "Race condition in UI visibility check"
| if (!state.isAvailable) { | ||
| logd(TAG, "Connection lost, attempting to reconnect") | ||
| ioScope { | ||
| delay(1000) // Wait before reconnecting | ||
| try { | ||
| CoreClient.Relay.connect { error: Core.Model.Error -> | ||
| loge(TAG, "CoreClient.Relay connect error: $error") | ||
| } | ||
| } catch (e: Exception) { | ||
| loge(TAG, "Error reconnecting: ${e.message}") | ||
| loge(e) | ||
| } | ||
| } | ||
| } else if (isRedirecting) { |
There was a problem hiding this comment.
The reconnection logic doesn't have a maximum retry limit or exponential backoff. This could lead to aggressive reconnection attempts that drain battery or network resources if the connection keeps failing.
2 participants
Related Issue
Summary of Changes
Need Regression Testing
Risk Assessment
Additional Notes
Screenshots (if applicable)