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: migrate MetaMetricsController to BaseControllerV2 #28113

Conversation

cryptodev-2s
Copy link
Contributor

@cryptodev-2s cryptodev-2s commented Oct 25, 2024

Description

This PR migrates MetaMetricsController to inherit from BaseController V2

Open in GitHub Codespaces

Related issues

Fixes: #25925

Manual testing steps

Screenshots/Recordings

Before

After

Pre-merge author checklist

Pre-merge reviewer checklist

  • I've manually tested the PR (e.g. pull and build branch, run the app, test code being changed).
  • I confirm that this PR addresses all acceptance criteria described in the ticket it closes and includes the necessary testing evidence such as recordings and or screenshots.

@cryptodev-2s cryptodev-2s self-assigned this Oct 25, 2024
@cryptodev-2s cryptodev-2s requested a review from a team as a code owner October 25, 2024 23:25
Copy link
Contributor

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.

@cryptodev-2s cryptodev-2s force-pushed the cryptodev2s/migration-metametrics-controller-to-base-controller-v2 branch 5 times, most recently from fc42304 to 578139f Compare October 27, 2024 19:51
Base automatically changed from feature/convert-metametrics-to-typescript to develop October 28, 2024 10:37
@cryptodev-2s cryptodev-2s force-pushed the cryptodev2s/migration-metametrics-controller-to-base-controller-v2 branch from 578139f to cc4012a Compare October 28, 2024 11:15
@cryptodev-2s cryptodev-2s marked this pull request as draft October 28, 2024 14:33
@cryptodev-2s cryptodev-2s force-pushed the cryptodev2s/migration-metametrics-controller-to-base-controller-v2 branch from 57db26d to 2040439 Compare October 29, 2024 16:03
@cryptodev-2s cryptodev-2s marked this pull request as ready for review October 29, 2024 16:03
@cryptodev-2s cryptodev-2s requested review from a team as code owners October 29, 2024 16:46
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.

Compared the JavaScript and TypeScript versions of the controller and everything looks pretty good. Most of my comments were minor, except for perhaps using ?? null to default MetaMetrics properties that would otherwise be undefined.

app/scripts/controllers/metametrics-controller.ts Outdated Show resolved Hide resolved
app/scripts/controllers/metametrics-controller.ts Outdated Show resolved Hide resolved
app/scripts/controllers/metametrics-controller.ts Outdated Show resolved Hide resolved
app/scripts/controllers/metametrics-controller.ts Outdated Show resolved Hide resolved
app/scripts/controllers/metametrics-controller.ts Outdated Show resolved Hide resolved
app/scripts/controllers/metametrics-controller.test.ts Outdated Show resolved Hide resolved
ui/components/app/assets/asset-list/asset-list.tsx Outdated Show resolved Hide resolved
@cryptodev-2s cryptodev-2s requested a review from mcmire November 7, 2024 05:37
@cryptodev-2s cryptodev-2s requested a review from mcmire November 8, 2024 07:16
@metamaskbot
Copy link
Collaborator

Builds ready [281415b]
Page Load Metrics (1818 ± 48 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint1654197418199847
domContentLoaded1642193817918842
load16511978181810048
domInteractive14230544421
backgroundConnect1099282713
firstReactRender532131083316
getState44610115
initialActions00000
loadScripts1177143613117737
setupStore685162110
uiStartup18532356207614871
Bundle size diffs [🚨 Warning! Bundle size has increased!]
  • background: 567 Bytes (0.01%)
  • ui: 0 Bytes (0.00%)
  • common: 0 Bytes (0.00%)

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.

Nice job!

@cryptodev-2s cryptodev-2s added this pull request to the merge queue Nov 11, 2024
Merged via the queue into develop with commit 6ae02e8 Nov 11, 2024
76 checks passed
@cryptodev-2s cryptodev-2s deleted the cryptodev2s/migration-metametrics-controller-to-base-controller-v2 branch November 11, 2024 12:25
@github-actions github-actions bot locked and limited conversation to collaborators Nov 11, 2024
@metamaskbot metamaskbot added the release-12.8.0 Issue or pull request that will be included in release 12.8.0 label Nov 11, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
release-12.8.0 Issue or pull request that will be included in release 12.8.0 team-wallet-framework
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Migrate MetaMetricsController to BaseController v2
4 participants