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

refactor: Update core controllers (v45) #6902

Merged
merged 1 commit into from
Aug 22, 2023
Merged

Conversation

Gudahtt
Copy link
Member

@Gudahtt Gudahtt commented Jul 25, 2023

Development & PR Process

  1. Follow MetaMask Mobile Coding Standards
  2. Add release-xx label to identify the PR slated for a upcoming release (will be used in release discussion)
  3. Add needs-dev-review label when work is completed
  4. Add needs-qa label when dev review is completed
  5. Add QA Passed label when QA has signed off

Description

Update core controllers to match the v45 monorepo release. The network controller has been updated to v7, and the assets controllers have been updated to v5.1.0.

The phishing controller and message manager updates have been held back to reduce the scope of the PR. They will be updated in a later PR.

Release notes: https://github.com/MetaMask/core/releases/tag/v45.0.0

Issue

This relates to https://github.com/MetaMask/mobile-planning/issues/877

Checklist

  • There is a related GitHub issue
  • Tests are included if applicable
  • Any added code is fully documented

@Gudahtt

This comment was marked as resolved.

@socket-security
Copy link

New and updated dependencies detected. Learn more about Socket for GitHub ↗︎

Packages Version New capabilities Transitives Size Publisher
@metamask/network-controller 7.0.0 None +0 73.2 kB metamaskbot
@metamask/assets-controllers 5.0.1...5.1.0 None +1/-0 665 kB metamaskbot

@Gudahtt Gudahtt force-pushed the update-controller-packages-v44 branch 3 times, most recently from 499481a to 190c453 Compare July 27, 2023 14:52
@Gudahtt Gudahtt force-pushed the update-controller-packages-v45 branch 2 times, most recently from 892a5ba to cc83957 Compare July 27, 2023 15:24
@Gudahtt Gudahtt force-pushed the update-controller-packages-v44 branch 4 times, most recently from ce809af to 284882e Compare August 3, 2023 16:39
@Gudahtt Gudahtt force-pushed the update-controller-packages-v45 branch from cc83957 to 7cfba9a Compare August 3, 2023 16:50
@Gudahtt Gudahtt force-pushed the update-controller-packages-v44 branch from 9b17491 to d58bab4 Compare August 15, 2023 15:05
@Gudahtt Gudahtt force-pushed the update-controller-packages-v45 branch 2 times, most recently from 7b3905e to 579487f Compare August 17, 2023 21:53
@Gudahtt Gudahtt changed the base branch from update-controller-packages-v44 to update-assets-controllers-patch August 17, 2023 21:53
@Gudahtt
Copy link
Member Author

Gudahtt commented Aug 17, 2023

This has been rebased onto #6898 .

Passing e2e test run: https://app.bitrise.io/app/be69d4368ee7e86d/pipelines/9577199b-c426-47ea-8371-7fbed7eb4a30

@Gudahtt Gudahtt force-pushed the update-assets-controllers-patch branch from c5a3479 to bd84729 Compare August 18, 2023 13:17
@Gudahtt Gudahtt force-pushed the update-controller-packages-v45 branch from 579487f to a1b4372 Compare August 18, 2023 13:18
@Gudahtt Gudahtt force-pushed the update-assets-controllers-patch branch from bd84729 to 61d86c2 Compare August 18, 2023 13:23
@Gudahtt Gudahtt force-pushed the update-controller-packages-v45 branch from a1b4372 to 6101dc3 Compare August 18, 2023 13:23
@codecov-commenter
Copy link

Codecov Report

Merging #6902 (a1b4372) into update-assets-controllers-patch (61d86c2) will increase coverage by 0.01%.
The diff coverage is 31.91%.

❗ Current head a1b4372 differs from pull request most recent head 6101dc3. Consider uploading reports for the commit 6101dc3 to get more accurate results

@@                         Coverage Diff                         @@
##           update-assets-controllers-patch    #6902      +/-   ##
===================================================================
+ Coverage                            32.68%   32.69%   +0.01%     
===================================================================
  Files                                  994      996       +2     
  Lines                                26649    26652       +3     
  Branches                              2088     2084       -4     
===================================================================
+ Hits                                  8710     8714       +4     
+ Misses                               17525    17524       -1     
  Partials                               414      414              
Files Changed Coverage Δ
app/components/UI/AddCustomToken/index.js 31.48% <0.00%> (ø)
.../UI/ApproveTransactionReview/AddNickname/index.tsx 7.01% <ø> (ø)
...proveTransactionReview/ShowBlockExplorer/index.tsx 14.28% <ø> (ø)
...ew/VerifyContractDetails/VerifyContractDetails.tsx 44.44% <0.00%> (ø)
...pp/components/UI/ApproveTransactionReview/index.js 3.96% <0.00%> (ø)
app/components/UI/DrawerView/index.js 3.72% <0.00%> (ø)
app/components/UI/NetworkModal/index.tsx 4.54% <0.00%> (ø)
app/components/UI/Ramp/Views/OrderDetails.tsx 6.25% <0.00%> (ø)
app/components/UI/Ramp/components/OrderDetails.tsx 11.29% <0.00%> (ø)
...mponents/UI/Ramp/hooks/useHandleSuccessfulOrder.ts 0.00% <0.00%> (ø)
... and 39 more

... and 19 files with indirect coverage changes

Copy link
Contributor

@tommasini tommasini left a comment

Choose a reason for hiding this comment

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

Amazing, very happy to see that we are pushing forward the version with phishing controller improvements, for that controller use less memory/bandwidth

Notice here that we would need to change the addAsset of asset controller where it's used, because the structures of the arguments changed on this assets controller version from
addToken(address, symbol, decimals, image, interactingAddress)
to
addToken(address, symbol, decimals, { name, image, interactingAddress })

with this said we should change on the following places:
useAddressBalance.ts --> line 45
TokensController.addToken( contractAddress, symbol, decimals, { image, name,} );

UI/AddCustomToken.js --> line 120
TokensController.addToken(address, symbol, decimals, {name})

useHandleSuccessfullOrder.ts --> line 48
TokensController.addToken(address, symbol, decimals, {name})

SearchTokenAutocomplete --> line 110
TokensController.addToken(address, symbol, decimals, {image: iconUrl, name})

UI/Swaps --> line 531
TokensController.addToken(address, symbol, decimals,{ name})

QuotesView --> line 351
TokensController.addToken(address, symbol, decimals,{ name})

SendFlow/Confirm --> lien 527
TokensController.addToken(address, symbol, decimals, {image, name})

@Gudahtt
Copy link
Member Author

Gudahtt commented Aug 21, 2023

Thanks @tommasini . I think those requested changes have already been made (in #6898, which this PR is based on). Let me know if there are any that were missed

tommasini
tommasini previously approved these changes Aug 21, 2023
Copy link
Contributor

@tommasini tommasini left a comment

Choose a reason for hiding this comment

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

Sorry, totally missed that dependency! Looks good to me!

@Gudahtt Gudahtt force-pushed the update-assets-controllers-patch branch from 61d86c2 to 32e5b32 Compare August 22, 2023 18:41
Base automatically changed from update-assets-controllers-patch to main August 22, 2023 21:06
@Gudahtt Gudahtt dismissed tommasini’s stale review August 22, 2023 21:06

The base branch was changed.

Update core controllers to match the v45 monorepo release. The network
controller has been updated to v7, and the assets controllers have been
updated to v5.1.0.

The phishing controller and message manager updates have been held back
to reduce the scope of the PR. They will be updated in a later PR.

Release notes: https://github.com/MetaMask/core/releases/tag/v45.0.0

This relates to MetaMask/mobile-planning#877
@Gudahtt Gudahtt force-pushed the update-controller-packages-v45 branch from 6101dc3 to 67fb70d Compare August 22, 2023 21:07
@Gudahtt Gudahtt marked this pull request as ready for review August 22, 2023 21:07
@Gudahtt Gudahtt requested a review from a team as a code owner August 22, 2023 21:07
@Gudahtt Gudahtt requested a review from tommasini August 22, 2023 21:12
@Gudahtt Gudahtt added needs-qa Any New Features that needs a full manual QA prior to being added to a release. needs-dev-review PR needs reviews from other engineers (in order to receive required approvals) labels Aug 22, 2023
@Gudahtt
Copy link
Member Author

Gudahtt commented Aug 22, 2023

All blockers have been resolved, so this has been rebased onto main. Diff should be the same as before, when it was approved by @tommasini

@sonarcloud
Copy link

sonarcloud bot commented Aug 22, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

100.0% 100.0% Coverage
0.0% 0.0% Duplication

warning 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.
Read more here

Copy link
Contributor

@Cal-L Cal-L left a comment

Choose a reason for hiding this comment

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

code LGTM

@Gudahtt Gudahtt removed the needs-dev-review PR needs reviews from other engineers (in order to receive required approvals) label Aug 22, 2023
@cortisiko cortisiko added QA Passed A successful QA run through has been done and removed needs-qa Any New Features that needs a full manual QA prior to being added to a release. labels Aug 22, 2023
Copy link
Member

@cortisiko cortisiko left a comment

Choose a reason for hiding this comment

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

🌮 🌮 🌮 🌮

@Gudahtt Gudahtt merged commit 25eeea1 into main Aug 22, 2023
12 of 17 checks passed
@Gudahtt Gudahtt deleted the update-controller-packages-v45 branch August 22, 2023 23:09
@github-actions github-actions bot locked and limited conversation to collaborators Aug 22, 2023
@metamaskbot metamaskbot added the release-7.7.0 Issue or pull request that will be included in release 7.7.0 label Aug 22, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
QA Passed A successful QA run through has been done release-7.7.0 Issue or pull request that will be included in release 7.7.0 team-mobile-platform
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants