Skip to content

Commit e607b7f

Browse files
zone-liveccharly
andauthored
chore: get assets list from MultichainAssetsController state (#5295)
## Explanation We are getting the full list of assets for an account via the MultichainAssetsController state, instead of only requesting the native token. ## References <!-- Are there any issues that this pull request is tied to? Are there other links that reviewers should consult to understand these changes better? Are there client or consumer pull requests to adopt any breaking changes? For example: * Fixes #12345 * Related to #67890 --> ## Changelog <!-- If you're making any consumer-facing changes, list those changes here as if you were updating a changelog, using the template below as a guide. (CATEGORY is one of BREAKING, ADDED, CHANGED, DEPRECATED, REMOVED, or FIXED. For security-related issues, follow the Security Advisory process.) Please take care to name the exact pieces of the API you've added or changed (e.g. types, interfaces, functions, or methods). If there are any breaking changes, make sure to offer a solution for consumers to follow once they upgrade to the changes. Finally, if you're only making changes to development scripts or tests, you may replace the template below with "None". --> ### `@metamask/assets-controllers` - **ADDED**: Adds the ability to request for all the assets that an account can have ## Checklist - [x] I've updated the test suite for new or updated code as appropriate - [x] I've updated documentation (JSDoc, Markdown, etc.) for new or updated code as appropriate - [x] I've highlighted breaking changes using the "BREAKING" category above as appropriate - [x] I've prepared draft pull requests for clients and consumer packages to resolve any breaking changes --------- Co-authored-by: Charly Chevalier <charly.chevalier@consensys.net>
1 parent 0d62854 commit e607b7f

File tree

11 files changed

+156
-423
lines changed

11 files changed

+156
-423
lines changed

packages/assets-controllers/src/MultichainBalancesController/MultichainBalancesController.test.ts

Lines changed: 93 additions & 59 deletions
Original file line numberDiff line numberDiff line change
@@ -15,14 +15,12 @@ import { KeyringTypes } from '@metamask/keyring-controller';
1515
import type { InternalAccount } from '@metamask/keyring-internal-api';
1616
import { v4 as uuidv4 } from 'uuid';
1717

18-
import {
19-
MultichainBalancesController,
20-
getDefaultMultichainBalancesControllerState,
21-
} from './MultichainBalancesController';
18+
import { MultichainBalancesController } from '.';
2219
import type {
2320
MultichainBalancesControllerMessenger,
2421
MultichainBalancesControllerState,
25-
} from './MultichainBalancesController';
22+
} from '.';
23+
import { getDefaultMultichainBalancesControllerState } from './MultichainBalancesController';
2624
import type {
2725
ExtractAvailableAction,
2826
ExtractAvailableEvent,
@@ -122,6 +120,31 @@ function getRootMessenger(): Messenger<RootAction, RootEvent> {
122120
return new Messenger<RootAction, RootEvent>();
123121
}
124122

123+
/**
124+
* Constructs the restricted messenger for the MultichainBalancesController.
125+
*
126+
* @param messenger - The root messenger.
127+
* @returns The unrestricted messenger suited for MultichainBalancesController.
128+
*/
129+
function getRestrictedMessenger(
130+
messenger: Messenger<RootAction, RootEvent>,
131+
): MultichainBalancesControllerMessenger {
132+
return messenger.getRestricted({
133+
name: 'MultichainBalancesController',
134+
allowedActions: [
135+
'SnapController:handleRequest',
136+
'AccountsController:listMultichainAccounts',
137+
'MultichainAssetsController:getState',
138+
],
139+
allowedEvents: [
140+
'AccountsController:accountAdded',
141+
'AccountsController:accountRemoved',
142+
'AccountsController:accountBalancesUpdated',
143+
'MultichainAssetsController:stateChange',
144+
],
145+
});
146+
}
147+
125148
const setupController = ({
126149
state = getDefaultMultichainBalancesControllerState(),
127150
mocks,
@@ -133,20 +156,7 @@ const setupController = ({
133156
};
134157
} = {}) => {
135158
const messenger = getRootMessenger();
136-
137-
const multichainBalancesMessenger: MultichainBalancesControllerMessenger =
138-
messenger.getRestricted({
139-
name: 'MultichainBalancesController',
140-
allowedActions: [
141-
'SnapController:handleRequest',
142-
'AccountsController:listMultichainAccounts',
143-
],
144-
allowedEvents: [
145-
'AccountsController:accountAdded',
146-
'AccountsController:accountRemoved',
147-
'AccountsController:accountBalancesUpdated',
148-
],
149-
});
159+
const multichainBalancesMessenger = getRestrictedMessenger(messenger);
150160

151161
const mockSnapHandleRequest = jest.fn();
152162
messenger.registerActionHandler(
@@ -164,6 +174,16 @@ const setupController = ({
164174
),
165175
);
166176

177+
const mockGetAssetsState = jest.fn().mockReturnValue({
178+
accountsAssets: {
179+
[mockBtcAccount.id]: [mockBtcNativeAsset],
180+
},
181+
});
182+
messenger.registerActionHandler(
183+
'MultichainAssetsController:getState',
184+
mockGetAssetsState,
185+
);
186+
167187
const controller = new MultichainBalancesController({
168188
messenger: multichainBalancesMessenger,
169189
state,
@@ -174,6 +194,7 @@ const setupController = ({
174194
messenger,
175195
mockSnapHandleRequest,
176196
mockListMultichainAccounts,
197+
mockGetAssetsState,
177198
};
178199
};
179200

@@ -193,7 +214,22 @@ async function waitForAllPromises(): Promise<void> {
193214

194215
describe('BalancesController', () => {
195216
it('initialize with default state', () => {
196-
const { controller } = setupController({});
217+
const messenger = getRootMessenger();
218+
const multichainBalancesMessenger = getRestrictedMessenger(messenger);
219+
220+
messenger.registerActionHandler('SnapController:handleRequest', jest.fn());
221+
messenger.registerActionHandler(
222+
'AccountsController:listMultichainAccounts',
223+
jest.fn().mockReturnValue([]),
224+
);
225+
messenger.registerActionHandler(
226+
'MultichainAssetsController:getState',
227+
jest.fn(),
228+
);
229+
230+
const controller = new MultichainBalancesController({
231+
messenger: multichainBalancesMessenger,
232+
});
197233
expect(controller.state).toStrictEqual({ balances: {} });
198234
});
199235

@@ -206,26 +242,6 @@ describe('BalancesController', () => {
206242
);
207243
});
208244

209-
it('updates balances when "AccountsController:accountAdded" is fired', async () => {
210-
const { controller, messenger, mockListMultichainAccounts } =
211-
setupController({
212-
mocks: {
213-
listMultichainAccounts: [],
214-
},
215-
});
216-
217-
mockListMultichainAccounts.mockReturnValue([mockBtcAccount]);
218-
messenger.publish('AccountsController:accountAdded', mockBtcAccount);
219-
220-
await waitForAllPromises();
221-
222-
expect(controller.state).toStrictEqual({
223-
balances: {
224-
[mockBtcAccount.id]: mockBalanceResult,
225-
},
226-
});
227-
});
228-
229245
it('updates balances when "AccountsController:accountRemoved" is fired', async () => {
230246
const { controller, messenger } = setupController();
231247

@@ -292,25 +308,6 @@ describe('BalancesController', () => {
292308
expect(controller.state.balances).toStrictEqual({});
293309
});
294310

295-
it('handles errors gracefully when constructing the controller', async () => {
296-
// This method will be used in the constructor of that controller.
297-
const updateBalanceSpy = jest.spyOn(
298-
MultichainBalancesController.prototype,
299-
'updateBalance',
300-
);
301-
updateBalanceSpy.mockRejectedValue(
302-
new Error('Something unexpected happen'),
303-
);
304-
305-
const { controller } = setupController({
306-
mocks: {
307-
listMultichainAccounts: [mockBtcAccount],
308-
},
309-
});
310-
311-
expect(controller.state.balances).toStrictEqual({});
312-
});
313-
314311
it('handles errors when trying to upgrade the balance of a non-existing account', async () => {
315312
const { controller } = setupController({
316313
mocks: {
@@ -392,4 +389,41 @@ describe('BalancesController', () => {
392389
mockBalanceResult,
393390
);
394391
});
392+
393+
it('handles an account with no assets in MultichainAssetsController state', async () => {
394+
const { controller, mockGetAssetsState } = setupController({
395+
mocks: {
396+
handleRequestReturnValue: {},
397+
},
398+
});
399+
400+
mockGetAssetsState.mockReturnValue({
401+
accountsAssets: {},
402+
});
403+
404+
await controller.updateBalance(mockBtcAccount.id);
405+
406+
expect(controller.state.balances[mockBtcAccount.id]).toStrictEqual({});
407+
});
408+
409+
it('updates balances when receiving "MultichainAssetsController:stateChange" event', async () => {
410+
const { controller, messenger } = setupController();
411+
412+
messenger.publish(
413+
'MultichainAssetsController:stateChange',
414+
{
415+
assetsMetadata: {},
416+
accountsAssets: {
417+
[mockBtcAccount.id]: [mockBtcNativeAsset],
418+
},
419+
},
420+
[],
421+
);
422+
423+
await waitForAllPromises();
424+
425+
expect(controller.state.balances[mockBtcAccount.id]).toStrictEqual(
426+
mockBalanceResult,
427+
);
428+
});
395429
});

packages/assets-controllers/src/MultichainBalancesController/MultichainBalancesController.ts

Lines changed: 61 additions & 32 deletions
Original file line numberDiff line numberDiff line change
@@ -24,8 +24,11 @@ import { HandlerType } from '@metamask/snaps-utils';
2424
import type { Json, JsonRpcRequest } from '@metamask/utils';
2525
import type { Draft } from 'immer';
2626

27-
import { NETWORK_ASSETS_MAP } from '.';
28-
import { getScopeForAccount } from './utils';
27+
import type {
28+
MultichainAssetsControllerGetStateAction,
29+
MultichainAssetsControllerState,
30+
MultichainAssetsControllerStateChangeEvent,
31+
} from '../MultichainAssetsController';
2932

3033
const controllerName = 'MultichainBalancesController';
3134

@@ -90,15 +93,18 @@ export type MultichainBalancesControllerEvents =
9093
*/
9194
type AllowedActions =
9295
| HandleSnapRequest
93-
| AccountsControllerListMultichainAccountsAction;
96+
| AccountsControllerListMultichainAccountsAction
97+
| MultichainAssetsControllerGetStateAction;
9498

9599
/**
96100
* Events that this controller is allowed to subscribe.
97101
*/
98102
type AllowedEvents =
99103
| AccountsControllerAccountAddedEvent
100104
| AccountsControllerAccountRemovedEvent
101-
| AccountsControllerAccountBalancesUpdatesEvent;
105+
| AccountsControllerAccountBalancesUpdatesEvent
106+
| MultichainAssetsControllerStateChangeEvent;
107+
102108
/**
103109
* Messenger type for the MultichainBalancesController.
104110
*/
@@ -152,18 +158,11 @@ export class MultichainBalancesController extends BaseController<
152158

153159
// Fetch initial balances for all non-EVM accounts
154160
for (const account of this.#listAccounts()) {
155-
this.updateBalance(account.id).catch((error) => {
156-
console.error(
157-
`Failed to fetch initial balance for account ${account.id}:`,
158-
error,
159-
);
160-
});
161+
// Fetching the balance is asynchronous and we cannot use `await` here.
162+
// eslint-disable-next-line no-void
163+
void this.updateBalance(account.id);
161164
}
162165

163-
this.messagingSystem.subscribe(
164-
'AccountsController:accountAdded',
165-
(account: InternalAccount) => this.#handleOnAccountAdded(account),
166-
);
167166
this.messagingSystem.subscribe(
168167
'AccountsController:accountRemoved',
169168
(account: string) => this.#handleOnAccountRemoved(account),
@@ -173,40 +172,68 @@ export class MultichainBalancesController extends BaseController<
173172
(balanceUpdate: AccountBalancesUpdatedEventPayload) =>
174173
this.#handleOnAccountBalancesUpdated(balanceUpdate),
175174
);
175+
// TODO: Maybe add a MultichainAssetsController:accountAssetListUpdated event instead of using the entire state.
176+
// Since MultichainAssetsController already listens for the AccountsController:accountAdded, we can rely in it for that event
177+
// and not listen for it also here, in this controller, since it would be redundant
178+
this.messagingSystem.subscribe(
179+
'MultichainAssetsController:stateChange',
180+
async (assetsState: MultichainAssetsControllerState) => {
181+
for (const accountId of Object.keys(assetsState.accountsAssets)) {
182+
await this.#updateBalance(
183+
accountId,
184+
assetsState.accountsAssets[accountId],
185+
);
186+
}
187+
},
188+
);
176189
}
177190

178191
/**
179192
* Updates the balances of one account. This method doesn't return
180193
* anything, but it updates the state of the controller.
181194
*
182195
* @param accountId - The account ID.
196+
* @param assets - The list of asset types for this account to upadte.
183197
*/
184-
async updateBalance(accountId: string): Promise<void> {
198+
async #updateBalance(
199+
accountId: string,
200+
assets: CaipAssetType[],
201+
): Promise<void> {
185202
try {
186203
const account = this.#getAccount(accountId);
187204

188205
if (account.metadata.snap) {
189-
const scope = getScopeForAccount(account);
190-
const assetTypes = NETWORK_ASSETS_MAP[scope];
191-
192206
const accountBalance = await this.#getBalances(
193207
account.id,
194208
account.metadata.snap.id,
195-
assetTypes,
209+
assets,
196210
);
197211

198212
this.update((state: Draft<MultichainBalancesControllerState>) => {
199213
state.balances[accountId] = accountBalance;
200214
});
201215
}
202216
} catch (error) {
217+
// FIXME: Maybe we shouldn't catch all errors here since this method is also being
218+
// used in the public methods. This means if something else uses `updateBalance` it
219+
// won't be able to catch and gets the error itself...
203220
console.error(
204221
`Failed to fetch balances for account ${accountId}:`,
205222
error,
206223
);
207224
}
208225
}
209226

227+
/**
228+
* Updates the balances of one account. This method doesn't return
229+
* anything, but it updates the state of the controller.
230+
*
231+
* @param accountId - The account ID.
232+
*/
233+
async updateBalance(accountId: string): Promise<void> {
234+
await this.#updateBalance(accountId, this.#listAccountAssets(accountId));
235+
}
236+
210237
/**
211238
* Lists the multichain accounts coming from the `AccountsController`.
212239
*
@@ -229,6 +256,21 @@ export class MultichainBalancesController extends BaseController<
229256
return accounts.filter((account) => this.#isNonEvmAccount(account));
230257
}
231258

259+
/**
260+
* Lists the accounts assets.
261+
*
262+
* @param accountId - The account ID.
263+
* @returns The list of assets for this account, returns an empty list if none.
264+
*/
265+
#listAccountAssets(accountId: string): CaipAssetType[] {
266+
// TODO: Add an action `MultichainAssetsController:getAccountAssets` maybe?
267+
const assetsState = this.messagingSystem.call(
268+
'MultichainAssetsController:getState',
269+
);
270+
271+
return assetsState.accountsAssets[accountId] ?? [];
272+
}
273+
232274
/**
233275
* Get a non-EVM account from its ID.
234276
*
@@ -261,19 +303,6 @@ export class MultichainBalancesController extends BaseController<
261303
);
262304
}
263305

264-
/**
265-
* Handles changes when a new account has been added.
266-
*
267-
* @param account - The new account being added.
268-
*/
269-
async #handleOnAccountAdded(account: InternalAccount): Promise<void> {
270-
if (!this.#isNonEvmAccount(account)) {
271-
return;
272-
}
273-
274-
await this.updateBalance(account.id);
275-
}
276-
277306
/**
278307
* Handles balance updates received from the AccountsController.
279308
*

0 commit comments

Comments
 (0)