Fix issue with legacy ledger account list

[jpuri]

Fixes: #6012

For ledger legacy the account list can get out of sync:

User can thus end up sending funds to wrong account.

I found that the reason was that we save the selected ledger type in app state:

Thus though the account list was for Ledger Live the dropdown was showing Ledger (Mew / MyCrypto). PR fixes the issue by resetting state when connect hardware component un-mounts.

[github-actions]

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.

[jpuri]

Hey @seaona : can you plz test this fix.

[metamaskbot]

Builds ready [4b70010]

• builds: chrome, firefox, opera
• builds (beta): chrome, firefox, opera
• builds (flask): chrome, firefox, opera
• build viz: Build System
• code coverage: Report
• storybook: Storybook
• all artifacts
• bundle viz:

  • background: 0, 1, 2, 3, 4, 5, 6, 7
  • common: 0, 1, 2, 3, 4, 5, 6, 7, 8
  • content-script: 0
  • contentscript: contentscript
  • disable-console: disable-console
  • inpage: inpage
  • phishing-detect: phishing-detect
  • policy-load: policy-load
  • sentry-install: sentry-install
  • ui: 0, 1, 2, 3, 4, 5, 6, 7, 8, 9

Page Load Metrics (1252 ± 36 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	77	182	93	23	11
		domContentLoaded	1162	1493	1237	79	38
		load	1162	1493	1252	75	36
		domInteractive	1162	1493	1237	79	38

[jpuri]

I am not sure why we are saving defaultHdPaths in app state. I could not find it being used anywhere except this component. A better approach may be to refactor use of defaultHdPaths and move it to this component state rather.

[seaona]

@jpuri the fix is working for me.
This PR also fixes this issue #14424

[metamaskbot]

Builds ready [4a17dc8]

• builds: chrome, firefox, opera
• builds (beta): chrome, firefox, opera
• builds (flask): chrome, firefox, opera
• build viz: Build System
• code coverage: Report
• storybook: Storybook
• all artifacts
• bundle viz:

  • background: 0, 1, 2, 3, 4, 5, 6, 7
  • common: 0, 1, 2, 3, 4, 5, 6, 7, 8
  • content-script: 0
  • contentscript: contentscript
  • disable-console: disable-console
  • inpage: inpage
  • phishing-detect: phishing-detect
  • policy-load: policy-load
  • sentry-install: sentry-install
  • ui: 0, 1, 2, 3, 4, 5, 6, 7, 8, 9

Page Load Metrics (1335 ± 53 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	88	1272	173	257	124
		domContentLoaded	1204	1725	1324	113	54
		load	1213	1725	1335	110	53
		domInteractive	1204	1725	1324	113	54

[seaona]

New Observations: at the moment,when you import some Hardware Wallet accounts, and later on import some more, the first ones disappear from the Metamask wallet. This happens with both Trezor and Ledger.

Steps:

1. Import a couple of Trezor/Ledger accounts and accept
2. Import again 2 other Trezor/Ledger accounts and accept.

Result: the first imported accounts have disappeared from Metamask wallet.

trezor-accounts-disappear.mp4

Maybe that was the reason for keeping the "state" @jpuri ?

[danjm]

Adding to @seaona's most recent comment, I think the default path in state aimed to ensure that if changing the path, the new selection would be the selection upon later returning to the flow

Using ledger I am able to generate at least 4 different account lists. I want to understand exactly what inputs are producing those

[jpuri]

@danjm: I see in that case when we return to connect hardware flow we should try to get accounts for saved default path.

Yep not sure what can cause the 4 different lists of accounts, I tried it again and I could not get 4 different set of account. I got only 3 for ledger corresponding to :

1. Ledger Live
2. Legacy (MEW / MyCrypto)
3. BIP44 Standard (e.g. MetaMask, Trezor)

[jpuri]

@seaona : the issue mentioned here does not look related to code changes in this PR - as in the PR I only reset the dropdown selection.

In fact I do expect metamask to forget account if hardware wallet is not more connected.

[metamaskbot]

Builds ready [8386dd4]

• builds: chrome, firefox, opera
• builds (beta): chrome, firefox, opera
• builds (flask): chrome, firefox, opera
• build viz: Build System
• code coverage: Report
• storybook: Storybook
• all artifacts
• bundle viz:

  • background: 0, 1, 2, 3, 4, 5, 6, 7, 8
  • common: 0, 1, 2, 3, 4, 5, 6, 7, 8
  • content-script: 0
  • contentscript: contentscript
  • disable-console: disable-console
  • inpage: inpage
  • phishing-detect: phishing-detect
  • policy-load: policy-load
  • sentry-install: sentry-install
  • ui: 0, 1, 2, 3, 4, 5, 6, 7, 8, 9

Page Load Metrics (1386 ± 48 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	78	163	102	21	10
		domContentLoaded	1206	1570	1349	88	42
		load	1218	1570	1386	100	48
		domInteractive	1206	1570	1349	88	42

[seaona]

@jpuri from what I've seen, on develop branch the behaviour is different: the accounts are not removed, when I add new ones. See below:

trezor-accounts-not-disappearing.mp4

All this process is without disconnecting the device. Just importing 2 accounts and then importing 2 more:

• on fix branch: seems device is "forgotten" in between, as I have to give permissions again for importing the 2 new accounts.
• on develop branch: I don't have to give permissions a second time (after importing the first 2 accounts)

[kumavis]

resolved here MetaMask/eth-ledger-bridge-keyring#146

[jpuri]

Issue has been fixed with PR here: MetaMask/eth-ledger-bridge-keyring#146

I will create a separate PR in extension repo to update eth-ledger-bridge-keyring.