Skip to content

Commit 92a41fa

Browse files
committed
fix: address cursor feedbacks
1 parent 65cd1a0 commit 92a41fa

3 files changed

Lines changed: 114 additions & 10 deletions

File tree

packages/keyring-eth-trezor/jest.config.js

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -23,10 +23,10 @@ module.exports = merge(baseConfig, {
2323
// An object that configures minimum threshold enforcement for coverage results
2424
coverageThreshold: {
2525
global: {
26-
branches: 59.58,
27-
functions: 92.95,
28-
lines: 93.15,
29-
statements: 93.25,
26+
branches: 60.92,
27+
functions: 93.15,
28+
lines: 93.5,
29+
statements: 93.59,
3030
},
3131
},
3232
});

packages/keyring-eth-trezor/src/trezor-keyring-v2.test.ts

Lines changed: 68 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -626,5 +626,73 @@ describe('TrezorKeyringV2', () => {
626626

627627
expect(account.options.entropy.derivationPath).toBe(`m/44'/60'/0'/0`);
628628
});
629+
630+
it('clears registry when switching HD paths', async () => {
631+
// Create wrapper with account on BIP44 path
632+
const inner = createInnerKeyring();
633+
inner.setAccountToUnlock(0);
634+
await inner.addAccounts(1);
635+
636+
const wrapper = new TrezorKeyringV2({
637+
legacyKeyring: inner,
638+
entropySource,
639+
});
640+
641+
// Get initial accounts - this populates the registry
642+
const initialAccounts = await wrapper.getAccounts();
643+
expect(initialAccounts).toHaveLength(1);
644+
const initialAccountId = getFirstAccount(initialAccounts).id;
645+
646+
// Switch to legacy MEW path and add account
647+
// This simulates what createAccounts does when path changes
648+
inner.setHdPath(`m/44'/60'/0'`);
649+
inner.hdk = fakeHdKey; // Reset HDKey after setHdPath clears it
650+
inner.setAccountToUnlock(0);
651+
await inner.addAccounts(1);
652+
653+
// Call createAccounts with the new path - this should clear registry
654+
await wrapper.createAccounts(derivePathOptions(`m/44'/60'/0'/1`));
655+
656+
// The old account should no longer be accessible
657+
await expect(wrapper.getAccount(initialAccountId)).rejects.toThrow(
658+
/Account not found/u,
659+
);
660+
});
661+
});
662+
663+
describe('locked device handling', () => {
664+
it('returns cached accounts when device is locked', async () => {
665+
const { wrapper, inner } = await createWrapperWithAccounts(2);
666+
667+
// First call with device unlocked populates the cache
668+
const unlockedAccounts = await wrapper.getAccounts();
669+
expect(unlockedAccounts).toHaveLength(2);
670+
671+
// Simulate locked device by clearing the HDKey
672+
inner.hdk = {} as typeof fakeHdKey;
673+
674+
// Should return cached accounts
675+
const lockedAccounts = await wrapper.getAccounts();
676+
expect(lockedAccounts).toHaveLength(2);
677+
expect(lockedAccounts).toStrictEqual(unlockedAccounts);
678+
});
679+
680+
it('throws error when device is locked and accounts not cached', async () => {
681+
const inner = createInnerKeyring();
682+
inner.setAccountToUnlock(0);
683+
await inner.addAccounts(1);
684+
685+
const wrapper = new TrezorKeyringV2({
686+
legacyKeyring: inner,
687+
entropySource,
688+
});
689+
690+
// Simulate locked device before accounts are cached
691+
inner.hdk = {} as typeof fakeHdKey;
692+
693+
await expect(wrapper.getAccounts()).rejects.toThrow(
694+
/Trezor device is locked/u,
695+
);
696+
});
629697
});
630698
});

packages/keyring-eth-trezor/src/trezor-keyring-v2.ts

Lines changed: 42 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -88,13 +88,20 @@ export class TrezorKeyringV2
8888
{
8989
readonly entropySource: EntropySourceId;
9090

91+
/**
92+
* Tracks the current HD path to detect path changes.
93+
* When the path changes, the registry must be cleared.
94+
*/
95+
#currentHdPath: AllowedHdPath;
96+
9197
constructor(options: TrezorKeyringV2Options) {
9298
super({
9399
type: options.type ?? KeyringType.Trezor,
94100
inner: options.legacyKeyring as TrezorKeyringAsEthKeyring,
95101
capabilities: trezorKeyringV2Capabilities,
96102
});
97103
this.entropySource = options.entropySource;
104+
this.#currentHdPath = this.inner.hdPath as AllowedHdPath;
98105
}
99106

100107
/**
@@ -123,6 +130,9 @@ export class TrezorKeyringV2
123130
// initialized HDKey. The registry will be populated lazily when
124131
// getAccounts() is called after the device is unlocked.
125132
await this.inner.deserialize(state);
133+
134+
// Sync the tracked HD path with the deserialized state
135+
this.#currentHdPath = this.inner.hdPath as AllowedHdPath;
126136
});
127137
}
128138

@@ -193,6 +203,30 @@ export class TrezorKeyringV2
193203
return [];
194204
}
195205

206+
// If the device is locked, we cannot derive addresses to find indices.
207+
// Return cached accounts if available, otherwise throw a clear error.
208+
if (!this.inner.isUnlocked()) {
209+
const cachedAccounts = addresses
210+
.map((address) => {
211+
const existingId = this.registry.getAccountId(address);
212+
return existingId ? this.registry.get(existingId) : undefined;
213+
})
214+
.filter(
215+
(account): account is Bip44Account<KeyringAccount> =>
216+
account !== undefined,
217+
);
218+
219+
// If we have all accounts cached, return them
220+
if (cachedAccounts.length === addresses.length) {
221+
return cachedAccounts;
222+
}
223+
224+
// Some accounts are not cached and device is locked
225+
throw new Error(
226+
'Trezor device is locked. Please unlock the device to access accounts.',
227+
);
228+
}
229+
196230
return addresses.map((address) => {
197231
// Check if we already have this account in the registry
198232
const existingId = this.registry.getAccountId(address);
@@ -267,12 +301,14 @@ export class TrezorKeyringV2
267301
}
268302

269303
// Derive the account at the specified index.
270-
// Note: setHdPath resets the inner keyring's accounts array when the path changes.
271-
// This mirrors the production behavior where switching HD paths (via the dropdown)
272-
// resets the entire account list. The TrezorKeyring operates on a single path at
273-
// a time - accounts from different paths cannot coexist. The V2 registry tracks
274-
// accounts independently, but getAccounts() relies on the inner keyring's account
275-
// list, so accounts from a previous path would become inaccessible after switching.
304+
// If the HD path is changing, clear the registry to avoid stale accounts.
305+
// The TrezorKeyring operates on a single path at a time - accounts from
306+
// different paths cannot coexist in the inner keyring.
307+
if (basePath !== this.#currentHdPath) {
308+
this.registry.clear();
309+
this.#currentHdPath = basePath;
310+
}
311+
276312
this.inner.setHdPath(basePath);
277313
this.inner.setAccountToUnlock(targetIndex);
278314
const [newAddress] = await this.inner.addAccounts(1);

0 commit comments

Comments
 (0)