Our mobile team reports that there are a lot of errors that trace back to this line https://github.com/MetaMask/KeyringController/blob/main/index.js#L679 -- we can improve this error message to provide additional details that will be helpful in diagnosing problems. At a minimum including when there is no address provided, when there are no candidates and/or winners, would be beneficial in troubleshooting. cc @andrepimenta

MetaMask does not currently support multiple keyrings.
Still, the keyring array can contain more than one HD keyring.

What I believe is a bug, when entering a password, the keyring does duplicate in the array, because submitPassword() calls unlockKeyrings() calls restoreKeyring() calls this.keyrings.push(), leading to duplicate keyrings.

See this scenario:

const password = 'abcd';
const keyringController = new KeyringController({
        initState: clone(firstTimeState),
        encryptor: mockEncryptor,
})

await keyringController.createNewVaultAndKeychain(password)
await keyringController.persistAllKeyrings()

// keyringController.keyrings.length = 1
await keyringController.submitPassword(password + 'a')
// keyringController.keyrings.length) = 2
await keyringController.submitPassword('')
// keyringController.keyrings.length) = 3

While I believe this currently does not cause any malfunction in MetaMask, I believe it is unintended and potentially dangerous:

• MetaMask does only support 1 HD keyring and hardcodes it so it always shows the seed phrase for the first keyring. So there is the possibility to show the wrong seed phrase. However currently it is not the case because it's the same keyring who is duplicated.

• This is also a memory leak and leads to higher memory usage for the user.

GridPlus' keyring recently added a state caching mechanism under a mistaken assumption that serialize would be called when the MetaMask extension locked. Since the Lattice is an internet-connected device with its own state, we would normally need to asynchronously connect to the device in order to sync state, which takes 2-4 seconds, depending on internet connection. With the latest changes, we can now dry state and rehydrate a connection without having to do any asynchronous calls.

However, this is dependent on they keyring controller persisting keyring state or else every time the extension locks, we will lose our most recent state data and we will need to asynchronously reconnect with a retry mechanism (which is actually slower than initially forcing asynchronous resync like we were doing before, since it adds another call that ends up failing and forcing the resync). This would not be an issue if setLocked called persistAllKeyrings to save state.

Is there any reason you do not call persistAllKeyrings in setLocked? And if the answer is no, would you be open to adding that serialization hook?

🚨 You need to enable Continuous Integration on all branches of this repository. 🚨

To enable Greenkeeper, you need to make sure that a commit status is reported on all branches. This is required by Greenkeeper because we are using your CI build statuses to figure out when to notify you about breaking changes.

Since we did not receive a CI status on the greenkeeper/initial branch, we assume that you still need to configure it.

If you have already set up a CI for this repository, you might need to check your configuration. Make sure it will run on all new branches. If you don’t want it to run on every branch, you can whitelist branches starting with greenkeeper/.

We recommend using Travis CI, but Greenkeeper will work with every other CI service as well.

Once you have installed CI on this repository, you’ll need to re-trigger Greenkeeper’s initial Pull Request. To do this, please delete the greenkeeper/initial branch in this repository, and then remove and re-add this repository to the Greenkeeper integration’s white list on Github. You'll find this list on your repo or organization’s settings page, under Installed GitHub Apps.

Is the key returned by getEncryptionPublicKey intended to be the wallet public key? Or is it something else?

In other words, should this test be green:

      await keyring.deserialize([testAccount.key]);

      const encryptionPublicKeyB64 = await keyring.getEncryptionPublicKey(testAccount.address);
      const encryptionPublicKey = Buffer.from(naclUtil.decodeBase64(encryptionPublicKeyB64));
      
      const address = ethUtil.publicToAddress(encryptionPublicKey);
      const addressHex = ethUtil.bufferToHex(address);
      expect(addressHex).toBe(testAccount.address);

Hey! Getting this error when trying to instantiate new eth-keyring-controller class in my background script environment:

So service worker registration fails, I am using "@metamask/eth-keyring-controller": "^10.0.0"
Pls help!

We want to migrate this package to TypeScript to reduce regressions and improve compatibility with our other TypeScript projects.

Blocked by

• MetaMask/eth-simple-keyring#154
  • MetaMask/eth-simple-keyring#156
  • MetaMask/eth-simple-keyring#152
    • MetaMask/eth-simple-keyring#151
      • MetaMask/eth-simple-keyring#155
        • MetaMask/eth-simple-keyring#148
      • MetaMask/eth-simple-keyring#149
• #266
  • #263

Blocking

• #265
  • MetaMask/core#1653
    • MetaMask/snaps#1728
      • update dependencies in https://github.com/MetaMask/keyring-api/

Issue to collect commits.

See MetaMask/metamask-extension#3757

In Readme under Installation section
yarn install eth-keyring-controller --save should be replaced with yarn add eth-keyring-controller

Hello, I'm trying to fork and build this package locally to edit and use as a dependency within the metamask-extension repo. Unfortunately I've failed to do so.

I've tried linking it within the package json using yarn install <filepath> which successfully creates a link to the local module however when running yarn start on the metamask-extension repo it produces an error: TypeError: Tried to access an environmental variable "_nodeLRUCacheForceNoSymbol" that wasn't declared in builds.yml I did try adding the env like it says however it keeps asking for envs to add to builds.yml which ultimately doesn't work.

I've also tried just putting the package directly inside the metamask-extension repo and change the imports to the filepath of the local package. But this caused many dependency issues.

Is there a better way to go about this? If you have any ideas I'd love to know. Thanks.

npm ERR! code ENOSELF
npm ERR! Refusing to install package with name "eth-keyring-controller" under a package
npm ERR! also called "eth-keyring-controller". Did you name your project the same
npm ERR! as the dependency you're installing?
npm ERR!
npm ERR! For more information, see:
npm ERR! https://docs.npmjs.com/cli/install#limitations-of-npms-install-algorithm

npm ERR! A complete log of this run can be found in:
npm ERR! /Users/zhangyi/.npm/_logs/2018-03-29T09_50_04_581Z-debug.log

• Case: a 3rd party, or an auditor you've engaged, wants to audit cryptography code.
• Problem: It takes a long time to understand what's happening and where.
• Solution: prepare the audit trail for them and place it somewhere in readme or docs.

I've prepared one and can submit a PR. Check out the graphs and audit trail: https://gist.github.com/paulmillr/4f268f448d4e4dc31ff99503f6935a29

Description

As part of the new KeyringController API, there's a need to improve the current encryption logic for the vault.

Today the user's password directly encrypts the vault, there's an improvement opportunity where the KeyringController can use an encryption key to encrypt the vault. Next is presented the specific steps for two use cases,

Use Case: Lock the vault

1. Create or get encryption-key
2. Encrypt vault with encryption-key
3. Encrypt encryption-key with password

Use Case: Unlock the vault

1. Decrypt encryption-key with password
2. Decrypt vault with encryption-key

Acceptance Criteria

• The encryption key must have a defined use policy
• All new code must have tests
• All new code must be document

Reference

• API Re-design Proposal

After the TS refactor the current code coverage is,

----------------------|---------|----------|---------|---------|----------------------------------------------------------------------------------------------
File                  | % Stmts | % Branch | % Funcs | % Lines | Uncovered Line #s                                                                            
----------------------|---------|----------|---------|---------|----------------------------------------------------------------------------------------------
All files             |   90.43 |    68.67 |   92.59 |    90.2 |                                                                                              
 KeyringController.ts |    89.3 |    67.09 |   95.83 |   89.18 | 285,305,391-397,421,449,473,493-499,522,542,562,601,606,678,761,819,825,863,891,906,939,1034 
 constants.ts         |     100 |      100 |     100 |     100 |                                                                                              
 index.ts             |     100 |      100 |      50 |     100 |                                                                                              
----------------------|---------|----------|---------|---------|----------------------------------------------------------------------------------------------

The code coverage needs to be increased to a 100% and to keep it at that level it's also needed to enable jest-it-up.

Currently the keyringController.encryptor is typed to any which provides no value to the codebase. This issue aims to the definition of the proper data type.

Refer to #202 (comment)

Hello, I'm trying to figure out how the keyring API will change after the release of EIP1559. ethers.io now supports EIP1559 and EIP2930 transaction types ([link]https://github.com/ethers-io/ethers.js/blob/master/packages/transactions/src.ts/index.ts#L166).

How should these transactions be built by 3rd party keyrings?

Although we use Browserify, many people use Webpack, so we should document how to use this library with Webpack.

We should strive for 100% unit test coverage, and to cover all input boundary conditions.

Currently, the encryptor class variable (and the corresponding constructor option) is typed with any.
As a controller client can inject its own encryptor methods, the minimum encryptor interface needed by the controller should be clear.

Moreover, the solution should take into account that:

• MetaMask extension and mobile use different encryptors
  • MetaMask extension uses the default encryptor value, provided by KeyringController, which is @metamask/browser-passworder
  • MetaMask mobile injects its own encryptor.
• When cacheEncryptionKey options is set to true, KeyringController needs an encryptor capable of exporting the encryption key string along with encrypted/decrypted strings. This is only needed by the extension, when using MV3
  • This means that when cacheEncryptionKey is set to true, the encryptor must implement additional methods specific for this

Example: https://github.com/MetaMask/KeyringController/actions/runs/4735751596/jobs/8406395379

I was working on this issue MetaMask/metamask-extension#2349 and turned out it would be easier if addNewAccount returns the new address instead of the state Object, but I'd like to avoid breaking change with this proposal...

Is there any way to know the new address in the callback of addNewAccount?

Blocked by

• publish @metamask/[email protected]
  • MetaMask/eth-hd-keyring#101
    • MetaMask/eth-hd-keyring#100
      • MetaMask/eth-hd-keyring#99

Blocking

• #265

This is another issue that I think does not lead to a bug in the end product, but I think is unintended and could potentially be dangerous when developing the extension further.

On line 160 and onwards:

const keyring = new Keyring(opts)
return keyring.deserialize(opts)
.then((accounts) => {
  return this.checkForDuplicate(type, accounts)
})

I don't know if you realize that you create a keyring assign it to a variable and then override it immediately (assuming an HD Key).

What I fear is that by accident the wrong seed phrase can be read, like this:

const keyring = new Keyring(opts)
// This seed will be overidden by the next line
const seed = keyring.mnemonic
return keyring.deserialize(opts)
.then((accounts) => {
  const seed2 = keyring.mnemonic
  // Not anymore the same as above
  // seed !== seed2
})

Why this happens is pretty obvious when reading the eth-hd-keyring source code.

It also does the unncecessary work of generating a seed, private key etc. twice.