feat: remove updateIdentities and make syncIdentities private in preferences controller#3976
Merged
cryptodev-2s merged 8 commits intoFeb 27, 2024
Conversation
…erences controller
cryptodev-2s
changed the title
mikesposito
reviewed
Feb 27, 2024
Comment on lines
-308
to
-418
| it('should sync identities', () => { | ||
| const controller = setupPreferencesController(); | ||
| controller.addIdentities(['0x00', '0x01']); | ||
| controller.syncIdentities(['0x00', '0x01']); | ||
| expect(controller.state.identities['0x00'].address).toBe('0x00'); | ||
| expect(controller.state.identities['0x00'].name).toBe('Account 1'); | ||
| expect(controller.state.identities['0x00'].importTime).toBeLessThanOrEqual( | ||
| Date.now(), | ||
| ); | ||
| expect(controller.state.identities['0x01'].address).toBe('0x01'); | ||
| expect(controller.state.identities['0x01'].name).toBe('Account 2'); | ||
| expect(controller.state.identities['0x01'].importTime).toBeLessThanOrEqual( | ||
| Date.now(), | ||
| ); | ||
| controller.syncIdentities(['0x00']); | ||
| expect(controller.state.identities['0x00'].address).toBe('0x00'); | ||
| expect(controller.state.identities['0x00'].name).toBe('Account 1'); | ||
| expect(controller.state.selectedAddress).toBe('0x00'); | ||
| }); | ||
|
|
||
| it('should throw error when syncing identities with empty array', () => { | ||
| const controller = setupPreferencesController(); | ||
| expect(() => { | ||
| controller.syncIdentities([]); | ||
| }).toThrow('Expected non-empty array of addresses'); | ||
| }); | ||
|
|
||
| it('should add new identities', () => { | ||
| const controller = setupPreferencesController(); | ||
| controller.updateIdentities(['0x00', '0x01']); | ||
| expect(controller.state.identities['0x00'].address).toBe('0x00'); | ||
| expect(controller.state.identities['0x00'].name).toBe('Account 1'); | ||
| expect(controller.state.identities['0x00'].importTime).toBeLessThanOrEqual( | ||
| Date.now(), | ||
| ); | ||
| expect(controller.state.identities['0x01'].address).toBe('0x01'); | ||
| expect(controller.state.identities['0x01'].name).toBe('Account 2'); | ||
| expect(controller.state.identities['0x01'].importTime).toBeLessThanOrEqual( | ||
| Date.now(), | ||
| ); | ||
| }); | ||
|
|
||
| it('should not update existing identities', () => { | ||
| const controller = setupPreferencesController({ | ||
| options: { | ||
| state: { | ||
| identities: { '0x01': { address: '0x01', name: 'Custom name' } }, | ||
| }, | ||
| }, | ||
| }); | ||
| controller.updateIdentities(['0x00', '0x01']); | ||
| expect(controller.state.identities['0x00'].address).toBe('0x00'); | ||
| expect(controller.state.identities['0x00'].name).toBe('Account 1'); | ||
| expect(controller.state.identities['0x00'].importTime).toBeLessThanOrEqual( | ||
| Date.now(), | ||
| ); | ||
| expect(controller.state.identities['0x01'].address).toBe('0x01'); | ||
| expect(controller.state.identities['0x01'].name).toBe('Custom name'); | ||
| expect(controller.state.identities['0x01'].importTime).toBeUndefined(); | ||
| }); | ||
|
|
||
| it('should remove identities', () => { | ||
| const controller = setupPreferencesController({ | ||
| options: { | ||
| state: { | ||
| identities: { | ||
| '0x01': { address: '0x01', name: 'Account 2' }, | ||
| '0x00': { address: '0x00', name: 'Account 1' }, | ||
| }, | ||
| }, | ||
| }, | ||
| }); | ||
| controller.updateIdentities(['0x00']); | ||
| expect(controller.state.identities).toStrictEqual({ | ||
| '0x00': { address: '0x00', name: 'Account 1' }, | ||
| }); | ||
| }); | ||
|
|
||
| it('should not update selected address if it is still among identities', () => { | ||
| const controller = setupPreferencesController({ | ||
| options: { | ||
| state: { | ||
| identities: { | ||
| '0x01': { address: '0x01', name: 'Account 2' }, | ||
| '0x00': { address: '0x00', name: 'Account 1' }, | ||
| }, | ||
| selectedAddress: '0x01', | ||
| }, | ||
| }, | ||
| }); | ||
| controller.updateIdentities(['0x00', '0x01']); | ||
| expect(controller.state.selectedAddress).toBe('0x01'); | ||
| }); | ||
|
|
||
| it('should update selected address to first identity if it was removed from identities', () => { | ||
| const controller = setupPreferencesController({ | ||
| options: { | ||
| state: { | ||
| identities: { | ||
| '0x01': { address: '0x01', name: 'Account 2' }, | ||
| '0x02': { address: '0x02', name: 'Account 3' }, | ||
| '0x00': { address: '0x00', name: 'Account 1' }, | ||
| }, | ||
| selectedAddress: '0x02', | ||
| }, | ||
| }, | ||
| }); | ||
| controller.updateIdentities(['0x00', '0x01']); | ||
| expect(controller.state.selectedAddress).toBe('0x00'); | ||
| }); | ||
|
|
Member
There was a problem hiding this comment.
syncIdentities has been made private, but we still need to cover the logic in it. Perhaps we should substitute some of these test cases that has been removed, triggering what we have to test using the onKeyringStateChangeMock listener
Contributor
Author
There was a problem hiding this comment.
Actually the behaviour intended by onKeyringStateChangeMock to sync identities is already covered in theses tests https://github.com/MetaMask/core/blob/main/packages/preferences-controller/src/PreferencesController.test.ts#L42
mikesposito
reviewed
Feb 27, 2024
Gudahtt
approved these changes
Feb 27, 2024
Member
Gudahtt
left a comment
Gudahtt
left a comment
There was a problem hiding this comment.
LGTM!
For the changelog, I think we can describe both as removed. It's not relevant to users of the package whether syncIdentities still exists as a private method or not; private methods are private.
cryptodev-2s
deleted the
refacto/remove-preferences-controller-unused-methods
branch
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Explanation
After completion of #3794 and #3699, the method
syncIdentitiesis now only used internally onKeyringController:stateChangeevent andupdateIdentitiesis no longer being used.References
Changelog
@metamask@preferences-controllersyncIdentitiesis now private as it's only used internally to update state on KeyringController:stateChange event.updateIdentitieshas been removed, as it's not in use anymore.Checklist