Fixes for wallets - Githubissues

RootSoft / walletconnect-dart-sdk

Open protocol for connecting dApps to mobile wallets with QR code scanning or deep linking.

MIT License

102 stars 61 forks source link

Fixes for wallets #30

Closed 3ph closed 2 years ago

3ph commented 2 years ago

@RootSoft - there are some issue I've discovered while trying to implement wallet side of WalletConnect:

PeerId and PeerMeta correctly returned in approveSession (otherwise dApp will ignore the response)
PeerId is added to subscriptions when the session approved
PeerId is added to subscriptions when connector is created with existing session (session restoration)

3ph commented 2 years ago

@RootSoft did you have a chance to look at the PR?

RootSoft commented 2 years ago

@3ph Sorry, didn't have a lot of time lately. Can you elaborate a bit more on the fixes for the wallet side? I try to align the SDK with the official monorepo, just don't want to break anything on the client side, e.g. handleIncomingMessages

3ph commented 2 years ago

Basically when client requests a new connection, peerId is passed which has to be returned in the approval and then used as a topic otherwise the client will ignore the messages from the wallet. A new subscription also has to be added as the topic is now changed. All the changes should only affect wallet side of things plus peerId restoration/subscription was added.

Another thing which was missing was inability to send a response for example for any eth requests.

RootSoft commented 2 years ago

Thanks @juampiq6 and @3ph for the wallet improvements. I just got back from my travels and will be going over the PRs this week. I'll keep you posted!

RootSoft commented 2 years ago

@juampiq6 I've merged your PR @3ph Can you take a look at the latest version (v0.0.11)? There was a problem with fromUri session not including the client meta data. https://github.com/RootSoft/walletconnect-dart-sdk/commit/fac8b45678d0aa17d42acda371c7eaf4dd91f166

Your PR looks good, but I try to align the official SDK with this community SDK.

Thanks for the PRs!

juampiq6 commented 2 years ago

Perfect, i assume the PR i did + your latests fixes are a replacemente for this PR. Nice work fellas @3ph @RootSoft!

3ph commented 2 years ago

@juampiq6 @RootSoft Thanks for that guys. I just had a quick look and from what I can see there would be still an issue with the new topic subscription (peerId) when session request is made? I didn't see that in any of the changes.

RootSoft commented 2 years ago

@3ph As soon as a uri is passed in the constructor, the fromUri method gets called and a client id is created (or passed). The wallet then subscribes to this client id & handshake topic (from the uri) https://github.com/RootSoft/walletconnect-dart-sdk/blob/f37e83b02a6a9d7e6b5e0ebf6c2332cb388587b9/lib/src/walletconnect.dart#L124 https://github.com/RootSoft/walletconnect-dart-sdk/blob/f37e83b02a6a9d7e6b5e0ebf6c2332cb388587b9/lib/src/walletconnect.dart#L135

Once the session is approved by the wallet, it returns its client id & metadata to the dapp, see monorepo.

The wallet example works as well with the change and fixes the null issue. Are you able to test with an implementation of your own?

3ph commented 2 years ago

It seems to be working now. If I encounter more issues I'll open a new PR. Thank!