Skip to content

Commit

Permalink
feat: add a canSync check for account syncing (#4690)
Browse files Browse the repository at this point in the history
## Explanation

This PR ensures we don't fire account syncing if the conditions are not
entirely met.

## References

## Changelog

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

- **ADDED**: Add a `canSync` check to account syncing

## 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
  • Loading branch information
mathieuartu authored Sep 11, 2024
1 parent 6cf64c6 commit 6f716b6
Show file tree
Hide file tree
Showing 2 changed files with 25 additions and 14 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -388,7 +388,7 @@ describe('user-storage/user-storage-controller - enableProfileSyncing() tests',
});

describe('user-storage/user-storage-controller - syncInternalAccountsWithUserStorage() tests', () => {
it('rejects if UserStorage is not enabled', async () => {
it('returns void if UserStorage is not enabled', async () => {
const arrangeMocks = async () => {
return {
messengerMocks: mockUserStorageMessenger(),
Expand All @@ -409,9 +409,9 @@ describe('user-storage/user-storage-controller - syncInternalAccountsWithUserSto
},
});

await expect(
controller.syncInternalAccountsWithUserStorage(),
).rejects.toThrow(expect.any(Error));
await controller.syncInternalAccountsWithUserStorage();

expect(messengerMocks.mockAccountsListAccounts).not.toHaveBeenCalled();
});

it('returns void if account syncing feature flag is disabled', async () => {
Expand Down Expand Up @@ -1083,7 +1083,7 @@ describe('user-storage/user-storage-controller - syncInternalAccountsWithUserSto
});

describe('user-storage/user-storage-controller - saveInternalAccountToUserStorage() tests', () => {
it('rejects if UserStorage is not enabled', async () => {
it('returns void if UserStorage is not enabled', async () => {
const arrangeMocks = async () => {
return {
messengerMocks: mockUserStorageMessenger(),
Expand All @@ -1103,11 +1103,13 @@ describe('user-storage/user-storage-controller - saveInternalAccountToUserStorag
},
});

await expect(
controller.saveInternalAccountToUserStorage(
MOCK_INTERNAL_ACCOUNTS.ONE[0].address,
),
).rejects.toThrow(expect.any(Error));
await controller.saveInternalAccountToUserStorage(
MOCK_INTERNAL_ACCOUNTS.ONE[0].address,
);

expect(
messengerMocks.mockAccountsGetAccountByAddress,
).not.toHaveBeenCalled();
});

it('returns void if account syncing feature flag is disabled', async () => {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -266,6 +266,17 @@ export default class UserStorageController extends BaseController<
// We will remove this once the feature will be released
isAccountSyncingEnabled: false,
isAccountSyncingInProgress: false,
canSync: () => {
try {
this.#assertProfileSyncingEnabled();

return (
this.#accounts.isAccountSyncingEnabled && this.#auth.isAuthEnabled()
);
} catch {
return false;
}
},
setupAccountSyncingSubscriptions: () => {
this.messagingSystem.subscribe(
'AccountsController:accountAdded',
Expand Down Expand Up @@ -718,13 +729,11 @@ export default class UserStorageController extends BaseController<
* It will add new accounts to the internal accounts list, update/merge conflicting names and re-upload the results in some cases to the user storage.
*/
async syncInternalAccountsWithUserStorage(): Promise<void> {
if (!this.#accounts.isAccountSyncingEnabled) {
if (!this.#accounts.canSync()) {
return;
}

try {
this.#assertProfileSyncingEnabled();

this.#accounts.isAccountSyncingInProgress = true;

const userStorageAccountsList =
Expand Down Expand Up @@ -858,7 +867,7 @@ export default class UserStorageController extends BaseController<
* @param address - The address of the internal account to save
*/
async saveInternalAccountToUserStorage(address: string): Promise<void> {
if (!this.#accounts.isAccountSyncingEnabled) {
if (!this.#accounts.canSync()) {
return;
}

Expand Down

0 comments on commit 6f716b6

Please sign in to comment.