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 OnboardingController to BaseController v2 #26880

Merged
merged 5 commits into from
Sep 16, 2024

Conversation

cryptodev-2s
Copy link
Contributor

@cryptodev-2s cryptodev-2s commented Sep 3, 2024

Description

This PR updates OnboardingController to inherit from BaseController V2.

Details about other changes are to come...

Open in GitHub Codespaces

Related issues

Fixes: #25927

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.

Copy link
Contributor

github-actions bot commented Sep 3, 2024

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 feat/upgrade-onboarding-controller branch from 8ab4d47 to 58dbff7 Compare September 10, 2024 21:27
@cryptodev-2s cryptodev-2s marked this pull request as draft September 10, 2024 21:27
@cryptodev-2s cryptodev-2s force-pushed the feat/upgrade-onboarding-controller branch from 58dbff7 to a8567e6 Compare September 11, 2024 17:22
Copy link

codecov bot commented Sep 11, 2024

Codecov Report

Attention: Patch coverage is 80.00000% with 6 lines in your changes missing coverage. Please review.

Project coverage is 70.12%. Comparing base (5053c3e) to head (7aa9438).
Report is 1 commits behind head on develop.

Files with missing lines Patch % Lines
app/scripts/controllers/onboarding.ts 80.00% 3 Missing ⚠️
app/scripts/background.js 0.00% 2 Missing ⚠️
app/scripts/lib/account-tracker.js 66.67% 1 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff            @@
##           develop   #26880   +/-   ##
========================================
  Coverage    70.12%   70.12%           
========================================
  Files         1426     1426           
  Lines        49705    49711    +6     
  Branches     13905    13905           
========================================
+ Hits         34853    34858    +5     
- Misses       14852    14853    +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@metamaskbot
Copy link
Collaborator

Builds ready [a8567e6]
Page Load Metrics (1757 ± 68 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint22620831529555266
domContentLoaded15392068173713665
load15482088175714168
domInteractive18164423215
Bundle size diffs [🚨 Warning! Bundle size has increased!]
  • background: 453 Bytes (0.01%)
  • ui: 0 Bytes (0.00%)
  • common: 0 Bytes (0.00%)

@cryptodev-2s cryptodev-2s marked this pull request as ready for review September 11, 2024 18:27
@metamaskbot
Copy link
Collaborator

Builds ready [1af5b44]
Page Load Metrics (1750 ± 70 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint41320711674322155
domContentLoaded15261980172312761
load15352073175014670
domInteractive135833115
Bundle size diffs [🚨 Warning! Bundle size has increased!]
  • background: 447 Bytes (0.01%)
  • ui: 0 Bytes (0.00%)
  • common: 0 Bytes (0.00%)

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.

This looks good overall! I just added a couple of small comments

app/scripts/lib/account-tracker.test.js Outdated Show resolved Hide resolved
app/scripts/controllers/onboarding.ts Show resolved Hide resolved
app/scripts/controllers/onboarding.ts Show resolved Hide resolved
@metamaskbot
Copy link
Collaborator

Builds ready [782a17b]
Page Load Metrics (1729 ± 84 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint24923711513548263
domContentLoaded14912357171317785
load14992363172917484
domInteractive1377392010
Bundle size diffs [🚨 Warning! Bundle size has increased!]
  • background: 447 Bytes (0.01%)
  • ui: 0 Bytes (0.00%)
  • common: 0 Bytes (0.00%)

Copy link

sonarcloud bot commented Sep 12, 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!

Pre-merge reviewer checklist

  • I've manually tested the PR (e.g. pull and build branch, run the app, test code being changed).
    • ✅ Tested onboarding from fresh install
    • ✅ Tested with an already onboarded account
  • 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.

@metamaskbot
Copy link
Collaborator

Builds ready [7aa9438]
Page Load Metrics (1800 ± 88 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint26224161720379182
domContentLoaded15702408178218488
load15792417180018488
domInteractive13197463818
Bundle size diffs [🚨 Warning! Bundle size has increased!]
  • background: 447 Bytes (0.01%)
  • ui: 0 Bytes (0.00%)
  • common: 0 Bytes (0.00%)

Copy link
Contributor

@itsyoboieltr itsyoboieltr left a comment

Choose a reason for hiding this comment

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

LGTM!

@cryptodev-2s cryptodev-2s merged commit 67cf369 into develop Sep 16, 2024
78 checks passed
@cryptodev-2s cryptodev-2s deleted the feat/upgrade-onboarding-controller branch September 16, 2024 19:11
@github-actions github-actions bot locked and limited conversation to collaborators Sep 16, 2024
@metamaskbot metamaskbot added the release-12.6.0 Issue or pull request that will be included in release 12.6.0 label Sep 16, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
release-12.6.0 Issue or pull request that will be included in release 12.6.0 team-wallet-framework
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

Migrate OnboardingController to BaseController v2
4 participants