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.

Sign up for GitHub

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

Jump to bottom

feat: Bump @metamask/keyring-controller to v6.0.0 #6770

Closed
wants to merge 88 commits into from
Closed

feat: Bump @metamask/keyring-controller to v6.0.0 #6770

wants to merge 88 commits into from

Conversation

gantunesr
Copy link
Member

@gantunesr gantunesr commented Jul 10, 2023 •
edited
Loading

Description

Bump @metamask/keyring-controller from v1.0.1 to v6.0.0.

  • Patches
    • micro-ftch+0.3.1: Added to comment out a specific node logic that was causing a compatibility issue with another library.
    • @metamask+message-manager+7.0.1: Updated since we're using the same version for both SignatureController and KeyringController.
    • @metamask+message-manager+1.0.1: Removed
    • @metamask+keyring-controller++@metamask+message-manager+7.3.1: Added to update the data type PersonalMessageParams.
  • UI/UX changes
    • To reveal the SRP or Private Key, the CTA to verify the password will be disabled until a string of 8 characters or more is provided. After it, the CTA will be enabled and will call to verify the password against the KeyringController and provide a result.

Test cases

  • Onboarding
    • Create new wallet
    • Import SRP
  • Authentication - Lock and unlock wallet using different authentication mechanisms (password, fingerprint, face ID, device pin)
  • Reveal SRP
  • Reveal private key
  • Import private key - Creation of Simple Keyring
    • Upgrade the app from previous version and import private key
  • Import QR wallet accounts - Creation of QR Keyring
    • Upgrade the app from previous version and import QR wallet accounts
  • Send a simple transaction using a HD account
  • Signature methods (Including QR wallet)
    • Eth Sign
    • Personal Sign
    • Sign Typed Data
    • Sign Typed Data V3
    • Sign Typed Data V4
  • Delete imported account (Simple Keyring)
  • Disconnect Keystone (QR Keyring)

Screenshots/Recordings

Screen.Recording.2023-07-24.at.13.33.03.mov

Issue

Fixes https://github.com/MetaMask/mobile-planning/issues/755

Checklist

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

@socket-security
Copy link

socket-security bot commented Jul 10, 2023 •
edited
Loading

👍 Dependency issues cleared. Learn more about Socket for GitHub ↗︎

This PR previously contained dependency changes with security issues that have been resolved, removed, or ignored.

@gantunesr gantunesr changed the title Bump @metamask/keyring-controller to v6.0.0 feat: Bump @metamask/keyring-controller to v6.0.0 Jul 10, 2023
NicolasMassart
NicolasMassart reviewed Sep 13, 2023
View reviewed changes
Copy link
Contributor

@NicolasMassart NicolasMassart left a comment

Choose a reason for hiding this comment

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

Q: what is the word we should use consistently? SRP, Mnemonic or Seed phrase? I can see all of them in this code and not sure it's expected to be the same thing or not.

@gantunesr
Copy link
Member Author

gantunesr commented Sep 13, 2023 •
edited
Loading

@jpuri I removed that patch because bumping the keyring-controller removed the @metamask/message-manager v1.0.1, I have included the patch in the @metamask/message-manager v7.0.1.

@OGPoyraz I have added the test cases in the PR description, let me know if there's one missing.

@NicolasMassart The BIP39 spec uses the word mnemonic. I think we should use it over seedphrase and srp in the code.

gantunesr
gantunesr commented Sep 13, 2023
View reviewed changes
app/core/Engine.ts Outdated Show resolved Hide resolved
mikesposito
mikesposito reviewed Sep 15, 2023
View reviewed changes
app/core/Engine.ts Outdated Show resolved Hide resolved
owencraston
owencraston previously approved these changes Sep 15, 2023
View reviewed changes
Copy link
Contributor

@owencraston owencraston left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Looks good to me!

Screen.Recording.2023-09-15.at.9.58.35.AM.mov

@gantunesr
Copy link
Member Author

gantunesr commented Sep 15, 2023 •
edited
Loading

E2E tests executing in #9753

Edit: E2E passed for both iOS and Android ✅

@Gudahtt
Copy link
Member

Gudahtt commented Sep 15, 2023

Looks great! A few minor suggestions, but functionally everything looks good except maybe for the missing updateIdentities call after calling forgetQRKeyring

@gantunesr
Copy link
Member Author

gantunesr commented Sep 17, 2023 •
edited
Loading

Smoke E2E executing in #9767

Edit: E2E passed for both iOS and Android ✅

@sonarqubecloud
Copy link

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

59.5% 59.5% Coverage
0.0% 0.0% Duplication

@gantunesr gantunesr requested a review from Gudahtt September 18, 2023 12:31
@gantunesr
Copy link
Member Author

I'll close this PR and open a new one because of a problem in GitHub.

@gantunesr gantunesr closed this Sep 19, 2023
@github-actions github-actions bot locked and limited conversation to collaborators Sep 19, 2023
@github-actions github-actions bot removed the needs-dev-review PR needs reviews from other engineers (in order to receive required approvals) label Sep 19, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@mikesposito mikesposito mikesposito left review comments

@Cal-L Cal-L Cal-L left review comments

@NicolasMassart NicolasMassart NicolasMassart left review comments

@owencraston owencraston owencraston left review comments

@Gudahtt Gudahtt Awaiting requested review from Gudahtt

Assignees
No one assigned
Labels
team-accounts
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
10 participants
@gantunesr @AlexJupiter @Gudahtt @codecov-commenter @jpuri @OGPoyraz @NicolasMassart @Cal-L @owencraston @mikesposito