Allow multiple Ledger & Trezor hardware accounts#10505
Allow multiple Ledger & Trezor hardware accounts#10505darkwing merged 14 commits intoMetaMask:developfrom
Conversation
|
CLA Signature Action: All authors have signed the CLA. You may need to manually re-run the blocking PR check if it doesn't pass in a few minutes. |
6584bab to
7315ba1
Compare
7e989be to
07c8a3b
Compare
07c8a3b to
5742d7e
Compare
There was a problem hiding this comment.
I tested this out yesterday and today, and tried adding/removing accounts, switching between wallets (Ledger Nano S => Ledger X => Trezor model T). Overall this is pretty slick!
The one additional thing I noticed was that routing was pretty... strange when using this. I'm not sure where this routing logic is - I didn't see it the diff so maybe it's out of scope of this PR? But here are the issues I ran into:
- When navigating to the "connect hardware" page with accounts already connected, I'd see a flash of the normal page rendered, then a loading screen, then the account list. If the intent is to auto-navigate to the account list, it should go straight to the loading screen without that initial render if possible.
When clicking "Forget device", all of my hardware wallet accounts remained in the account list. When I click on these accounts, I'd be redirected to the connect hardware page. This prevents me from viewing the account balance and transaction history, and prevents removing the account. I would expect either that the accounts would be removed when clicking "Forget device", or that I'd at least be able to look at them. This weird in-between state is unexpected, to me at least.
Edit: After revisiting that second scenario, I think I misunderstood what was going on. Here are updated notes on the 'Forget device' UX:
- After clicking 'Forget device', I was routed to the connect hardware landing page (where you choose between Ledger and Trezor). From this page, attempting to navigate to any account in the account menu fails. It just refreshes the page.
- When trying to remove an account from a hardware wallet that has been forgotten, the removal succeeds but the remove account modal stays visible, despite the account having been removed already. It stays up until dismissing/cancelling the removal.
- After removing a hardware wallet account from a forgotten device, the next attempt to connect accounts on this device will also automatically disconnect any accounts that remain from prior to the device being forgotten.
There was a problem hiding this comment.
I think we want to ensure that all accounts are on the current page here. This tests that at least one account is on the current page.
There was a problem hiding this comment.
Removed all this :)
There was a problem hiding this comment.
Now it preserves the selected accounts on pages that aren't visible. This could be confusing 🤔
But it's required to connect a set of accounts that spans multiple pages...
I guess this is fine for now, but the design is sub-optimal. We should have some indicator that there are accounts selected that aren't shown. Or maybe a list of all selected accounts? Not sure.
529583f to
3e502fd
Compare
|
HI I have tried to run your latest change here, but getting a white dialog (nothing loads). The debug screen show the following
|
Gudahtt
left a comment
There was a problem hiding this comment.
LGTM aside from the extraneous prop!
The one remaining nice-to-have for this PR would be having some representation of the HD path in the auto-generated account names, but that doesn't need to block. The routing / "Forget device" issues are probably best handled in a separate PR, since they're pre-existing.
010f062 to
4737e30
Compare
77bfdbb to
edca0b6
Compare
066419d to
353c2c6
Compare
There was a problem hiding this comment.
Curious that hdPath wasn't being set before 🤔
I investigated this and found that the HD path was being set correctly when we retrieve a page of accounts, and it persists in the keyring after that point. So luckily it's almost always set correctly.
I did discover that on develop if you add an account on one path, start the connect flow again, switch paths, then cancel, it leaves the pre-existing account but in a broken state. So that's interesting. That is fixed by this PR though.
Gudahtt
left a comment
There was a problem hiding this comment.
LGTM!
The localization isn't great here because we're still baking in the assumption that it makes sense to attach an equivalent to "(legacy)" as a suffix in every locale. We'd have to generate the entire account name with a localized message to ensure this is flexible enough to handle any locale (e.g. have the message be $1 $2 (legacy), where $1 and $2 are "Ledger" and "1" respectively for the first Ledger account). But this wasn't localized well to begin with, so doesn't seem worth blocking on at this point.
|
Updated to throw instead of reject. |
8a6beb0 to
350cdb9
Compare
Fix:
Explanation:
This PR will allow users to add multiple hardware wallet accounts and keep them for any given amount of time. While that in itself is awesome, this should also prevent the need for users to re-add tokens each time they use a different hardware wallet account.
Manual testing steps:
To do:
setTimeoutThis PR will require an update from both of our keyring repos: