Fix/WC - handling lost connections in wallet connect #16580
Merged
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
There is no need to emit `changed` event for the QObject exposed from nim if only the underlying data changes.
…s SDK This commit exposes the `emitSessionEvent` function to qml
…able + Disable the `Connect` button after the first request
alexjba
requested review from
Cuteivist and
saledjenic
and removed request for
a team
Jenkins BuildsClick to see older builds (21)
|
caybro
approved these changes
Oct 23, 2024
ui/app/AppLayouts/Wallet/helpers/ChainsAvailibilityWatchdog.qml
Outdated
Show resolved
Hide resolved
ui/app/AppLayouts/Wallet/helpers/ChainsAvailibilityWatchdog.qml
Outdated
Show resolved
Hide resolved
ui/app/AppLayouts/Wallet/services/dapps/plugins/ChainsSupervisorPlugin.qml
Outdated
Show resolved
Hide resolved
alexjba
force-pushed
the
fix/WC-handling-lost-connections
branch
from
4b6b0cc to
fb0fe93
Compare
This commits implements the `connect` `disconnect` session events for WC and also disables primary buttons for WC whenever there is no connection to internet or chains. + update tests
…isorPlugin + move other tests from the wrong folder
alexjba
force-pushed
the
fix/WC-handling-lost-connections
branch
from
fb0fe93 to
8163e95
Compare
|
Added qml tests as well |
These QML tests are totally broken on linux 5.15.2. Or maybe it's just the CI configuration as I see some SSL warnings on the failing tests |
|
Works on Mac with 5.15.2. Must be something else |
The ui-tests failures seem legit: |
Yes, it's a linux 5.15.2 thing..Looking into it |
|
It can't find the openSSL dependency. It works on a local linux when the openssl libs are provided in the LD_LIBRARY_PATH |
caybro
reviewed
Oct 25, 2024
alexjba
force-pushed
the
fix/WC-handling-lost-connections
branch
from
2378e3b to
2f26b43
Compare
✔️ status-desktop/prs/linux/x86_64/tests-nim/PR-16580#5 🔹 ~6 min 57 sec 🔹 2f26b43 🔹 📦 tests/nim package |
saledjenic
approved these changes
Nov 1, 2024
alexjba
merged commit dbda82f
into
14996-add-siwe-to-sign-in-via-wallet-connect
9 checks passed
8 tasks
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
Closes: #16180 #15636
Not included:
2. Polished UI - needs design
WC/EIP documentation for the `connect` and `disconnect` events.
Note: This is also covered by EIP documentation https://eips.ethereum.org/EIPS/eip-1193
This event can signal the following events:
connect
The Provider emits connect when it:
first connects to a chain after being initialized.
first connects to a chain, after the disconnect event was emitted.
interface ProviderConnectInfo {
readonly chainId: string;
}
The event emits an object with a hexadecimal string chainId per the eth_chainId Ethereum RPC method, and other properties as determined by the Provider.
disconnect
The Provider emits disconnect when it becomes disconnected from all chains.
This event emits a ProviderRpcError. The error code follows the table of CloseEvent status codes.
In the new flow there are 2 main components involved
ui/app/AppLayouts/Wallet/helpers/ChainsAvailibilityWatchdog.qml. Component watching the chains availability and network availabilityui/app/AppLayouts/Wallet/services/dapps/plugins/ChainsSupervisorPlugin.qml. Component reacting on chains availability changes by sending the appropriate events to the sdk.Commits:
000d3c5 - A small fix in nim to optimise the qml signals whenever a chain online state changes. Here we've had a QObject exposed from nim with properties like
blockedNetworks. Thechangedevent on the exposedQObjectwas being sent when the underlying data might or might not have been changed. This was not needed as the underlying property emits proper signals.3028034 - Exposing
emitSessionEventto qmlb552f24 - Add an option to fully disable any button that triggers a WC interaction from the UI
16fc962 - Adding the
ChainsAvailibilityWatchdog.qmlandChainsSupervisorPlugin.qmlcomponents to handle the network or chain connection loss. These events will send theconnect,disconnectsession events to WC when needed.Affected areas
WalletConnect
Architecture compliance
My PR is consistent with this document: Status Desktop Architecture Guide
Screenshot of functionality (including design for comparison)
Screen.Recording.2024-10-23.at.16.19.09.mov
Screen.Recording.2024-10-23.at.16.21.06.mov