Skip to content

Commit

Permalink
feat: add sentry context to erroneous situation callbacks (#5139)
Browse files Browse the repository at this point in the history
## Explanation

In order to better understand what's happening during erroneous
situations, we are adding with this PR an object to all
`onAccountSyncErroneousSituation` calls. This object will be used as a
Sentry context on clients.

## 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

### `@metamask/profile-sync-controller`

- **ADDED**: Add a context object to `onAccountSyncErroneousSituation`
for better error understanding


## 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
  • Loading branch information
mathieuartu authored Jan 14, 2025
1 parent adea818 commit 36d5319
Show file tree
Hide file tree
Showing 4 changed files with 162 additions and 22 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -173,9 +173,8 @@ describe('user-storage/user-storage-controller - performGetStorageAllFeatureEntr
getMetaMetricsState: () => true,
});

const result = await controller.performGetStorageAllFeatureEntries(
'notifications',
);
const result =
await controller.performGetStorageAllFeatureEntries('notifications');
mockAPI.done();
expect(result).toStrictEqual([MOCK_STORAGE_DATA]);
});
Expand Down Expand Up @@ -893,7 +892,7 @@ describe('user-storage/user-storage-controller - syncInternalAccountsWithUserSto
) => {
onAccountAdded?.();
onAccountNameUpdated?.();
onAccountSyncErroneousSituation?.('error message');
onAccountSyncErroneousSituation?.('error message', {});
getMessenger();
getUserStorageControllerInstance();
return undefined;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -161,6 +161,7 @@ type ControllerConfig = {
onAccountSyncErroneousSituation?: (
profileId: string,
situationMessage: string,
sentryContext?: Record<string, unknown>,
) => void;
};

Expand Down Expand Up @@ -864,10 +865,11 @@ export default class UserStorageController extends BaseController<
this.#config?.accountSyncing?.onAccountAdded?.(profileId),
onAccountNameUpdated: () =>
this.#config?.accountSyncing?.onAccountNameUpdated?.(profileId),
onAccountSyncErroneousSituation: (situationMessage) =>
onAccountSyncErroneousSituation: (situationMessage, sentryContext) =>
this.#config?.accountSyncing?.onAccountSyncErroneousSituation?.(
profileId,
situationMessage,
sentryContext,
),
},
{
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -298,28 +298,32 @@ describe('user-storage/account-syncing/controller-integration - syncInternalAcco

describe('handles corrupted user storage gracefully', () => {
const arrangeMocksForBogusAccounts = async () => {
const accountsList =
MOCK_INTERNAL_ACCOUNTS.ONE_DEFAULT_NAME as InternalAccount[];
const { messengerMocks, config, options } = await arrangeMocks({
messengerMockOptions: {
accounts: {
accountsList:
MOCK_INTERNAL_ACCOUNTS.ONE_DEFAULT_NAME as InternalAccount[],
accountsList,
},
},
});

const userStorageList =
MOCK_USER_STORAGE_ACCOUNTS.TWO_DEFAULT_NAMES_WITH_ONE_BOGUS;

return {
config,
options,
messengerMocks,
accountsList,
userStorageList,
mockAPI: {
mockEndpointGetUserStorage:
await mockEndpointGetUserStorageAllFeatureEntries(
USER_STORAGE_FEATURE_NAMES.accounts,
{
status: 200,
body: await createMockUserStorageEntries(
MOCK_USER_STORAGE_ACCOUNTS.TWO_DEFAULT_NAMES_WITH_ONE_BOGUS,
),
body: await createMockUserStorageEntries(userStorageList),
},
),
mockEndpointBatchDeleteUserStorage:
Expand Down Expand Up @@ -363,20 +367,86 @@ describe('user-storage/account-syncing/controller-integration - syncInternalAcco
expect(mockAPI.mockEndpointBatchDeleteUserStorage.isDone()).toBe(true);
});

it('fires the onAccountSyncErroneousSituation callback in erroneous situations', async () => {
const onAccountSyncErroneousSituation = jest.fn();
describe('Fires the onAccountSyncErroneousSituation callback on erroneous situations', () => {
it('And logs if the final state is incorrect', async () => {
const onAccountSyncErroneousSituation = jest.fn();

const { config, options } = await arrangeMocksForBogusAccounts();
const { config, options, userStorageList, accountsList } =
await arrangeMocksForBogusAccounts();

await AccountSyncingControllerIntegrationModule.syncInternalAccountsWithUserStorage(
{
...config,
onAccountSyncErroneousSituation,
},
options,
);
await AccountSyncingControllerIntegrationModule.syncInternalAccountsWithUserStorage(
{
...config,
onAccountSyncErroneousSituation,
},
options,
);

expect(onAccountSyncErroneousSituation).toHaveBeenCalledTimes(1);
expect(onAccountSyncErroneousSituation).toHaveBeenCalledTimes(2);
expect(onAccountSyncErroneousSituation.mock.calls).toEqual([
[
'An account was present in the user storage accounts list but was not found in the internal accounts list after the sync',
{
internalAccountsList: accountsList,
internalAccountsToBeSavedToUserStorage: [],
refreshedInternalAccountsList: accountsList,
userStorageAccountsList: userStorageList,
userStorageAccountsToBeDeleted: [userStorageList[1]],
},
],
[
'Erroneous situations were found during the sync, and final state does not match the expected state',
{
finalInternalAccountsList: accountsList,
finalUserStorageAccountsList: null,
},
],
]);
});

it('And logs if the final state is correct', async () => {
const onAccountSyncErroneousSituation = jest.fn();

const { config, options, userStorageList, accountsList } =
await arrangeMocksForBogusAccounts();

await mockEndpointGetUserStorageAllFeatureEntries(
USER_STORAGE_FEATURE_NAMES.accounts,
{
status: 200,
body: await createMockUserStorageEntries([userStorageList[0]]),
},
);

await AccountSyncingControllerIntegrationModule.syncInternalAccountsWithUserStorage(
{
...config,
onAccountSyncErroneousSituation,
},
options,
);

expect(onAccountSyncErroneousSituation).toHaveBeenCalledTimes(2);
expect(onAccountSyncErroneousSituation.mock.calls).toEqual([
[
'An account was present in the user storage accounts list but was not found in the internal accounts list after the sync',
{
internalAccountsList: accountsList,
internalAccountsToBeSavedToUserStorage: [],
refreshedInternalAccountsList: accountsList,
userStorageAccountsList: userStorageList,
userStorageAccountsToBeDeleted: [userStorageList[1]],
},
],
[
'Erroneous situations were found during the sync, but final state matches the expected state',
{
finalInternalAccountsList: accountsList,
finalUserStorageAccountsList: [userStorageList[0]],
},
],
]);
});
});
});

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -96,7 +96,10 @@ type SyncInternalAccountsWithUserStorageConfig = AccountSyncingConfig & {
maxNumberOfAccountsToAdd?: number;
onAccountAdded?: () => void;
onAccountNameUpdated?: () => void;
onAccountSyncErroneousSituation?: (errorMessage: string) => void;
onAccountSyncErroneousSituation?: (
errorMessage: string,
sentryContext?: Record<string, unknown>,
) => void;
};

/**
Expand Down Expand Up @@ -141,6 +144,9 @@ export async function syncInternalAccountsWithUserStorage(
);
return;
}
// Keep a record if erroneous situations are found during the sync
// This is done so we can send the context to Sentry in case of an erroneous situation
let erroneousSituationsFound = false;

// Prepare an array of internal accounts to be saved to the user storage
const internalAccountsToBeSavedToUserStorage: InternalAccount[] = [];
Expand Down Expand Up @@ -191,8 +197,18 @@ export async function syncInternalAccountsWithUserStorage(
if (!userStorageAccount) {
// If the account was just added in the previous step, skip saving it, it's likely to be a bogus account
if (newlyAddedAccounts.includes(internalAccount)) {
erroneousSituationsFound = true;
onAccountSyncErroneousSituation?.(
'An account was added to the internal accounts list but was not present in the user storage accounts list',
{
internalAccount,
userStorageAccount,
newlyAddedAccounts,
userStorageAccountsList,
internalAccountsList,
refreshedInternalAccountsList,
internalAccountsToBeSavedToUserStorage,
},
);
continue;
}
Expand Down Expand Up @@ -295,11 +311,64 @@ export async function syncInternalAccountsWithUserStorage(
USER_STORAGE_FEATURE_NAMES.accounts,
userStorageAccountsToBeDeleted.map((account) => account.a),
);
erroneousSituationsFound = true;
onAccountSyncErroneousSituation?.(
'An account was present in the user storage accounts list but was not found in the internal accounts list after the sync',
{
userStorageAccountsToBeDeleted,
internalAccountsList,
refreshedInternalAccountsList,
internalAccountsToBeSavedToUserStorage,
userStorageAccountsList,
},
);
}

if (erroneousSituationsFound) {
const [finalUserStorageAccountsList, finalInternalAccountsList] =
await Promise.all([
getUserStorageAccountsList(options),
getInternalAccountsList(options),
]);

const doesEveryAccountInInternalAccountsListExistInUserStorageAccountsList =
finalInternalAccountsList.every((account) =>
finalUserStorageAccountsList?.some(
(userStorageAccount) => userStorageAccount.a === account.address,
),
);

// istanbul ignore next
const doesEveryAccountInUserStorageAccountsListExistInInternalAccountsList =
(finalUserStorageAccountsList?.length || 0) > maxNumberOfAccountsToAdd
? true
: finalUserStorageAccountsList?.every((account) =>
finalInternalAccountsList.some(
(internalAccount) => internalAccount.address === account.a,
),
);

const doFinalListsMatch =
doesEveryAccountInInternalAccountsListExistInUserStorageAccountsList &&
doesEveryAccountInUserStorageAccountsListExistInInternalAccountsList;

const context = {
finalUserStorageAccountsList,
finalInternalAccountsList,
};
if (doFinalListsMatch) {
onAccountSyncErroneousSituation?.(
'Erroneous situations were found during the sync, but final state matches the expected state',
context,
);
} else {
onAccountSyncErroneousSituation?.(
'Erroneous situations were found during the sync, and final state does not match the expected state',
context,
);
}
}

// We do this here and not in the finally statement because we want to make sure that
// the accounts are saved / updated / deleted at least once before we set this flag
await getUserStorageControllerInstance().setHasAccountSyncingSyncedAtLeastOnce(
Expand Down

0 comments on commit 36d5319

Please sign in to comment.