Skip to content

feat: bump keyring controller 7.5.0#8035

Merged
plasmacorral merged 24 commits intomainfrom
feat/bump-keyring-controller-7.5.0
Jan 6, 2024
Merged

feat: bump keyring controller 7.5.0#8035
plasmacorral merged 24 commits intomainfrom
feat/bump-keyring-controller-7.5.0

Conversation

@gantunesr
Copy link
Copy Markdown
Member

@gantunesr gantunesr commented Dec 7, 2023 •
edited by plasmacorral
Loading

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.

@gantunesr gantunesr added the team-accounts-framework Accounts team label Dec 7, 2023
@socket-security
Copy link
Copy Markdown

socket-security bot commented Dec 7, 2023 •
edited
Loading

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

@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented Dec 14, 2023

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
Copy Markdown

codecov-commenter commented Dec 18, 2023 •
edited
Loading

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
owencraston
owencraston reviewed Dec 21, 2023
View reviewed changes
@owencraston
Copy link
Copy Markdown
Contributor

owencraston commented Dec 21, 2023

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
View reviewed changes
Copy link
Copy Markdown
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

Gudahtt
Gudahtt reviewed Dec 21, 2023
View reviewed changes
@github-actions
Copy link
Copy Markdown
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

@sonarqubecloud
Copy link
Copy Markdown

sonarqubecloud bot commented Jan 4, 2024

Quality Gate Passed Quality Gate passed

Kudos, no new issues were introduced!

0 New issues
0 Security Hotspots
5.6% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud

@gantunesr gantunesr requested a review from owencraston January 4, 2024 19:55
@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
Copy Markdown
Contributor

vivek-consensys commented Jan 5, 2024

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
Copy Markdown
Contributor

plasmacorral commented Jan 6, 2024

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 QA testing has been completed and passed and removed QA in Progress QA has started on the feature. labels Jan 6, 2024
@plasmacorral plasmacorral mentioned this pull request Jan 6, 2024
Closed
@plasmacorral plasmacorral merged commit 445ef2b into main Jan 6, 2024
@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.

Reviewers

@Gudahtt Gudahtt Gudahtt left review comments

@owencraston owencraston owencraston approved these changes

Assignees

No one assigned

Labels

QA Passed QA testing has been completed and passed release-7.15.0 Issue or pull request that will be included in release 7.15.0 team-accounts-framework Accounts team

Projects

None yet

Milestone

Q1 2024 (CET hw wallet)

Development

Successfully merging this pull request may close these issues.

Bump @metamask/keyring-controller from v6 to v7

8 participants

@gantunesr @codecov-commenter @owencraston @vivek-consensys @plasmacorral @Gudahtt @metamaskbot @angelcheung22