fix(WalletConnect): Fixing sign for uniswap and paraswap #16168
Conversation
alexjba
requested review from
alaibe,
dlipicar,
Khushboo-dev-cpp,
saledjenic and
clauxx
| @@ -68,7 +68,7 @@ function buildSupportedNamespaces(chainIds, addresses, methods) { | |||
| "eip155":{ | |||
| "chains": [${eipChainIds.join(',')}], | |||
| "methods": [${methodsStr}], | |||
| "events": ["accountsChanged", "chainChanged"], | |||
| "events": ["chainChanged","accountsChanged","message","disconnect","connect"], | |||
There was a problem hiding this comment.
@saledjenic @clauxx Do you know if we'll need additional implementations in the integration layer for these extra events? Can't find anything meaningful in docs, integration or testing.
There was a problem hiding this comment.
do you mean the "chainChanged" and "accountsChanged" events? we don't use them as of know from what I know, and doesn't seem like we need them, since we agree ahead of time with the dapp on the account/chains. e.g. whenever the dApp changes the chain, we don't need to react to that.
There was a problem hiding this comment.
I'm referring to "message","disconnect","connect". The Paraswap pairing fails because they mark these evens as required and we don't declare them as supported.
There was a problem hiding this comment.
whenever the dApp changes the chain, we don't need to react to that
Really? Didn't know that was our intention.
Do you know if we'll need additional implementations in the integration layer for these extra events?
When the dapp asks for supported namespaces it actually asks if our wallet supports all events/methods that the dapp needs for functionating. In order to connect to the dapp we should at least support "required" events/methods, otherwise that dapp won't be successfully connected to our wallet.
Now if we extend namespace we want to support, we have to add necessary listeners for those events (or connect them to appropriate handlers if already added) and also extend functionality with newly added methods to support all that the namespaces said.
At least that's how I see it.
There was a problem hiding this comment.
@saledjenic Looks like we're being called through methods and we're supposed to call WC APIs through events. https://docs.walletconnect.com/walletkit/web/usage#emitting-session-events
IDK why Paraswap requires us to send all these events, but I think it's up to us to decide when we should send these events. The difficulty I see is to find the appropriate when to send these events.
There was a problem hiding this comment.
Yes, that's how it should be, we have to listen for events sent from the dapp and react to them and notify the dapp about the change on the wallet side.
During the negotiation process, when we're building a namespace, we should always respond with all chains/events/actions we're supporting in our wallet.
There was a problem hiding this comment.
Makes sense! I'll add the events to enable the connection and add another task to go deeper in each signal. Looks like we're missing more handlers, not just these here and it's probably worth it's own space.
Jenkins Builds
|
There are two fixes needed here. 1. Status-go fix for unknown primitive when signing the message 2. Fix for Paraswap pairing failure due to required namespaces mismatch Needs: status-im/status-go#5755
alexjba
force-pushed
the
fix/solidity-types
branch
from
8009cee to
cc655f2
Compare
|
This PR is ready to review 😄 |
What does the PR do
There are two fixes needed here.
Needs for: status-im/status-go#5755
Affected areas
Wallet Connect
Message signing
Architecture compliance
My PR is consistent with this document: Architecture guidelines
Screenshot of functionality (including design for comparison)
Impact on end user
Before:
The user could not use Paraswap services
The user could not sign messages using uint160 type
After:
The user can use Paraswap services
The user can sign messages using all types
How to test
Case 1:
Connect to Paraswap (make sure the events in requiredNamespaces.eip155.events is not empty in the pairing message)
Case 2:
Try to swap more than USDC to DAI using Uniswap. Not sure of the repro steps. Probably can be reproduced when connecting new accounts because it would reproduce while signing the authorisation request. The message needs to contain a value of
uint160. This can be seen in the message we're showing in the sign dialog.Risk
Described potential risks and worst case scenarios.
Tick one: