Skip to content
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

[preferences-controller] React to keyring state changes #3794

Closed
Gudahtt opened this issue Jan 18, 2024 · 1 comment · Fixed by #3799
Closed

[preferences-controller] React to keyring state changes #3794

Gudahtt opened this issue Jan 18, 2024 · 1 comment · Fixed by #3799
Assignees
Labels

Comments

@Gudahtt
Copy link
Member

Gudahtt commented Jan 18, 2024

The PreferencesController should listen for KeyringController state changes, and react to them appropriately. Specifically it should listen for account additions and removals, and ensure the selected address always points at an address that exists.

Today it doesn't react to these event changes because it relies upon the KeyringController calling the PreferencesController methods to update state. This is error prone and makes the KeyringController more complex than we'd like, and introduces a circular dependency between the controllers that would be nice to avoid.

@Gudahtt Gudahtt added enhancement New feature or request team-wallet-framework labels Jan 18, 2024
@desi
Copy link
Contributor

desi commented Jan 18, 2024

Hey team! Please add your planning poker estimate with Zenhub @Gudahtt @kanthesha @MajorLift @mcmire

@Gudahtt Gudahtt self-assigned this Jan 18, 2024
Gudahtt added a commit that referenced this issue Jan 19, 2024
The `PreferencesController` now listens for `KeyringController` state
changes, and updates its own state in response to account removals or
additions. New accounts are added to the `identities` state and given
default labels. Removed accounts are removed from the `identities`
state, and the `selectedAddress` is updated if it was removed.

A `getDefaultKeyringState` export was added to the
`@metamask/keyring-controller` package to make it easier to write the
necessary `PreferencesController` tests.

Closes #3794
Gudahtt added a commit that referenced this issue Jan 19, 2024
The `PreferencesController` now listens for `KeyringController` state
changes, and updates its own state in response to account removals or
additions. New accounts are added to the `identities` state and given
default labels. Removed accounts are removed from the `identities`
state, and the `selectedAddress` is updated if it was removed.

The dependency between these two packages has been flipped; the
`@metamask/preferences-controller` package now depends upon
`@metamask/keyring-controller` rather than the other away around, so
that the `KeyringController` state type and state change event can be
accessed. The dependency the other way was just to get the types for
the four `PreferencesController` methods that are passed into the
`KeyringController` constructor. These types were easily inlined, and
these methods will soon be removed anyway.

A `getDefaultKeyringState` export was added to the
`@metamask/keyring-controller` package to make it easier to write the
necessary `PreferencesController` tests.

Closes #3794
Gudahtt added a commit that referenced this issue Jan 19, 2024
The `PreferencesController` now listens for `KeyringController` state
changes, and updates its own state in response to account removals or
additions. New accounts are added to the `identities` state and given
default labels. Removed accounts are removed from the `identities`
state, and the `selectedAddress` is updated if it was removed.

The dependency between these two packages has been flipped; the
`@metamask/preferences-controller` package now depends upon
`@metamask/keyring-controller` rather than the other away around, so
that the `KeyringController` state type and state change event can be
accessed. The dependency the other way was just to get the types for
the four `PreferencesController` methods that are passed into the
`KeyringController` constructor. These types were easily inlined, and
these methods will soon be removed anyway.

A `getDefaultKeyringState` export was added to the
`@metamask/keyring-controller` package to make it easier to write the
necessary `PreferencesController` tests.

Closes #3794
Gudahtt added a commit that referenced this issue Jan 23, 2024
## Explanation

The `PreferencesController` now listens for `KeyringController` state
changes, and updates its own state in response to account removals or
additions. New accounts are added to the `identities` state and given
default labels. Removed accounts are removed from the `identities`
state, and the `selectedAddress` is updated if it was removed.

The dependency between these two packages has been flipped; the
`@metamask/preferences-controller` package now depends upon
`@metamask/keyring-controller` rather than the other away around, so
that the `KeyringController` state type and state change event can be
accessed. The dependency the other way was just to get the types for the
four `PreferencesController` methods that are passed into the
`KeyringController` constructor. These types were easily inlined, and
these methods will soon be removed anyway.

A `getDefaultKeyringState` export was added to the
`@metamask/keyring-controller` package to make it easier to write the
necessary `PreferencesController` tests.

## References

Closes #3794

## Changelog

### `@metamask/keyring-controller`

#### Added
- Add `getDefaultKeyringState` function

#### Removed
- Remove `peerDependency` and `devDependency` upon
`@metamask/preferences-controller`
- This dependency was just used to access the types of four methods.
Those types are now inlined instead.

### `@metamask/preferences-controller`

#### Changed
- **BREAKING:** Keep `PreferencesController` state synchronized with
`KeyringController` state
- The `KeyringController:stateChange` event is now required by the
`PreferencesController` messenger, which is a breaking change.
- The package `@metamask/keyring-controller` has been added as a
`peerDependency` and as a `devDependency`, which is a breaking change.
- Previously the state was synchronized manually by calling
`syncIdentities` or `updateIdentities`. Calling these methods is no
longer required.

## 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
cryptodev-2s added a commit that referenced this issue Feb 27, 2024
…erences controller (#3976)

## Explanation

After completion of #3794 and
#3699, the method
`syncIdentities` is now only used internally on
`KeyringController:stateChange` event and `updateIdentities` is no
longer being used.

## References

* Fixes #3795

## Changelog

### `@metamask@preferences-controller`

- **REMOVED**: `syncIdentities` is now private as it's only used
internally to update state on KeyringController:stateChange event.
- **REMOVED**: `updateIdentities` has been removed, as it's not in use
anymore.

## 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
- [ ] I've highlighted breaking changes using the "BREAKING" category
above as appropriate
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants