Add method for setting the hdpath #107
Conversation
index.js
Outdated
| if (this.hdPath !== hdPath) { | ||
| this.hdk = new HDKey() | ||
| } | ||
| this.hdPath = hdPath |
There was a problem hiding this comment.
Did you consider resetting the other instance variables? (e.g. account).
There was a problem hiding this comment.
The instance variables paths and unlockedAccount are still not being reset.
There was a problem hiding this comment.
Thanks. Fixed in latest commit
There was a problem hiding this comment.
Could you add unit tests for this? Also a JSDoc comment would be great, as we will be migrating this to our new lint config eventually with requires JSDoc comments for all functions.
A cautionary note might be warranted for this method as well, as this could be an extremely dangerous method. With the Ledger equivalent, we found that calling it with HD paths not supported by Ledger would result in accounts getting generated incorrectly, leaving the user with an account they can't actually sign on behalf of. We never got to the bottom of why that happened, opting instead to stop using it in that manner, but it's a good example of why we should only use this with HD paths that we are confident are supported by Trezor. Thoroughly QA'd.
…uding the slip0044 testnet path
danjm
force-pushed
the
add-setHdPath-method
branch
from
34c087e
to
4bbe807
Compare
|
I have added tests and a documentation comment. Also, given the sensitivity of this method, I updated it so that it only allows setting the hdPath to ones that we explicitly allow. The two paths that are currently allowed are the current default path, and the path for ethereum testnets as defined by the Slip-0044 spec. This gives us a path to allowing users to selecting that hdPath, while the default behaviour can still be to use the same (mainnet) hdPath on all networks by default. I am here thinking of addressing this issue: MetaMask/metamask-extension#10450, and also making it easier for us to QA trezor. |
|
Good idea! Having a list of allowed HD paths makes sense. |
Co-authored-by: Mark Stacey <markjstacey@gmail.com>
Co-authored-by: Mark Stacey <markjstacey@gmail.com>
Co-authored-by: Mark Stacey <markjstacey@gmail.com>
Currently, the trezor keyring controller cannot easily have it's hdpath updated. Making it hard to connect wallets on other chains or which were generated with different hdpaths in other wallets. This PR adds a method, the same as the method we have in the ledger keyring, that allows the client to set the hd path after class instantiation.