draft: Remove the business logic from UI components #16387
+981
−589
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Jenkins BuildsClick to see older builds (62)
|
alexjba
force-pushed
the
feat/wc-handle-sign-timeout
branch
3 times, most recently
from
2ec49fc
to
40085b5
Compare
alexjba
commented
Sep 26, 2024
Makefile
Outdated
| @@ -628,6 +628,7 @@ $(STATUS_CLIENT_APPIMAGE): nim_status_client $(APPIMAGE_TOOL) nim-status.desktop | |||
| -no-copy-copyright-files \ | |||
| -qmldir=ui -qmlimport=$(QT5_QMLDIR) \ | |||
| -bundle-non-qt-libs \ | |||
| -exclude-libs="libnss3.so,libnssutil3.so,libgmodule-2.0.so.0,libgthread-2.0.so.0" \ | |||
There was a problem hiding this comment.
@yakimant This seems to fix the app crash on initialisation. But I still have issues with missing resources for QtWebEngineView.
saledjenic
reviewed
Sep 27, 2024
| const session = { url, name, iconUrl } | ||
| root.wcService.connectorDAppsProvider.addSession(JSON.stringify(session)) | ||
|
|
||
| root.wcService.connectorDAppsProvider.addSession(url, name, iconUrl) |
There was a problem hiding this comment.
@alexjba account param is missing here.
There was a problem hiding this comment.
It could be that's the reason, or maybe something else, but after I locally added the address I still was not able to connect to https://rarible.com vai browser connect (checked, the entry was in the db, but no connection was made on the dapp's side neither in the Stauts app).
|
In general, you've definitely made nice improvements here. I made a data flow map of how I see BC and WC in the Status app working together and would like to discuss it. Please have a look. Basically, a dapp is a dapp, and we don't defer daps in the UI how they were connected (via WC or BC).
@alexjba wdyt about that idea? |
micieslak
reviewed
Oct 1, 2024
There was a problem hiding this comment.
Looks good in general, that's nice that DAppsWorkflow is UI-only now.
What is missing for from my perspective (I had no earlier experience with that topic) is description of the DAppsWorkflow API - what's the expected structure of the models, what are the parameters of signals/methods, what's the expected flow of signals/method calls.
It would be nice to have also subcomponents in their own storybook pages, instantiated with some simple, synthetic data. Maybe even DAppsWorkflow could have SB page which is fully mocked. I think it would clearly show what's the intended flow/order of actions.
alexjba
force-pushed
the
feat/wc-handle-sign-timeout
branch
2 times, most recently
from
e96cde4
to
0a26f8d
Compare
✔️ status-desktop/prs/macos/aarch64/package/PR-16387#9 🔹 ~5 min 26 sec 🔹 0a26f8d 🔹 📦 macos/aarch64 package |
This commit brings a separation of concerns for the UI components involved in dApp interactions. Issue: The UI components depend on the WalletConnectService and also on its dependencies like DAppsRequestHAndler. As a result the UI components have a hard dependency on the WalletConnect specifics and are incompatible with BC. This results in duplication of logic. Issue: The UI components operate on WalletConnect specific JSON object. E.g. session objects, session proposal etc. As a result the UI is built around the WalletConnect message format. Issue: The UI components operate on ListModel items received through functions and stored internally. Any change in the model would result in a crash. Solution: Remove the WalletConnectService dependency from DAppsWorkflow. The DAppsWorkflow now operates with models, signals and functions. This is the first step in the broader refactoring. Moving the logic into the service itself will allow us to further refactor the WC and BC. How does it work now: Dependencies - The UI components have a dependency on models. SessionRequestsModel and DAppsModel. Pairing - The pairing is initiated in the UI. On user input a pairingValidationRequested signal is emitted and the result is received as a function pairingValidated. If the url is valid the UI requests a pairingRequested. When the WalletConnectService is refactored we can go further and request only pairingRequested and to receive a pairingResult call as a function with the result. In the current implementation on pairingRequested we'll receive a connectDApp request. Connecting dApps - The flow is initiated with connectDApp function. This call currently contains all the needed info as args. In the next step it could be replaced with a ConnectionRequests model. The connectDApp call triggered a connection popup if we're not currently showing one to the user. If we're currently showing one it will be queued (corner case). The connection can be accepted with connectionAccepted and rejected with connectionDeclined. Once the connection is accepted we're expecting a result connectionSuccessful or connectionFailed. The connectionSuccessful also expects a new id for the established connection. Signing - The signing flow orbits around the SessionRequestsModel. Each item from the model will generate a popup showing the sign details to the user. Sign can be accepted or rejected using signRequestAccepted or signRequestRejected. No response is currently expected. The model is expected to remove the sign request item.
alexjba
force-pushed
the
feat/wc-handle-sign-timeout
branch
from
c85a031
to
05ec6a4
Compare
alexjba
force-pushed
the
feat/wc-handle-sign-timeout
branch
from
05ec6a4
to
fde39e6
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What does the PR do
Separation of concerns
WalletConnectServiceand also on its dependencies likeDAppsRequestHAndler. As a result the UI components have a hard dependency on the WalletConnect specifics and are incompatible with BC. This results in duplication of logic.DAppsWorkflow. TheDAppsWorkflownow operates with models, signals and functions. This is the first step in the broader refactoring. Moving the logic into the service itself will allow us to further refactor the WC and BC.How does it work now:
SessionRequestsModelandDAppsModel.pairingValidationRequestedsignal is emitted and the result is received as a functionpairingValidated. If the url is valid the UI requests apairingRequested. When the WalletConnectService is refactored we can go further and request onlypairingRequestedand to receive apairingResultcall as a function with the result. In the current implementation onpairingRequestedwe'll receive aconnectDApprequest.connectDAppfunction. This call currently contains all the needed info as args. In the next step it could be replaced with aConnectionRequestsmodel. TheconnectDAppcall triggered a connection popup if we're not currently showing one to the user. If we're currently showing one it will be queued (corner case). The connection can be accepted withconnectionAcceptedand rejected withconnectionDeclined. Once the connection is accepted we're expecting a resultconnectionSuccessfulorconnectionFailed. TheconnectionSuccessfulalso expects a new id for the established connection.SessionRequestsModel. Each item from the model will generate a popup showing the sign details to the user. Sign can be accepted or rejected usingsignRequestAcceptedorsignRequestRejected. No response is currently expected. The model is expected to remove the sign request item.Next steps:
WC service refactoring to bring in a separation of concerns and to make it interoperable with WC and BC. This should result in flow simplification and remove the duplication of logic.
Affected areas
Architecture compliance
My PR is consistent with this document: Architecture guidelines
Screenshot of functionality (including design for comparison)