Skip to content
Open
Show file tree
Hide file tree
Changes from 6 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions packages/snap/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -38,8 +38,8 @@
"@jest/globals": "^30.0.5",
"@metamask/bitcoindevkit": "^0.1.13",
"@metamask/key-tree": "^10.1.1",
"@metamask/keyring-api": "^20.1.1",
"@metamask/keyring-snap-sdk": "^6.0.0",
"@metamask/keyring-api": "^21.1.0",
"@metamask/keyring-snap-sdk": "^7.1.0",
"@metamask/slip44": "^4.2.0",
"@metamask/snaps-cli": "^8.3.0",
"@metamask/snaps-jest": "^9.4.0",
Expand Down
4 changes: 2 additions & 2 deletions packages/snap/snap.manifest.json
Original file line number Diff line number Diff line change
@@ -1,13 +1,13 @@
{
"version": "1.2.0",
"version": "1.3.0",
"description": "Manage Bitcoin using MetaMask",
"proposedName": "Bitcoin",
"repository": {
"type": "git",
"url": "https://github.com/MetaMask/snap-bitcoin-wallet.git"
},
"source": {
"shasum": "6Tzrjzyccj6Ane17A/FBIUEVNaB6pgoUEWRTQUbpB4o=",
"shasum": "hEGVUC7N1d/6dL1ejo2APeWUY70YKz7OEN7721KEmVU=",
"location": {
"npm": {
"filePath": "dist/bundle.js",
Expand Down
23 changes: 22 additions & 1 deletion packages/snap/src/handlers/CronHandler.test.ts
Original file line number Diff line number Diff line change
@@ -1,33 +1,49 @@
import { getSelectedAccounts } from '@metamask/keyring-snap-sdk';
import type { SnapsProvider } from '@metamask/snaps-sdk';
import type { JsonRpcRequest } from '@metamask/utils';
import { mock } from 'jest-mock-extended';

import type { BitcoinAccount, SnapClient } from '../entities';
import type { SendFlowUseCases, AccountUseCases } from '../use-cases';
import { CronHandler, CronMethod } from './CronHandler';

jest.mock('@metamask/keyring-snap-sdk', () => ({
getSelectedAccounts: jest.fn(),
}));

describe('CronHandler', () => {
const mockSendFlowUseCases = mock<SendFlowUseCases>();
const mockAccountUseCases = mock<AccountUseCases>();
const mockSnapClient = mock<SnapClient>();
const mockSnap = mock<SnapsProvider>();

const handler = new CronHandler(
mockAccountUseCases,
mockSendFlowUseCases,
mockSnapClient,
mockSnap,
);

beforeEach(() => {
jest.clearAllMocks();
mockSnapClient.getClientStatus.mockResolvedValue({
active: true,
locked: false,
});
});

describe('synchronizeAccounts', () => {
const mockAccounts = [mock<BitcoinAccount>(), mock<BitcoinAccount>()];
const mockAccounts = [
mock<BitcoinAccount>({ id: 'account-1' }),
mock<BitcoinAccount>({ id: 'account-2' }),
];
const request = { method: 'synchronizeAccounts' } as JsonRpcRequest;

it('synchronizes all accounts', async () => {

Choose a reason for hiding this comment

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

Suggested change
it('synchronizes all accounts', async () => {
it('synchronizes selected accounts', async () => {

(getSelectedAccounts as jest.Mock).mockResolvedValue([
'account-1',
'account-2',
]);
mockAccountUseCases.list.mockResolvedValue(mockAccounts);

await handler.route(request);
Expand All @@ -41,6 +57,7 @@ describe('CronHandler', () => {

it('propagates errors from list', async () => {
const error = new Error();
(getSelectedAccounts as jest.Mock).mockResolvedValue(['account-1']);
mockAccountUseCases.list.mockRejectedValue(error);

await expect(handler.route(request)).rejects.toThrow(error);
Expand All @@ -57,6 +74,10 @@ describe('CronHandler', () => {
});

it('throws error if some account fails to synchronize', async () => {
(getSelectedAccounts as jest.Mock).mockResolvedValue([
'account-1',
'account-2',
]);
mockAccountUseCases.list.mockResolvedValue(mockAccounts);
mockAccountUseCases.synchronize.mockRejectedValue(new Error('error'));

Expand Down
15 changes: 14 additions & 1 deletion packages/snap/src/handlers/CronHandler.ts
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
import { getSelectedAccounts } from '@metamask/keyring-snap-sdk';
import type { SnapsProvider } from '@metamask/snaps-sdk';
import type { JsonRpcRequest } from '@metamask/utils';
import { assert, object, string } from 'superstruct';

Expand All @@ -24,14 +26,18 @@ export class CronHandler {

readonly #snapClient: SnapClient;

readonly #snap: SnapsProvider;

constructor(
accounts: AccountUseCases,
sendFlow: SendFlowUseCases,
snapClient: SnapClient,
snap: SnapsProvider,
) {
this.#accountsUseCases = accounts;
this.#sendFlowUseCases = sendFlow;
this.#snapClient = snapClient;
this.#snap = snap;
}

async route(request: JsonRpcRequest): Promise<void> {
Expand All @@ -56,7 +62,14 @@ export class CronHandler {
}

async synchronizeAccounts(): Promise<void> {
const accounts = await this.#accountsUseCases.list();
const selectedAccounts: Set<string> = new Set(
await getSelectedAccounts(this.#snap),
);

const accounts = (await this.#accountsUseCases.list()).filter((account) => {
return selectedAccounts.has(account.id);
});

const results = await Promise.allSettled(
accounts.map(async (account) => {
return this.#accountsUseCases.synchronize(account, 'cron');
Expand Down
8 changes: 4 additions & 4 deletions packages/snap/src/handlers/KeyringHandler.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -183,7 +183,7 @@ describe('KeyringHandler', () => {
index: 1,
addressType: 'p2wpkh',
entropySource: 'entropy2',
synchronize: true,
synchronize: false,
};

await handler.createAccount(options);
Expand All @@ -207,7 +207,7 @@ describe('KeyringHandler', () => {
index: 0,
addressType,
entropySource: 'm',
synchronize: true,
synchronize: false,
};

await handler.createAccount(options);
Expand Down Expand Up @@ -290,7 +290,7 @@ describe('KeyringHandler', () => {
index: 5,
addressType: 'p2wpkh',
entropySource: 'm',
synchronize: true,
synchronize: false,
};

await handler.createAccount(options);
Expand Down Expand Up @@ -359,7 +359,7 @@ describe('KeyringHandler', () => {
expect(discovered).toStrictEqual(expect.arrayContaining(expected));
});

it('filters out accounts that have no transaction history', async () => {
it.skip('filters out accounts that have no transaction history', async () => {

Choose a reason for hiding this comment

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

Why do we skip it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So the filtering was happening when we were discovering accounts which included a full scan. Since now we don't scan the chain for activity we can't know if there were txs or not.

const addressTypes = Object.values(BtcAccountType);
const totalCombinations = scopes.length * addressTypes.length;

Expand Down
34 changes: 29 additions & 5 deletions packages/snap/src/handlers/KeyringHandler.ts
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ import {
ListAccountsRequestStruct,
ListAccountTransactionsRequestStruct,
MetaMaskOptionsStruct,
SetSelectedAccountsRequestStruct,
SubmitRequestRequestStruct,
} from '@metamask/keyring-api';
import type {
Expand Down Expand Up @@ -60,6 +61,7 @@ import {
mapToKeyringAccount,
mapToTransaction,
} from './mappings';
import { validateSelectedAccounts } from './validation';
import type { AccountUseCases } from '../use-cases/AccountUseCases';

export const CreateAccountRequest = object({
Expand Down Expand Up @@ -143,6 +145,11 @@ export class KeyringHandler implements Keyring {
assert(request, SubmitRequestRequestStruct);
return this.submitRequest(request.params);
}
case `${KeyringRpcMethod.SetSelectedAccounts}`: {
assert(request, SetSelectedAccountsRequestStruct);
await this.setSelectedAccounts(request.params.accounts);
return null;
}

default: {
throw new InexistentMethodError('Keyring method not supported', {
Expand Down Expand Up @@ -174,7 +181,7 @@ export class KeyringHandler implements Keyring {
index,
derivationPath,
addressType,
synchronize = true,
synchronize = false,
accountNameSuggestion,
} = options;

Expand Down Expand Up @@ -257,10 +264,7 @@ export class KeyringHandler implements Keyring {
),
);

// Return only accounts with history.
return accounts
.filter((account) => account.listTransactions().length > 0)
.map(mapToDiscoveredAccount);
return accounts.map(mapToDiscoveredAccount);
}

async getAccountBalances(
Expand Down Expand Up @@ -330,6 +334,26 @@ export class KeyringHandler implements Keyring {
return this.#keyringRequest.route(request);
}

async setSelectedAccounts(accounts: string[]): Promise<void> {
const accountIdSet = new Set(accounts);
const allAccounts = await this.#accountsUseCases.list();

validateSelectedAccounts(
accountIdSet,
allAccounts.map((acc) => acc.id),
);

const selectedAccounts = allAccounts.filter((account) =>
accountIdSet.has(account.id),
);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

BTC snap has 2 methods for updating accounts. One is a full scan which previously was used when an account was created/discovered and the other one is a lighter synchronize which identified updates.

Since now we don't do any scanning on discovery we don't really know when it is the first time we see an account (without storing anything on snaps state).

Thus, on this PR we are going to run a full scan everytime the keyring method gets called and a synchronize on the cron job running every 30 seconds.

Feel free to add your thoughts. We can always store "seen" accounts on the snap state but I am not sure how efficient is going to be if a user creates loads of them.

Choose a reason for hiding this comment

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

Makes a lot of sense to me 👍

const scanPromises = selectedAccounts.map(async (account) => {
await this.#accountsUseCases.fullScan(account);
});

Choose a reason for hiding this comment

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

Can simplify to:

Suggested change
const scanPromises = selectedAccounts.map(async (account) => {
await this.#accountsUseCases.fullScan(account);
});
const scanPromises = selectedAccounts.map(async (account) => this.#accountsUseCases.fullScan(account));


await Promise.all(scanPromises);
Copy link

@xavier-brochard xavier-brochard Oct 16, 2025

Choose a reason for hiding this comment

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

What about this?

Suggested change
await Promise.all(scanPromises);
await Promise.allSettled(scanPromises);

Ensures that a failing fullScan call doesn't prevent the others to complete

}

#extractAddressType(path: string): AddressType {
const segments = path.split('/');
if (segments.length < 4) {
Expand Down
23 changes: 23 additions & 0 deletions packages/snap/src/handlers/validation.ts
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ import {
} from 'superstruct';

import type { BitcoinAccount, CodifiedError, Logger } from '../entities';
import { ValidationError } from '../entities';

export enum RpcMethod {
StartSendTransactionFlow = 'startSendTransactionFlow',
Expand Down Expand Up @@ -149,3 +150,25 @@ export function validateAccountBalance(

return NO_ERRORS_RESPONSE;
}

/**
* Validates that all account IDs are part of the existing accounts.
*
* @param accountIds - Set of account IDs to validate
* @param existingAccountIds - Array of existing account IDs
* @throws {ValidationError} If any account ID is not part of existing accounts
*/
export function validateSelectedAccounts(
accountIds: Set<string>,
existingAccountIds: string[],
): void {
const isSubset = (first: Set<string>, second: Set<string>): boolean => {
return Array.from(first).every((element) => second.has(element));
};
Comment on lines +165 to +167

Choose a reason for hiding this comment

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

👌


if (!isSubset(accountIds, new Set(existingAccountIds))) {
throw new ValidationError(
'Account IDs were not part of existing accounts.',
);
}
}
1 change: 1 addition & 0 deletions packages/snap/src/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -89,6 +89,7 @@ const cronHandler = new CronHandler(
accountsUseCases,
sendFlowUseCases,
snapClient,
snap,
);
const rpcHandler = new RpcHandler(sendFlowUseCases, accountsUseCases, logger);
const userInputHandler = new UserInputHandler(
Expand Down
13 changes: 0 additions & 13 deletions packages/snap/src/use-cases/AccountUseCases.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -326,7 +326,6 @@ describe('AccountUseCases', () => {
discoverParams.network,
tAddressType,
);
expect(mockChain.fullScan).toHaveBeenCalledWith(mockAccount);
},
);

Expand Down Expand Up @@ -359,7 +358,6 @@ describe('AccountUseCases', () => {
tNetwork,
discoverParams.addressType,
);
expect(mockChain.fullScan).toHaveBeenCalledWith(mockAccount);
},
);

Expand Down Expand Up @@ -393,17 +391,6 @@ describe('AccountUseCases', () => {
expect(mockRepository.getByDerivationPath).toHaveBeenCalled();
expect(mockRepository.create).toHaveBeenCalled();
});

it('propagates an error if fullScan throws', async () => {
const error = new Error('fullScan failed');
mockChain.fullScan.mockRejectedValue(error);

await expect(useCases.discover(discoverParams)).rejects.toBe(error);

expect(mockRepository.getByDerivationPath).toHaveBeenCalled();
expect(mockRepository.create).toHaveBeenCalled();
expect(mockChain.fullScan).toHaveBeenCalled();
});
});

describe('synchronize', () => {
Expand Down
6 changes: 0 additions & 6 deletions packages/snap/src/use-cases/AccountUseCases.ts
Original file line number Diff line number Diff line change
Expand Up @@ -128,12 +128,6 @@ export class AccountUseCases {
addressType,
);

await this.#chain.fullScan(newAccount);

this.#logger.info(
'Bitcoin account discovered successfully. Request: %o',
req,
);
return newAccount;
}

Expand Down
26 changes: 19 additions & 7 deletions yarn.lock
Original file line number Diff line number Diff line change
Expand Up @@ -2879,8 +2879,8 @@ __metadata:
"@jest/globals": "npm:^30.0.5"
"@metamask/bitcoindevkit": "npm:^0.1.13"
"@metamask/key-tree": "npm:^10.1.1"
"@metamask/keyring-api": "npm:^20.1.1"
"@metamask/keyring-snap-sdk": "npm:^6.0.0"
"@metamask/keyring-api": "npm:^21.1.0"
"@metamask/keyring-snap-sdk": "npm:^7.1.0"
"@metamask/slip44": "npm:^4.2.0"
"@metamask/snaps-cli": "npm:^8.3.0"
"@metamask/snaps-jest": "npm:^9.4.0"
Expand Down Expand Up @@ -3159,19 +3159,31 @@ __metadata:
languageName: node
linkType: hard

"@metamask/keyring-snap-sdk@npm:^6.0.0":
version: 6.0.0
resolution: "@metamask/keyring-snap-sdk@npm:6.0.0"
"@metamask/keyring-api@npm:^21.1.0":
version: 21.1.0
resolution: "@metamask/keyring-api@npm:21.1.0"
dependencies:
"@metamask/keyring-utils": "npm:^3.1.0"
"@metamask/superstruct": "npm:^3.1.0"
"@metamask/utils": "npm:^11.1.0"
bitcoin-address-validation: "npm:^2.2.3"
checksum: 10/3371a5ab0e9ba0e9b23b30b03a7d83d029e223def5485ab1aa2ec793ba18ff3738422dbe3c47f9cf82411d2ca6ca918928bf998741f0977055071b7bf3042314
languageName: node
linkType: hard

"@metamask/keyring-snap-sdk@npm:^7.1.0":
version: 7.1.0
resolution: "@metamask/keyring-snap-sdk@npm:7.1.0"
dependencies:
"@metamask/keyring-utils": "npm:^3.1.0"
"@metamask/snaps-sdk": "npm:^9.0.0"
"@metamask/superstruct": "npm:^3.1.0"
"@metamask/utils": "npm:^11.1.0"
webextension-polyfill: "npm:^0.12.0"
peerDependencies:
"@metamask/keyring-api": ^20.0.0
"@metamask/keyring-api": ^21.1.0
"@metamask/providers": ^19.0.0
checksum: 10/c306615cee12f5669a1e5e5a22a7e0d1a8aa8eb981578ff4cd125067d8aa9fbf788d02ffc17e123ef47458133b6ec8b5a01966599800ea443b4b07f0cdd0ee7e
checksum: 10/1a1809733c1f21af87f3491d292c499c5441afa0780e848718ec2b6aff50d76bb03ea44ee93ecaa80d79453a98926d84cd13ff406256ab6a2136d9e31250faa8
languageName: node
linkType: hard

Expand Down
Loading