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: bump keyring controller 7.5.0 #8035

Merged
merged 24 commits into from
Jan 6, 2024

Conversation

gantunesr
Copy link
Member

@gantunesr gantunesr commented Dec 7, 2023

Description

This PR bumps the @metamask/keyring-controller version from 6.0.0 to 7.5.0. These are the relevant changes to take into consideration during review,

  • BREAKING: Remove keyringTypes property from the KeyringController state (#1441)
  • BREAKING: Constructor KeyringControllerOptions type changed (#1441)
    • The KeyringControllerOptions.state accepted type is now { vault?: string }
    • The KeyringControllerOptions.keyringBuilders type is now { (): Keyring<Json>; type: string }[]
  • BREAKING: The address type accepted by the removeAccount method is now Hex (#1441)
  • BREAKING: The signTypedMessage method now returns a Promise<string> (#1441)
  • BREAKING: The signTransaction method now requires a TypedTransaction from @ethereumjs/tx@^4 for the transaction argument, and returns a Promise<TxData> (#1441)
  • BREAKING: Rename Keyring type to KeyringObject (#1441)
  • BREAKING: addNewAccount now throws if address of new account is not a hex string (#1441)
  • BREAKING: exportSeedPhrase now throws if first keyring does not have a mnemonic (#1441)
  • BREAKING: verifySeedPhrase now throws if HD keyring does not have a mnemonic (#1441)

Related issues

Fixes: #8180

Manual testing steps

Use cases or flows to verify,

  1. Onboarding
  2. Import SRP
  3. Import private key
  4. Reveal SRP
  5. Reveal private key
  6. Add new account
  7. Connect QR wallet
  8. Signed type messages
  9. Remove imported account
  10. Lock and unlock wallet

Screenshots/Recordings

Before

After

Pre-merge author checklist

  • I’ve followed MetaMask Coding Standards.
  • I've clearly explained what problem this PR is solving and how it is solved.
  • I've linked related issues
  • I've included manual testing steps
  • I've included screenshots/recordings if applicable
  • I’ve included tests if applicable
  • I’ve documented my code using JSDoc format if applicable
  • I’ve applied the right labels on the PR (see labeling guidelines). Not required for external contributors.
  • I’ve properly set the pull request status:
    • In case it's not yet "ready for review", I've set it to "draft".
    • In case it's "ready for review", I've changed it from "draft" to "non-draft".

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

socket-security bot commented Dec 7, 2023

Updated dependencies detected. Learn more about Socket for GitHub ↗︎

Packages Version New capabilities Transitives Size Publisher
@metamask/keyring-controller 6.1.0...7.5.0 None +8/-8 1.08 MB metamaskbot

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.

@gantunesr gantunesr force-pushed the feat/bump-keyring-controller-7.5.0 branch 3 times, most recently from d862696 to 522b894 Compare December 14, 2023 14:24
@codecov-commenter
Copy link

codecov-commenter commented Dec 18, 2023

Codecov Report

Attention: 17 lines in your changes are missing coverage. Please review.

Comparison is base (97d3a47) 39.61% compared to head (28f05a6) 39.61%.

Files Patch % Lines
app/core/Ledger/Ledger.ts 5.55% 17 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##             main    #8035   +/-   ##
=======================================
  Coverage   39.61%   39.61%           
=======================================
  Files        1233     1233           
  Lines       29830    29820   -10     
  Branches     2840     2840           
=======================================
- Hits        11816    11814    -2     
+ Misses      17321    17313    -8     
  Partials      693      693           

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

@gantunesr gantunesr marked this pull request as ready for review December 21, 2023 01:35
@gantunesr gantunesr requested a review from a team as a code owner December 21, 2023 01:35
@gantunesr gantunesr added the needs-dev-review PR needs reviews from other engineers (in order to receive required approvals) label Dec 21, 2023
app/core/Engine.ts Outdated Show resolved Hide resolved
@owencraston
Copy link
Contributor

Confirmed the engine is still being backed up
Screenshot 2023-12-21 at 12 42 30 PM

owencraston
owencraston previously approved these changes Dec 21, 2023
Copy link
Contributor

@owencraston owencraston left a comment

Choose a reason for hiding this comment

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

I was able to import and create an account. The balances were correct and I was able to send a transaction.
Simulator Screenshot - iPhone 15 Pro - 2023-12-21 at 12 43 20

app/core/Ledger/Ledger.ts Outdated Show resolved Hide resolved
Copy link
Contributor

github-actions bot commented Jan 4, 2024

E2E test started on Bitrise: https://app.bitrise.io/app/be69d4368ee7e86d/pipelines/2cebefad-8b8e-4a96-8668-da0b444a3ede
You can also kick off another Bitrise E2E smoke test by removing and re-applying the (Run Smoke E2E) label

Copy link

sonarcloud bot commented Jan 4, 2024

@gantunesr gantunesr 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 Jan 4, 2024
@vivek-consensys
Copy link
Contributor

All tests have passed, all manual test cases were carried out using both OS Android 13 and iOS 17.1.1.
Bitrise QA Build used to test:- https://app.bitrise.io/app/be69d4368ee7e86d/pipelines/1b46d210-0487-4f99-a638-09c33c6d4706

@plasmacorral plasmacorral added QA in Progress QA has started on the feature. and removed needs-qa Any New Features that needs a full manual QA prior to being added to a release. labels Jan 6, 2024
@plasmacorral
Copy link
Contributor

Tested commit 28f05a6 with both Samsung A515f on Android 12 with fingerprint and iPhone 13 mini on iOS 15.6.2 with faceID.

Tested the following with no issues:

  • Upgrade testing from V7.12.2 build 1224
    • all accounts present
    • custom account names preserved
  • Onboarding
    • create new SRP
    • import SRP
  • Delete wallet from settings
  • Reset wallet from login
  • Import private key
  • Reveal SRP
  • Reveal private key
  • Add new HD account
  • Edit account name
  • HD and imported account
    • Testnet send tx
    • Personal sign
    • Typed data V1
    • Typed data V3
    • Typed data V4
    • Sign permit
    • SIWE
  • Lock and unlock wallet
    • Manually lock from settings, then unlock
    • app autolock at 30s when in background with display remaining active, then open and unlock
    • Kill app and open then unlock
    • Set device display lock setting to 1 min with default autolock 30s and let screen timeout, then unlock device and unlock wallet

@plasmacorral plasmacorral added QA Passed A successful QA run through has been done and removed QA in Progress QA has started on the feature. labels Jan 6, 2024
@plasmacorral plasmacorral merged commit 445ef2b into main Jan 6, 2024
40 of 42 checks passed
@plasmacorral plasmacorral deleted the feat/bump-keyring-controller-7.5.0 branch January 6, 2024 05:01
@github-actions github-actions bot locked and limited conversation to collaborators Jan 6, 2024
@metamaskbot metamaskbot added the release-7.15.0 Issue or pull request that will be included in release 7.15.0 label Jan 6, 2024
@angelcheung22 angelcheung22 added this to the Q1 2024 milestone Apr 9, 2024
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.15.0 Issue or pull request that will be included in release 7.15.0 team-accounts
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Bump @metamask/keyring-controller from v6 to v7
8 participants