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.

Sign up for GitHub

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

Jump to bottom

fix(29917): Trezor accounts not able to add multiple HD path accounts into the account list #187

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

fix(29917): Trezor accounts not able to add multiple HD path accounts into the account list #187

wants to merge 3 commits into from

Conversation

Akaryatrh
Copy link
Contributor

Currently it's not possible to add accounts from different HDPaths with Trezor related devices. For some reason, on a path change we do the exact same operations than when we "forget" a device. This is not something that we do for ledger devices so it appears useless (and then creates bugs).

Fixes MetaMask/metamask-extension#29917

@Akaryatrh Akaryatrh requested a review from a team as a code owner February 3, 2025 11:50
@Akaryatrh Akaryatrh added the team-hardware-wallets This should be handled by the Hardware Wallets Team label Feb 3, 2025
@Akaryatrh
Copy link
Contributor Author

@metamaskbot publish-preview

@github-actions GitHub Actions
Copy link

github-actions bot commented Feb 3, 2025

Preview builds have been published. See these instructions (from the core monorepo) for more information about preview builds.

Expand for full list of packages and versions. 
{
  "@metamask-previews/keyring-api": "16.1.0-776292e",
  "@metamask-previews/eth-hd-keyring": "9.0.1-776292e",
  "@metamask-previews/eth-ledger-bridge-keyring": "8.0.3-776292e",
  "@metamask-previews/eth-simple-keyring": "8.0.1-776292e",
  "@metamask-previews/eth-trezor-keyring": "6.0.1-776292e",
  "@metamask-previews/keyring-internal-api": "4.0.1-776292e",
  "@metamask-previews/keyring-internal-snap-client": "3.0.3-776292e",
  "@metamask-previews/eth-snap-keyring": "9.1.1-776292e",
  "@metamask-previews/keyring-snap-client": "3.0.3-776292e",
  "@metamask-previews/keyring-snap-sdk": "2.1.2-776292e",
  "@metamask-previews/keyring-utils": "2.0.0-776292e"
}

mikesposito
mikesposito reviewed Feb 4, 2025
View reviewed changes
packages/keyring-eth-trezor/src/trezor-keyring.ts
Comment on lines -542 to -546
this.accounts = [];
this.page = 0;
this.perPage = 5;
this.unlockedAccount = 0;
this.paths = {};
Copy link
Member

Choose a reason for hiding this comment

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

Will the keyring still be able to sign transactions with the accounts previously derived with a different path? Since this.hdk is specific to a path, after changing path we won't have the original public key used to derive the first accounts

Copy link
Contributor Author

@Akaryatrh Akaryatrh Feb 5, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Indeed, previously derived accounts are not able to sign or send tx. I guess i need to check what we're doing with Ledger keyring as it's working there.
Thanks for catching this 🙂

Copy link
Member

Choose a reason for hiding this comment

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

I think it's warranted to add a test case for this scenario as well

Copy link
Member

Choose a reason for hiding this comment

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

the LedgerKeyring stores a map of AccountDetails for each derived address, so even if the path is switched it can be retrieved again when signing:

accounts/packages/keyring-eth-ledger-bridge/src/ledger-keyring.ts

Line 470 in dd7b8df

const { hdPath } = accountDetails;

mikesposito
mikesposito reviewed Feb 4, 2025
View reviewed changes
packages/keyring-eth-trezor/src/trezor-keyring.test.ts
Comment on lines -729 to -734
expect(keyring.accounts).toStrictEqual([]);
expect(keyring.page).toBe(0);
expect(keyring.perPage).toBe(5);
expect(keyring.hdk.publicKey).toBeNull();
expect(keyring.unlockedAccount).toBe(0);
expect(keyring.paths).toStrictEqual({});
Copy link
Member

Choose a reason for hiding this comment

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

should we test that these ones are unchanged instead?

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

@mikesposito mikesposito mikesposito left review comments

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
team-hardware-wallets This should be handled by the Hardware Wallets Team
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[Bug] Trezor accounts not able to add multiple HD path accounts into the account list
2 participants
@Akaryatrh @mikesposito