Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixes for wallets #30

Closed
wants to merge 2 commits into from
Closed

Fixes for wallets #30

wants to merge 2 commits into from

Conversation

3ph
Copy link
Contributor

@3ph 3ph commented Mar 18, 2022

@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
Copy link
Contributor Author

3ph commented May 17, 2022

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

@RootSoft
Copy link
Owner

@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
Copy link
Contributor Author

3ph commented May 24, 2022

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.

Copy link
Contributor

@juampiq6 juampiq6 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi, juampi here, also contributing to the wallet part of this lib...
To be compliant with the client API reference (https://docs.walletconnect.com/quick-start/wallets/react-native#manage-call-requests) and to copy the react native implementation (https://docs.walletconnect.com/quick-start/wallets/react-native#manage-call-requests), i would separate the 'sendCustomResponse' method into two methods 'approveRequest' & 'rejectRequest'. This also allows for verbosity. @3ph if you want i can make this change happen and make you a PR?
@RootSoft these methods are vital for a wallet implementation since there is no other way we can communicate with the dapp

@RootSoft
Copy link
Owner

RootSoft commented Jun 7, 2022

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
Copy link
Owner

@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. fac8b45

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

Thanks for the PRs!

@juampiq6
Copy link
Contributor

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

@3ph
Copy link
Contributor Author

3ph commented Jun 16, 2022

@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
Copy link
Owner

@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)

clientId: clientId ?? const Uuid().v4(),
subscriptions: [session.clientId],

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
Copy link
Contributor Author

3ph commented Jul 7, 2022

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

@3ph 3ph closed this Jul 7, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants