Skip to content

feat: add support for setSelectedAccounts #543

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

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

feat: add support for setSelectedAccounts #543

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
bf99101
feat: introduce `setSelectedAccounts`
orestiseth Oct 14, 2025
dd19e37
feat: add jsdoc for new methods
orestiseth Oct 14, 2025
576d098
feat: throw validation error if selected accounts dont overlap with e…
orestiseth Oct 14, 2025
5a6d17f
feat: simplify logic and add unit tests
orestiseth Oct 14, 2025
8fd8a3f
feat: fix linter
orestiseth Oct 14, 2025
d0f5244
feat: fix params passing
orestiseth Oct 16, 2025
741cddf
feat: address PR comments
orestiseth Oct 16, 2025
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
View file
Open in desktop
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
View file
Open in desktop
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
22 changes: 21 additions & 1 deletion packages/snap/src/handlers/CronHandler.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,33 +1,48 @@
import { getSelectedAccounts } from '@metamask/keyring-snap-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 = {};

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 () => {
Copy link

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 +56,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 +73,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
View file
Open in desktop
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
View file
Open in desktop
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 () => {
Copy link

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
View file
Open in desktop
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 as any).params.accountIds);
Copy link

Choose a reason for hiding this comment

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

I don't think the as any is needed here.

On line 149, assert narrows the type of request, so that on line 150 request is of type inferred from the struct.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually it should be await this.setSelectedAccounts(request.params.accounts);. I have the change locally but haven't pushed it yet. Will do in a bit.

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.

Copy link

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);
});
Copy link

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 •
edited
Loading

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
View file
Open in desktop
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
Copy link

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
View file
Open in desktop
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
View file
Open in desktop
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
View file
Open in desktop
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
View file
Open in desktop
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