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

Added getPersistentState support for V2 controllers persist metadata #12007

Merged
merged 1 commit into from
Sep 6, 2021

Conversation

shanejonas
Copy link
Contributor

@shanejonas shanejonas commented Sep 2, 2021

This PR adds getPersistentState support for BaseController V2 to provide persist metadata to localstorage of the extension

@shanejonas shanejonas requested a review from a team as a code owner September 2, 2021 19:38
@github-actions
Copy link
Contributor

github-actions bot commented Sep 2, 2021

CLA Signature Action: All authors have signed the CLA. You may need to manually re-run the blocking PR check if it doesn't pass in a few minutes.

@shanejonas shanejonas closed this Sep 2, 2021
@shanejonas shanejonas reopened this Sep 2, 2021
@github-actions github-actions bot locked and limited conversation to collaborators Sep 2, 2021
@shanejonas shanejonas closed this Sep 2, 2021
@shanejonas shanejonas reopened this Sep 2, 2021
@shanejonas shanejonas closed this Sep 2, 2021
@shanejonas shanejonas reopened this Sep 2, 2021
@metamaskbot
Copy link
Collaborator

Builds ready [000d1c5]
Page Load Metrics (509 ± 47 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint13966348112058
domContentLoaded36165949010149
load3836665099747
domInteractive36065949010149

@metamaskbot
Copy link
Collaborator

Builds ready [000d1c5]
Page Load Metrics (509 ± 47 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint13966348112058
domContentLoaded36165949010149
load3836665099747
domInteractive36065949010149

@Gudahtt
Copy link
Member

Gudahtt commented Sep 3, 2021

I've scanned through all of our BaseControllerV2-based controllers to get a sense of what the impact of this will be.

  • ApprovalController: Not persisted, no impact
  • CurrencyRateController: pendingCurrentCurrency and pendingNativeCurrency will now no longer be stored. Huh. The fact that they were persisted in the first place sounds like a bug 🤔. In the right conditions, this could show the wrong conversion rate temporarily upon startup. But it's a self-correcting bug, so not a huge deal. And this PR fixes it anyway.
  • TokenListController: It's all persisted now, and persist is true for all state properties, so no impact.
  • GasFeeController: It's all persisted now, and persist is true for all state properties, so no impact.

Copy link
Member

@Gudahtt Gudahtt left a comment

Choose a reason for hiding this comment

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

LGTM!

@shanejonas shanejonas requested a review from a team September 3, 2021 16:49
Copy link
Member

@rekmarks rekmarks left a comment

Choose a reason for hiding this comment

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

Nice!

@shanejonas shanejonas merged commit 362b54b into develop Sep 6, 2021
@shanejonas shanejonas deleted the get-persistent-state-support branch September 6, 2021 02:33
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.

4 participants