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

Re-enable WalletConnect and add TypeScript definitions #37

Merged
merged 11 commits into from
Jun 29, 2020

Conversation

JamesLefrere
Copy link
Contributor

Changes

  • Re-enable the WalletConnect connector
  • Upgrade connectors from web3-react
  • Bug fix: Some providers require a jsonrpc and id property for all requests; ensure sendCompat sends a request with these properties
  • Add TypeScript definitions

Hope this is useful! I noticed the sendCompat issue with WalletLink, where the requests were failing; hopefully this is ok for the other providers?

Cheers
James

@CLAassistant
Copy link

CLAassistant commented Jun 25, 2020

CLA assistant check
All committers have signed the CLA.

Copy link
Contributor

@sohkai sohkai left a comment

Choose a reason for hiding this comment

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

Looking pretty good—could we also update the examples to include the WalletConnect connector?

I'll leave it to @andy-hook or @bpierre to check the type definition—🙇‍♂️ for adding it!

src/utils.js Show resolved Hide resolved
@JamesLefrere
Copy link
Contributor Author

@sohkai Do you have an RPC endpoint to use for the examples?

@sohkai
Copy link
Contributor

sohkai commented Jun 25, 2020

@JamesLefrere Not sure I understand; I was thinking of adding WalletConnect to this list (and a similar on in the other examples), and that shouldn't require an RPC endpoint?

Copy link
Contributor

@andy-hook andy-hook left a comment

Choose a reason for hiding this comment

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

Maybe drop the semis but otherwise LGTM! 😄

* Re-enable the WalletConnect connector
* Upgrade connectors from `web3-react`
* Bug fix: Some providers require a `jsonrpc` and `id` property for all requests; ensure `sendCompat` sends a request with these properties
@JamesLefrere
Copy link
Contributor Author

@sohkai Yes, I have added them (with empty strings for the RPC urls) to the examples - it's just that it won't be able to connect without a real endpoint, but I guess that's ok for these examples.

Also, semis removed 👍

@sohkai
Copy link
Contributor

sohkai commented Jun 29, 2020

@JamesLefrere Ah, I didn't realize that they needed RPC endpoints to work!

You can use the endpoints defined here: https://github.com/aragon/aragon/blob/master/src/network-config.js#L18!

@JamesLefrere
Copy link
Contributor Author

Thanks @sohkai , added straight to the configs :)

Copy link
Contributor

@bpierre bpierre left a comment

Choose a reason for hiding this comment

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

Thank you so much @JamesLefrere!

I added a few changes:

  • Examples: tweak the buttons layout + add titles.
  • Bump @web3-react/walletconnect-connect to 6.1.4.
  • Move to HTTP endpoints only. WS doesn’t seem to work with either WalletConnect or WalletLink to get the block number or the balance. It might be related to the way we are querying them? For now, I think the safest is to prevent users to set WS endpoints.

@JamesLefrere
Copy link
Contributor Author

Great, thanks for taking care of that @bpierre 🙌

All sounds good, look forward to the release 👍

@bpierre bpierre merged commit f204860 into aragon:master Jun 29, 2020
@JamesLefrere JamesLefrere deleted the re-enable-walletconnect branch June 30, 2020 08:57
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

5 participants