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

refactor!: LedgerKeyring implements Keyring type #194

Draft
wants to merge 9 commits into
base: main
Choose a base branch
from

Conversation

mikesposito
Copy link
Member

@mikesposito mikesposito commented Feb 5, 2025

The LedgerKeyring is being overhauled to implement the Keyring type from @metamask/utils. Most of the changes are due to the addresses type that is now Hex

### Changed

- **BREAKING:** `LedgerKeyring` now implements the `Keyring<Json>` type ([#194](https://github.com/MetaMask/accounts/pull/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 `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

### Removed

- **BREAKING:** The `exportAccount` method has been removed

@mikesposito mikesposito marked this pull request as ready for review February 5, 2025 12:36
@mikesposito mikesposito requested a review from a team as a code owner February 5, 2025 12:36
Comment on lines +212 to +216
async unlock(hdPath?: string, updateHdk = true): Promise<Hex> {
if (this.isUnlocked() && !hdPath) {
return 'already unlocked';
return this.#getChecksumHexAddress(
ethUtil.publicToAddress(this.hdk.publicKey, true).toString('hex'),
);
Copy link
Member Author

Choose a reason for hiding this comment

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

I'm not sure how to handle this case here. The original code returned already unlocked if:

  • there is already a this.hdk defined
  • hdPath is not passed to the function (i.e. the consumer calls keyring.unlock()

However, since we want this function to return a value of type Hex we must return a valid address. Based on the lines below the actual publicKey returned from this function comes from the device (payload.publicKey) and it's used to construct an HDKey to be stored in this.hdk. So, the new code assumes that we want to return the address derived from the public key stored in this.hdk.publicKey

@mikesposito mikesposito marked this pull request as draft February 5, 2025 15:48
@mikesposito
Copy link
Member Author

Marking the PR as draft again because a new @metamask/utils major version is being prepared for release, which will ship breaking changes to the Keyring type

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

LedgerKeyring class from @metamask/eth-ledger-bridge-keyring should implement the Keyring type
1 participant