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

Able to connect to a non-supported chain on metamask on a reconnect to app.shapeshift.com. #1843

Closed
tolltalcrypto opened this issue May 19, 2022 · 7 comments · Fixed by #2085
Assignees
Milestone

Comments

@tolltalcrypto
Copy link
Member

Overview

After successfully connecting to app.shapeshift.com with metamask, if you close the site and change the network that metamask is connected to an unsupported network when reconnecting to the app.shapeshift.com, there is no warning/alert that you are using an unsupported network.

image

References and additional details

reproduction steps.

  1. go to app.shapeshift.com
  2. connect metamask via supported(Ethereum) network
  3. close app.shapeshift.com
  4. change network on metamask to another non-eth network.
  5. go back to app.shapeshift.com
  6. observe connection to metamask on an unsupported network.

Acceptance Criteria

Proper handling of this would likely check the network again after returning to app.shapeshift.com to ensure a supported network is currently connected. Or at least check the network metamask is connected to before allowing any sort of transaction.

Need By Date

No response

Screenshots/Mockups

No response

Estimated effort

No response

@tolltalcrypto tolltalcrypto added needs bounty Issues that should be bountied. needs engineering Requires engineering input before bounty labels May 19, 2022
@gomesalexandre
Copy link
Contributor

Nice catch!

Or at least check the network metamask is connected to before allowing any sort of transaction.

This one is a bit tricky as we start supporting more EVM chains, our definition of a "wrong network" might change

IMO we could iterate on the logic added in #1732 to either:

  1. disconnect MetaMask on a wrong network on reconnect
  2. show some Tooltip if a network other than Mainnet is detected
  3. Open a "Switch Network" popup when reconecting, like so:

image

IMO, 3. would probably be the best option. It's a UX that users are expected to see across DApps, and is also going to make the most sense as we start supporting other EVM chains (we would interact with x contract on y chain).

@Lychbot
Copy link

Lychbot commented May 24, 2022

@reallybeard could you please check on this and provide input thanks!

@Lychbot Lychbot added the needs product Requires product input before bounty label May 28, 2022
@Lychbot
Copy link

Lychbot commented May 28, 2022

@reallybeard / @DiggyDiggy2 - hoping to get your input on how products want to handle this ticket. Gomes provided some options to fix this above. Thanks!

@DiggyDiggy2
Copy link
Collaborator

Option #3 is definitely the best way to handle.

@DiggyDiggy2 DiggyDiggy2 removed the needs product Requires product input before bounty label May 31, 2022
@Lychbot
Copy link

Lychbot commented Jun 1, 2022

@0xean / @gomesalexandre would this be a good candidate for a bounty?

@gomesalexandre
Copy link
Contributor

gomesalexandre commented Jun 2, 2022

@0xean / @gomesalexandre would this be a good candidate for a bounty?

The web part can be, but there is groundwork that should be done by core engineering first IMO, at the very least in hdwallet.

The high-level idea is we would need 3 PRs for that:

  • An hdwallet PR adding a method (named ethSwitchChain or similar to switch chains, calling wallet_switchEthereumChain Ethereum JsonRPC method. It will have to be added to
    • ETHWallet interface
    • hdwallet-xdefi
    • hdwallet-metamask
    • hdwallet-tallyho
    • Note that hdwallet-native and hdwallet-portis automatically/only connect to mainnet currently and don't make ethereum.request() calls so there shouldn't be any work needed there
  • A chain-adapters PR adding a new method consuming said hdwallet method in EthereumChainAdapter and adding it as an optional method in ChainAdapter type
  • Finally, checking for the current network and calling the chain-adapters method if not on mainnet.

We might need some similar groundwork to actually "check for the current network": In theory, we have a method for that called ethSupportsNetwork, but it's really just used internally.
We also don't really pass chainId down in a programmatic way (the "real" chainId from MetaMask), but rather hardcode it to 1 in EthereumChainAdapter which is the root cause of the problem.

We don't have any method in hdwallet to check for the current chainId and just use provider.ChainId in web to check this, however, this property is both non-standard and deprecated.

We will probably need to have a similar hdwallet implementation to get the actual MetaMask chainId using eth_chainId Ethereum JsonRPC method.

@pastaghost Thoughts?

@MBMaria
Copy link

MBMaria commented Jun 6, 2022

GM @pastaghost @gomesalexandre @0xean any movement on this ticket? Should we be putting it up bounty?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

7 participants