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 Metamask connection option #788

Merged
merged 10 commits into from
Dec 1, 2023
Merged

Add Metamask connection option #788

merged 10 commits into from
Dec 1, 2023

Conversation

jagodarybacka
Copy link
Contributor

@jagodarybacka jagodarybacka commented Nov 29, 2023

Resolves #771

What

Add Metamask to supported wallets 🦊 🥲

Metamask is visible only if you don't have Taho wallet installed.

image

Testing

  1. Testing Taho without Metamask installed
  • make sure you are able to connect to Taho
  • make sure you are able to do transactions (stake, unstake) using Taho
  • make sure you are able to disconnect and reconnect using Taho
  1. Testing Taho with Metamask installed
  • make sure you are able to connect to Taho
  • make sure you are able to do transactions (stake, unstake) using Taho
  • make sure you are able to disconnect and reconnect using Taho
  1. Testing Metamask without Taho installed
  • make sure you are able to connect to Metamask
  • make sure you are able to do transactions (stake, unstake) using Metamask
  • make sure you are able to disconnect and reconnect using Metamask
  • make sure selecting Taho as connection option fails gracefully

Copy link

netlify bot commented Nov 29, 2023

Deploy Preview for taho-development ready!

Name Link
🔨 Latest commit 5a8f2a6
🔍 Latest deploy log https://app.netlify.com/sites/taho-development/deploys/6569d20ce279460008331e7f
😎 Deploy Preview https://deploy-preview-788--taho-development.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@jagodarybacka jagodarybacka marked this pull request as draft November 29, 2023 12:30
@jagodarybacka jagodarybacka marked this pull request as ready for review November 29, 2023 15:23
@VladUXUI
Copy link
Contributor

Might be edge case, but if:
1 - Taho was connected (MM Disabled)
2 - Taho was Disabled - MM Enabled
It seems to break everything, as it redirects me to chrome store instantly

CleanShot.2023-11-29.at.18.15.57.mp4

@VladUXUI
Copy link
Contributor

Another small issue,
IF> MM is connected and i change address in MM, i need to hard-refresh Subscape in order to connect another address

@michalinacienciala
Copy link
Contributor

Might be edge case, but if: 1 - Taho was connected (MM Disabled) 2 - Taho was Disabled - MM Enabled It seems to break everything, as it redirects me to chrome store instantly

CleanShot.2023-11-29.at.18.15.57.mp4

Got the same issue. After having both MM and Taho installed (MM not connected to dapp yet, on Ethereum Mainnet, Arb Sepolia added to the chains list; Taho actively connected to dapp) I disabled the Taho plugin and reloaded the Subscape page (to test connecting to dapp with MM). I got black screen, and Sending metamask:reload message in dapp's logs. Switching to Arb Sepolia in MM did not make a difference. Then I noticed that Chrome blocks the pop-up window and once I allowed it to show the pop-up I was directed to Taho Google Store.

Screen.Recording.2023-11-30.at.07.02.54.mov

Here is what I did next, that helped with the issue:

  1. Enabled Taho extension
  2. Opened Taho and disconnected from Subscape from the wallet
  3. Reloaded Subscape. A Taho extension poped up asking for connecting to dapp. chose Connect and got in to the Subscape.
  4. I disconnected the wallet from dapp and got the Connect Taho to open portal view.
  5. I disabled the Taho extension
  6. Reloaded the dapp again, got the Connect Taho to open portal view.
  7. Pressed Connect wallet. Finally got to the screen with choice for MM/Taho (Taho correctly pointing to Taho Store and MM correctly popping up the MM connection method window)

@michalinacienciala
Copy link
Contributor

When I try to connect do dapp with MM on a Ethereum network I'm asked if I allow for changing the network (that's correct). But in the dapp's logs I can see a bunch of errors at that point (I'm not sure if they are expected). Once I allow, I'm successfully connected to dapp and the network changes to Arbitrum Sepolia in MM (correct).

image

@michalinacienciala
Copy link
Contributor

michalinacienciala commented Nov 30, 2023

Got an issue with staking:

  1. I disabled Taho in Brave
  2. Installed MM there and imported an account. Did not change networks.
  3. I entered https://deploy-preview-788--taho-development.netlify.app/ and did the steps required in order to connect with MM. At the step where MM asks if I allow adding new network (Arbitrum Sepolia) to MM I saw these errors:
    image
  4. I allowed adding new network and got connected, entered the Island. Network in MM switched to Arbitrum Sepolia.
  5. I tried to stake to Base realm (where I already had some stake), but got this error (right after clicking Approve&stake):
Failed to send transaction Error: underlying network changed (event="changed", network={"name":"homestead","chainId":1,"ensAddress":"0x00000000000C2E074eC69A0dFb2997BA6C7d2e1e"}, detectedNetwork={"chainId":421614,"name":"unknown"}, code=NETWORK_ERROR, version=providers/5.7.2)
    at u.makeError (index.js:224:23)
    at pe.<anonymous> (base-provider.js:998:38)
    at Generator.next (<anonymous>)
    at c (base-provider.js:5:58)
  1. When I tried again after reloading the page, I was able to stake.

The behavior is repeatable, I was able to grab a recording:
https://github.com/tahowallet/dapp/assets/78352137/d2874818-bed5-4c44-845c-643318d962f3

EDIT: I did more testing and the fact that this was a fresh MM install and that dapp was adding the Arb Sepolia to MM did not matter here. Issue occurs also when Arb Sepolia is already added to MM, but user is on a different network.

@michalinacienciala
Copy link
Contributor

Another small issue, IF> MM is connected and i change address in MM, i need to hard-refresh Subscape in order to connect another address

For me when I stuck in a loop when changing accounts in MM it was ok to do just a normal refresh:

Screen.Recording.2023-11-30.at.09.13.43.mov

@michalinacienciala
Copy link
Contributor

I did some testing on Brave/Chrome, in various configurations of installed extensions (but mostly with MM installed, Taho disabled). In MM I used account imported from seed phrase and also a Ledger account. The only problems I observed are the ones mentioned in the above comments.

Web3onboard remembers last wallet that was used. If it was MM but MM
becomes unavailable then web3onboard will try to auto connect to MM
and will fail. So to avoid problem with attempting auto connection
to the wallet that is not available on the wallet lists let's just disable
autoConnect feature for Metamask for now.
Copy link
Contributor

@xpaczka xpaczka left a comment

Choose a reason for hiding this comment

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

:shipit:

@xpaczka xpaczka merged commit 8186cf7 into main Dec 1, 2023
4 checks passed
@xpaczka xpaczka deleted the metamask-support branch December 1, 2023 13:01
jagodarybacka added a commit that referenced this pull request Dec 1, 2023
We've added possibility to connect using MM (when Taho is not visible)
(added in #788) and changed copy
on a Portal screen for users with no TAHO (in
#792).
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.

UI frozen (no portal graphic shown) before connecting the wallet
4 participants