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

frontend: integrate Wallet Connect v2 #1472

Closed
wants to merge 4 commits into from

Conversation

GabrielBuragev
Copy link
Contributor

@GabrielBuragev GabrielBuragev commented Feb 9, 2023

closes #1227

This PR is rebased on #1464

There were several obstacles when integrating the version 2 of their SDK.

  1. Testing was quite cumbersome. Some wallet apps have still not migrated to v2 and therefore finding out which one works takes a lot of time.
    I found one that works good. It is BitKeep.
    If you want to test this, use BitKeep and make sure you run the app with yarn dev --mode production so BitKeep can recognize the networks (doesn't support testnets).

  2. All of the test suites for the provider implementations were failing due to a misconfigured import resolution by @walletconnect/ethereum-provider library.
    Some workarounds were needed in order to get the tests running.
    Check the commit messages to get more info on this.
    There is also an issue for this in their monorepo which hopefully will be addressed soon: Import error when using @walletconnect/ethereum-provider - CommonJs/ES modules related WalletConnect/walletconnect-monorepo#1943

  3. There is another bug in their library.
    The await provider.enable() call doesn't resolve when the connection failed due to a modal close. This causes our loading indicator inside the wallet selection menu to spin indefinitely because the state of the action is not updated (nothing is returned from the async call).
    A workaround is implemented for this and you will find a comment explaining it inside walletconnect-provider-2.ts.
    Also, created an issue for this in their repo: Resolve promise when qr modal is closed WalletConnect/walletconnect-monorepo#1957

I left both WalletConnect 1 & WalletConnect 2 as available options since i noticed inconsistent behavior in different wallet apps while manually testing it. I would suggest we offer both options until we see consistent behavior, full migration of all wallet apps & maturity of WalletConnect 2.

NOTE:
To be merged & deployed on 1st of March!
Read more here: https://medium.com/walletconnect/how-to-prepare-for-the-walletconnect-v1-0-shutdown-1a954da1dbff

@GabrielBuragev GabrielBuragev force-pushed the frontend/wallet-connect-v2 branch from 9a17a83 to 374ac8b Compare February 10, 2023 15:10
@manuelwedler manuelwedler self-requested a review February 15, 2023 16:31
@GabrielBuragev GabrielBuragev force-pushed the frontend/wallet-connect-v2 branch from 374ac8b to f183ae3 Compare February 15, 2023 17:38
Copy link
Contributor

@manuelwedler manuelwedler left a comment

Choose a reason for hiding this comment

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

Looks mostly good, just some smaller comments

frontend/tests/unit/composables/useWallet.spec.ts Outdated Show resolved Hide resolved
frontend/tests/setup/window.ts Show resolved Hide resolved
Copy link
Contributor

@manuelwedler manuelwedler left a comment

Choose a reason for hiding this comment

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

Looks perfect now 👍

@GabrielBuragev GabrielBuragev force-pushed the frontend/wallet-connect-v2 branch from 9a77c7c to f36a6b1 Compare February 20, 2023 15:32
@GabrielBuragev
Copy link
Contributor Author

Note: this will have to wait with merging until June 29.

For more info see https://medium.com/walletconnect/weve-reset-the-clock-on-the-walletconnect-v1-0-shutdown-now-scheduled-for-june-28-2023-ead2d953b595

@GabrielBuragev
Copy link
Contributor Author

One of the issues for which we implemented a workaround here has been fixed: WalletConnect/walletconnect-monorepo#1957

Try to update the walletconnect sdk version and possibly remove the workaround from our code.

@fredo
Copy link
Contributor

fredo commented Feb 23, 2023

@GabrielBuragev Does this have any implications that it will be even mergable in 4 months from now or will it have conflicts?

We should probably pull it out of the sprint then @andfletcher

@andfletcher
Copy link
Contributor

Yes, we can talk about ut next week in the planning Session

@GabrielBuragev
Copy link
Contributor Author

GabrielBuragev commented Feb 23, 2023

@fredo if our internal provider abstractions don't change then this will have no conflicts at the moment of merging.
I think its safe to say that making such changes to our abstractions is a very unlikely scenario.

We can move it out of the sprint though.

@compojoom compojoom force-pushed the frontend/wallet-connect-v2 branch from f36a6b1 to d7f4314 Compare April 25, 2023 14:57
@GabrielBuragev GabrielBuragev force-pushed the frontend/wallet-connect-v2 branch from d7f4314 to 280ac64 Compare April 27, 2023 07:25
@GabrielBuragev
Copy link
Contributor Author

When rebasing this PR again, make sure that the closeExternalConnection function is declared as an optional method in the IEthereumProvider interface. Also, remove the default implementation inside the EthereumProvider and adjust the composables or any other module to check if the closeExternalConnection is implemented on the provider before trying to call it (TS will anyways complain).

@GabrielBuragev
Copy link
Contributor Author

Closing in favor of #1972

@compojoom compojoom deleted the frontend/wallet-connect-v2 branch September 12, 2023 12:58
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.

Frontend: Migrate to WalletConnect v2
5 participants