refactor!: migrate `HdKeyring` to typescript #166

mikesposito · 2025-01-28T14:54:54Z

The @metamask/eth-hd-keyring is being migrated to Typescript.

The changes applied can be seen as diff in this commit: bc89c46

This package migration can be tested on this extension PR: MetaMask/metamask-extension#29961

There is no consumer-facing change, besides this one:

BREAKING: Added types to HdKeyring
- All methods on HdKeyring retain their existing signatures, but now have types.

Fixes: #92

socket-security · 2025-01-28T14:55:09Z

Updated dependencies detected. Learn more about Socket for GitHub ↗︎

Package	New capabilities	Transitives	Size	Publisher
npm/@metamask/[email protected] 🔁 npm/@metamask/[email protected]	None	`+39`	6.05 MB	gudahtt

View full report↗︎

mikesposito · 2025-01-28T15:35:48Z

packages/keyring-eth-hd/src/index.ts

+    opts: HDKeyringAccountSelectionOptions = {},
+  ): Promise<string> {
+    const privKey = this.#getPrivateKeyFor(withAccount, opts);
+    const publicKey = getEncryptionPublicKey(bytesToHex(privKey));


In the original implementation privKey was passed to getEncryptionPublicKey directly:

const publicKey = getEncryptionPublicKey(privKey);

Though, the addition of types evidenced that getEncryptionPublicKey only accept hex strings, hence the conversion with bytesToHex

mikesposito · 2025-01-28T15:36:10Z

packages/keyring-eth-hd/src/index.ts

+  /**
+   * Get the wallet for the specified account.
+   *
+   * @param account - The address of the account.
+   * @returns The wallet for the account as HDKey.
+   */
+  #getWalletForAccount(account: string): HDKey;
+
+  /**
+   * Get the wallet for the specified account and app origin.
+   *
+   * @param account - The address of the account.
+   * @param opts - The options for selecting the account.
+   * @returns A key pair representing the wallet.
+   */
+  #getWalletForAccount(
+    accounts: string,
+    opts: { withAppKeyOrigin: string },
+  ): { privateKey: Buffer; publicKey: Buffer };


This internal method is overloaded because the objects returned can vary significantly based on whether an app key origin is specified (i.e. a Buffer keypair is returned instead of an entire HDKey instance).

The actual signature of the implementation is left unchanged.

mikesposito · 2025-01-28T15:37:15Z

packages/keyring-eth-hd/package.json

-    "@metamask/eth-hd-keyring": "4.0.1",
    "@ts-bridge/cli": "^0.6.1",
    "@types/jest": "^29.5.12",
    "deepmerge": "^4.2.2",
-    "jest": "^29.5.0"
+    "jest": "^29.5.0",
+    "old-hd-keyring": "npm:@metamask/eth-hd-keyring@^4.0.1"


This package used for testing backward compatibility was overwriting the current version of @metamask/eth-hd-keyring, causing TS to apply the same types of the new package. With this line we can import the package with a different name.

The version 4.0.1 was pinned, but yarn lint was failing and I guess that ^4.0.1 serves the same purpose

mikesposito · 2025-01-28T15:37:59Z

packages/keyring-eth-hd/test/index.test.ts

There should be no functional change in these test cases, besides the ones to make TS happy

packages/keyring-eth-hd/CHANGELOG.md

mikesposito · 2025-01-28T16:12:26Z

@metamaskbot publish-preview

github-actions · 2025-01-28T16:14:00Z

Preview builds have been published. See these instructions (from the core monorepo) for more information about preview builds.

Expand for full list of packages and versions.

{
  "@metamask-previews/keyring-api": "15.0.0-15c90e6",
  "@metamask-previews/eth-hd-keyring": "9.0.1-15c90e6",
  "@metamask-previews/eth-ledger-bridge-keyring": "8.0.3-15c90e6",
  "@metamask-previews/eth-simple-keyring": "8.0.1-15c90e6",
  "@metamask-previews/eth-trezor-keyring": "6.0.1-15c90e6",
  "@metamask-previews/keyring-internal-api": "3.0.0-15c90e6",
  "@metamask-previews/keyring-internal-snap-client": "3.0.1-15c90e6",
  "@metamask-previews/eth-snap-keyring": "9.0.0-15c90e6",
  "@metamask-previews/keyring-snap-client": "3.0.1-15c90e6",
  "@metamask-previews/keyring-snap-sdk": "2.1.0-15c90e6",
  "@metamask-previews/keyring-utils": "1.3.0-15c90e6"
}

mikesposito · 2025-01-28T16:23:49Z

packages/keyring-eth-hd/src/index.ts

+      // WARN: verify this cast to Buffer
+      msgSig.v as unknown as Buffer,


It looks like @metamask/eth-sig-util only accepts a Buffer here, while msgSig.v seems to be a bigint

Hmm, does that cause an issue? A bigint is not a Buffer.

The implementation is unchanged and is the same that is in prod already:

accounts/packages/keyring-eth-hd/src/index.js

Line 185 in 3111c56

const rawMsgSig = concatSig(msgSig.v, msgSig.r, msgSig.s);

- bigint is not a Buffer but that's what we have always passed to concatSig (which expects a Buffer)

Though now the same code is throwing a type error. I think I left that // WARN there to come back at this and see if we can apply a conversion without breaking things

Ah, strange, okay.

mikesposito · 2025-01-28T16:29:36Z

packages/keyring-eth-hd/test/index.test.ts

@@ -154,7 +159,7 @@ describe('hd-keyring', () => {
        numberOfAccounts: 2,
      });

-      const accounts = await keyring.getAccounts();
+      const accounts = keyring.getAccounts();


getAccounts() was awaited everywhere in the tests, but the function implementation was not marked as async

we can change the tests here, and then change them back along with fixing the implementation when HDKeyring will implement the Keyring type

mcmire

Looks like a good conversion overall, just had some questions on specific lines.

mcmire · 2025-01-28T16:24:38Z

packages/keyring-eth-hd/src/index.ts

+      this.#wallets.push(wallet);
+    }
+    const hexWallets = newWallets.map((wallet) => {
+      return this.#addressfromPublicKey(wallet.publicKey as Uint8Array);


Why do we have to make this type assertion?

mcmire · 2025-01-28T16:26:22Z

packages/keyring-eth-hd/src/index.ts

+   */
+  getAccounts(): string[] {
+    return this.#wallets.map((wallet) => {
+      assert(wallet.publicKey, 'Expected public key to be set');


Should we simply throw an error instead of using assert? Otherwise this library is Node-only (or else, assert needs to be polyfilled). Unless it's already Node-only?

Suggested change

assert(wallet.publicKey, 'Expected public key to be set');

if (wallet.publicKey === undefined) {

throw new Error('Expected public key to be set');

}

Hmm, this assert comes from @metamask/utils instead of the built-in Node.js' assert. Looking at its implementation it seems to be compatible with a browser environment as well: https://github.com/MetaMask/utils/blob/90837bad1d85277aa5b8f16fe40beb81e0a1e09d/src/assert.ts#L81

Having this in mind, do you still think we should substitute these with explicit if undefined ?

mcmire · 2025-01-28T16:27:30Z

packages/keyring-eth-hd/src/index.ts

+    const wallet = this.#getWalletForAccount(address, {
+      withAppKeyOrigin: origin,
+    });
+    assert(wallet.publicKey, 'Expected public key to be set');


Similar as above?

Suggested change

assert(wallet.publicKey, 'Expected public key to be set');

if (wallet.publicKey === undefined) {

throw new Error('Expected public key to be set');

}

mcmire · 2025-01-28T16:28:55Z

packages/keyring-eth-hd/src/index.ts

+      ? this.#getWalletForAccount(address, opts)
+      : this.#getWalletForAccount(address);
+    const { privateKey } = wallet;
+    assert(


Another case where we could possibly throw an error manually instead of using assert? (You get the idea)

mcmire · 2025-01-28T16:30:28Z

packages/keyring-eth-hd/src/index.ts

+    const msgSig = ecsign(Buffer.from(message, 'hex'), Buffer.from(privKey));
+    const rawMsgSig = concatSig(
+      // WARN: verify this cast to Buffer
+      msgSig.v as unknown as Buffer,


~~Curious, why do we need to make a type assertion here?~~ I think you answered this question above

mcmire · 2025-01-28T16:32:00Z

packages/keyring-eth-hd/src/index.ts

+
+    const privateKey = this.#getPrivateKeyFor(withAccount, opts);
+    return signTypedData({
+      privateKey: Buffer.from(privateKey),


It looks like we are converting privateKey to a Buffer when we weren't before. Is this a breaking change or was this supposed to be this way all along?

Since #getPrivateKeyFor returns a Uint8Array or a Buffer depending on whether we are passing { withAppKeyOrigin: string } as opts, signTypedData could receive something that is not a Buffer.

This is to say that it was supposed to be this way all along. Using Buffer.from(privateKey) should convert the Uint8Array to a Buffer or just return another Buffer with the same content as the original privateKey

mcmire · 2025-01-28T16:52:11Z

packages/keyring-eth-hd/src/index.ts

+    mnemonic: Buffer | JsCastedBuffer | string | Uint8Array | number[],
+  ): Uint8Array {
+    let mnemonicData: unknown = mnemonic;
+    // when encrypted/decrypted, buffers get cast into js object with a property type set to buffer


This explanation sounds a bit vague to me. What does it mean to cast a buffer into a JS object? Are we trying to serialize the buffer? I guess we could leave this comment here for now but I'm curious if there is a more descriptive name for this function.

The comment is left unchanged from the original repo, though I think that it is referring to the output of buffer.toJSON() which returns an object with this shape:

{ type: 'Buffer', data: [ 71, 101, 101, 107, 115, 102, 111, 114, 71, 101, 101, 107, 115 ] }

Perhaps we can change it to be something like this:

Suggested change

// when encrypted/decrypted, buffers get cast into js object with a property type set to buffer

// When using Buffer.toJSON(), the Buffer is serialized into an object

// with the structure `{ type: 'Buffer', data: [...] }`

mcmire · 2025-01-28T16:54:00Z

packages/keyring-eth-hd/src/index.ts

+   * @param publicKey - The public key of the account.
+   * @returns The address of the account.
+   */
+  #addressfromPublicKey(publicKey: Uint8Array): string {


Does it make sense for this return type to be Hex? Or does that screw things up elsewhere?

(I think we have a function in @metamask/utils that we can use instead of bufferToHex which already returns a Hex, so if we want to / plan on using that later, that's fine, too.)

I changed all types related to addresses from string to Hex. I guess you were referring to add0x which is now used to normalize addresses when needed (added HdKeyring.#normalizeAddress): 4d86d8e

mikesposito · 2025-01-29T12:38:51Z

The following extension PR is using a package preview from this PR, which appears to be working fine: MetaMask/metamask-extension#29961

mikesposito added 3 commits January 28, 2025 15:47

rename files

e83b6e3

migrate HdKeyring to typescript

bc89c46

rename @metamask/eth-hd-keyring@4 to old-hd-keyring

ae242da

mikesposito mentioned this pull request Jan 28, 2025

refactor!: remove HdKeyring as default export #161

Draft

edit changelog entries

02928f5

mikesposito changed the title ~~Migrate HdKeyring to typescript~~ refactor!: migrate HdKeyring to typescript Jan 28, 2025

mikesposito marked this pull request as ready for review January 28, 2025 15:31

mikesposito requested a review from a team as a code owner January 28, 2025 15:31

mikesposito requested review from cryptodev-2s, Gudahtt and mcmire January 28, 2025 15:31

mikesposito commented Jan 28, 2025

View reviewed changes

revert remove default export

8d0c282

mikesposito commented Jan 28, 2025

View reviewed changes

packages/keyring-eth-hd/CHANGELOG.md Outdated Show resolved Hide resolved

mikesposito and others added 2 commits January 28, 2025 16:51

Update packages/keyring-eth-hd/CHANGELOG.md

6e3ede7

fix changelog PR number

15c90e6

revert: getAccounts returns promise

2e07ee6

mikesposito commented Jan 28, 2025

View reviewed changes

test: remove await from getAccounts

f9883bc

mikesposito commented Jan 28, 2025

View reviewed changes

mcmire reviewed Jan 28, 2025

View reviewed changes

refactor: change JsCastedBuffer to SerializedBuffer

46687f4

mikesposito mentioned this pull request Jan 29, 2025

chore: update @metamask/eth-hd-keyring MetaMask/metamask-extension#29961

Draft

7 tasks

mikesposito and others added 2 commits January 29, 2025 14:12

refactor: use Hex instead of String

4d86d8e

Merge branch 'main' into mikesposito/hdkeyring-ts-only

6ca6fd7

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor!: migrate `HdKeyring` to typescript #166

refactor!: migrate `HdKeyring` to typescript #166

mikesposito commented Jan 28, 2025 •

edited

Loading

socket-security bot commented Jan 28, 2025 •

edited

Loading

mikesposito Jan 28, 2025

mikesposito Jan 28, 2025

mikesposito Jan 28, 2025

mikesposito Jan 28, 2025

mikesposito commented Jan 28, 2025

github-actions bot commented Jan 28, 2025

mikesposito Jan 28, 2025

mcmire Jan 28, 2025

mikesposito Jan 28, 2025

mcmire Jan 29, 2025

mikesposito Jan 28, 2025

mikesposito Jan 28, 2025

mcmire left a comment

mcmire Jan 28, 2025

mcmire Jan 28, 2025

mikesposito Jan 29, 2025 •

edited

Loading

mcmire Jan 28, 2025

mcmire Jan 28, 2025

mcmire Jan 28, 2025 •

edited

Loading

mcmire Jan 28, 2025

mikesposito Jan 29, 2025 •

edited

Loading

mcmire Jan 28, 2025

mikesposito Jan 29, 2025

mcmire Jan 28, 2025

mikesposito Jan 29, 2025 •

edited

Loading

mikesposito commented Jan 29, 2025

		// WARN: verify this cast to Buffer
		msgSig.v as unknown as Buffer,

-      assert(wallet.publicKey, 'Expected public key to be set');
+      if (wallet.publicKey === undefined) {
+        throw new Error('Expected public key to be set');
+      }

	// when encrypted/decrypted, buffers get cast into js object with a property type set to buffer
	// When using Buffer.toJSON(), the Buffer is serialized into an object
	// with the structure `{ type: 'Buffer', data: [...] }`

refactor!: migrate HdKeyring to typescript #166

Are you sure you want to change the base?

refactor!: migrate HdKeyring to typescript #166

Conversation

mikesposito commented Jan 28, 2025 • edited Loading

socket-security bot commented Jan 28, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mikesposito commented Jan 28, 2025

github-actions bot commented Jan 28, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mcmire left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mikesposito Jan 29, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mcmire Jan 28, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mikesposito Jan 29, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mikesposito Jan 29, 2025 • edited Loading

Choose a reason for hiding this comment

mikesposito commented Jan 29, 2025

refactor!: migrate `HdKeyring` to typescript #166

refactor!: migrate `HdKeyring` to typescript #166

mikesposito commented Jan 28, 2025 •

edited

Loading

socket-security bot commented Jan 28, 2025 •

edited

Loading

mikesposito Jan 29, 2025 •

edited

Loading

mcmire Jan 28, 2025 •

edited

Loading

mikesposito Jan 29, 2025 •

edited

Loading

mikesposito Jan 29, 2025 •

edited

Loading