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

add walletconnect support #20

Open
wants to merge 6 commits into
base: main
Choose a base branch
from
Open

add walletconnect support #20

wants to merge 6 commits into from

Conversation

zxqx
Copy link
Member

@zxqx zxqx commented Dec 17, 2021

This PR adds wallet provider support for WalletConnect.

As part of this integration, a few issues were addressed to ensure everything works properly:

Most of these are known issues with the web3 library, but only surface in certain build systems (e.g. vite doesn't define process.env by default) and during specific flows.

Wallet Modal: Step 1

Screen Shot 2021-12-17 at 10 26 37 AM

Wallet Modal: Step 2

Screen Shot 2021-12-17 at 10 26 11 AM

TODO

  • Show modal prompt to sign using connected wallet when executing transaction w/ WalletConnect
  • Execute test transactions against testnet

@zxqx zxqx requested a review from EzraWeller December 17, 2021 16:36
@netlify
Copy link

netlify bot commented Dec 17, 2021

✔️ Deploy Preview for hypervibes-app ready!

🔨 Explore the source changes: 5906534

🔍 Inspect the deploy log: https://app.netlify.com/sites/hypervibes-app/deploys/61c0dc84978da0000810d022

😎 Browse the preview: https://deploy-preview-20--hypervibes-app.netlify.app/

<UseWalletProvider
connectors={{
walletconnect: {
rpc: RPC_URLS,
Copy link
Member Author

Choose a reason for hiding this comment

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

This is the correct configuration, but differs from what's defined in the use-wallet documentation -- will open a PR against the repo soon to resolve that.

vite.config.ts Outdated
@@ -9,4 +9,7 @@ export default defineConfig({
sourcemap: true,
manifest: true,
},
define: {
'process.env': process.env,
Copy link
Member Author

Choose a reason for hiding this comment

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

@therealparmesh Lemme know if there's a more preferable way to do this in vite-land.

Copy link
Contributor

@therealparmesh therealparmesh Dec 17, 2021

Choose a reason for hiding this comment

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

@zakangelle

I believe Vite already provides these variables within import.meta: https://vitejs.dev/guide/env-and-mode.html#env-variables

FWIW, this seems to be closer to browser standards than process.env: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Statements/import.meta

Also, here are some usages of import.meta in this repository.

Copy link
Member Author

Choose a reason for hiding this comment

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

The tricky part is we're using a library (as a transitive dependency) which references process.env assuming its variables are string replaced at build-time (which isn't the case w/ Vite), so we need to inject an empty process.env object to unbreak a runtime error thrown in that library. Not sure if there's a better way to do that though.

This section of the library README alludes to a similar issue which happens with Angular 11 builds.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah I see what you mean! Then yeah, this is the best way to go.

Copy link
Contributor

Choose a reason for hiding this comment

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

Doh! I should've read the PR description first.

As a side note: Those last 2 points are definitely worth fixing in that library.

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.

2 participants