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

Feat/support multiple wallets #424

Merged
merged 24 commits into from
Jun 7, 2022
Merged

Conversation

mattyg
Copy link
Collaborator

@mattyg mattyg commented May 28, 2022

Resolves #352

Overview

  • remove dependency web3-react
  • add dependency @rainbow-me/rainbowkit
  • refactor LoginPage and ActivatePage to use a shared SignMessageLayout
  • refactor SignMessageLayout to use rainbowkit connection component
  • add all wallets currently available in rainbowkit

To reduce the scope, I did not include WalletConnect in the rainbowkit ux, as it will require additional setup per-instance (registering each app instance with WalletConnect). Though this could be added in the future if desired.

@mattyg mattyg requested a review from kristoferlund May 28, 2022 21:29
Copy link
Member

@kristoferlund kristoferlund left a comment

Choose a reason for hiding this comment

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

Rainbow looks nice, definitely supports more wallets than web3-react!

Here are some initial comments:

I've only tried the Argent Wallet integration so far. Connecting wallet worked fine, logging into website didn't work - no error specified. Argent didn't present me with the message to sign, only a generic "Praise is requesting your signature". I think we need to verify all supported wallets work before enabling them.

While in the connected state but before signing login message the user is sort of stuck. To switch back to Metamask I had to clear LocalStorage. We need to provide a "Disconnect" button. Let's make a popup similar to how Rainbow does it. Accessed by a "down chevron" next to the eth address. This popup we can reuse when user is fully logged in as well.

image

I am getting a lot of build errors when starting through VSCode:

image

And finally. Rainbow adds a lot to the frontend package size. Is there any way to shrink that?

@mattyg
Copy link
Collaborator Author

mattyg commented Jun 5, 2022

Rainbow looks nice, definitely supports more wallets than web3-react!

Here are some initial comments:

I've only tried the Argent Wallet integration so far. Connecting wallet worked fine, logging into website didn't work - no error specified. Argent didn't present me with the message to sign, only a generic "Praise is requesting your signature". I think we need to verify all supported wallets work before enabling them.

Verified all wallets working properly except for Argent & SteakWallet -- removed them.

While in the connected state but before signing login message the user is sort of stuck. To switch back to Metamask I had to clear LocalStorage. We need to provide a "Disconnect" button. Let's make a popup similar to how Rainbow does it. Accessed by a "down chevron" next to the eth address. This popup we can reuse when user is fully logged in as well.

image

Done

I am getting a lot of build errors when starting through VSCode:

image

It looks like these are caused to changes in Webpack 5 which requires dependencies to publish their source files. See
mswjs/msw#1030
facebook/create-react-app#11767

As a workaround, I ignored these warnings via webpack config.

And finally. Rainbow adds a lot to the frontend package size. Is there any way to shrink that?

Hopefully #426 will reduce the overall app size. Otherwise will have to dig into other options.

@mattyg mattyg requested a review from kristoferlund June 5, 2022 21:17
kristoferlund
kristoferlund previously approved these changes Jun 7, 2022
Copy link
Member

@kristoferlund kristoferlund left a comment

Choose a reason for hiding this comment

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

Popup looks great!

Since Argent also has WalletConnect, you can attempt to login using that method. And that still don't work.

Also tried using Ledger Live and got this:
image

But, I'm merging this now and will try more wallets later. Let's make separate issue for any more problems.

@kristoferlund kristoferlund merged commit 3a4b4f4 into main Jun 7, 2022
@kristoferlund
Copy link
Member

Popup looks great!

Since Argent also has WalletConnect, you can attempt to login using that method. And that still don't work.

Also tried using Ledger Live and got this: image

But, I'm merging this now and will try more wallets later. Let's make separate issue for any more problems.

Updating to a newer version of Ledger Live solved that issue. 👍👍

@mattyg
Copy link
Collaborator Author

mattyg commented Jun 7, 2022

Popup looks great!

Since Argent also has WalletConnect, you can attempt to login using that method. And that still don't work.

Yeah I think Argent has some different requirements to setup message signing since it's a "smart contract wallet" -- I didn't dig into the details much more: https://docs.argent.xyz/wallet-connect-and-argent#message-signature

@kristoferlund kristoferlund deleted the feat/support_multiple_wallets branch June 15, 2022 10:50
@kristoferlund kristoferlund mentioned this pull request Jun 20, 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.

Support for other wallets than MetaMask
2 participants