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

Upgrade PreferencesController to BaseControllerV2 #3708

Closed
mcmire opened this issue Jan 3, 2024 · 0 comments · Fixed by #3713
Closed

Upgrade PreferencesController to BaseControllerV2 #3708

mcmire opened this issue Jan 3, 2024 · 0 comments · Fixed by #3713
Assignees
Labels

Comments

@mcmire
Copy link
Contributor

mcmire commented Jan 3, 2024

There should not be too many changes necessary to upgrade this controller to BaseControllerV2, as it does not emit any events. Mainly we would just need to upgrade places in which state is updated to use BaseControllerV2's syntax.

@Gudahtt Gudahtt self-assigned this Jan 4, 2024
@Gudahtt Gudahtt added the enhancement New feature or request label Jan 4, 2024
Gudahtt added a commit that referenced this issue Jan 4, 2024
The `PreferencesController` has been migrated to `BaseControllerV2`. As
part of this migration, the unused `name` state property has also been
removed.

Closes #3708
Gudahtt added a commit that referenced this issue Jan 4, 2024
The `PreferencesController` has been migrated to `BaseControllerV2`. As
part of this migration, the unused `name` state property has also been
removed.

Closes #3708
Gudahtt added a commit that referenced this issue Jan 4, 2024
The `PreferencesController` has been migrated to `BaseControllerV2`. As
part of this migration, the unused `name` state property has also been
removed.

Closes #3708
Gudahtt added a commit that referenced this issue Jan 4, 2024
## Explanation

The `PreferencesController` types are now declared as types rather than
interfaces, in accordance with our conventions, and they are now fully
documented. The state properties have also been alphabetized so that
they are easier to maintain.

Additionally, the old "ContactEntry" type has been renamed to
"Identity", which is a more accurate name for what it is being used for
here.

## References

This was an effort to reduce the scope of the BaseControllerV2
migration, tracked by #3708

## Changelog

### `@metamask/preferences-controller`

#### Changed
- **BREAKING**: Replace `ContactEntry` interface with `Identity` type
- **BREAKING:** Convert `PreferencesState` from an interface to a type

## 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
Gudahtt added a commit that referenced this issue Jan 5, 2024
The `AccountTrackerController` tests have been updated to remove
instances of the `PreferencesController`. Mocks are now used instead of
the real controller.

It was useful to have a reference in tests to the default
`PreferencesController` state, so that has been added as an export.

Relates to #3708
Gudahtt added a commit that referenced this issue Jan 5, 2024
The `AccountTrackerController` tests have been updated to remove
instances of the `PreferencesController`. Mocks are now used instead of
the real controller.

It was useful to have a reference in tests to the default
`PreferencesController` state, so that has been added as an export.

Relates to #3708
Gudahtt added a commit that referenced this issue Jan 5, 2024
The `AccountTrackerController` tests have been updated to remove
instances of the `PreferencesController`. Mocks are now used instead of
the real controller.

It was useful to have a reference in tests to the default
`PreferencesController` state, so I added a `getDefaultState` static
method. This was added as a method rather than an export to protect
against accidental mutations.

Relates to #3708
Gudahtt added a commit that referenced this issue Jan 5, 2024
The `AssetsContractController` tests now use mocks rather than real
`PreferencesController` instances. This should make the tests easier to
read, and it decouples the `PreferencesController` further from the
asset controllers.

Relates to #3708
Gudahtt added a commit that referenced this issue Jan 5, 2024
The `NftController` tests now use mocks instead of real
`AssetsContractController` and `PreferencesController` instances. This
should make the tests easier to read, and it further decouples them
from those controllers.

Relates to #3708
Gudahtt added a commit that referenced this issue Jan 5, 2024
The `NftController` tests now use mocks instead of real
`AssetsContractController` and `PreferencesController` instances. This
should make the tests easier to read, and it further decouples them
from those controllers.

Additionally, the `setupController` helper method has been updated to
accept a partial set of controller options rather than custom options.

Relates to #3708
Gudahtt added a commit that referenced this issue Jan 5, 2024
The `NftController` tests now use mocks instead of real
`AssetsContractController` and `PreferencesController` instances. This
should make the tests easier to read, and it further decouples them
from those controllers.

Additionally, the `setupController` helper method has been updated to
accept a partial set of controller options rather than custom options.

Relates to #3708
Gudahtt added a commit that referenced this issue Jan 5, 2024
The `NftController` tests now use mocks instead of real
`AssetsContractController` and `PreferencesController` instances. This
should make the tests easier to read, and it further decouples them
from those controllers.

Additionally, the `setupController` helper method has been updated to
accept a partial set of controller options rather than custom options.

Relates to #3708
Gudahtt added a commit that referenced this issue Jan 5, 2024
The `NftController` tests now use mocks instead of real
`AssetsContractController` and `PreferencesController` instances. This
should make the tests easier to read, and it further decouples them
from those controllers.

Additionally, the `setupController` helper method has been updated to
accept a partial set of controller options rather than custom options.

Relates to #3708
Gudahtt added a commit that referenced this issue Jan 5, 2024
The `NftController` tests now use mocks instead of real
`AssetsContractController` and `PreferencesController` instances. This
should make the tests easier to read, and it further decouples them
from those controllers.

Additionally, the `setupController` helper method has been updated to
accept a partial set of controller options rather than custom options.

Relates to #3708
Gudahtt added a commit that referenced this issue Jan 5, 2024
The `NftController` tests now use mocks instead of real
`AssetsContractController` and `PreferencesController` instances. This
should make the tests easier to read, and it further decouples them
from those controllers.

Additionally, the `setupController` helper method has been updated to
accept a partial set of controller options rather than custom options.

Relates to #3708
Gudahtt added a commit that referenced this issue Jan 5, 2024
The `NftController` tests now use mocks instead of real
`AssetsContractController` and `PreferencesController` instances. This
should make the tests easier to read, and it further decouples them
from those controllers.

Additionally, the `setupController` helper method has been updated to
accept a partial set of controller options rather than custom options.

Relates to #3708
Gudahtt added a commit that referenced this issue Jan 8, 2024
…ts and add `getDefaultPreferencesState` (#3736)

## Explanation

The `AccountTrackerController` tests have been updated to remove
instances of the `PreferencesController`. Mocks are now used instead of
the real controller.

It was useful to have a reference in tests to the default
`PreferencesController` state, so I added a `getDefaultPreferencesState`
function. This was added as a function to protect against accidental
mutations.

## References

Relates to #3708

## Changelog

### `@metamask/preferences-controller`

#### Added
- Add `getDefaultPreferencesState` function

## 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
Gudahtt added a commit that referenced this issue Jan 8, 2024
The `AssetsContractController` tests now use mocks rather than real
`PreferencesController` instances. This should make the tests easier to
read, and it decouples the `PreferencesController` further from the
asset controllers.

Relates to #3708
Gudahtt added a commit that referenced this issue Jan 8, 2024
The `NftController` tests now use mocks instead of real
`AssetsContractController` and `PreferencesController` instances. This
should make the tests easier to read, and it further decouples them
from those controllers.

Additionally, the `setupController` helper method has been updated to
accept a partial set of controller options rather than custom options.

Relates to #3708
Gudahtt added a commit that referenced this issue Jan 8, 2024
The `NftDetectionController` tests have been refactored to use mocks
rather than full `AssetsContractController`, `PreferencesController`,
and `NftController` references. The `withController` pattern has been
introduced as well, to simplify the controller setup and ensure all
polling has stopped after each test.

A few tests were found to be testing functionality that was internal
to the `NftController`, so they have been removed. Coverage for the
`NftDetectionController` remains unchanged.

Relates to #3708
Gudahtt added a commit that referenced this issue Jan 8, 2024
The `TokenBalancesController` tests have been refactored to use mocks
instead of `NetworkController` and `PreferencesController` instances.
This should make the tests easier to read, and it decouples the tests
further from the other controllers.

Relates to #3708
Gudahtt added a commit that referenced this issue Jan 8, 2024
The `TokenDetectionController` tests have been refactored to use mocks
instead of real controller instances, and to use the `withController`
pattern to simplify controller setup and ensure that polling has
stopped after each test.

A `getDefaultTokenListState` function has been added to the
`TokenListController` to enable us to use that default state in
tests.

Relates to #3708
Gudahtt added a commit that referenced this issue Jan 8, 2024
The `TokenDetectionController` tests have been refactored to use mocks
instead of real controller instances, and to use the `withController`
pattern to simplify controller setup and ensure that polling has
stopped after each test.

A `getDefaultTokenListState` function has been added to the
`TokenListController` to enable us to use that default state in
tests.

Relates to #3708
Gudahtt added a commit that referenced this issue Jan 8, 2024
The `TokensController` tests have been refactored to use mocks instead
of a real `PreferencesController` instance. This should make the tests
easier to read, and it will decouple them from the
`PreferencesController`.

Relates to #3708
Gudahtt added a commit that referenced this issue Jan 8, 2024
…ts (#3737)

## Explanation

The `AssetsContractController` tests now use mocks rather than real
`PreferencesController` instances. This should make the tests easier to
read, and it decouples the `PreferencesController` further from the
asset controllers.

## References

Relates to #3708

## Changelog

No changes

## 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
Gudahtt added a commit that referenced this issue Jan 8, 2024
The `NftController` tests now use mocks instead of real
`AssetsContractController` and `PreferencesController` instances. This
should make the tests easier to read, and it further decouples them
from those controllers.

Additionally, the `setupController` helper method has been updated to
accept a partial set of controller options rather than custom options.

Relates to #3708
Gudahtt added a commit that referenced this issue Jan 8, 2024
## Explanation

The `NftController` tests now use mocks instead of real
`AssetsContractController` and `PreferencesController` instances. This
should make the tests easier to read, and it further decouples them from
those controllers.

## References

Relates to #3708

## Changelog

No changes

## 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
Gudahtt added a commit that referenced this issue Jan 8, 2024
The `TokensController` tests have been refactored to use mocks instead
of a real `PreferencesController` instance. This should make the tests
easier to read, and it will decouple them from the
`PreferencesController`.

Relates to #3708
Gudahtt added a commit that referenced this issue Jan 8, 2024
## Explanation

The `TokensController` tests have been refactored to use mocks instead
of a real `PreferencesController` instance. This should make the tests
easier to read, and it will decouple them from the
`PreferencesController`.

## References

Relates to #3708

## Changelog

No changes

## 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
Gudahtt added a commit that referenced this issue Jan 8, 2024
The `TokenBalancesController` tests have been refactored to use mocks
instead of `NetworkController` and `PreferencesController` instances.
This should make the tests easier to read, and it decouples the tests
further from the other controllers.

Relates to #3708
Gudahtt added a commit that referenced this issue Jan 8, 2024
#3743)

## Explanation

The `TokenBalancesController` tests have been refactored to use mocks
instead of `NetworkController` and `PreferencesController` instances.
This should make the tests easier to read, and it decouples the tests
further from the other controllers.

## References

Relates to #3708

## Changelog

### `@metamask/assets-controllers`

#### Added
- Add `getDefaultTokensState` function to the `TokensController`

## 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
Gudahtt added a commit that referenced this issue Jan 8, 2024
The `NftDetectionController` tests have been refactored to use mocks
rather than full `AssetsContractController`, `PreferencesController`,
and `NftController` references. The `withController` pattern has been
introduced as well, to simplify the controller setup and ensure all
polling has stopped after each test.

A few tests were found to be testing functionality that was internal
to the `NftController`, so they have been removed. Coverage for the
`NftDetectionController` remains unchanged.

Relates to #3708
Gudahtt added a commit that referenced this issue Jan 8, 2024
The `TokenDetectionController` tests have been refactored to use mocks
instead of real controller instances, and to use the `withController`
pattern to simplify controller setup and ensure that polling has
stopped after each test.

A `getDefaultTokenListState` function has been added to the
`TokenListController` to enable us to use that default state in
tests.

Relates to #3708
Gudahtt added a commit that referenced this issue Jan 8, 2024
…#3742)

## Explanation

The `NftDetectionController` tests have been refactored to use mocks
rather than full `AssetsContractController`, `PreferencesController`,
and `NftController` references. The `withController` pattern has been
introduced as well, to simplify the controller setup and ensure all
polling has stopped after each test.

A few tests were found to be testing functionality that was internal to
the `NftController`, so they have been removed. Coverage for the
`NftDetectionController` remains unchanged.

## References

Relates to #3708

## Changelog

### `@metamask/assets-controllers`

#### Added
- Add `getDefaultNftState` function to the `NftController`

## 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
Gudahtt added a commit that referenced this issue Jan 8, 2024
The `TokenDetectionController` tests have been refactored to use mocks
instead of real controller instances, and to use the `withController`
pattern to simplify controller setup and ensure that polling has
stopped after each test.

A `getDefaultTokenListState` function has been added to the
`TokenListController` to enable us to use that default state in
tests.

Relates to #3708
Gudahtt added a commit that referenced this issue Jan 8, 2024
…ts (#3744)

## Explanation

The `TokenDetectionController` tests have been refactored to use mocks
instead of real controller instances, and to use the `withController`
pattern to simplify controller setup and ensure that polling has stopped
after each test.

A `getDefaultTokenListState` function has been added to the
`TokenListController` to enable us to use that default state in tests.

One test was added to the `TokensController` tests to prevent a coverage
drop (it was previously being tested indirectly via the
`TokenDetectionController` tests).

## References

Relates to #3708

## Changelog

### `@metamask/assets-controllers`

#### Added
- Add `getDefaultTokenListState` function to `TokenListController

## 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
Gudahtt added a commit that referenced this issue Jan 8, 2024
The `PreferencesController` has been migrated to `BaseControllerV2`. As
part of this migration, the unused `name` state property has also been
removed.

Closes #3708
Gudahtt added a commit that referenced this issue Jan 8, 2024
The `PreferencesController` has been migrated to `BaseControllerV2`. As
part of this migration, the unused `name` state property has also been
removed.

Closes #3708
Gudahtt added a commit that referenced this issue Jan 8, 2024
The `PreferencesController` has been migrated to `BaseControllerV2`. As
part of this migration, the unused `name` state property has also been
removed.

Closes #3708
Gudahtt added a commit that referenced this issue Jan 8, 2024
The `PreferencesController` has been migrated to `BaseControllerV2`. As
part of this migration, the unused `name` state property has also been
removed.

Closes #3708
Gudahtt added a commit that referenced this issue Jan 9, 2024
The `PreferencesController` has been migrated to `BaseControllerV2`. As
part of this migration, the unused `name` state property has also been
removed.

Closes #3708
Gudahtt added a commit that referenced this issue Jan 9, 2024
The `PreferencesController` has been migrated to `BaseControllerV2`. As
part of this migration, the unused `name` state property has also been
removed.

Closes #3708
Gudahtt added a commit that referenced this issue Jan 9, 2024
## Explanation

The `PreferencesController` has been migrated to `BaseControllerV2`. As
part of this migration, the unused `name` state property has also been
removed.

## References

Closes #3708

## Changelog

### `@metamask/preferences-controller`

#### Changed
- **BREAKING:** Convert to `BaseControllerV2`
- The constructor parameters have changed; rather than accepting an
empty "config" parameter and a "state" parameter, there is now just a
single object for all constructor arguments. This object has a mandatory
`messenger` and an optional `state` property.
- Additional type exports have been added for the controller messenger
and associated types

## 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
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