Skip to content

Update @metamask/eth-keyring-controller - #1441

Merged
mikesposito merged 19 commits into
mainfrom
chore/update-eth-keyring-controller
Jul 18, 2023
Merged

Update @metamask/eth-keyring-controller#1441
mikesposito merged 19 commits into
mainfrom
chore/update-eth-keyring-controller

Conversation

@mikesposito

@mikespositomikesposito commented Jun 21, 2023

Copy link
Copy Markdown
Member

Explanation

This PR updates @metamask/eth-keyring-controller in KeyringController to ^13.
As after v11.0.0 the library has been migrated to TS there is a number of changes needed to support it.

  • Type guards have been added to mitigate some issues with the Keyring type used by @metamask/eth-keyring-controller, which is still not supported by the latest released versions of the HD and simple keyrings.
  • Keyring type from @metamask/utils has been added, superseding local type declarations

References

Changelog

@metamask/keyring-controller

  • BREAKING: Removed keyringTypes property from the KeyringController state
  • BREAKING: Constructor KeyringControllerOptions type changed
    • 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
  • BREAKING: The signTypedMessage method now returns a Promise<string>
  • BREAKING: The signTransaction method now requires a TypedTransaction from @ethereumjs/tx@^4 for the transaction argument, and returns a Promise<TxData>
  • BREAKING: Rename Keyring type to KeyringObject
  • BREAKING:addNewAccount now throws if address of new account is not a hex string
  • BREAKING:exportSeedPhrase now throws if first keyring does not have a mnemonic
  • BREAKING:verifySeedPhrase now throws if HD keyring does not have a mnemonic
  • CHANGED: Update return type of getAccountKeyringType to Promise<string>
  • CHANGED: The private method addQRKeyring has been renamed to #addQRKeyring
  • CHANGED: Updated @metamask/eth-keyring-controller to ^13.0.0

Checklist

  • I've updated the test suite for new or updated code as appropriate
  • I've updated documentation (JSDoc, Markdown, etc.) for new or updated code as appropriate
  • I've highlighted breaking changes using the "BREAKING" category above as appropriate

@socket-security

socket-securityBot commented Jun 21, 2023

Copy link
Copy Markdown

) {
const messageParamsClone = { ...messageParams };

if ((await this.getAccountKeyringType(address)) === KeyringTypes.qr) {

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

Starting from @metamask/eth-keyring-controller@11.0.0 any attempt to add a Keyring of a type with a missing keyring builder throws an error instead of returning undefined.

@mikesposito

Copy link
Copy Markdown
MemberAuthor

signPersonalMessageshould sign personal message even if empty data is passed
This test is currently failing with this error: Missing data parameter.

Seems like this behavior is changed in @metamask/eth-keyring-controller due to this line added. The data is being normalized too, using normalize function from @metamask/eth-sig-util, which returns undefined with an empty string,

@mikesposito

Copy link
Copy Markdown
MemberAuthor

Update of @ethereumjs/tx, @ethereumjs/common, @keystonehq/metamask-airgapped-keyring and @metamask/utils has been moved to #1514

@mikesposito
mikespositoforce-pushed the chore/update-eth-keyring-controller branch 2 times, most recently from 95242fc to 13a2ec7CompareJuly 17, 2023 14:43
@socket-security

socket-securityBot commented Jul 17, 2023

Copy link
Copy Markdown

👍 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.

Ignoring: @sinonjs/text-encoding@0.7.2, get-package-type@0.1.0

Next steps

Take a deeper look at the dependency

Take a moment to review the security alert above. Review the linked package source code to understand the potential risk. Ensure the package is not malicious before proceeding. If you're unsure how to proceed, reach out to your security team or ask the Socket team for help at support [AT] socket [DOT] dev.

Remove the package

If you happen to install a dependency that Socket reports as Known Malware you should immediately remove it and select a different dependency. For other alert types, you may may wish to investigate alternative packages or consider if there are other ways to mitigate the specific risk posed by the dependency.

Mark a package as acceptable risk

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

    @mikesposito
    mikesposito marked this pull request as ready for review July 17, 2023 15:24
    @mikesposito
    mikesposito requested a review from a team as a code ownerJuly 17, 2023 15:24

    @mcmiremcmire left a comment

    Copy link
    Copy Markdown
    Collaborator

    Choose a reason for hiding this comment

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

    Looks good! Just spotted some typecasting and had some questions.

    Comment threadpackages/keyring-controller/src/KeyringController.test.ts Outdated
    Comment threadpackages/keyring-controller/src/KeyringController.ts Outdated
    Comment threadpackages/keyring-controller/src/KeyringController.ts Outdated
    Comment threadpackages/keyring-controller/src/KeyringController.ts Outdated
    Comment threadpackages/keyring-controller/src/KeyringController.test.ts Outdated
    Comment threadpackages/keyring-controller/src/KeyringController.test.ts Outdated
    Comment threadtypes/obs-store.d.ts Outdated
    @legobeat

    Copy link
    Copy Markdown
    Contributor
    @SocketSecurity ignore @sinonjs/text-encoding@0.7.2
    @SocketSecurity ignore get-package-type@0.1.0
    

    Comment threadpackages/keyring-controller/src/KeyringController.ts Outdated
    Comment threadpackages/keyring-controller/src/KeyringController.ts Outdated
    Comment threadpackages/keyring-controller/src/KeyringController.ts Outdated
    Comment threadpackages/keyring-controller/src/KeyringController.ts Outdated
    Comment threadpackages/keyring-controller/src/KeyringController.ts Outdated
    Comment threadpackages/keyring-controller/src/KeyringController.ts Outdated
    Comment threadpackages/keyring-controller/src/KeyringController.ts Outdated
    Comment threadtypes/obs-store.d.ts Outdated
    Comment threadpackages/keyring-controller/src/KeyringController.ts Outdated
    Comment threadpackages/keyring-controller/src/KeyringController.ts Outdated
    Comment threadpackages/keyring-controller/src/KeyringController.test.ts Outdated
    Comment threadpackages/keyring-controller/src/KeyringController.ts Outdated

    @mcmiremcmire left a comment

    Copy link
    Copy Markdown
    Collaborator

    Choose a reason for hiding this comment

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

    Looked over these changes and they make sense to me.

    @mcmire

    Copy link
    Copy Markdown
    Collaborator

    Would it be worth it to list these in the changelog as well?

    • BREAKING: Rename Keyring type to KeyringObject
    • BREAKING:addNewAccount now throws if address of new account is not a hex string
    • BREAKING:exportSeedPhrase now throws if first keyring does not have a mnemonic
    • BREAKING:verifySeedPhrase now throws if HD keyring does not have a mnemonic
    • CHANGED: Update return type of getAccountKeyringType to Promise<string>

    Also you had listed that the setLocked method is now async, but it seems like that was already the case before, no?

    @mikesposito
    mikespositoforce-pushed the chore/update-eth-keyring-controller branch from 4ac5946 to 62e1617CompareJuly 18, 2023 18:38
    @mikesposito
    mikesposito merged commit 4eb66e5 into mainJul 18, 2023
    @mikesposito
    mikesposito deleted the chore/update-eth-keyring-controller branch July 18, 2023 18:45
    plasmacorral pushed a commit to MetaMask/metamask-mobile that referenced this pull request Jan 6, 2024
    ## **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](MetaMask/core#1441))
    - **BREAKING**: Constructor `KeyringControllerOptions` type changed
    ([#1441](MetaMask/core#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](MetaMask/core#1441))
    - **BREAKING**: The `signTypedMessage` method now returns a
    `Promise<string>` ([#1441](MetaMask/core#1441))
    - **BREAKING**: The `signTransaction` method now requires a
    `TypedTransaction` from `@ethereumjs/tx@^4` for the `transaction`
    argument, and returns a `Promise<TxData>`
    ([#1441](MetaMask/core#1441))
    - **BREAKING:** Rename `Keyring` type to `KeyringObject`
    ([#1441](MetaMask/core#1441))
    - **BREAKING:** `addNewAccount` now throws if address of new account is
    not a hex string ([#1441](MetaMask/core#1441))
    - **BREAKING:** `exportSeedPhrase` now throws if first keyring does not
    have a mnemonic ([#1441](MetaMask/core#1441))
    - **BREAKING:** `verifySeedPhrase` now throws if HD keyring does not
    have a mnemonic ([#1441](MetaMask/core#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**
    <!-- If applicable, add screenshots and/or recordings to visualize the
    before and after of your change. -->
    ### **Before**
    <!-- [screenshots/recordings] -->
    ### **After**
    <!-- [screenshots/recordings] -->
    ## **Pre-merge author checklist**
    - [ ] I’ve followed [MetaMask Coding
    Standards](https://github.com/MetaMask/metamask-mobile/blob/main/.github/guidelines/CODING_GUIDELINES.md).
    - [ ] 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](https://jsdoc.app/) format
    if applicable
    - [ ] I’ve applied the right labels on the PR (see [labeling
    guidelines](https://github.com/MetaMask/metamask-mobile/blob/main/.github/guidelines/LABELING_GUIDELINES.md)).
    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**
    - [X ] I've manually tested the PR (e.g. pull and build branch, run the
    app, test code being changed).
    - [X ] 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.
    Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

    Labels

    None yet

    Projects

    None yet

    Development

    Successfully merging this pull request may close these issues.

    4 participants

    @mikesposito@legobeat@mcmire@Gudahtt