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

fix: switching hardware wallet networks #1085

Merged
merged 1 commit into from
Nov 10, 2022
Merged

Conversation

iamacook
Copy link
Member

@iamacook iamacook commented Nov 7, 2022

What it solves

Resolves #1073

How this PR fixes it

If a hardware wallet is connected, it is diconnected and reconnected when switching network as it is not possible to change the network of a hardware wallet.

How to test it

Open the Safe on a network where a Ledger/Trezor is enabled and connect it. Switch to another network that also supports Ledger/Trezor. Observe the address scanner modal open.


if (wallet && isHardwareWallet(wallet)) {
onboard?.disconnectWallet({ label: wallet.label }).then(() => {
connectWallet(onboard, { autoSelect: wallet.label })
Copy link
Member Author

Choose a reason for hiding this comment

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

Note: the address scanner modal opens on Ethereum by default and you must manually select the current network. I will investigate setting this (it would need to be implemented in onboard, unless we reinitialise onboard with 1 chain configuration before connecting a hardware wallet).

image

@cloudflare-workers-and-pages
Copy link

Deploying with  Cloudflare Pages  Cloudflare Pages

Latest commit: 5ad9f7d
Status: ✅  Deploy successful!
Preview URL: https://4a4afd68.web-core.pages.dev
Branch Preview URL: https://hw-wallet-network-switch.web-core.pages.dev

View logs

@iamacook iamacook requested a review from katspaugh November 7, 2022 15:10
@github-actions
Copy link

github-actions bot commented Nov 7, 2022

ESLint Summary View Full Report

Annotations are provided inline on the Files Changed tab. You can also see all annotations that were generated on the annotations page.

Type Occurrences Fixable
Errors 0 0
Warnings 0 0
Ignored 0 N/A
  • Result: ✅ success
  • Annotations: 0 total

Report generated by eslint-plus-action

Copy link
Member

@katspaugh katspaugh left a comment

Choose a reason for hiding this comment

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

👍

@katspaugh
Copy link
Member

@francovenica please test this with Keystone as well.

@francovenica
Copy link
Contributor

@francovenica please test this with Keystone as well.

I don't have a keystone (those are illegal in Argentina :D ). @liliya-soroka I think you have one

@francovenica
Copy link
Contributor

I tried both with trezor one and ledger NanoS and in both cases the connection persisted while changing networks.
It is expected to disconnect and makes you reconnect manually choosing the new network.

trezor connect

@francovenica
Copy link
Contributor

Miss understood the moment were it disconnects. You actually have to click "Change to NETWORK" for the disconnect to happen, which is correct. So disregard my last comment

@francovenica
Copy link
Contributor

I've checked that the onboard doesn't allow the user to connect the device to a network where is not compatible. Example: connection to ETH with trezor, then switching to aurora and clicking the button "Switch to Aurora" shows an error message in the onboard, which is the correct behavior
image

@francovenica
Copy link
Contributor

LGTM
Tested with a Trezor One and a ledger NanoS. Started in the Eth network and change it to goerli, setting that network in the onboard js.
I was able to create a safe and add a new owner for both devices.

@iamacook iamacook merged commit e5f9809 into dev Nov 10, 2022
@iamacook iamacook deleted the hw-wallet-network-switch branch November 10, 2022 07:08
@usame-algan usame-algan mentioned this pull request Nov 14, 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.

Unable to execute transactions on Goerli via Trezor & Ledger
3 participants