-
-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
refactor: Update core controllers (v47) #6964
Conversation
Updated dependencies detected. Learn more about Socket for GitHub ↗︎
|
This comment was marked as resolved.
This comment was marked as resolved.
7b3905e
to
579487f
Compare
4ca5a12
to
0bb0c41
Compare
This has been rebased onto #6902 Passing e2e test run: https://app.bitrise.io/app/be69d4368ee7e86d/pipelines/5e6851f8-0d0f-40c2-8c13-655ee997779b |
579487f
to
a1b4372
Compare
0bb0c41
to
20984a2
Compare
a1b4372
to
6101dc3
Compare
20984a2
to
cdfaa59
Compare
Codecov Report
@@ Coverage Diff @@
## main #6964 +/- ##
=======================================
Coverage 32.73% 32.73%
=======================================
Files 993 993
Lines 26614 26614
Branches 2083 2083
=======================================
Hits 8713 8713
Misses 17487 17487
Partials 414 414
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since setProviderType
is now async, should we create an issue to change the codebase?
Since this controller update only changes the assets-controllers version, I don't know if it would be okay to do it here.
Let me know what do you think
I'm a little bit confused, the setProviderType
on the change log of version 47 is turned to async, but on the change log of the network controller it's just on version 8.0.0. I believe this will not crash anything, but just checking if we should update the network controllers to 8.0.0 to do this update on assets-controllers.
Thanks for looking into that @tommasini . I hadn't noticed that entry before. It looks like it's invalid. The linked PR is indeed for the network controller - the only impact it had on assets-controllers was in a test. That network controller change wasn't even included in that release - it was in v48, the release after this one. So that entry is invalid for two different reasons. I'll update it now. (Edit: done here: MetaMask/core#1619 ) |
This was the only functional change for assets-controllers in that release: MetaMask/core#1166 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the explanation and for looking into that!!
Looks good to me 🚀
6101dc3
to
67fb70d
Compare
cdfaa59
to
0188a38
Compare
The package `@metamask/assets-controllers` has been updated to v6, which brings core packages up-to-date with v47 of the core monorepo. This closes MetaMask/mobile-planning#877
0188a38
to
123356c
Compare
The blockers have been merged, so this has been rebased onto |
Kudos, SonarCloud Quality Gate passed! 0 Bugs 100.0% Coverage The version of Java (11.0.20) you have used to run this analysis is deprecated and we will stop accepting it soon. Please update to at least Java 17. |
Hey @Gudahtt, testing is still ongoing. I did however notice a bug. After importing a wallet, I notice my transaction history is not displayed. See the recording here: http://recordit.co/Wsn77E1xy2 Compared to prod v7.5: http://recordit.co/02UDZRtvci To reproduce: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good. ✅
Development & PR Process
release-xx
label to identify the PR slated for a upcoming release (will be used in release discussion)needs-dev-review
label when work is completedneeds-qa
: PR requires manual QA.No QA/E2E only
: PR does not require any manual QA effort. Prior to merging, ensure that you have successful end-to-end test runs in Bitrise.Spot check on release build
: PR does not require feature QA but needs non-automated verification. In the description section, provide test scenarios. Add screenshots, and or recordings of what was tested.QA Passed
label when QA has signed off (Only required if the PR was labeled withneeds-qa
)team-
(orexternal-contributor
label if your not a MetaMask employee)Description
The package
@metamask/assets-controllers
has been updated to v6, which brings core packages up-to-date with v47 of the core monorepo.Issue
This closes https://github.com/MetaMask/mobile-planning/issues/877
Checklist