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

Update controller packages to match core v40 #6124

Merged
merged 2 commits into from
May 5, 2023
Merged

Conversation

Gudahtt
Copy link
Member

@Gudahtt Gudahtt commented Apr 5, 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

The controller packages have been updated to match the versions in the core monorepo v40 release. The keyring controller update was held back due to incompatibilities related to BigInt.

The only breaking change was to the network controller state. The state property properties was renamed to networkDetails. Luckily there was only a single direct reference to this property (in a test) so the number of changes required was minimal, but we did need a state migration.

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

Manual Testing Instructions

This release includes a bug fix related to NFTs, details here: MetaMask/core#1082

We might be able to test that the issue is resolved by doing something like this:

  • Setup a test account with an NFT
  • Send the NFT away, and check to ensure that it is recognized as "previously owned" (I am unfamiliar with this feature so I'm not sure about this step)
  • Send the NFT back to the test account
  • Ensure that the NFT is recognized as owned, not previously owned

I am not confident in those steps though.

For the network controller update, there should be no functional changes. There is a state migration, but it's hard to see the effects of that migration as a user (effectively it saves a single network call upon app load). The state gets overwritten upon each network switch. The e2e tests should suffice to catch unexpected network related regressions.

Issue

Progresses https://github.com/MetaMask/mobile-planning/issues/798

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

socket-security bot commented Apr 5, 2023

New dependency changes detected. Learn more about Socket for GitHub ↗︎


👍 No new dependency issues detected in pull request

Bot Commands

To ignore an alert, reply with a comment starting with @SocketSecurity ignore followed by a space separated list of package-name@version specifiers. e.g. @SocketSecurity ignore foo@1.0.0 bar@* or ignore all packages with @SocketSecurity ignore-all

Pull request alert summary
Issue Status
Critical CVE ✅ 0 issues
CVE ✅ 0 issues
Mild CVE ✅ 0 issues
Install scripts ✅ 0 issues
Native code ✅ 0 issues
Bin script confusion ✅ 0 issues
Bin script shell injection ✅ 0 issues
Filesystem access ✅ 0 issues
Network access ✅ 0 issues
Shell access ✅ 0 issues
Unresolved require ✅ 0 issues
Invalid package.json ✅ 0 issues
HTTP dependency ✅ 0 issues
Git dependency ✅ 0 issues
GitHub dependency ✅ 0 issues
No bug tracker ✅ 0 issues
No contributors or author data ✅ 0 issues
No README ✅ 0 issues
Deprecated ✅ 0 issues
New author ✅ 0 issues
Unstable ownership ✅ 0 issues
Non-existent author ✅ 0 issues
Unmaintained ✅ 0 issues
Unpublished package ✅ 0 issues
Potential typo squat ✅ 0 issues
Known Malware ✅ 0 issues
Telemetry ✅ 0 issues
Protestware/Troll package ✅ 0 issues
AI detected security risk ✅ 0 issues
AI warning ✅ 0 issues

📊 Modified Dependency Overview:

➕ Added Package Capability Access +/- Transitive Count Publisher
@metamask/network-controller@4.0.0 None +0 metamaskbot
⬆️ Updated Package Version Diff Added Capability Access +/- Transitive Count Publisher
@metamask/assets-controllers@4.0.1 4.0.0...4.0.1 None +1/-0 metamaskbot

@github-actions
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.

@Gudahtt Gudahtt changed the base branch from update-controllers-v39 to main April 18, 2023 19:44
@Gudahtt Gudahtt added the needs-dev-review PR needs reviews from other engineers (in order to receive required approvals) label Apr 18, 2023
@Gudahtt Gudahtt marked this pull request as ready for review April 18, 2023 19:48
@Gudahtt Gudahtt requested a review from a team as a code owner April 18, 2023 19:48
@Gudahtt
Copy link
Member Author

Gudahtt commented Apr 18, 2023

Bitrise run here: https://app.bitrise.io/build/f3875ff0-2694-4d29-b4f3-c5af26ef9e82

Seems to indicate no regressions (matches the most recent run on main)

Copy link
Member

@gantunesr gantunesr left a comment

Choose a reason for hiding this comment

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

LGTM

@Gudahtt Gudahtt added needs-qa Any New Features that needs a full manual QA prior to being added to a release. and removed needs-dev-review PR needs reviews from other engineers (in order to receive required approvals) labels Apr 18, 2023
@Gudahtt
Copy link
Member Author

Gudahtt commented Apr 19, 2023

The description has just been updated with manual testing steps

The controller packages have been updated to match the versions in the
core monorepo v40 release. The keyring controller update was held back
due to incompatibilities related to BigInt.

The only breaking change was to the network controller state. The state
property `properties` was renamed to `networkDetails`. Luckily there
was only a single direct reference to this property (in a test) so the
number of changes required was minimal, but we did need a state
migration.
@Gudahtt
Copy link
Member Author

Gudahtt commented May 5, 2023

This has been rebased again to resolve a conflict (it was the migration; a new one was added, so the migration added here was bumped from 15 to 16).

@cortisiko cortisiko removed the needs-qa Any New Features that needs a full manual QA prior to being added to a release. label May 5, 2023
@cortisiko cortisiko added the QA in Progress QA has started on the feature. label May 5, 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.

This PR looks good. Areas I manually tested:

  • adding a custom mainnet then performing
    • a couple of swaps
    • a couple of send transactions
  • payment request deep links
  • Minting NFTs and sending NFTs between addresses. I did not notice anything unusual there.
  • Keystone (hardware wallet) transactions.

This ticket is 🌮

@cortisiko cortisiko merged commit 5a6488a into main May 5, 2023
15 checks passed
@cortisiko cortisiko deleted the update-controllers-v40 branch May 5, 2023 22:48
@github-actions github-actions bot locked and limited conversation to collaborators May 5, 2023
@cortisiko cortisiko added QA Passed A successful QA run through has been done and removed QA in Progress QA has started on the feature. labels May 5, 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 team-mobile-platform team-wallet-framework
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants