-
-
Notifications
You must be signed in to change notification settings - Fork 1k
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
chore: Accounts/PreferencesController state syncing #9564
base: main
Are you sure you want to change the base?
Conversation
CLA Signature Action: All authors have signed the CLA. You may need to manually re-run the blocking PR check if it doesn't pass in a few minutes. |
855f642
to
fc37b9f
Compare
daa0d74
to
dd5a939
Compare
dd5a939
to
b95cef4
Compare
} from '@metamask/preferences-controller'; | ||
|
||
export function syncSelectedAddress( | ||
preferencesState: PreferencesState, |
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.
How about just passing the selectedAddress
here? Since you don't really need the entire state of the PreferencesController
this.controllerMessenger.subscribe( | ||
'PreferencesController:stateChange', | ||
(preferencesState: PreferencesState) => { | ||
syncSelectedAddress( | ||
preferencesState, | ||
() => accountsController, | ||
() => preferencesController, | ||
); | ||
syncAccountName(preferencesState, () => accountsController); | ||
}, | ||
); |
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.
Should we also have a subscription to the other direction too?
} | ||
|
||
export function syncAccountName( | ||
preferencesState: PreferencesState, |
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.
Similarly here since we're just using identities
const preferencesController = getPreferencesController(); | ||
const selectedAddressFromPreferences = preferencesState.selectedAddress; | ||
const { selectedAccount: currentAccountId, accounts: internalAccounts } = | ||
accountsController.state.internalAccounts; |
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.
is there a way to get internalAccounts
without accessing the state directly?
for (const accountId in accountsController.state.internalAccounts.accounts) { | ||
const account = | ||
accountsController.state.internalAccounts.accounts[accountId]; |
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.
Similar comment here about getting internalAccounts
without the need to access the state
Description
The methods
Engine.setAccountLabel
andEngine.setSelectedAddress
were added in in these two ( one and two) Prs. These mehtods are used to keep the shared state between the AccountsController and PreferenfesController in sync. However is not a good method since it is prone to error and requires a lot of context. This pr removes these methods in favour of automated state sync listeners.Listen for state change on the PreferencesController and automatically update the relevant state inside the accounts controller.
These listeners will ensure that the selected address and the account names stay in sync.
Related issues
Fixes: https://github.com/MetaMask/accounts-planning/issues/445
Manual testing steps
Screenshots/Recordings
Before
After
Pre-merge author checklist
Pre-merge reviewer checklist