15647 dapps watched accounts and keycard accounts shouldnt be considered for wallet connect interactions #15877
Conversation
Jenkins BuildsClick to see older builds (66)
|
| @@ -264,7 +272,37 @@ QObject { | |||
|
|
|||
| sdk: root.wcSDK | |||
| store: root.store | |||
| supportedAccountsModel: d.supportedAccountsModel | |||
| supportedAccountsModel: SortFilterProxyModel { | |||
There was a problem hiding this comment.
check in 'disconnect' if proper dApp from proper wallet address is disconnected.
There was a problem hiding this comment.
Disconnect works correctly
Seitseman
force-pushed
the
15647-dapps-watched-accounts-and-keycard-accounts-shouldnt-be-considered-for-wallet-connect-interactions
branch
3 times, most recently
from
a2c2693
to
ea619c7
Compare
|
Shouldn't we fix issues on master and then cherry-pick them for RC? |
|
|
||
| readonly property SortFilterProxyModel dappsModel: SortFilterProxyModel { | ||
| objectName: "DAppsModelFiltered" | ||
| sourceModel: d.dappsModel.count ? d.dappsModel : null |
There was a problem hiding this comment.
| sourceModel: d.dappsModel.count ? d.dappsModel : null | |
| sourceModel: d.dappsModel.count ? d.dappsModel : null |
that check looks unusual, why is it needed?
There was a problem hiding this comment.
It was done because SFPM once its source model is bind, and roles initialized, will not "refresh" roles for the usecases of dynamicRole: true of ListModel.
Right now when app starts, walletconnect service will send data to d.dappsModel that has slightly different structure then the following data entries.
There was a problem hiding this comment.
That's something not clear for me. dynamicRole: true only allows various types for the same role.
So it's valid then:
append({name:"X"})
append({name:4})
but
append({name:"X"})
append({name:4, surname: "Y"})
will result with a model with only name role. surname won't be added.
And SFPM doesn't care about role types at all. So I wonder why it's needed here :)
There was a problem hiding this comment.
It looks like right now dApps are stored in local cache in slightly different format than what is actually needed to properly filter them by connected address. And when locally stored dApps are overwritten by up-to-date data from wallet connect, dappsModel is cleared first. dynamicRoles: true seems to be covering that case.
Anyways, based on our discussions with you and then with Stefan, I will adjust that data and remove both dynamicRoles and that binding in "DAppsModelFiltered" SFML.
ui/app/AppLayouts/Wallet/services/dapps/WalletConnectService.qml
Outdated
Show resolved
Hide resolved
ui/app/AppLayouts/Wallet/services/dapps/WalletConnectService.qml
Outdated
Show resolved
Hide resolved
@saledjenic , per https://www.notion.so/Desktop-app-Release-process-356bbe7c40d14db3b64ab364800a48c9 points 8 and 9, fix goes into the release branch and then into master |
There was a problem hiding this comment.
After testing and looking into the code it seems the behavior for multiple session per dapps is not working as expected. Please see my comments inline. Basically, multiple sessions for one dapp will not work. Also disconnecting a dapp with an account selected will disconnect the sessions for other accounts with the same dapp.
ui/app/AppLayouts/Wallet/services/dapps/WalletConnectService.qml
Outdated
Show resolved
Hide resolved
Seitseman
force-pushed
the
15647-dapps-watched-accounts-and-keycard-accounts-shouldnt-be-considered-for-wallet-connect-interactions
branch
from
ea619c7
to
6c040f2
Compare
20240731_220317.mp4 |
Seitseman
force-pushed
the
15647-dapps-watched-accounts-and-keycard-accounts-shouldnt-be-considered-for-wallet-connect-interactions
branch
from
6c040f2
to
0add75c
Compare
Seitseman
force-pushed
the
15647-dapps-watched-accounts-and-keycard-accounts-shouldnt-be-considered-for-wallet-connect-interactions
branch
2 times, most recently
from
e8b49c4
to
e25f212
Compare
|
The CI test failure looks legit! |
There was a problem hiding this comment.
Once concern! What about keycard status accounts? Should we disable the dApps entirely for these accounts? Currently I can't sign any transaction with the keycard status account even if the wallet account is not on keycard. When signing, the keycard auth screen is prompted and I can't use it for signing!
Maybe it's a general issue, will continue looking on it!
| @@ -35,6 +35,8 @@ QObject { | |||
| readonly property alias dappsModel: dappsProvider.dappsModel | |||
| readonly property alias requestHandler: requestHandler | |||
|
|
|||
| readonly property bool isServiceAvailableForAddressSelection: dappsProvider.supportedAccountsModel.count | |||
There was a problem hiding this comment.
We're assuming here the dappsProvider.supportedAccountsModel is and will always be a ListModel. This can be changed to dappsProvider.supportedAccountsModel.ModelCount.count
It's not a general issue! Only dapps fail to sign non keycard wallet accounts with keycard status account! |
| sourceModel: d.supportedAccountsModel | ||
| filters: ValueFilter { | ||
| roleName: "address" | ||
| value: root.walletRootStore.selectedAddress === model.address |
There was a problem hiding this comment.
WRN 2024-08-01 11:19:17.779+03:00 qt warning topics="qt" tid=4930042 text="ReferenceError: model is not defined" file=qrc:/app/AppLayouts/Wallet/services/dapps/WalletConnectService.qml:284 category=default
There was a problem hiding this comment.
The value filter expect a value, like root.walletRootStore.selectedAddress. Although this is not 100% safe because we need the filter not to be case sensitive.
An alternative would be a FastExpressionFilter comparing the strings and ignoring the case
There was a problem hiding this comment.
Yeah, on a second look, I think it's safer to do a FastExpressionFilter unless we want to fiddle around with a RegExpFilter and setting the case sensitivity
There was a problem hiding this comment.
To be reverted back to FastExpressionFilter
There was a problem hiding this comment.
If it's only about case sensitivity, RegExpFilter is better indeed (faster) and not complicated to use.
There is RegExpFilter.FixedString syntax mode available and caseSensitivity property as well. So there is even no need to deal with any regexes.
|
I pushed a temporary change as documentation on how the mapping of dapps with sessions and accounts work. Then joined @Seitseman in a conversation around this proposal. We agreed with @Seitseman to get back to the original idea and deduplicate dapps given that now user has no way to know for the same dapp which account is connected to in all accounts selection. |
| const sessions = DAppsHelpers.filterActiveSessionsForKnownAccounts(allSessions, root.supportedAccountsModel) | ||
| for (const key in sessions) { | ||
| const dapp = sessions[key].peer.metadata | ||
| const dAppsMap = [] |
There was a problem hiding this comment.
shoudn't it be an object?
| const dAppsMap = [] | |
| const dAppsMap = {} |
| for (const topic in dAppsMap) { | ||
| dapps.append(dAppsMap[topic]); |
There was a problem hiding this comment.
It's good practice to append array in one call whenever possible.
E.g. sth like that (not tested):
dapps.append(dAppsMap.map(t => dAppsMap[t]))
There was a problem hiding this comment.
Btw, ideally at this stage we should more carefully inspect the incoming data before putting into the model. We can take only what's needed and sanitize (e.g. if some fields are optional and may be not present we should provide e.g. "" instead in order to have always the same set of roles). Then dynamicRoles and tricks with SFPM won't be needed at all.
There was a problem hiding this comment.
Great job Roman.
To have the same behavior for persistence you have to propagate the NIM's getActiveSessions to the controller and use it through the DAppsStore and run the same logic as the SDK's getActiveSessions
Seitseman
force-pushed
the
15647-dapps-watched-accounts-and-keycard-accounts-shouldnt-be-considered-for-wallet-connect-interactions
branch
2 times, most recently
from
50ecd64
to
dea9a9c
Compare
ui/app/AppLayouts/Wallet/services/dapps/WalletConnectService.qml
Outdated
Show resolved
Hide resolved
Seitseman
force-pushed
the
15647-dapps-watched-accounts-and-keycard-accounts-shouldnt-be-considered-for-wallet-connect-interactions
branch
from
dea9a9c
to
5edbcff
Compare
|
@iurimatias , should this PR be merged into 2.30 release? |
|
cherry-pick into master: #15985 |
jrainville
deleted the
15647-dapps-watched-accounts-and-keycard-accounts-shouldnt-be-considered-for-wallet-connect-interactions
branch
fix(dApps): Improved handling of connected dApps.
closes: #15589
closes: #15647
What does the PR do
Affected areas
dApps
StatusQ checklist
Screenshot of functionality (including design for comparison)
Screen.Recording.2024-07-31.at.11.35.16.mov
Impact on end user
End users can see DApps button only when either all accounts are selected, or supported wallet address is selected (non watched, not keycard accounts).
In addition to that, the dApps list will show only dApps connected to the selected wallet address
How to test
Please create several wallet accounts, at least 1 watched address and 1 keycard address
Risk
Described potential risks and worst case scenarios.
Cool Spaceship Picture
🚀