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

Migrate to new CurrencyRateController #11005

Merged
merged 1 commit into from
May 20, 2021
Merged

Conversation

Gudahtt
Copy link
Member

@Gudahtt Gudahtt commented May 6, 2021

The CurrencyRateController has been migrated to the BaseControllerV2 API, which includes various API changes. These changes include:

  • The constructor now expects to be passed a RestrictedControllerMessenger.
  • State changes are subscribed to via the ControllerMessenger now, rather than via a subscribe function.
  • The state and configration are passed in as one "options" object, rather than as two separate parameters
  • The polling needs to be started explicitly by calling start. It can be stopped and started on-demand now as well.
  • Changing the current currency or native currency will now throw an error if we fail to update the conversion rate.

Manual testing instructions:

  • Check that fiat is shown correctly when the extension starts
  • Check that fiat values are shown correctly after the currency is changed in settings.
  • Check that fiat values are shown correctly after switching networks

Base automatically changed from remove-set-current-fiat-action to develop May 7, 2021 14:02
@Gudahtt Gudahtt force-pushed the update-currency-rate-controller branch 3 times, most recently from 3aa7d1e to 43420f4 Compare May 7, 2021 14:28
@metamaskbot
Copy link
Collaborator

Builds ready [43420f4]
Page Load Metrics (621 ± 54 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint47736084
domContentLoaded39689061911354
load40489162111254
domInteractive39588961811354

@Gudahtt Gudahtt force-pushed the update-currency-rate-controller branch 2 times, most recently from 01dc7cb to f2762f9 Compare May 7, 2021 19:06
@Gudahtt
Copy link
Member Author

Gudahtt commented May 7, 2021

Depends upon #11009, and upon a new major release of @metamask/controllers

@Gudahtt Gudahtt changed the base branch from develop to replace-has-own-property-calls May 7, 2021 19:07
@metamaskbot
Copy link
Collaborator

Builds ready [f2762f9]
Page Load Metrics (643 ± 41 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint458763126
domContentLoaded4297666428641
load4317676438641
domInteractive4297666418641

Base automatically changed from replace-has-own-property-calls to develop May 11, 2021 16:39
@Gudahtt Gudahtt mentioned this pull request May 18, 2021
@Gudahtt Gudahtt force-pushed the update-currency-rate-controller branch from f2762f9 to 8431f3f Compare May 18, 2021 21:03
@metamaskbot
Copy link
Collaborator

Builds ready [8431f3f]
Page Load Metrics (527 ± 45 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint44696063
domContentLoaded3746105269545
load3756125279545
domInteractive3746105259545

@Gudahtt Gudahtt force-pushed the update-currency-rate-controller branch from 8431f3f to 3379752 Compare May 19, 2021 17:07
@metamaskbot
Copy link
Collaborator

Builds ready [3379752]
Page Load Metrics (618 ± 37 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint448261126
domContentLoaded3897056177837
load3907056187837
domInteractive3887046177837

@metamaskbot
Copy link
Collaborator

Builds ready [3379752]
Page Load Metrics (621 ± 40 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint478064105
domContentLoaded4307356208440
load4327406218440
domInteractive4307346198440

The CurrencyRateController has been migrated to the BaseControllerV2
API, which includes various API changes. These changes include:
* The constructor now expects to be passed a
`RestrictedControllerMessenger`.
* State changes are subscribed to via the `ControllerMessenger` now,
rather than via a `subscribe` function.
* The state and configration are passed in as one "options" object,
rather than as two separate parameters
* The polling needs to be started explicitly by calling `start`. It
can be stopped and started on-demand now as well.
* Changing the current currency or native currency will now throw an
error if we fail to update the conversion rate.

The `ComposableObservableStore` has been updated to accomodate these
new types of controllers. The constructor has been updated to use an
options bag pattern as well, to make the addition of the new required
`controllerMessenger` parameter a bit less unweildly.
@Gudahtt Gudahtt force-pushed the update-currency-rate-controller branch from 3379752 to 02e06c4 Compare May 19, 2021 19:33
@metamaskbot
Copy link
Collaborator

Builds ready [02e06c4]
Page Load Metrics (639 ± 45 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint477761105
domContentLoaded3438396379445
load3458406399445
domInteractive3438396379445

@Gudahtt Gudahtt marked this pull request as ready for review May 19, 2021 19:53
@Gudahtt Gudahtt requested a review from a team as a code owner May 19, 2021 19:53
@Gudahtt Gudahtt requested a review from ryanml May 19, 2021 19:53
@Gudahtt Gudahtt merged commit 5009cea into develop May 20, 2021
@Gudahtt Gudahtt deleted the update-currency-rate-controller branch May 20, 2021 02:57
@github-actions github-actions bot locked and limited conversation to collaborators May 20, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants