-
Notifications
You must be signed in to change notification settings - Fork 883
remote-wallet: Trezor support #8378
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
Conversation
|
feels #306 (review) |
I did incorporate a lot of that feedback in this branch, but let me know what's still missing. All of the |
|
|
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #8378 +/- ##
=========================================
- Coverage 82.6% 82.6% -0.1%
=========================================
Files 890 891 +1
Lines 320903 321273 +370
=========================================
+ Hits 265277 265493 +216
- Misses 55626 55780 +154 🚀 New features to boost your workflow:
|
|
Bug: Omitting Ideal fix: Behavior matches Ledger: the transaction succeeds even when To reproduce:
On Trezor, this triggers full on-device signing flow, then throws Adding |
I tried this command, and it works on my side. I tried other commands like |
Interesting. Same device, clean build, updated firmware... but I still get "Dynamic program error: no device found" errors under this very specific circumstance. Explicitly including a fee-payer in the CLI command instantly fixes the issue, and the only difference between the 2 commands is the explicit inclusion of fee-payer. Given that it's a very minor issue, I suggest you ship this, and hopefully I can uncover the issue subsequently? |
|
Feel free to approve if you dare! |
rustopian
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🐿️
| } | ||
| } | ||
|
|
||
| for device in trezor_client::find_devices(false) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like we don't handle multiple Trezors connected at the same time, for one thing since pubkey is set to None and so multiple Trezors will seem identical to the locator. An edge case for sure, but I just had multiple hardware wallets connected two days ago, so it does happen:D
Not enough to block shipping, but we could use e.g. get_device_id to do this.
p.s.: turns out that this is also why the CLI was demanding 'usb://trezor?key=0/0' rather than just usb://trezor
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah good call! I came up with a way to fix this with 8103d15, let me know what you think
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great! I don't have two functional Trezors, but will try with two someday. Tested to ensure it doesn't break functionality with one.
Got it! Upon running Once I cc @joncinque, not sure there's anything to be done here; maybe some clearer message if |
rustopian
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great. I figured out the issue I was having, see #8378 (comment)
Up to you, whether actionable or not.
Ah gotcha, let's address that in follow-up work to make errors clearer. It doesn't impact the Trezor implementation. |
* Trezor support for Solana CLI * Cleanup and prep for PR * Sort workspace deps * Update trezor-client dependency * Support multiple wallets, add changelog --------- Co-authored-by: stevenbooke <[email protected]>
Problem
As noted at #306 and solana-labs#4911, the Trezor wallet is not supported in the Solana CLI.
Summary of Changes
Following the work at #306, get Trezor support into shape to be used. I've been keeping this branch up to date for most of this year and using a personal build with a Trezor wallet, and I haven't run into any issues yet.
@t-nelson you reviewed the previous PR a few times, so if you have time, your input will be appreciated. Otherwise, @rustopian can you take a look?