From ccd406e3330a571210ba0efbb346d03a908142ef Mon Sep 17 00:00:00 2001 From: Michele Esposito Date: Fri, 17 Jan 2025 12:21:44 +0100 Subject: [PATCH 1/5] chore: fix lint warnings in `KeyringController` --- .../src/KeyringController.test.ts | 79 ++++++------ .../src/KeyringController.ts | 112 +++++++----------- 2 files changed, 74 insertions(+), 117 deletions(-) diff --git a/packages/keyring-controller/src/KeyringController.test.ts b/packages/keyring-controller/src/KeyringController.test.ts index 18a0c2319d1..7dfa9fe8bf2 100644 --- a/packages/keyring-controller/src/KeyringController.test.ts +++ b/packages/keyring-controller/src/KeyringController.test.ts @@ -25,13 +25,6 @@ import { import * as sinon from 'sinon'; import * as uuid from 'uuid'; -import MockEncryptor, { - MOCK_ENCRYPTION_KEY, -} from '../tests/mocks/mockEncryptor'; -import { MockErc4337Keyring } from '../tests/mocks/mockErc4337Keyring'; -import { MockKeyring } from '../tests/mocks/mockKeyring'; -import MockShallowGetAccountsKeyring from '../tests/mocks/mockShallowGetAccountsKeyring'; -import { buildMockTransaction } from '../tests/mocks/mockTransaction'; import { KeyringControllerError } from './constants'; import type { KeyringControllerEvents, @@ -47,6 +40,13 @@ import { isCustodyKeyring, keyringBuilderFactory, } from './KeyringController'; +import MockEncryptor, { + MOCK_ENCRYPTION_KEY, +} from '../tests/mocks/mockEncryptor'; +import { MockErc4337Keyring } from '../tests/mocks/mockErc4337Keyring'; +import { MockKeyring } from '../tests/mocks/mockKeyring'; +import MockShallowGetAccountsKeyring from '../tests/mocks/mockShallowGetAccountsKeyring'; +import { buildMockTransaction } from '../tests/mocks/mockTransaction'; jest.mock('uuid', () => { return { @@ -181,12 +181,10 @@ describe('KeyringController', () => { it('should not add a new account if called twice with the same accountCount param', async () => { await withController(async ({ controller, initialState }) => { const accountCount = initialState.keyrings[0].accounts.length; - const firstAccountAdded = await controller.addNewAccount( - accountCount, - ); - const secondAccountAdded = await controller.addNewAccount( - accountCount, - ); + const firstAccountAdded = + await controller.addNewAccount(accountCount); + const secondAccountAdded = + await controller.addNewAccount(accountCount); expect(firstAccountAdded).toBe(secondAccountAdded); expect(controller.state.keyrings[0].accounts).toHaveLength( accountCount + 1, @@ -220,13 +218,11 @@ describe('KeyringController', () => { const accountCount = initialState.keyrings[0].accounts.length; // We add a new account for "index 1" (not existing yet) - const firstAccountAdded = await controller.addNewAccount( - accountCount, - ); + const firstAccountAdded = + await controller.addNewAccount(accountCount); // Adding an account for an existing index will return the existing account's address - const secondAccountAdded = await controller.addNewAccount( - accountCount, - ); + const secondAccountAdded = + await controller.addNewAccount(accountCount); expect(firstAccountAdded).toBe(secondAccountAdded); expect(controller.state.keyrings[0].accounts).toHaveLength( accountCount + 1, @@ -258,9 +254,8 @@ describe('KeyringController', () => { const [primaryKeyring] = controller.getKeyringsByType( KeyringTypes.hd, ) as Keyring[]; - const addedAccountAddress = await controller.addNewAccountForKeyring( - primaryKeyring, - ); + const addedAccountAddress = + await controller.addNewAccountForKeyring(primaryKeyring); expect(initialState.keyrings).toHaveLength(1); expect(initialState.keyrings[0].accounts).not.toStrictEqual( controller.state.keyrings[0].accounts, @@ -306,9 +301,8 @@ describe('KeyringController', () => { const [primaryKeyring] = controller.getKeyringsByType( KeyringTypes.hd, ) as Keyring[]; - const addedAccountAddress = await controller.addNewAccountForKeyring( - primaryKeyring, - ); + const addedAccountAddress = + await controller.addNewAccountForKeyring(primaryKeyring); expect(initialState.keyrings).toHaveLength(1); expect(initialState.keyrings[0].accounts).not.toStrictEqual( controller.state.keyrings[0].accounts, @@ -407,9 +401,8 @@ describe('KeyringController', () => { await withController( { cacheEncryptionKey }, async ({ controller, initialState }) => { - const currentSeedWord = await controller.exportSeedPhrase( - password, - ); + const currentSeedWord = + await controller.exportSeedPhrase(password); await controller.createNewVaultAndRestore( password, @@ -475,9 +468,8 @@ describe('KeyringController', () => { async ({ controller }) => { await controller.createNewVaultAndKeychain(password); - const currentSeedPhrase = await controller.exportSeedPhrase( - password, - ); + const currentSeedPhrase = + await controller.exportSeedPhrase(password); expect(currentSeedPhrase.length).toBeGreaterThan(0); expect( @@ -565,17 +557,15 @@ describe('KeyringController', () => { await withController( { cacheEncryptionKey }, async ({ controller, initialState }) => { - const initialSeedWord = await controller.exportSeedPhrase( - password, - ); + const initialSeedWord = + await controller.exportSeedPhrase(password); expect(initialSeedWord).toBeDefined(); const initialVault = controller.state.vault; await controller.createNewVaultAndKeychain(password); - const currentSeedWord = await controller.exportSeedPhrase( - password, - ); + const currentSeedWord = + await controller.exportSeedPhrase(password); expect(initialState).toStrictEqual(controller.state); expect(initialSeedWord).toBe(currentSeedWord); expect(initialVault).toStrictEqual(controller.state.vault); @@ -2556,21 +2546,18 @@ describe('KeyringController', () => { ), ); - const firstPage = await signProcessKeyringController.connectQRHardware( - 0, - ); + const firstPage = + await signProcessKeyringController.connectQRHardware(0); expect(firstPage).toHaveLength(5); expect(firstPage[0].index).toBe(0); - const secondPage = await signProcessKeyringController.connectQRHardware( - 1, - ); + const secondPage = + await signProcessKeyringController.connectQRHardware(1); expect(secondPage).toHaveLength(5); expect(secondPage[0].index).toBe(5); - const goBackPage = await signProcessKeyringController.connectQRHardware( - -1, - ); + const goBackPage = + await signProcessKeyringController.connectQRHardware(-1); expect(goBackPage).toStrictEqual(firstPage); await signProcessKeyringController.unlockQRHardwareWalletAccount(0); diff --git a/packages/keyring-controller/src/KeyringController.ts b/packages/keyring-controller/src/KeyringController.ts index bf5b853aeeb..729cbc52464 100644 --- a/packages/keyring-controller/src/KeyringController.ts +++ b/packages/keyring-controller/src/KeyringController.ts @@ -52,32 +52,19 @@ const name = 'KeyringController'; * Available keyring types */ export enum KeyringTypes { - // TODO: Either fix this lint violation or explain why it's necessary to ignore. - // eslint-disable-next-line @typescript-eslint/naming-convention simple = 'Simple Key Pair', - // TODO: Either fix this lint violation or explain why it's necessary to ignore. - // eslint-disable-next-line @typescript-eslint/naming-convention hd = 'HD Key Tree', - // TODO: Either fix this lint violation or explain why it's necessary to ignore. - // eslint-disable-next-line @typescript-eslint/naming-convention qr = 'QR Hardware Wallet Device', - // TODO: Either fix this lint violation or explain why it's necessary to ignore. - // eslint-disable-next-line @typescript-eslint/naming-convention trezor = 'Trezor Hardware', - // TODO: Either fix this lint violation or explain why it's necessary to ignore. - // eslint-disable-next-line @typescript-eslint/naming-convention ledger = 'Ledger Hardware', - // TODO: Either fix this lint violation or explain why it's necessary to ignore. - // eslint-disable-next-line @typescript-eslint/naming-convention lattice = 'Lattice Hardware', - // TODO: Either fix this lint violation or explain why it's necessary to ignore. - // eslint-disable-next-line @typescript-eslint/naming-convention snap = 'Snap Keyring', } /** * Custody keyring types are a special case, as they are not a single type * but they all start with the prefix "Custody". + * * @param keyringType - The type of the keyring. * @returns Whether the keyring type is a custody keyring. */ @@ -86,15 +73,9 @@ export const isCustodyKeyring = (keyringType: string): boolean => { }; /** - * @type KeyringControllerState + * KeyringControllerState * - * Keyring controller state - * @property vault - Encrypted string representing keyring data - * @property isUnlocked - Whether vault is unlocked - * @property keyringTypes - Account types - * @property keyrings - Group of accounts - * @property encryptionKey - Keyring encryption key - * @property encryptionSalt - Keyring encryption salt + * The KeyringController state */ export type KeyringControllerState = { vault?: string; @@ -251,11 +232,9 @@ export type KeyringControllerOptions = { ); /** - * @type KeyringObject + * KeyringObject * * Keyring object to return in fullUpdate - * @property type - Keyring type - * @property accounts - Associated accounts */ export type KeyringObject = { accounts: string[]; @@ -266,11 +245,7 @@ export type KeyringObject = { * A strategy for importing an account */ export enum AccountImportStrategy { - // TODO: Either fix this lint violation or explain why it's necessary to ignore. - // eslint-disable-next-line @typescript-eslint/naming-convention privateKey = 'privateKey', - // TODO: Either fix this lint violation or explain why it's necessary to ignore. - // eslint-disable-next-line @typescript-eslint/naming-convention json = 'json', } @@ -404,13 +379,11 @@ export type KeyringSelector = * * @param releaseLock - A function to release the lock. */ -// TODO: Either fix this lint violation or explain why it's necessary to ignore. -// eslint-disable-next-line @typescript-eslint/naming-convention -type MutuallyExclusiveCallback = ({ +type MutuallyExclusiveCallback = ({ releaseLock, }: { releaseLock: MutexInterface.Releaser; -}) => Promise; +}) => Promise; /** * Get builder function for `Keyring` @@ -586,17 +559,17 @@ export class KeyringController extends BaseController< readonly #vaultOperationMutex = new Mutex(); - #keyringBuilders: { (): EthKeyring; type: string }[]; + readonly #keyringBuilders: { (): EthKeyring; type: string }[]; - #keyrings: EthKeyring[]; + readonly #unsupportedKeyrings: SerializedKeyring[]; - #unsupportedKeyrings: SerializedKeyring[]; + readonly #encryptor: GenericEncryptor | ExportableKeyEncryptor; - #password?: string; + readonly #cacheEncryptionKey: boolean; - #encryptor: GenericEncryptor | ExportableKeyEncryptor; + #keyrings: EthKeyring[]; - #cacheEncryptionKey: boolean; + #password?: string; #qrKeyringStateListener?: ( state: ReturnType, @@ -765,6 +738,7 @@ export class KeyringController extends BaseController< * If there is a pre-existing locked vault, it will be replaced. * * @param password - Password to unlock the new vault. + * @returns Promise resolving when the operation ends successfully. */ async createNewVaultAndKeychain(password: string) { return this.#persistOrRollback(async () => { @@ -989,7 +963,7 @@ export class KeyringController extends BaseController< return this.#persistOrRollback(async () => { let privateKey; switch (strategy) { - case 'privateKey': + case AccountImportStrategy.privateKey: const [importedKey] = args; if (!importedKey) { throw new Error('Cannot import an empty key.'); @@ -1013,7 +987,7 @@ export class KeyringController extends BaseController< privateKey = remove0x(prefixed); break; - case 'json': + case AccountImportStrategy.json: let wallet; const [input, password] = args; try { @@ -1024,7 +998,7 @@ export class KeyringController extends BaseController< privateKey = bytesToHex(wallet.getPrivateKey()); break; default: - throw new Error(`Unexpected import strategy: '${strategy}'`); + throw new Error(`Unexpected import strategy: '${String(strategy)}'`); } const newKeyring = (await this.#newKeyring(KeyringTypes.simple, [ privateKey, @@ -1054,7 +1028,6 @@ export class KeyringController extends BaseController< // The `removeAccount` method of snaps keyring is async. We have to update // the interface of the other keyrings to be async as well. - // eslint-disable-next-line @typescript-eslint/await-thenable // FIXME: We do cast to `Hex` to makes the type checker happy here, and // because `Keyring.removeAccount` requires address to be `Hex`. Those // type would need to be updated for a full non-EVM support. @@ -2203,7 +2176,6 @@ export class KeyringController extends BaseController< // cases and allow retro-compatibility too. // FIXME: For some reason, it seems that eslint is complaining about this call being non-thenable // even though it is... For now, we just disable it: - // eslint-disable-next-line @typescript-eslint/await-thenable await keyring.generateRandomMnemonic(); await keyring.addAccounts(1); } @@ -2353,14 +2325,14 @@ export class KeyringController extends BaseController< * and save the keyrings to state after it, or rollback to their * previous state in case of error. * - * @param fn - The function to execute. + * @param callback - The function to execute. * @returns The result of the function. */ - // TODO: Either fix this lint violation or explain why it's necessary to ignore. - // eslint-disable-next-line @typescript-eslint/naming-convention - async #persistOrRollback(fn: MutuallyExclusiveCallback): Promise { + async #persistOrRollback( + callback: MutuallyExclusiveCallback, + ): Promise { return this.#withRollback(async ({ releaseLock }) => { - const callbackResult = await fn({ releaseLock }); + const callbackResult = await callback({ releaseLock }); // State is committed only if the operation is successful await this.#updateVault(); @@ -2372,18 +2344,18 @@ export class KeyringController extends BaseController< * Execute the given function after acquiring the controller lock * and rollback keyrings and password states in case of error. * - * @param fn - The function to execute atomically. + * @param callback - The function to execute atomically. * @returns The result of the function. */ - // TODO: Either fix this lint violation or explain why it's necessary to ignore. - // eslint-disable-next-line @typescript-eslint/naming-convention - async #withRollback(fn: MutuallyExclusiveCallback): Promise { + async #withRollback( + callback: MutuallyExclusiveCallback, + ): Promise { return this.#withControllerLock(async ({ releaseLock }) => { const currentSerializedKeyrings = await this.#getSerializedKeyrings(); const currentPassword = this.#password; try { - return await fn({ releaseLock }); + return await callback({ releaseLock }); } catch (e) { // Keyrings and password are restored to their previous state await this.#restoreSerializedKeyrings(currentSerializedKeyrings); @@ -2414,13 +2386,13 @@ export class KeyringController extends BaseController< * controller and that changes its state is executed in a mutually exclusive way, * preventing unsafe concurrent access that could lead to unpredictable behavior. * - * @param fn - The function to execute while the controller mutex is locked. + * @param callback - The function to execute while the controller mutex is locked. * @returns The result of the function. */ - // TODO: Either fix this lint violation or explain why it's necessary to ignore. - // eslint-disable-next-line @typescript-eslint/naming-convention - async #withControllerLock(fn: MutuallyExclusiveCallback): Promise { - return withLock(this.#controllerOperationMutex, fn); + async #withControllerLock( + callback: MutuallyExclusiveCallback, + ): Promise { + return withLock(this.#controllerOperationMutex, callback); } /** @@ -2431,15 +2403,15 @@ export class KeyringController extends BaseController< * This ensures that each operation that interacts with the vault * is executed in a mutually exclusive way. * - * @param fn - The function to execute while the vault mutex is locked. + * @param callback - The function to execute while the vault mutex is locked. * @returns The result of the function. */ - // TODO: Either fix this lint violation or explain why it's necessary to ignore. - // eslint-disable-next-line @typescript-eslint/naming-convention - async #withVaultLock(fn: MutuallyExclusiveCallback): Promise { + async #withVaultLock( + callback: MutuallyExclusiveCallback, + ): Promise { this.#assertControllerMutexIsLocked(); - return withLock(this.#vaultOperationMutex, fn); + return withLock(this.#vaultOperationMutex, callback); } } @@ -2449,19 +2421,17 @@ export class KeyringController extends BaseController< * error is thrown. * * @param mutex - The mutex to lock. - * @param fn - The function to execute while the mutex is locked. + * @param callback - The function to execute while the mutex is locked. * @returns The result of the function. */ -// TODO: Either fix this lint violation or explain why it's necessary to ignore. -// eslint-disable-next-line @typescript-eslint/naming-convention -async function withLock( +async function withLock( mutex: Mutex, - fn: MutuallyExclusiveCallback, -): Promise { + callback: MutuallyExclusiveCallback, +): Promise { const releaseLock = await mutex.acquire(); try { - return await fn({ releaseLock }); + return await callback({ releaseLock }); } finally { releaseLock(); } From 100e5f024ee4969311e9aefa26b9fe23aa7d5095 Mon Sep 17 00:00:00 2001 From: Michele Esposito <34438276+mikesposito@users.noreply.github.com> Date: Wed, 29 Jan 2025 18:11:09 +0100 Subject: [PATCH 2/5] Update KeyringController.ts Co-authored-by: Mark Stacey --- packages/keyring-controller/src/KeyringController.ts | 2 -- 1 file changed, 2 deletions(-) diff --git a/packages/keyring-controller/src/KeyringController.ts b/packages/keyring-controller/src/KeyringController.ts index 729cbc52464..b3c1099a04e 100644 --- a/packages/keyring-controller/src/KeyringController.ts +++ b/packages/keyring-controller/src/KeyringController.ts @@ -2174,8 +2174,6 @@ export class KeyringController extends BaseController< // NOTE: Not all keyrings implement this method in a asynchronous-way. Using `await` for // non-thenable will still be valid (despite not being really useful). It allows us to cover both // cases and allow retro-compatibility too. - // FIXME: For some reason, it seems that eslint is complaining about this call being non-thenable - // even though it is... For now, we just disable it: await keyring.generateRandomMnemonic(); await keyring.addAccounts(1); } From 0bb484e375fa2dcfeefde95b2de1f00acf414c87 Mon Sep 17 00:00:00 2001 From: Michele Esposito Date: Wed, 29 Jan 2025 18:48:10 +0100 Subject: [PATCH 3/5] update `eslint-warning-threshold.json` --- eslint-warning-thresholds.json | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/eslint-warning-thresholds.json b/eslint-warning-thresholds.json index f13eddff828..134c16b4663 100644 --- a/eslint-warning-thresholds.json +++ b/eslint-warning-thresholds.json @@ -2,24 +2,24 @@ "@typescript-eslint/consistent-type-exports": 19, "@typescript-eslint/no-base-to-string": 3, "@typescript-eslint/no-duplicate-enum-values": 2, - "@typescript-eslint/no-unsafe-enum-comparison": 34, + "@typescript-eslint/no-unsafe-enum-comparison": 29, "@typescript-eslint/no-unused-vars": 41, "@typescript-eslint/prefer-promise-reject-errors": 33, - "@typescript-eslint/prefer-readonly": 142, + "@typescript-eslint/prefer-readonly": 138, "import-x/namespace": 189, "import-x/no-named-as-default": 1, "import-x/no-named-as-default-member": 8, - "import-x/order": 211, + "import-x/order": 208, "jest/no-conditional-in-test": 113, "jest/prefer-lowercase-title": 2, "jest/prefer-strict-equal": 2, - "jsdoc/check-tag-names": 375, - "jsdoc/require-returns": 25, - "jsdoc/tag-lines": 334, + "jsdoc/check-tag-names": 365, + "jsdoc/require-returns": 24, + "jsdoc/tag-lines": 333, "n/no-unsupported-features/node-builtins": 4, "n/prefer-global/text-encoder": 4, "n/prefer-global/text-decoder": 4, - "prettier/prettier": 116, + "prettier/prettier": 103, "promise/always-return": 3, "promise/catch-or-return": 2, "promise/param-names": 8, From 1334e78782775c9d0261a429b90141f72672af93 Mon Sep 17 00:00:00 2001 From: Michele Esposito Date: Wed, 29 Jan 2025 19:57:56 +0100 Subject: [PATCH 4/5] apply @Gudahtt suggestion --- .../src/KeyringController.ts | 30 +++++++++++++++---- 1 file changed, 25 insertions(+), 5 deletions(-) diff --git a/packages/keyring-controller/src/KeyringController.ts b/packages/keyring-controller/src/KeyringController.ts index b3c1099a04e..bfc5ae1dcb3 100644 --- a/packages/keyring-controller/src/KeyringController.ts +++ b/packages/keyring-controller/src/KeyringController.ts @@ -73,15 +73,31 @@ export const isCustodyKeyring = (keyringType: string): boolean => { }; /** - * KeyringControllerState - * * The KeyringController state */ export type KeyringControllerState = { + /** + * Encrypted array of serialized keyrings data. + */ vault?: string; + /** + * Whether the vault has been decrypted successfully and + * keyrings contained within are deserialized and available. + */ isUnlocked: boolean; + /** + * Representations of managed keyrings. + */ keyrings: KeyringObject[]; + /** + * The encryption key derived from the password and used to encrypt + * the vault. This is only stored if the `cacheEncryptionKey` option + * is enabled. + */ encryptionKey?: string; + /** + * The salt used to derive the encryption key from the password. + */ encryptionSalt?: string; }; @@ -232,12 +248,16 @@ export type KeyringControllerOptions = { ); /** - * KeyringObject - * - * Keyring object to return in fullUpdate + * A keyring object representation. */ export type KeyringObject = { + /** + * Accounts associated with the keyring. + */ accounts: string[]; + /** + * Keyring type. + */ type: string; }; From 6a11b3a1c3e96ebc2d5e989e0f3e89281746e2e5 Mon Sep 17 00:00:00 2001 From: Michele Esposito <34438276+mikesposito@users.noreply.github.com> Date: Thu, 30 Jan 2025 19:00:27 +0100 Subject: [PATCH 5/5] Update eslint-warning-thresholds.json Co-authored-by: Elliot Winkler --- eslint-warning-thresholds.json | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/eslint-warning-thresholds.json b/eslint-warning-thresholds.json index 8899c8bf276..da1b9add867 100644 --- a/eslint-warning-thresholds.json +++ b/eslint-warning-thresholds.json @@ -2,7 +2,7 @@ "@typescript-eslint/consistent-type-exports": 19, "@typescript-eslint/no-base-to-string": 3, "@typescript-eslint/no-duplicate-enum-values": 2, - "@typescript-eslint/no-unsafe-enum-comparison": 29, + "@typescript-eslint/no-unsafe-enum-comparison": 32, "@typescript-eslint/no-unused-vars": 41, "@typescript-eslint/prefer-promise-reject-errors": 33, "@typescript-eslint/prefer-readonly": 138,