Skip to content
This repository was archived by the owner on Jun 23, 2023. It is now read-only.

Add Trezor Suite connection details - #413

Open
sime wants to merge 14 commits into
getumbrel:masterfrom
sime:add-trezor-connection
Open

Add Trezor Suite connection details#413
sime wants to merge 14 commits into
getumbrel:masterfrom
sime:add-trezor-connection

Conversation

@sime

@sime sime commented Apr 5, 2022

Copy link
Copy Markdown

The recent release of Trezor Suite include Electrum server support..

@louneskmt louneskmt left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Hey @sime @marekrjpolak, thanks for the PR!

As Trezor Suite seems to use an external Tor proxy (default 127.0.0.1:9050), we need to make sure the user has a Tor instance running on their computer. All the necessary code should be in my review comments.

Comment thread src/components/ConnectWallet/Wallets/TrezorSuite.vue
Comment thread src/components/ConnectWallet/Wallets/TrezorSuite.vue
Comment thread src/components/ConnectWallet/Wallets/TrezorSuite.vue Outdated
Comment thread src/components/ConnectWallet/Wallets/TrezorSuite.vue
Comment thread src/components/ConnectWallet/Wallets/TrezorSuite.vue

@louneskmt louneskmt left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

And these are just some minor style changes to keep consistency with the other Connect Wallet instructions.

Comment thread src/components/ConnectWallet/Wallets/TrezorSuite.vue Outdated
Comment thread src/components/ConnectWallet/Wallets/TrezorSuite.vue Outdated
Comment thread src/components/ConnectWallet/Wallets/TrezorSuite.vue Outdated
Comment thread src/components/ConnectWallet/Wallets/TrezorSuite.vue Outdated
Comment thread src/components/ConnectWallet/Wallets/TrezorSuite.vue Outdated
@louneskmt

Copy link
Copy Markdown
Contributor

Btw, it looks like the use of a custom Electrum server is not available for testnet (I wanted to review this PR on my testnet instance). It really isn't a big deal, just wondering why, as it is basically the same thing than for mainnet.

@louneskmt louneskmt mentioned this pull request Apr 7, 2022
@tsusanka

tsusanka commented Apr 7, 2022

Copy link
Copy Markdown

Btw, it looks like the use of a custom Electrum server is not available for testnet (I wanted to review this PR on my testnet instance). It really isn't a big deal, just wondering why, as it is basically the same thing than for mainnet.

It was more of an oversight. Definitely smth we want: trezor/trezor-suite#5005.

sime and others added 10 commits April 7, 2022 12:44
Co-authored-by: Lounès Ksouri <dev@louneskmt.com>
Co-authored-by: Lounès Ksouri <dev@louneskmt.com>
Co-authored-by: Lounès Ksouri <dev@louneskmt.com>
Co-authored-by: Lounès Ksouri <dev@louneskmt.com>
Co-authored-by: Lounès Ksouri <dev@louneskmt.com>
Co-authored-by: Lounès Ksouri <dev@louneskmt.com>
Co-authored-by: Lounès Ksouri <dev@louneskmt.com>
Co-authored-by: Lounès Ksouri <dev@louneskmt.com>
Co-authored-by: Lounès Ksouri <dev@louneskmt.com>
Co-authored-by: Lounès Ksouri <dev@louneskmt.com>
@sime

sime commented Apr 7, 2022

Copy link
Copy Markdown
Author

@louneskmt Thanks for the thorough review.

Though, I was a little trigger-happy with the TOR suggestions. Enabling TOR on Trezor Suite is simply a toggle, no need to instal anything else. Are you sure we need the TOR modal?

@louneskmt

Copy link
Copy Markdown
Contributor

You mean there is a Tor instance built in Trezor Suite?

@sime

sime commented Apr 7, 2022

Copy link
Copy Markdown
Author

@louneskmt Yes exactly.

@louneskmt

Copy link
Copy Markdown
Contributor

Oh okay, so Trezor Suite starts its own Tor instance and uses it, unless the user specify a custom proxy address?

image

If so, I misunderstood the configuration. I thought it was trying to connect by default to an existing instance on 127.0.0.1:9050 ("You can configure the address to point to a different Tor instance. Default: 127.0.0.1:9050.").

@louneskmt

Copy link
Copy Markdown
Contributor

That's great then, good job guys! My mistake, you can revert the commits related to the Tor modal and Tor first step.

@louneskmt

Copy link
Copy Markdown
Contributor

Looks like it doesn't connect, while it is working in Sparrow. I've tried with both running my usual Tor instance and stopping it to let Trezor Suite run it.

image image

@sime

sime commented Apr 7, 2022

Copy link
Copy Markdown
Author

@louneskmt I pushed changes which removed the TOR setup steps.

Regarding you connecting from suite: Could you try again, please? I just tested Trezor Suite with Umbrel 0.4.17. It didn't work initially, but it eventually connected.

@sime

sime commented Apr 7, 2022

Copy link
Copy Markdown
Author

You might be experiencing this bug: trezor/trezor-suite#5140

@louneskmt

Copy link
Copy Markdown
Contributor

Maybe it comes from the fact that I don't have a Trezor configured with Suite?

@louneskmt

Copy link
Copy Markdown
Contributor

You might be experiencing this bug: trezor/trezor-suite#5140

There should definitely be a "Test connection" button (like on Sparrow), or something like that.

@sime

sime commented Apr 7, 2022

Copy link
Copy Markdown
Author

@louneskmt Yes, it's currently not possible to 'test' the connection without a Trezor connected.

@louneskmt

Copy link
Copy Markdown
Contributor

Aaah that's why. I guess I just gonna approve this then, the PR looks good 👌

@louneskmt louneskmt left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

That should be good to merge @mayankchhabra @lukechilds 👌

@tsusanka

tsusanka commented May 4, 2022

Copy link
Copy Markdown

Ping. Could we get this released? :)

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants