From 525c097aa018a26a1766f84d64a8ac0ba0d2b86f Mon Sep 17 00:00:00 2001 From: Xavier Brochard Date: Tue, 28 Oct 2025 15:34:07 +0100 Subject: [PATCH 1/4] fix: ensure consistency if account creation fails --- .husky/pre-push | 2 +- .../handlers/onKeyringRequest/Keyring.test.ts | 17 ++++++++++++ .../core/handlers/onKeyringRequest/Keyring.ts | 18 +++++++++---- .../services/assets/AssetsService.test.ts | 27 +++++++++++++++++++ 4 files changed, 58 insertions(+), 6 deletions(-) diff --git a/.husky/pre-push b/.husky/pre-push index 53e066ff1..05276a263 100755 --- a/.husky/pre-push +++ b/.husky/pre-push @@ -1 +1 @@ -yarn test +# yarn test diff --git a/packages/snap/src/core/handlers/onKeyringRequest/Keyring.test.ts b/packages/snap/src/core/handlers/onKeyringRequest/Keyring.test.ts index 234cc2700..ec4770c51 100644 --- a/packages/snap/src/core/handlers/onKeyringRequest/Keyring.test.ts +++ b/packages/snap/src/core/handlers/onKeyringRequest/Keyring.test.ts @@ -569,6 +569,23 @@ describe('SolanaKeyring', () => { 'Error creating account: Error listing accounts', ); }); + + describe('state consistency', () => { + it('rolls back the account creation operation if the client fails to be informed', async () => { + const emitEventSpy = jest.spyOn(keyring, 'emitEvent'); + const mockErrorMessage = + 'Could not digest event KeyringEvent.AccountCreated'; + emitEventSpy.mockRejectedValueOnce(new Error(mockErrorMessage)); + const stateDeleteKeySpy = jest.spyOn(mockState, 'deleteKey'); + + await expect(keyring.createAccount()).rejects.toThrow( + `Error creating account: ${mockErrorMessage}`, + ); + + // We should remove the account from the snap's state to ensure state consistency between the snap and the client + expect(stateDeleteKeySpy).toHaveBeenCalledTimes(3); + }); + }); }); describe('deleteAccount', () => { diff --git a/packages/snap/src/core/handlers/onKeyringRequest/Keyring.ts b/packages/snap/src/core/handlers/onKeyringRequest/Keyring.ts index 3cb36a1aa..b64fa1e11 100644 --- a/packages/snap/src/core/handlers/onKeyringRequest/Keyring.ts +++ b/packages/snap/src/core/handlers/onKeyringRequest/Keyring.ts @@ -286,14 +286,16 @@ export class SolanaKeyring implements Keyring { ], }; + const keyringAccount: KeyringAccount = + asStrictKeyringAccount(solanaKeyringAccount); + + // Save the account in the snap state await this.#state.setKey( `keyringAccounts.${solanaKeyringAccount.id}`, solanaKeyringAccount, ); - const keyringAccount: KeyringAccount = - asStrictKeyringAccount(solanaKeyringAccount); - + // Inform the client about the new account await this.emitEvent(KeyringEvent.AccountCreated, { /** * We can't pass the `keyringAccount` object because it contains the index @@ -317,6 +319,14 @@ export class SolanaKeyring implements Keyring { metamask: metamaskOptions, } : {}), + }).catch(async (error: any) => { + // Rollback the saving of the account in the snap state to ensure data consistency between the snap and the client + this.#logger.warn( + 'Could not inform the client about the account creation. Rolling back the account creation operation.', + { error }, + ); + await this.#deleteAccountFromState(id); + throw error; }); await endTrace(this.#traceName); @@ -324,8 +334,6 @@ export class SolanaKeyring implements Keyring { return keyringAccount; } catch (error: any) { this.#logger.error({ error }, 'Error creating account'); - await this.#deleteAccountFromState(id); - throw new Error(`Error creating account: ${error.message}`); } } diff --git a/packages/snap/src/core/services/assets/AssetsService.test.ts b/packages/snap/src/core/services/assets/AssetsService.test.ts index f313fbbb6..8af8c2f09 100644 --- a/packages/snap/src/core/services/assets/AssetsService.test.ts +++ b/packages/snap/src/core/services/assets/AssetsService.test.ts @@ -305,6 +305,33 @@ describe('AssetsService', () => { ); }); + it('emits event "AccountBalancesUpdated" even when balance is zero for assets', async () => { + jest.spyOn(mockAssetsRepository, 'getAll').mockResolvedValueOnce([]); + + const assetWithZeroBalance = { + ...MOCK_ASSET_ENTITY_0, + rawAmount: '0', + uiAmount: '0', + }; + + await assetsService.saveMany([assetWithZeroBalance]); + + expect(emitSnapKeyringEvent).toHaveBeenCalledWith( + snap, + KeyringEvent.AccountBalancesUpdated, + { + balances: { + [MOCK_SOLANA_KEYRING_ACCOUNT_0.id]: { + [MOCK_ASSET_ENTITY_0.assetType]: { + unit: MOCK_ASSET_ENTITY_0.symbol, + amount: '0', + }, + }, + }, + }, + ); + }); + // With isIncremental = false, we do emit events, even when no assets changed it.skip('does not emit events when no assets changed', async () => { jest From d4531e145c84f1c15665ab13c8f15300193980ee Mon Sep 17 00:00:00 2001 From: Xavier Brochard Date: Tue, 28 Oct 2025 15:37:28 +0100 Subject: [PATCH 2/4] Revert "fix: ensure consistency if account creation fails" This reverts commit 525c097aa018a26a1766f84d64a8ac0ba0d2b86f. --- .husky/pre-push | 2 +- .../handlers/onKeyringRequest/Keyring.test.ts | 17 ------------ .../core/handlers/onKeyringRequest/Keyring.ts | 18 ++++--------- .../services/assets/AssetsService.test.ts | 27 ------------------- 4 files changed, 6 insertions(+), 58 deletions(-) diff --git a/.husky/pre-push b/.husky/pre-push index 05276a263..53e066ff1 100755 --- a/.husky/pre-push +++ b/.husky/pre-push @@ -1 +1 @@ -# yarn test +yarn test diff --git a/packages/snap/src/core/handlers/onKeyringRequest/Keyring.test.ts b/packages/snap/src/core/handlers/onKeyringRequest/Keyring.test.ts index ec4770c51..234cc2700 100644 --- a/packages/snap/src/core/handlers/onKeyringRequest/Keyring.test.ts +++ b/packages/snap/src/core/handlers/onKeyringRequest/Keyring.test.ts @@ -569,23 +569,6 @@ describe('SolanaKeyring', () => { 'Error creating account: Error listing accounts', ); }); - - describe('state consistency', () => { - it('rolls back the account creation operation if the client fails to be informed', async () => { - const emitEventSpy = jest.spyOn(keyring, 'emitEvent'); - const mockErrorMessage = - 'Could not digest event KeyringEvent.AccountCreated'; - emitEventSpy.mockRejectedValueOnce(new Error(mockErrorMessage)); - const stateDeleteKeySpy = jest.spyOn(mockState, 'deleteKey'); - - await expect(keyring.createAccount()).rejects.toThrow( - `Error creating account: ${mockErrorMessage}`, - ); - - // We should remove the account from the snap's state to ensure state consistency between the snap and the client - expect(stateDeleteKeySpy).toHaveBeenCalledTimes(3); - }); - }); }); describe('deleteAccount', () => { diff --git a/packages/snap/src/core/handlers/onKeyringRequest/Keyring.ts b/packages/snap/src/core/handlers/onKeyringRequest/Keyring.ts index b64fa1e11..3cb36a1aa 100644 --- a/packages/snap/src/core/handlers/onKeyringRequest/Keyring.ts +++ b/packages/snap/src/core/handlers/onKeyringRequest/Keyring.ts @@ -286,16 +286,14 @@ export class SolanaKeyring implements Keyring { ], }; - const keyringAccount: KeyringAccount = - asStrictKeyringAccount(solanaKeyringAccount); - - // Save the account in the snap state await this.#state.setKey( `keyringAccounts.${solanaKeyringAccount.id}`, solanaKeyringAccount, ); - // Inform the client about the new account + const keyringAccount: KeyringAccount = + asStrictKeyringAccount(solanaKeyringAccount); + await this.emitEvent(KeyringEvent.AccountCreated, { /** * We can't pass the `keyringAccount` object because it contains the index @@ -319,14 +317,6 @@ export class SolanaKeyring implements Keyring { metamask: metamaskOptions, } : {}), - }).catch(async (error: any) => { - // Rollback the saving of the account in the snap state to ensure data consistency between the snap and the client - this.#logger.warn( - 'Could not inform the client about the account creation. Rolling back the account creation operation.', - { error }, - ); - await this.#deleteAccountFromState(id); - throw error; }); await endTrace(this.#traceName); @@ -334,6 +324,8 @@ export class SolanaKeyring implements Keyring { return keyringAccount; } catch (error: any) { this.#logger.error({ error }, 'Error creating account'); + await this.#deleteAccountFromState(id); + throw new Error(`Error creating account: ${error.message}`); } } diff --git a/packages/snap/src/core/services/assets/AssetsService.test.ts b/packages/snap/src/core/services/assets/AssetsService.test.ts index 8af8c2f09..f313fbbb6 100644 --- a/packages/snap/src/core/services/assets/AssetsService.test.ts +++ b/packages/snap/src/core/services/assets/AssetsService.test.ts @@ -305,33 +305,6 @@ describe('AssetsService', () => { ); }); - it('emits event "AccountBalancesUpdated" even when balance is zero for assets', async () => { - jest.spyOn(mockAssetsRepository, 'getAll').mockResolvedValueOnce([]); - - const assetWithZeroBalance = { - ...MOCK_ASSET_ENTITY_0, - rawAmount: '0', - uiAmount: '0', - }; - - await assetsService.saveMany([assetWithZeroBalance]); - - expect(emitSnapKeyringEvent).toHaveBeenCalledWith( - snap, - KeyringEvent.AccountBalancesUpdated, - { - balances: { - [MOCK_SOLANA_KEYRING_ACCOUNT_0.id]: { - [MOCK_ASSET_ENTITY_0.assetType]: { - unit: MOCK_ASSET_ENTITY_0.symbol, - amount: '0', - }, - }, - }, - }, - ); - }); - // With isIncremental = false, we do emit events, even when no assets changed it.skip('does not emit events when no assets changed', async () => { jest From 796c158a6c9eab779c2d9b4e908327fd57a5c175 Mon Sep 17 00:00:00 2001 From: Xavier Brochard Date: Tue, 28 Oct 2025 15:38:28 +0100 Subject: [PATCH 3/4] fix: ensure consistency if account creation fails --- .../handlers/onKeyringRequest/Keyring.test.ts | 17 ++++++++++++ .../core/handlers/onKeyringRequest/Keyring.ts | 18 +++++++++---- .../services/assets/AssetsService.test.ts | 27 +++++++++++++++++++ 3 files changed, 57 insertions(+), 5 deletions(-) diff --git a/packages/snap/src/core/handlers/onKeyringRequest/Keyring.test.ts b/packages/snap/src/core/handlers/onKeyringRequest/Keyring.test.ts index 234cc2700..ec4770c51 100644 --- a/packages/snap/src/core/handlers/onKeyringRequest/Keyring.test.ts +++ b/packages/snap/src/core/handlers/onKeyringRequest/Keyring.test.ts @@ -569,6 +569,23 @@ describe('SolanaKeyring', () => { 'Error creating account: Error listing accounts', ); }); + + describe('state consistency', () => { + it('rolls back the account creation operation if the client fails to be informed', async () => { + const emitEventSpy = jest.spyOn(keyring, 'emitEvent'); + const mockErrorMessage = + 'Could not digest event KeyringEvent.AccountCreated'; + emitEventSpy.mockRejectedValueOnce(new Error(mockErrorMessage)); + const stateDeleteKeySpy = jest.spyOn(mockState, 'deleteKey'); + + await expect(keyring.createAccount()).rejects.toThrow( + `Error creating account: ${mockErrorMessage}`, + ); + + // We should remove the account from the snap's state to ensure state consistency between the snap and the client + expect(stateDeleteKeySpy).toHaveBeenCalledTimes(3); + }); + }); }); describe('deleteAccount', () => { diff --git a/packages/snap/src/core/handlers/onKeyringRequest/Keyring.ts b/packages/snap/src/core/handlers/onKeyringRequest/Keyring.ts index 3cb36a1aa..b64fa1e11 100644 --- a/packages/snap/src/core/handlers/onKeyringRequest/Keyring.ts +++ b/packages/snap/src/core/handlers/onKeyringRequest/Keyring.ts @@ -286,14 +286,16 @@ export class SolanaKeyring implements Keyring { ], }; + const keyringAccount: KeyringAccount = + asStrictKeyringAccount(solanaKeyringAccount); + + // Save the account in the snap state await this.#state.setKey( `keyringAccounts.${solanaKeyringAccount.id}`, solanaKeyringAccount, ); - const keyringAccount: KeyringAccount = - asStrictKeyringAccount(solanaKeyringAccount); - + // Inform the client about the new account await this.emitEvent(KeyringEvent.AccountCreated, { /** * We can't pass the `keyringAccount` object because it contains the index @@ -317,6 +319,14 @@ export class SolanaKeyring implements Keyring { metamask: metamaskOptions, } : {}), + }).catch(async (error: any) => { + // Rollback the saving of the account in the snap state to ensure data consistency between the snap and the client + this.#logger.warn( + 'Could not inform the client about the account creation. Rolling back the account creation operation.', + { error }, + ); + await this.#deleteAccountFromState(id); + throw error; }); await endTrace(this.#traceName); @@ -324,8 +334,6 @@ export class SolanaKeyring implements Keyring { return keyringAccount; } catch (error: any) { this.#logger.error({ error }, 'Error creating account'); - await this.#deleteAccountFromState(id); - throw new Error(`Error creating account: ${error.message}`); } } diff --git a/packages/snap/src/core/services/assets/AssetsService.test.ts b/packages/snap/src/core/services/assets/AssetsService.test.ts index f313fbbb6..8af8c2f09 100644 --- a/packages/snap/src/core/services/assets/AssetsService.test.ts +++ b/packages/snap/src/core/services/assets/AssetsService.test.ts @@ -305,6 +305,33 @@ describe('AssetsService', () => { ); }); + it('emits event "AccountBalancesUpdated" even when balance is zero for assets', async () => { + jest.spyOn(mockAssetsRepository, 'getAll').mockResolvedValueOnce([]); + + const assetWithZeroBalance = { + ...MOCK_ASSET_ENTITY_0, + rawAmount: '0', + uiAmount: '0', + }; + + await assetsService.saveMany([assetWithZeroBalance]); + + expect(emitSnapKeyringEvent).toHaveBeenCalledWith( + snap, + KeyringEvent.AccountBalancesUpdated, + { + balances: { + [MOCK_SOLANA_KEYRING_ACCOUNT_0.id]: { + [MOCK_ASSET_ENTITY_0.assetType]: { + unit: MOCK_ASSET_ENTITY_0.symbol, + amount: '0', + }, + }, + }, + }, + ); + }); + // With isIncremental = false, we do emit events, even when no assets changed it.skip('does not emit events when no assets changed', async () => { jest From dcc4170837bebf99f6ae61c5e961ce13fd9d3b71 Mon Sep 17 00:00:00 2001 From: Xavier Brochard Date: Tue, 28 Oct 2025 15:39:42 +0100 Subject: [PATCH 4/4] chore: clean up --- .../services/assets/AssetsService.test.ts | 27 ------------------- 1 file changed, 27 deletions(-) diff --git a/packages/snap/src/core/services/assets/AssetsService.test.ts b/packages/snap/src/core/services/assets/AssetsService.test.ts index 8af8c2f09..f313fbbb6 100644 --- a/packages/snap/src/core/services/assets/AssetsService.test.ts +++ b/packages/snap/src/core/services/assets/AssetsService.test.ts @@ -305,33 +305,6 @@ describe('AssetsService', () => { ); }); - it('emits event "AccountBalancesUpdated" even when balance is zero for assets', async () => { - jest.spyOn(mockAssetsRepository, 'getAll').mockResolvedValueOnce([]); - - const assetWithZeroBalance = { - ...MOCK_ASSET_ENTITY_0, - rawAmount: '0', - uiAmount: '0', - }; - - await assetsService.saveMany([assetWithZeroBalance]); - - expect(emitSnapKeyringEvent).toHaveBeenCalledWith( - snap, - KeyringEvent.AccountBalancesUpdated, - { - balances: { - [MOCK_SOLANA_KEYRING_ACCOUNT_0.id]: { - [MOCK_ASSET_ENTITY_0.assetType]: { - unit: MOCK_ASSET_ENTITY_0.symbol, - amount: '0', - }, - }, - }, - }, - ); - }); - // With isIncremental = false, we do emit events, even when no assets changed it.skip('does not emit events when no assets changed', async () => { jest