Trezor DPath Fix #196
Trezor DPath Fix #196
Conversation
| @@ -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)), | |||
There was a problem hiding this comment.
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.
common/libs/wallet/trezor.js
Outdated
| stripAndLower(tx.gasLimit.toString()), | ||
| stripAndLower(tx.to), | ||
| stripAndLower(tx.value), | ||
| stripAndLower(tx.data), |
There was a problem hiding this comment.
|
|
||
| constructor(address: string, dPath: string) { | ||
| constructor(address: string, dPath: string, index: number) { |
There was a problem hiding this comment.
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!
There was a problem hiding this comment.
I think thats fine, whoever next works on this should be able to figure out dPath is really dPath - index in that case.
common/libs/wallet/trezor.js
Outdated
| import type { RawTransaction } from 'libs/transaction'; | ||
|
|
||
| // Identical to ethFuncs.getNakedAddress | ||
| function stripAndLower(str) { |
There was a problem hiding this comment.
Any reason not to move into libs/values? Seems like this would be useful outside of just trezor.
There was a problem hiding this comment.
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));
});
});
}
...
}
There was a problem hiding this comment.
We should probably figure out why that discrepancy exists (not that it needs to be fixed as a part of this PR)
There was a problem hiding this comment.
stripHex already lowercases the input
export function stripHex(address: string): string {
return address.replace('0x', '').toLowerCase();
}There was a problem hiding this comment.
If anything, the stripHex in /libs/values.js should be renamed to stripHexAndLower since thats what it actually does
There was a problem hiding this comment.
I was successfully able to broadcast a trezor signed tx.
One adjustment was recommended, and this PR also made me discover that PrivKeyWallet does not lowercase in their getNakedAddress method, however your stripAndLower is supposed to be equivalent to getNakedAddress. More investigation needed to understand why this discrepancy exists.
…to stripHexPrefixAndLower
* add 'bip39' package * add mnemonic decrypt, wallet wrapper * add mnemonic path config * add mnemonic support to deterministic components * add mnemonic support * accomodate for ledger ETH path * remove comments * update comments regarding path length * rename modal open handler * make several props optional * add basic tests for mnemonic decrypt * make flow happy, add user error notifications * convert dpaths to js file, update references * add ledger path to test * Trezor DPath Fix (#196) * Match v3 more closely. * Require wallet index on deterministic wallets, update trezor to send index. * remove redundent stripAndLower function and rename existing stripHex to stripHexPrefixAndLower
This fixes Trezor (And any deterministic wallet that extends that class) to use the index of the wallet as a part of the dpath. This was causing Trezor sends to fail.
In the process of debugging, I also trued up a few spots in the send process to better match v3. I'll drop notes linking to where they exist in v3.