metamask/eth-trezor-keyring

A JS wrapper around Trezor Connect libraries, to support the KeyringController protocol used by MetaMask

eth-trezor-keyring's Issues

Add a LICENSE file

This project is missing a LICENSE file

Rename Trezor's `dispose` method to `destroy`

Currently, the type we use for Keyrings does not include a "destroy" function. On the other hand, Ledger and Trezor keyring have a function for this and is named dispose for Trezor and destroy for Ledger.

We should align the two hardware keyrings, renaming Trezor's dispose to destroy

Rename package to use `@metamask` scope

We should rename this package to use the @metamask/ scope.

TrezorKeyring should implement Keyring interface

After merging the TypeScript migration PR (#161) we should now implement the Keyring interface from utils package as for other keyrings.

Migrate to TypeScript

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

Enable Pipelines in CircleCI

From an email from CircleCI:

You are receiving this email because you are a member of one or more paid projects building on CircleCI that does not have pipelines enabled.

We are reaching out to let you know about two upcoming changes to CircleCI:

• Beginning on March 31, 2020, we will start automatically enabling pipelines for all projects. Turning on pipelines will allow your team to access new features. Watch our webinar recording or read our docs for detailed information about the transition to pipelines and the benefits that you can expect to see.
• The ability to opt-out of our new user interface will be removed starting on April 15th, 2020. Our team has been hard at work building a new interface that is more responsive and makes it easier for you to accomplish your most important tasks in CircleCI. Beginning on April 15, we will start to remove the ability to switch back to our old UI.

To get ahead of these changes we recommend that you update your project settings to enable pipelines and opt in to the new UI if you haven’t already. At the end of this message, we have included links to the settings for your projects that are not yet using pipelines to make it easy to make the switch.

We’re constantly working to improve CircleCI. These changes will help us deliver the best possible experience for our customers.

From the project settings on CircleCI:

Enable pipelines

Turn on pipelines for this project to gain access to CircleCI’s improved UI and other new features including reusable config, auto-cancellation of redundant builds, and more.

If you experience any issues when enabling pipelines, check out these troubleshooting tips.

NOTE: If for any reason you’d like to disable pipelines again for a project on a usage-based plan, you can do so by filing a ticket with CircleCI support.

Migrate from Mocha/Chai to Jest

The module template uses Jest as a standard test library. We should migrate from Mocha/Chai to it.

• Remove mocha, chai, and chai-spies as dependencies and replace it with jest
• Copy jest.config.js from the module template
• Convert existing tests from Mocha/Chai syntax to Jest
• Replace the test script in package.json with the equivalent jest line (see the module template), and add a test:watch script
• Run yarn test. If Jest does not report that 100% test coverage has not been achieved, then add jest-it-up as a development dependency; otherwise, adjust the coverage thresholds in jest.config.js to match the numbers reported.

Update Node version

This project is currently using 8.11.3, we should upgrade to an LTS version

Sync linting pipeline with module template

Our ESLint config is out of date with our other repos, and this repo is missing depcheck.

• Copy the versions of our ESLint config packages that the module template is using. If this issue is addressed before #156 and #165, we would want to omit Jest and TypeScript packages.
• Ensure that .eslintrc.js is up to date. If this issue is addressed before #156 and #165, we would want to omit Jest and TypeScript-related config.
• Copy .prettierrc.js from the module template
• Ensure that all lint:* scripts in the module template are reflected in this repo.
• Add depcheck as a development dependency, and copy .depcheckrc.json from the module template.
• Fix lint violations caused by the upgrade. Running yarn lint:fix should automatically fix most of them, but we will need to manually fill in missing JSDocs.

Update trezor connect dependency: auto-closing of popup mitigation

Auto closing popup

The following change in Trezor Connect 9.1.1 introduces a mitigation about the auto-closing of popups.

feat(connect-popup): when a call to TrezorConnect returns success: false popup remains opened and displays error page instead.

This is driven by the client, and requires an NPM upgrade. This is an ongoing issue and MetaMask users are still experiencing it.

The goal is users will be able to report which errors are occurring.

Missing package are now available

Patches like this can be removed. Since the missing dependency is available on npm.

Screenshot

Add Ledger hd paths support on Trezor device

Hi, I have made my Trezor with migrated Ledger seed worked on Metamask. But I am amateur at this so I'd like to open an issue for discussion before opening a PR.

The reason to do this is because when I imported my broken Ledger seed to the new Trezor, the metamask could not find the correct address, they are using different path. But this totally don't make sense to me, seeds should be interoperability.

So I made these changes to make it work. Tested on my Trezor device.

First, for eth-trezor-keyring, allow Ledger paths:
Lafudoci@ca525ac

Second, add Ledger path options to metamask trezor connection script:
Lafudoci/metamask-extension@49d433e#diff-0a2a225e26cc8840b2d6e5b4acf30923de787fbcf19efb5766f04ba8bd8e433f

Besides, change safety checks setting from strict to prompt in the Trezor suite is also needed.

How do you think? Are these changes good to put into public release? I'll need to open 2 PR for eth-trezor-keyring and metamask.

Remove the assert devDependency

The assert dependency is intended for the browser—this project uses it in Node.js where we can instead leverage the built-in assert.

Remove one of the lockfiles

This project doesn't need two lockfiles. The package-lock.json file should be removed in favour of the yarn.lock file.

Re-Entering a passphrase

Hi,
as you can imagine, typing in a wrongly spelled / mistyped password into the trezor-device when opening a new (empty) wallet can be pretty devastating. Dont ask me how I know.
The Trezor-Suite implements a double check that requires you to put in your passphrase twice if the wallet is found to be empty.
I understand that with metamask such an empty-detection is not trivial to implement. However I would argue, for user-safety, that the dialog that prompts the address-numbers (organized in pages of 5 addresses) would benefit from a button "double-check passphrase", that the user could click on and go through the passphrase-procedure again to see if the same addresses will be derived from the second attempt.
Mistyped passwords can happen due to user-mistakes, or, if the passphrase is entered on the trezor-device, even touch-screen shenanigans.

I am not sure if it can be easily detected if the passphrase was used at all or if only the standard account was returned from the device. If such detection is possible the enable-status of such button could be linked to this condition.
Also if there are funds detected within the displayed page of 5 addresses (if any address has funds other than 0.00000 ETH) such a button could be grayed out.
If a full empty-check can be easily implemented (iterating over all networks for all addresses over a given page), then i would suggest that double-entering the passphrase should become mandatory if an empty wallet is detected.

I think for user-clarity a hint (when the mouse hovers over the button) could explain its purpose:
"Re-enter your passphrase to check your spelling. This is helpful to ensure the correctness of your passphrase when opening empty wallets."

This is purely a safety improvement suggestion.

Add nvmrc file

The project should have an nvmrc file

Trezor signedTypeData released

According to the implementation here https://github.com/MetaMask/eth-trezor-keyring/blob/main/index.js#L389 eth_signTypedData are not supported by Trezor.

However, Trezor supports it now:
Implementation : trezor/connect@08e4382
Doc: https://github.com/trezor/connect/blob/develop/docs/methods/ethereumSignTypedData.md

Do you have any plan to add this to Metamask ?

Thank you.

Add a CHANGELOG

This project should be updated to include a CHANGELOG file

Add security policy

See https://help.github.com/en/github/managing-security-vulnerabilities/adding-a-security-policy-to-your-repository

Elaborate Serializing and Deserializing of Data

There seems to be some inconsistencies with how data is serialized and deserialized in some of the Metamask keyring libraries.

For the Trezor keyring:

• this.paths is included in serialization, but not in deserialization
• this.hdk can/should be serialised and deserialised
  • Only the publicKey and chainCode need to be serialised/deserialised
• this.hdPath can be changed via deserialization without triggering the this.setHdPath method
• Some "resetting" of data can be refactored (i.e., see forgetDevice, setHdPath and the constructor)
• Setting this.hdKey can be refactored via a setHdKey method

Standardize repository

We should update this repository to follow the conventions used in our module template.

Promise from `dispose` method

The dispose method of the keyring calls a dispose function from the Trezor connect that returns a promise, but our method doesn't return that promise. That makes it impossible for the caller to know when this operation has completed. We should return the promise so that it's possible to know when the disposal has completed.

Suggested here: #161 (comment)

Use shared ESLint config

This project should be updated to use the shared ESLint config, @metamask/eslint-config