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.

Sign up for GitHub

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

Trezor DPath Fix #196

Merged
merged 3 commits into from
Sep 14, 2017
Merged

Trezor DPath Fix #196

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
a53c7dd
Match v3 more closely.
wbobeirne Sep 14, 2017
028665d
Require wallet index on deterministic wallets, update trezor to send …
wbobeirne Sep 14, 2017
57aa62e
remove redundent stripAndLower function and rename existing stripHex …
dternyak Sep 14, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions common/components/WalletDecrypt/Trezor.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,8 +65,8 @@ export default class TrezorDecrypt extends Component {
});
};

_handleUnlock = (address: string) => {
this.props.onUnlock(new TrezorWallet(address, this.state.dPath));
_handleUnlock = (address: string, index: number) => {
this.props.onUnlock(new TrezorWallet(address, this.state.dPath, index));
};

render() {
Expand Down
2 changes: 1 addition & 1 deletion common/libs/transaction.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -137,7 +137,7 @@ export async function generateCompleteTransactionFromRawTransaction(
nonce: cleanHex(nonce),
gasPrice: cleanHex(gasPrice.toString(16)),
gasLimit: cleanHex(gasLimit.toString(16)),
to: cleanHex(to),
to: toChecksumAddress(cleanHex(to)),
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I couldn't find where in code this happens, but all of the transactions I generated showed the checksum'd version in v3, whereas v4 was allowing all lower / all caps versions into the raw tx.

value: token ? '0x00' : cleanHex(value.toString(16)),
data: data ? cleanHex(data) : '',
chainId: chainId || 1
Expand Down
6 changes: 4 additions & 2 deletions common/libs/wallet/deterministic.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,17 +4,19 @@ import type { IWallet } from './IWallet';
export default class DeterministicWallet implements IWallet {
address: string;
dPath: string;
index: number;

constructor(address: string, dPath: string) {
constructor(address: string, dPath: string, index: number) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I chose to make this a separate param, rather than sending it all in as the dPath, to make sure people don't make the same mistake I do!

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think thats fine, whoever next works on this should be able to figure out dPath is really dPath - index in that case.

this.address = address;
this.dPath = dPath;
this.index = index;
}

getAddress(): Promise<string> {
return Promise.resolve(this.address);
}

getPath(): string {
return this.dPath;
return `${this.dPath}/${this.index}`;
}
}
18 changes: 11 additions & 7 deletions common/libs/wallet/trezor.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,21 +5,25 @@ import Big from 'bignumber.js';
import { addHexPrefix } from 'ethereumjs-util';
import DeterministicWallet from './deterministic';
import { stripHex } from 'libs/values';

import type { RawTransaction } from 'libs/transaction';

// Identical to ethFuncs.getNakedAddress
function stripAndLower(str) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any reason not to move into libs/values? Seems like this would be useful outside of just trezor.

Copy link
Contributor

@dternyak dternyak Sep 14, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PrivKeyWallet actually implements it on the class, but without lowercase?:

export default class PrivKeyWallet implements IWallet {
...

  getNakedAddress(): Promise<string> {
    return new Promise(resolve => {
      this.getAddress().then(address => {
        resolve(stripHex(address));
      });
    });
  }
...
}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should probably figure out why that discrepancy exists (not that it needs to be fixed as a part of this PR)

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

stripHex already lowercases the input

export function stripHex(address: string): string {
  return address.replace('0x', '').toLowerCase();
}

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If anything, the stripHex in /libs/values.js should be renamed to stripHexAndLower since thats what it actually does

return stripHex(str).toLowerCase();
}

export default class TrezorWallet extends DeterministicWallet {
signRawTransaction(tx: RawTransaction): Promise<string> {
return new Promise((resolve, reject) => {
TrezorConnect.ethereumSignTx(
// Args
this.getPath(),
stripHex(tx.nonce),
stripHex(tx.gasPrice.toString()),
stripHex(tx.gasLimit.toString()),
stripHex(tx.to),
stripHex(tx.value),
stripHex(tx.data),
stripAndLower(tx.nonce),
stripAndLower(tx.gasPrice.toString()),
stripAndLower(tx.gasLimit.toString()),
stripAndLower(tx.to),
stripAndLower(tx.value),
stripAndLower(tx.data),
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

https://github.com/kvhnuke/etherwallet/blob/b5e1b75beb7c393e06e5ffd7e540c6bccd9eface/app/scripts/uiFuncs.js#L46

tx.chainId,
// Callback
result => {
Expand Down