Feat(DApps): Refactoring wallet connect service and bring BrowserConnect inline with WC #16730

alexjba · 2024-11-07T09:24:59Z

What does the PR do

Needs status-go PR: status-im/status-go#6051

This is the final PR in the dapps refactoring. At this point the WalletConnect and BrowserConnect is using the same UI and the same qml processing.

417ce05 - Unify dapp sessions between Wallet Connect and Browser Connect
e74fe00 - Unify dapp signing between WalletConnect and Browser Connect

Next step for later: Refactor the test_DAppsWorkflow.qml. Currently it's testing everything E2E. But now we have the component separation needed to test everything in isolation.

Affected areas

Wallet Connect
Browser Connect

Architecture compliance

I am familiar with the application architecture and agreed good practices.
My PR is consistent with this document: Status Desktop Architecture Guide

Screenshot of functionality (including design for comparison)

I've checked the design and this PR matches it

status-im-auto · 2024-11-07T09:32:10Z

Jenkins Builds

Click to see older builds (12)

❔	Commit	#️⃣	Finished (UTC)	Duration	Platform	Result
✔️	`e74fe00`	#1	2024-11-07 09:32:09	~6 min	`tests/nim`	📄`log`
✔️	`e74fe00`	#1	2024-11-07 09:32:13	~6 min	`macos/aarch64`	🍎`dmg`
❌	`e74fe00`	#1	2024-11-07 09:35:42	~10 min	`tests/ui`	📄`log`
✔️	`e74fe00`	#1	2024-11-07 09:44:29	~19 min	`linux/x86_64`	📦`tgz`
✔️	`e74fe00`	#1	2024-11-07 09:44:40	~19 min	`macos/x86_64`	🍎`dmg`
✔️	`e74fe00`	#1	2024-11-07 09:47:15	~22 min	`linux-nix/x86_64`	📦`tgz`
✔️	`e74fe00`	#1	2024-11-07 09:53:38	~28 min	`windows/x86_64`	💿`exe`
❌	`e74fe00`	#2	2024-11-07 10:04:05	~9 min	`tests/ui`	📄`log`
✔️	`e74fe00`	#2	2024-11-07 12:18:26	~4 min	`macos/aarch64`	🍎`dmg`
❌	`e74fe00`	#2	2024-11-07 12:20:24	~6 min	`windows/x86_64`	📄`log`
✔️	`e74fe00`	#2	2024-11-07 12:20:50	~7 min	`tests/nim`	📄`log`
❌	`e74fe00`	#3	2024-11-07 12:23:54	~10 min	`tests/ui`	📄`log`

❔	Commit	#️⃣	Finished (UTC)	Duration	Platform	Result
✔️	`b6aae86`	#3	2024-11-07 12:31:19	~4 min	`macos/aarch64`	🍎`dmg`
❌	`b6aae86`	#3	2024-11-07 12:31:23	~4 min	`linux-nix/x86_64`	📄`log`
✔️	`b6aae86`	#3	2024-11-07 12:33:33	~7 min	`tests/nim`	📄`log`
❌	`b6aae86`	#4	2024-11-07 12:36:59	~10 min	`tests/ui`	📄`log`
✔️	`b6aae86`	#3	2024-11-07 12:44:13	~17 min	`linux/x86_64`	📦`tgz`
✔️	`b6aae86`	#3	2024-11-07 12:44:33	~18 min	`macos/x86_64`	🍎`dmg`
✔️	`b6aae86`	#3	2024-11-07 12:51:14	~24 min	`windows/x86_64`	💿`exe`

✔️	`698cbee`	#4	2024-11-08 14:52:41	~4 min	`macos/aarch64`	🍎`dmg`
✔️	`698cbee`	#4	2024-11-08 14:55:30	~7 min	`tests/nim`	📄`log`
❌	`698cbee`	#5	2024-11-08 14:57:24	~9 min	`tests/ui`	📄`log`
✔️	`698cbee`	#4	2024-11-08 15:03:17	~15 min	`macos/x86_64`	🍎`dmg`
✔️	`698cbee`	#4	2024-11-08 15:04:15	~16 min	`linux/x86_64`	📦`tgz`
✔️	`698cbee`	#4	2024-11-08 15:05:30	~17 min	`linux-nix/x86_64`	📦`tgz`

alexjba · 2024-11-07T09:29:56Z

src/app/core/signals/remote_signals/connector.nim

@@ -21,6 +21,8 @@ type ConnectorGrantDAppPermissionSignal* = ref object of Signal
  url*: string
  name*: string
  iconUrl*: string


These arguments were missing from the integration. We need to know in the UI on what account a dapp is connected and on what chains.

alexjba · 2024-11-07T09:31:37Z

src/app/modules/shared_modules/connector/controller.nim

-  proc dappValidatesTransaction*(self: Controller, requestId: string, payload: string) {.signal.}
-  proc dappGrantDAppPermission*(self: Controller, payload: string) {.signal.}
-  proc dappRevokeDAppPermission*(self: Controller, payload: string) {.signal.}
+  proc connectRequested*(self: Controller, requestId: string, payload: string) {.signal.}


Added request-response pair for the API. This is needed in the UI because it's reacting only on the response. From this point on the nim requests can be done asynchronous.

alexjba · 2024-11-07T09:32:10Z

src/app_service/service/connector/service.nim

@@ -137,4 +137,17 @@ QtObject:

    except Exception as e:
      error "recallDAppPermissionFinishedRpc failed: ", err=e.msg
-      return false


There was no way to fetch all the dapps from persistence.

alexjba · 2024-11-07T09:33:09Z

ui/app/AppLayouts/Wallet/panels/DAppsWorkflow.qml

@@ -69,8 +69,8 @@ DappsComboBox {
    signal disconnectRequested(string connectionId)
    signal pairingRequested(string uri)
    signal pairingValidationRequested(string uri)
-    signal connectionAccepted(var pairingId, var chainIds, string selectedAccount)
-    signal connectionDeclined(var pairingId)
+    signal connectionAccepted(string pairingId, var chainIds, string selectedAccount)


Tried to keep things inline and the pairing ids and topics to be strings in all signals

alexjba · 2024-11-07T09:34:00Z

ui/app/AppLayouts/Wallet/panels/WalletHeader.qml

@@ -138,37 +138,37 @@ Item {
                id: dappsWorkflow


In this file I've only renamed the WalletConnectService to DAppsService

alexjba · 2024-11-07T09:48:27Z

ui/app/AppLayouts/Wallet/services/dapps/types/SessionRequest.qml

+            }
+        }
+    */
+    function parse(event, hexToDec = (hex) => { return parseInt(hex, 16) }) {


Now all the session request parsing happens in this singletone component. There's also a storybook page where we can paste any WC/BC session request to investigate the resulted object.

alexjba · 2024-11-07T09:49:29Z

ui/app/AppLayouts/Wallet/services/dapps/types/SessionRequest.qml

Code moved from the request handler. I've split the stateful implementation and the parsing. This component provides just the parsing.

alexjba · 2024-11-07T09:50:38Z

ui/app/AppLayouts/Wallet/services/dapps/types/SessionRequestResolved.qml

@@ -48,30 +46,11 @@ QObject {
    /// maps to Constants.TransactionEstimatedTime values
    property int estimatedTimeCategory: 0

-    function resolveDappInfoFromSession(session) {


This is not needed. The dapp metadata is known in advanced. This is now provided by the DAppsModel and we don't need to always do asynchronous calls to the sdk to get this kind of info.

alexjba · 2024-11-07T09:51:35Z

ui/app/AppLayouts/Wallet/services/dapps/types/SessionRequestResolver.qml

This component will parse the session request and validated based on app data (accountsModel, networksModel)

alexjba · 2024-11-07T09:52:53Z

ui/imports/shared/stores/BrowserConnectStore.qml

QML entry point for BrowserConnect calls.

New component introduced (DAppsModel) to provide a common model for WC and BC. The WCDappsProvider and BCDappsProvider components are responsible to fill the model from different sources

This PR is refactoring the dapps service to avoid code duplication between SDKs and also to avoid overlapping requests/responses. It brings Browser Connect inline with Wallet Connect in terms of session management and sign transactions. New architecture: WalletConnectService becomes DAppsService. Its responsibility is to provide dapp access to the app. This is the component currently used by the UI What does it do: 1. Provide dapp APIs line connect, disconnect, session requests etc 2. Spawn app notifications on dapp events 3. Timeout requests if the dapp does not respons DAppsRequestHandler becomes DAppsModule. This component is consumed by the DAppService. Its responsibility is to aggregate all the building blocks for the dapps, but does not control any of the dapp features or consume the SDKs requests. What does it do: 1. Aggregate all the building blocks for dapps (currently known as plugins) DAppConnectionsPlugin - This component provides the session management features line connect, disconnect and provide a model with the connected dapps. SignRequestPlugin - This component provides the sign request management. It receives the sign request from the dapp, translates it to what Status understands and manages the lifecycle of the request.

alexjba · 2024-11-08T14:49:02Z

ui/app/AppLayouts/Wallet/services/dapps/plugins/SignRequestPlugin.qml

Moved the sign logic from the dappsRequestHandler to be reused for both SDKs

alexjba · 2024-11-08T14:50:48Z

ui/app/AppLayouts/Wallet/services/dapps/plugins/SignRequestPlugin.qml

+                return { obj: null, code: SessionRequest.RuntimeError }
+            }
+
+            updateFeesOnPreparedData(request)


@saledjenic minor changes to the new fees while resolving the conflicts. It's split in 2 parts. updateFeesOnPreparedData - Updating the preparedData for the user whenever the fees are null

updateFeesParamsToPassedObj - updates the fees for the modal

alexjba · 2024-11-08T14:52:05Z

ui/app/AppLayouts/Wallet/services/dapps/plugins/SignRequestPlugin.qml

+            }
+
+            request.setExpired()
+            d.unsubscribeForFeeUpdates(request.topic, request.requestId)


another change is here, where we unsubscribe for fee updates if the request has expired.
cc @saledjenic

alexjba · 2024-11-08T15:12:09Z

ui/imports/shared/panels/AnimatedText.qml

@@ -7,7 +7,7 @@ SequentialAnimation {

    property var target: null
    property color fromColor: Theme.palette.directColor1
-    property color toColor: Theme.palette.baseColor5


@saledjenic updated this when resolving the merge conflict. I don't see any difference for the Theme.palette.directColor1 and now it also works for other colors like danger1

alexjba commented Nov 7, 2024

View reviewed changes

alexjba force-pushed the feat/enable_Wallet_connect branch 2 times, most recently from ae0a35f to b4e22be Compare November 7, 2024 10:38

Base automatically changed from feat/enable_Wallet_connect to 14996-add-siwe-to-sign-in-via-wallet-connect November 7, 2024 12:13

alexjba force-pushed the feat/wallet-connect-service branch from e74fe00 to b6aae86 Compare November 7, 2024 12:26

alexjba force-pushed the 14996-add-siwe-to-sign-in-via-wallet-connect branch from 8efad11 to 4636f80 Compare November 7, 2024 19:06

alexjba added 2 commits November 7, 2024 21:10

feat(BC): Unify dapp sessions between WalletConnect and BrowserConnect

1ecd960

New component introduced (DAppsModel) to provide a common model for WC and BC. The WCDappsProvider and BCDappsProvider components are responsible to fill the model from different sources

alexjba force-pushed the feat/wallet-connect-service branch from b6aae86 to 698cbee Compare November 8, 2024 14:47

alexjba commented Nov 8, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feat(DApps): Refactoring wallet connect service and bring BrowserConnect inline with WC #16730

Feat(DApps): Refactoring wallet connect service and bring BrowserConnect inline with WC #16730

alexjba commented Nov 7, 2024 •

edited

Loading

status-im-auto commented Nov 7, 2024 •

edited

Loading

alexjba Nov 7, 2024

alexjba Nov 7, 2024

alexjba Nov 7, 2024

alexjba Nov 7, 2024

alexjba Nov 7, 2024

alexjba Nov 7, 2024

alexjba Nov 7, 2024

alexjba Nov 7, 2024

alexjba Nov 7, 2024

alexjba Nov 7, 2024

alexjba Nov 8, 2024

alexjba Nov 8, 2024

alexjba Nov 8, 2024

alexjba Nov 8, 2024

Feat(DApps): Refactoring wallet connect service and bring BrowserConnect inline with WC #16730

Are you sure you want to change the base?

Feat(DApps): Refactoring wallet connect service and bring BrowserConnect inline with WC #16730

Conversation

alexjba commented Nov 7, 2024 • edited Loading

What does the PR do

Affected areas

Architecture compliance

Screenshot of functionality (including design for comparison)

status-im-auto commented Nov 7, 2024 • edited Loading

Jenkins Builds

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexjba commented Nov 7, 2024 •

edited

Loading

status-im-auto commented Nov 7, 2024 •

edited

Loading