-
-
Notifications
You must be signed in to change notification settings - Fork 213
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.
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
feat: add a canSync
check for account syncing
#4690
Conversation
try { | ||
this.#assertProfileSyncingEnabled(); | ||
|
||
return ( | ||
this.#accounts.isAccountSyncingEnabled && this.#auth.isAuthEnabled() | ||
); | ||
} catch { | ||
return false; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep fair.
The assert seems to just check state
core/packages/profile-sync-controller/src/controllers/user-storage/UserStorageController.ts
Lines 638 to 644 in 6cf64c6
#assertProfileSyncingEnabled(): void { | |
if (!this.state.isProfileSyncingEnabled) { | |
throw new Error( | |
`${controllerName}: Unable to call method, user is not authenticated`, | |
); | |
} | |
} |
We might be able just create a safe one of these or copy the line.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the try/catch is still valid, since not sure what this.#auth.isAuthEnabled
would call under the hood or in the future (if it changes)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this.#auth.isAuthEnabled
checks the state of AuthenticationController
in order to get isSignedIn
.
I agree, I don't think it's 100% ideal to be honest, and we're again going to be checking UI-side if all of this is enabled. I'll look into it when I have a bit less on my plate to kind of centralize these checks.
Explanation
This PR ensures we don't fire account syncing if the conditions are not entirely met.
References
Changelog
@metamask/profile-sync-controller
canSync
check to account syncingChecklist