Skip to content

Comments

refactor!: addNewKeyring returns metadata instead of keyring#5372

Merged
mikesposito merged 11 commits intomainfrom
mikesposito/add-new-keyring-return-type
Feb 25, 2025
Merged

refactor!: addNewKeyring returns metadata instead of keyring#5372
mikesposito merged 11 commits intomainfrom
mikesposito/add-new-keyring-return-type

Conversation

@mikesposito
Copy link
Member

@mikesposito mikesposito commented Feb 21, 2025 •
edited
Loading

Explanation

This PR changes the return type of addNewKeyring to return a promise resolving in KeyringMetadata instead of the keyring instance. The reason is that accessing keyring instances outside of KeyringController safeguards is considered unsafe.

The withKeyring method signature has also been changed: the operation callback now will be called with {keyring: SelectedKeyring; metadata: KeyringMetadata } instead of SelectedKeyring

References

Changelog

@metamask/keyring-controller

  • BREAKING: addNewKeyring method now returns Promise<KeyringMetadata> instead of Promise<unknown> (#5372)
    • Consumers can use the returned KeyringMetadata.id to access the created keyring instance via withKeyring.
  • BREAKING: withKeyring method now requires a callback argument of type ({ keyring: SelectedKeyring; metadata: KeyringMetadata }) => Promise<CallbackResult> (#5372)

@metamask/profile-sync-controller

  • No consumer-facing changes

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
  • I've prepared draft pull requests for clients and consumer packages to resolve any breaking changes

@mikesposito mikesposito marked this pull request as ready for review February 21, 2025 11:36
@mikesposito mikesposito requested review from a team as code owners February 21, 2025 11:36
mathieuartu
mathieuartu previously approved these changes Feb 21, 2025
View reviewed changes
Copy link
Contributor

@mathieuartu mathieuartu left a comment

Choose a reason for hiding this comment

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

Owned files LGTM, thanks!

mikesposito
mikesposito commented Feb 21, 2025
View reviewed changes
packages/keyring-controller/src/KeyringController.ts
* @param keyring - The keyring instance to get the metadata for.
* @returns The keyring metadata.
*/
#getKeyringMetadata(keyring: unknown): KeyringMetadata {
Copy link
Member Author

@mikesposito mikesposito Feb 21, 2025

Choose a reason for hiding this comment

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

I'm keeping this method as internal because consumers are not supposed to carry keyring instances around

mikesposito
mikesposito commented Feb 21, 2025
View reviewed changes
packages/keyring-controller/src/KeyringController.ts
Comment on lines +1517 to +1518
keyring: SelectedKeyring;
metadata: KeyringMetadata;

This comment was marked as outdated.

Sign in to view

cryptodev-2s
cryptodev-2s approved these changes Feb 24, 2025
View reviewed changes
Copy link
Contributor

@cryptodev-2s cryptodev-2s left a comment

Choose a reason for hiding this comment

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

LGTM!

@mikesposito mikesposito enabled auto-merge (squash) February 25, 2025 14:40
@mikesposito mikesposito merged commit 8e15f01 into main Feb 25, 2025
268 checks passed
@mikesposito mikesposito deleted the mikesposito/add-new-keyring-return-type branch February 25, 2025 14:45
@mikesposito mikesposito mentioned this pull request Mar 3, 2025
Merged
7 tasks
github-merge-queue bot pushed a commit to MetaMask/metamask-extension that referenced this pull request Mar 7, 2025
@mikesposito @metamaskbot
chore: bump @metamask/{keyring, profile-sync}-controller (#30637)
74d4186 
<!--
Please submit this PR as a draft initially.
Do not mark it as "Ready for review" until the template has been
completely filled out, and PR status checks have passed at least once.
-->

## **Description**

<!--
Write a short description of the changes included in this pull request,
also include relevant motivation and context. Have in mind the following
questions:
1. What is the reason for the change?
2. What is the improvement/solution?
-->
These packages are being bumped to their latest version:
### `@metamask/keyring-controller`

- **BREAKING:** `addNewKeyring` method now returns
`Promise<KeyringMetadata>` instead of `Promise<unknown>`
([#5372](MetaMask/core#5372))
- Consumers can use the returned `KeyringMetadata.id` to access the
created keyring instance via `withKeyring`.
- **BREAKING:** `withKeyring` method now requires a callback argument of
type `({ keyring: SelectedKeyring; metadata: KeyringMetadata }) =>
Promise<CallbackResult>`
([#5372](MetaMask/core#5372))
- Bump `@metamask/keyring-internal-api` from `^4.0.3` to `^5.0.0`
([#5405](MetaMask/core#5405))
- Bump `@metamask/eth-hd-keyring` from `^10.0.0` to `^11.0.0`
([#5405](MetaMask/core#5405))
- Bump `@metamask/eth-simple-keyring` from `^8.1.0` to `^9.0.0`
([#5405](MetaMask/core#5405))

### `@metamask/profile-sync-controller`

- Bump `@metamask/keyring-internal-api` from `^4.0.3` to `^5.0.0`
([#5405](MetaMask/core#5405))

### `@metamask/eth-ledger-bridge-keyring`
- **BREAKING:** `LedgerKeyring` now implements the `Keyring` type
([#194](MetaMask/accounts#194))
    - The class does not extend `EventEmitter` anymore.
- The `LedgerKeyring.accounts` class variable is now a `readonly Hex[]`
array.
    - The `addAccounts` method signature has been changed:
- An `amount` number parameter is now required to specify the number of
accounts to add.
- The method now returns a promise resolving to an array of `Hex`
addresses.
    - The `unlock` method now returns `Promise<Hex>`.
    - The `getAccounts` method now returns `Promise<Hex[]>`.
- The `deserialize` method now requires a `LedgerKeyringSerializedState`
typed parameter.
- The `signTransaction` method now accepts an `Hex` typed value as the
`address` parameter.
- The `signMessage` method now accepts an `Hex` typed value as the
`withAccount` parameter.
- The `signPersonalMessage` method now accepts an `Hex` typed value as
the `withAccount` parameter.
- The `signTypedData` method now accepts an `Hex` typed value as the
`withAccount` parameter.
- The `unlockAccountByAddress` method now accepts an `Hex` typed value
as the `address` parameter.

[![Open in GitHub
Codespaces](https://github.com/codespaces/badge.svg)](https://codespaces.new/MetaMask/metamask-extension/pull/30637?quickstart=1)

## **Related issues**

Fixes:

## **Manual testing steps**

1. Go to this page...
2.
3.

## **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 Contributor
Docs](https://github.com/MetaMask/contributor-docs) and [MetaMask
Extension Coding
Standards](https://github.com/MetaMask/metamask-extension/blob/main/.github/guidelines/CODING_GUIDELINES.md).
- [ ] I've completed the PR template to the best of my ability
- [ ] 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-extension/blob/main/.github/guidelines/LABELING_GUIDELINES.md)).
Not required for external contributors.

## **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.

---------

Co-authored-by: MetaMask Bot <metamaskbot@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mathieuartu mathieuartu mathieuartu approved these changes

@montelaidev montelaidev montelaidev approved these changes

@cryptodev-2s cryptodev-2s cryptodev-2s approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

KeyringController addNewKeyring should not return the keyring instance

4 participants

@mikesposito @mathieuartu @montelaidev @cryptodev-2s