Skip to content
Open
Show file tree
Hide file tree
Changes from all 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
1 change: 1 addition & 0 deletions packages/snap-networks-utils/CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0

### Added

- Add `InFlightCoalescer`, exported from a new `./dedupe` entry point, which coalesces concurrent async operations by key so callers share one in-flight run ([#149](https://github.com/MetaMask/internal-snaps/pull/149))
- Add a `safeMerge` utility for shallowly merging objects. ([#166](https://github.com/MetaMask/internal-snaps/pull/166))

### Changed
Expand Down
10 changes: 10 additions & 0 deletions packages/snap-networks-utils/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,16 @@
"default": "./dist/index.cjs"
}
},
"./dedupe": {
"import": {
"types": "./dist/dedupe/index.d.mts",
"default": "./dist/dedupe/index.mjs"
},
"require": {
"types": "./dist/dedupe/index.d.cts",
"default": "./dist/dedupe/index.cjs"
}
},
"./logger": {
"import": {
"types": "./dist/logger/index.d.mts",
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,77 @@
import { InFlightCoalescer } from './InFlightCoalescer';

describe('InFlightCoalescer', () => {
it('returns the result of the wrapped function', async () => {
const coalescer = new InFlightCoalescer();

const result = await coalescer.run('key', async () => 'value');

expect(result).toBe('value');
});

it('shares one in-flight run between concurrent callers with the same key', async () => {
const coalescer = new InFlightCoalescer();
let resolveRun: (value: string) => void = () => undefined;
const fn = jest.fn(
async () =>
new Promise<string>((resolve) => {
resolveRun = resolve;
}),
);

const first = coalescer.run('key', fn);
const second = coalescer.run('key', fn);
resolveRun('shared');

expect(await first).toBe('shared');
expect(await second).toBe('shared');
expect(fn).toHaveBeenCalledTimes(1);
});

it('runs again once the previous run for the key has settled', async () => {
const coalescer = new InFlightCoalescer();
const fn = jest.fn(async () => 'value');

await coalescer.run('key', fn);
await coalescer.run('key', fn);

expect(fn).toHaveBeenCalledTimes(2);
});

it('runs concurrent callers with different keys independently', async () => {
const coalescer = new InFlightCoalescer();
const fnA = jest.fn(async () => 'a');
const fnB = jest.fn(async () => 'b');

const [resultA, resultB] = await Promise.all([
coalescer.run('a', fnA),
coalescer.run('b', fnB),
]);

expect(resultA).toBe('a');
expect(resultB).toBe('b');
expect(fnA).toHaveBeenCalledTimes(1);
expect(fnB).toHaveBeenCalledTimes(1);
});

it('propagates rejections to coalesced callers and clears the entry', async () => {
const coalescer = new InFlightCoalescer();
let rejectRun: (error: Error) => void = () => undefined;
const failing = jest.fn(
async () =>
new Promise<string>((_resolve, reject) => {
rejectRun = reject;
}),
);

const first = coalescer.run('key', failing);
const second = coalescer.run('key', failing);
rejectRun(new Error('boom'));

await expect(first).rejects.toThrow('boom');
await expect(second).rejects.toThrow('boom');
expect(failing).toHaveBeenCalledTimes(1);

expect(await coalescer.run('key', async () => 'ok')).toBe('ok');
});
});
24 changes: 24 additions & 0 deletions packages/snap-networks-utils/src/dedupe/InFlightCoalescer.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
/**
* Coalesces concurrent async operations by key: while a call for a key is in
* flight, subsequent calls with the same key await the same promise instead of
* starting duplicate work. Once a run settles, the next call starts a fresh one.
*
* Note that coalesced callers share the run's outcome, including rejections.
*/
export class InFlightCoalescer {
readonly #inFlight = new Map<string, Promise<unknown>>();

async run<Result>(key: string, fn: () => Promise<Result>): Promise<Result> {
const pending = this.#inFlight.get(key);
if (pending) {
return pending as Promise<Result>;
}

const task = fn().finally(() => {
this.#inFlight.delete(key);
});
this.#inFlight.set(key, task);

return task;
}
}
1 change: 1 addition & 0 deletions packages/snap-networks-utils/src/dedupe/index.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
export { InFlightCoalescer } from './InFlightCoalescer';
9 changes: 9 additions & 0 deletions packages/tron-wallet-snap/CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -13,9 +13,18 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0

### Changed

- Reduce BIP-44 account discovery to a single entropy fetch by reusing the coin-type deriver for the on-chain activity check ([#149](https://github.com/MetaMask/internal-snaps/pull/149))
- Reduce extension RPC round trips in `keyring_createAccounts` from 5 to at most 4 ([#149](https://github.com/MetaMask/internal-snaps/pull/149))
- `mergeKeyringAccounts` now returns the merge result instead of requiring a post-merge state re-read, and the existing-accounts read runs in parallel with the BIP-32 entropy fetch.
- `snap_getBip32Entropy` is now called even when all requested indices already exist (this path only occurs on idempotent retries); no new permissions are required.
- Extract shared asset util functions and inject `SnapAssetsAdapter` from `context` into `AssetsService` ([#143](https://github.com/MetaMask/internal-snaps/pull/143))
- Rename `getByKeyringAccountId` to `getAccountAssets` (with essential-asset synthesis) and update keyring callers ([#143](https://github.com/MetaMask/internal-snaps/pull/143))

### Fixed

- Fix account deletion failing against keyring v2 clients by removing the `AccountDeleted` event emission from `keyring_deleteAccount` ([#149](https://github.com/MetaMask/internal-snaps/pull/149))
- v2 clients reject v1 lifecycle events, which aborted the deletion before the account was removed from state. Deletion is client-initiated in v2, so no event is needed.
- Coalesce concurrent account synchronization runs for the same accounts so stacked triggers (cronjob and background events) share one run instead of duplicating network fetches, state writes, and keyring events ([#149](https://github.com/MetaMask/internal-snaps/pull/149))
- Bump `@metamask/utils` from `^11.9.0` to `^11.11.9` ([#161](https://github.com/MetaMask/internal-snaps/pull/161))

## [3.1.0]
Expand Down
2 changes: 1 addition & 1 deletion packages/tron-wallet-snap/snap.manifest.json
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@
"url": "https://github.com/MetaMask/internal-snaps.git"
},
"source": {
"shasum": "MbqwOXbHFI83/qWOj9zDSXJizgEt5oQ+QnpHq0g/sls=",
"shasum": "BWejLCfSNzan3omP7THB+pL6KhsuVTCGaW39L3v6KTU=",
"location": {
"npm": {
"filePath": "dist/bundle.js",
Expand Down
29 changes: 27 additions & 2 deletions packages/tron-wallet-snap/src/handlers/keyring/keyring.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -73,9 +73,8 @@ describe('KeyringHandler', () => {
mockAccountsService = {
findById: jest.fn().mockResolvedValue(mockAccount),
findByIdOrThrow: jest.fn().mockResolvedValue(mockAccount),
deriveAccount: jest.fn(),
create: jest.fn(),
createAccounts: jest.fn(),
delete: jest.fn().mockResolvedValue(undefined),
getAll: jest.fn().mockResolvedValue([mockAccount]),
deriveTronKeypair: jest.fn().mockResolvedValue({
privateKeyHex: 'a'.repeat(64),
Expand Down Expand Up @@ -561,6 +560,32 @@ describe('KeyringHandler', () => {
});
});

describe('deleteAccount', () => {
it('deletes the account without emitting keyring events', async () => {
await keyringHandler.deleteAccount(mockAccount.id);

expect(mockAccountsService.delete).toHaveBeenCalledWith(mockAccount.id);
});

it('throws for an invalid account id', async () => {
await expect(keyringHandler.deleteAccount('not-a-uuid')).rejects.toThrow(
expect.anything(),
);

expect(mockAccountsService.delete).not.toHaveBeenCalled();
});

it('throws when the account does not exist', async () => {
mockAccountsService.findById.mockResolvedValue(null);

await expect(
keyringHandler.deleteAccount(mockAccount.id),
).rejects.toThrow(`Account "${mockAccount.id}" not found`);

expect(mockAccountsService.delete).not.toHaveBeenCalled();
});
});

describe('createAccounts', () => {
it('delegates to accountsService.createAccounts and returns the result', async () => {
const createdAccounts = [
Expand Down
15 changes: 5 additions & 10 deletions packages/tron-wallet-snap/src/handlers/keyring/keyring.ts
Original file line number Diff line number Diff line change
@@ -1,7 +1,4 @@
import {
KeyringEvent,
ListAccountAssetsResponseStruct,
} from '@metamask/keyring-api';
import { ListAccountAssetsResponseStruct } from '@metamask/keyring-api';
import type {
Balance,
CreateAccountOptions as KeyringBatchCreateAccountOptions,
Expand All @@ -16,7 +13,6 @@ import type {
ExportedAccount,
KeyringSnapRpc,
} from '@metamask/keyring-api/v2';
import { emitSnapKeyringEvent } from '@metamask/keyring-snap-sdk';
import { handleKeyringRequest } from '@metamask/keyring-snap-sdk/v2';
import type { Logger } from '@metamask/snap-networks-utils';
import {
Expand Down Expand Up @@ -380,12 +376,11 @@ export class KeyringHandler implements KeyringSnapRpc {
try {
validateRequest({ accountId }, DeleteAccountStruct);

const account = await this.#getAccountOrThrow(accountId);

await emitSnapKeyringEvent(snap, KeyringEvent.AccountDeleted, {
id: account.id,
});
await this.#getAccountOrThrow(accountId);

// No AccountDeleted event: deletion is client-initiated in keyring v2,
// and v2 clients reject v1 lifecycle events (which would abort the
// deletion below).
await this.#accountsService.delete(accountId);
} catch (error: unknown) {
this.#logger.error({ error }, 'Error deleting account');
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -144,6 +144,34 @@ describe('AccountsRepository', () => {
]);
});

it('returns the merged state and added accounts from mergeKeyringAccounts', async () => {
const existing = createTestAccount({ id: 'existing-0' });
const repository = new AccountsRepository(
createEmptyState({ [existing.id]: existing }),
);
const newIndexAccount = createTestAccount({
id: 'new-index',
index: 1,
derivationPath: "m/44'/195'/0'/0/1",
address: 'TAddress1',
});

const result = await repository.mergeKeyringAccounts({
'duplicate-index': {
...existing,
id: 'duplicate-index',
},
[newIndexAccount.id]: newIndexAccount,
});

// The conflict loser is omitted from `added`; the winner is in `merged`.
expect(Object.keys(result.added)).toStrictEqual(['new-index']);
expect(result.merged).toStrictEqual({
'existing-0': existing,
'new-index': newIndexAccount,
});
});

it('skips duplicate indices within the same merge batch', async () => {
const base = createTestAccount({ id: 'first' });
const repository = new AccountsRepository(createEmptyState());
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,18 @@ type AccountCreationRange = {

type KeyringAccountsState = Record<string, TronKeyringAccount>;

/**
* Result of merging accounts into `keyringAccounts`.
*
* @param merged - The full post-merge keyring accounts state.
* @param added - The subset of incoming accounts that was actually persisted;
* conflict losers are omitted (their winners are present in `merged`).
*/
export type KeyringAccountsMergeResult = {
merged: Record<string, TronKeyringAccount>;
added: Record<string, TronKeyringAccount>;
};

/**
* Tron accounts use a fixed BIP-44 path template; uniqueness is entropy + index.
*
Expand Down Expand Up @@ -165,17 +177,24 @@ export class AccountsRepository {
* Merges multiple keyring accounts into `keyringAccounts` in a single atomic state update.
*
* @param newAccounts - The new accounts to merge.
* @returns The post-merge state and the subset of accounts actually added,
* so callers can resolve persisted accounts (including conflict winners)
* without re-reading state.
*/
async mergeKeyringAccounts(
newAccounts: Record<string, TronKeyringAccount>,
): Promise<void> {
): Promise<KeyringAccountsMergeResult> {
let result: KeyringAccountsMergeResult = { merged: {}, added: {} };

await this.#state.setKeyWith<KeyringAccountsState>(
this.#storageKey,
(current) => {
const existing = current ?? {};
return mergeAccountsWithoutIndexConflicts(existing, newAccounts).merged;
result = mergeAccountsWithoutIndexConflicts(current ?? {}, newAccounts);
return result.merged;
},
);

return result;
}

async delete(id: string): Promise<void> {
Expand Down
Loading