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

[WIP] Feature/subproviders/trezor subprovider #1431

Open
wants to merge 12 commits into
base: development
from

Conversation

Projects
None yet
3 participants
@MarcZenn
Copy link

commented Dec 13, 2018

Description

This provides a TrezorSubprovider class within 0x/subproviders package. It helps dapps integrate trezor support in conjunction with web3 provider engine so that users may retrieve their account, sign transactions, & sign personal messages using their Trezor device.

Please note that the TrezorSubprovider class MUST be initialized with trezor-connect module as such:

import TrezorConnect from 'trezor-connect';
import { TrezorSubprovider } from '@0x/subproviders';

const trezorSubproviderConfig = { trezorConnectClientApi: TrezorConnect  };

const trezorSubprovider - new TrezorSubprovider(trezorSubproviderConfig);

Testing instructions

This could be setup into a codesandbox and tested with a Trezor device so that use is able to retrieve their Trezor ethereum address, sign transactions via eth_sign and sign personal messages i.e. personal_sign.

Types of changes

  • New feature (non-breaking change which adds functionality)

Checklist:

  • Prefix PR title with [WIP] if necessary.
  • Add tests to cover changes as needed. - might need some help with this!
  • Update documentation as needed. - where/how can I update the docs?
  • Add new entries to the relevant CHANGELOG.jsons.
@fabioberger
Copy link
Contributor

left a comment

Thanks for the submission @MarcZenn!

*/
constructor(config: TrezorSubproviderConfig) {
super();
this._publicKeyPath = PRIVATE_KEY_PATH;

This comment has been minimized.

Copy link
@fabioberger

fabioberger Dec 22, 2018

Contributor

Is it the public or private key path? I know it's kind of both, but can we remain consistent here?

This comment has been minimized.

Copy link
@MarcZenn

MarcZenn Dec 31, 2018

Author

member var should now be this._privateKeyPath and consistent throughout.


if (response.success) {
const payload: TrezorGetAddressResponsePayload = response.payload;
accounts.push(payload.address);

This comment has been minimized.

Copy link
@fabioberger

fabioberger Dec 22, 2018

Contributor

Is there only one address Trezor users use? I know that for Ledger for instance, users usually use multiple addresses per device.

It's typically considered security best practice not to use the same account many times.

This comment has been minimized.

Copy link
@MarcZenn

MarcZenn Dec 31, 2018

Author

You're right. We can fetch multiple via public key. Ive updated this and it should fetch multiple addresses but I had to add a dependency ethereumjs-wallet/hdkeyto do it. I suspect you have a util somewhere that handles this and eliminates the need for that lib?

const accountIndex = this._cachedAccounts.indexOf(txData.from);

const response: TrezorConnectResponse = await this._trezorConnectClientApi.ethereumSignTransaction({
path: this._publicKeyPath + `${accountIndex}`,

This comment has been minimized.

Copy link
@fabioberger

fabioberger Dec 22, 2018

Contributor

Our convention is to always use template strings instead of concatenation. Please fix everywhere in this file.

This comment has been minimized.

Copy link
@MarcZenn

MarcZenn Dec 31, 2018

Author

updated

to: txData.to,
value: txData.value,
data: txData.data,
chainId: 1,

This comment has been minimized.

Copy link
@fabioberger

fabioberger Dec 22, 2018

Contributor

Why is this hard-coded? They might be on a testnet. Is this a required property? If so, let's have them pass in the networkId to the constructor.

This comment has been minimized.

Copy link
@MarcZenn
this._trezorConnectClientApi = config.trezorConnectClientApi;
}
/**
* Retrieve a users Trezor account. The accounts are private key path derived, This method

This comment has been minimized.

Copy link
@fabioberger

fabioberger Dec 22, 2018

Contributor
Suggested change
* Retrieve a users Trezor account. The accounts are private key path derived, This method
* Retrieve a users Trezor account. This method
* instance.
* @return An array of accounts
*/
public async getAccountsAsync(): Promise<string[]> {

This comment has been minimized.

Copy link
@fabioberger

fabioberger Dec 22, 2018

Contributor

Let's pass in an amount they wish to fetch and fetch multiple.

This comment has been minimized.

Copy link
@MarcZenn

MarcZenn Dec 31, 2018

Author

Donezo

const accounts: string[] = [];
const response: TrezorConnectResponse = await this._trezorConnectClientApi.ethereumGetAddress({
path: this._publicKeyPath,
showOnTrezor: true,

This comment has been minimized.

Copy link
@fabioberger

fabioberger Dec 22, 2018

Contributor

If we default this to false, it's less cumbersome to fetch multiple addresses. My understanding is that this doesn't actually make the process more secure, it just gives the user a false sense of security.

This comment has been minimized.

Copy link
@fabioberger

fabioberger Jan 8, 2019

Contributor

Thoughts on this @MarcZenn?

*/
// tslint:disable-next-line:prefer-function-over-method
public async signTypedDataAsync(address: string, typedData: any): Promise<string> {
throw new Error(WalletSubproviderErrors.MethodNotSupported);

This comment has been minimized.

Copy link
@fabioberger

fabioberger Dec 22, 2018

Contributor

Let's add a // TODO comment here so it's clear that this is a remaining TODO for a future dev.

This comment has been minimized.

Copy link
@MarcZenn
path: {
0: number;
1: number;
2: number;

This comment has been minimized.

Copy link
@fabioberger

fabioberger Dec 22, 2018

Contributor

Are there only 2 paths? If there are more, let's use type: {[index: number]: number} instead.

This comment has been minimized.

Copy link
@MarcZenn

MarcZenn Dec 31, 2018

Author

updated

s: string;
}

export interface TrezorSignMssgResponsePayload {

This comment has been minimized.

Copy link
@fabioberger

fabioberger Dec 22, 2018

Contributor
Suggested change
export interface TrezorSignMssgResponsePayload {
export interface TrezorSignMsgResponsePayload {

This comment has been minimized.

Copy link
@MarcZenn

MarcZenn Dec 31, 2018

Author

updated

MarcZenn added some commits Dec 31, 2018

@MarcZenn

This comment has been minimized.

Copy link
Author

commented Dec 31, 2018

@fabioberger @dekz PR should be updated now with your requested changes. Take a look at the comment responses. I was able to fetch multiple addresses although it required an additional dependency, not sure if there is already a method in place to handle that.

@fabioberger
Copy link
Contributor

left a comment

After this last requested fix, we can get this merged. Thanks for the improvements so far!

txData.gas = txData.gas ? txData.gas : '0x0';
txData.gasPrice = txData.gasPrice ? txData.gasPrice : '0x0';

const accountIndex = this._cachedAccounts.indexOf(txData.from);

This comment has been minimized.

Copy link
@fabioberger

fabioberger Jan 8, 2019

Contributor

@MarcZenn I'm still not a fan of this approach of caching the result of calling getAccountsAsync as it adds an implicit dependency where a caller to signTransactionAsync must first call getAccountsAsync, even if they ALREADY know the account from which they wish to send a transaction.

This is why we decided to implement the LedgerSubprovider to find the accountIndex given a from address using wallet_utils.findDerivedKeyInfoForAddressIfExists(). Can you try and re-use walletUtils here so that the behavior is the same and there is no implicit dependencies between method calls?

Link to wallet_utils.ts: https://github.com/0xProject/0x-monorepo/blob/development/packages/subproviders/src/utils/wallet_utils.ts

@MarcZenn

This comment has been minimized.

Copy link
Author

commented Jan 15, 2019

@fabioberger Take a look now. I've removed caching of addresses and am utilizing wallet_utils given the trezor public key request response payload.

@stale

This comment has been minimized.

Copy link

commented Mar 1, 2019

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale label Mar 1, 2019

@fabioberger fabioberger removed the stale label Mar 14, 2019

@fabioberger
Copy link
Contributor

left a comment

@MarcZenn so sorry this got lost in the shuffle. Thanks a lot for addressing me feedback. Let's get this baby merged! Could you please merge in the latest development and fix the merge conflicts? Then I'll merge it.

@NoahZinsmeister

This comment has been minimized.

Copy link

commented Apr 4, 2019

Hi @MarcZenn / @fabioberger ! Just wanted to check in before this gets merged.

The private _initialDerivedKeyInfoAsync method gets called in each of the core functions: getAccountsAsync, signTransactionAsync, and signPersonalMessageAsync. However, the output of this function doesn't change according to the calling context, and every call after the first unnecessarily prompts the user to export their public key again, which leads to pretty bad UX.

I'd strongly propose that _initialDerivedKeyInfoAsync be modified to cache the result after the first (successful) call, and return appropriately thereafter. I would be more than happy to contribute this (fairly small) fix, then we could move forward with merging?

UPDATE: Added a reference PR to @MarcZenn's repo to show what I mean.

NoahZinsmeister added a commit to NoahZinsmeister/0x-monorepo that referenced this pull request Apr 4, 2019

@fabioberger

This comment has been minimized.

Copy link
Contributor

commented Apr 23, 2019

Thanks for chiming in @NoahZinsmeister. That change makes sense to me. What do you think @MarcZenn? If I don't hear from him in the next week, I'll comandeer this PR and get it merged. Do you need it soon @NoahZinsmeister?

@NoahZinsmeister

This comment has been minimized.

Copy link

commented Apr 24, 2019

@fabioberger I'm using a ported version for now, so no huge rush. Looking forward to merge though!

@NoahZinsmeister

This comment has been minimized.

Copy link

commented May 20, 2019

@fabioberger it'd be nice to have this sooner rather than later, if you ever have a free moment :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.