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

feat: Keep PreferencesController in sync with keyring state #3799

Merged
merged 8 commits into from
Jan 23, 2024

Conversation

Gudahtt
Copy link
Member

@Gudahtt Gudahtt commented Jan 19, 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

  • I've updated the test suite for new or updated code as appropriate
  • 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

@Gudahtt Gudahtt force-pushed the keep-preferences-controller-in-sync-with-keyrings branch from f628c6e to 656be76 Compare January 19, 2024 04:10
@Gudahtt Gudahtt marked this pull request as ready for review January 19, 2024 04:17
@Gudahtt Gudahtt requested a review from a team as a code owner January 19, 2024 04:17
legobeat
legobeat previously approved these changes Jan 19, 2024
Copy link
Contributor

@legobeat legobeat left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Copy link
Contributor

@mcmire mcmire left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One question, but otherwise looks good.

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 Gudahtt force-pushed the keep-preferences-controller-in-sync-with-keyrings branch from bd4819d to f8b7715 Compare January 19, 2024 19:46
mikesposito
mikesposito previously approved these changes Jan 22, 2024
Copy link
Member

@mikesposito mikesposito left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

…roller-in-sync-with-keyrings

* origin/main:
  chore(deps): bump @metamask/eth-keyring-controller from 17.0.0 to 17.0.1 (#3805)
  fix: custody keyring name (#3803)
  chore: update dependencies for `@metamask/accounts-controller` (#3747)
  fix: quick succession of submit password causing Accounts Controller state to be cleared (#3802)
  feat: add methods to support ERC-4337 accounts (#3602)
  feat: add getAccount action to AccountsController (#1892)
@Gudahtt
Copy link
Member Author

Gudahtt commented Jan 22, 2024

Merged in main to resolve conflicts

@Gudahtt Gudahtt force-pushed the keep-preferences-controller-in-sync-with-keyrings branch from abdaeea to ee153a6 Compare January 22, 2024 15:32
@Gudahtt Gudahtt force-pushed the keep-preferences-controller-in-sync-with-keyrings branch from ee153a6 to 1ee4f74 Compare January 22, 2024 15:33
mikesposito
mikesposito previously approved these changes Jan 22, 2024
legobeat
legobeat previously approved these changes Jan 23, 2024
@Gudahtt Gudahtt dismissed stale reviews from legobeat and mikesposito via e1a92c9 January 23, 2024 15:07
@Gudahtt
Copy link
Member Author

Gudahtt commented Jan 23, 2024

Updated to use new latest keyring-controller version and bump lockfile

Copy link
Contributor

@MajorLift MajorLift left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good!

@Gudahtt Gudahtt merged commit 7b139dc into main Jan 23, 2024
136 checks passed
@Gudahtt Gudahtt deleted the keep-preferences-controller-in-sync-with-keyrings branch January 23, 2024 15:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[preferences-controller] React to keyring state changes
5 participants