Trezor Unlock + Deterministic Wallet Groundwork #137
Conversation
| import TrezorWallet from 'libs/wallet/trezor'; | ||
|
|
||
| /* eslint-disable quotes */ | ||
| const TREZOR_PATHS = [ |
There was a problem hiding this comment.
consider moving out of component (and into config)
| @@ -47,8 +48,8 @@ export function getTransactionFields(tx: EthTx) { | |||
| const [nonce, gasPrice, gasLimit, to, value, data, v, r, s] = tx.toJSON(); | |||
|
|
|||
| return { | |||
| // No value comes back as '0x', but most things expect '0x0' | |||
| value: value === '0x' ? '0x0' : value, | |||
| // No value comes back as '0x', but most things expect '0x00' | |||
There was a problem hiding this comment.
any link to this? seems like we're changing it once already, so would be good to know where this info is coming from.
There was a problem hiding this comment.
Yeah, would definitely be good to figure out the "correct" behavior here. I just used these values based on complaining libraries. 0x0 from 0x came from the node, 0x00 came from Trezor wanting padded values.
common/libs/transaction.js
Outdated
| @@ -121,13 +122,14 @@ export async function generateTransaction( | |||
|
|
|||
| // Generate the raw transaction | |||
| const txCount = await node.getTransactionCount(tx.from); | |||
| const cleanHex = hex => addHexPrefix(padToEven(stripHex(hex))); | |||
There was a problem hiding this comment.
What kind of padding are we doing here? Maybe some comments to explain?
There was a problem hiding this comment.
This was taken from v3's sanitizeHex behavior, Trezor wanted %2 length fields. I can definitely add some comments to v3, but I don't truly understand the reasoning.
common/libs/validators.js
Outdated
| @@ -139,3 +139,7 @@ export function isValidRawTx(rawTx: RawTransaction): boolean { | |||
|
|
|||
| return true; | |||
| } | |||
|
|
|||
| export function isValidPath(dPath: string) { | |||
| return dPath.split('\'/').length === 4; | |||
There was a problem hiding this comment.
maybe a comment here explaining what's going on. I get that the deterministic path is being split, but its not extremely clear why 4 is important
There was a problem hiding this comment.
https://github.com/bitcoin/bips/blob/master/bip-0032.mediawiki has more information, and test cases, I'll try to add a useful comment and implement some of its test cases.
There was a problem hiding this comment.
Awesome PR @wbobeirne! This looks like a solid foundation from which to add in mneumonic and ledger support. Also huge fan of the simplified HD path modal 👍
|
Added clarifying comments and split dpaths out into a config json, didn't end up implementing the bip32 test cases because they were more about Ready to have this merged whenever! |
|
🎉 |
What This Does
This is the first pass at hardware wallet support, starting with Trezor, since that's the one I own. It allows for the unlocking of, and signing transactions for Trezor wallets. It also lays down a lot of the groundwork for future deterministic path generated wallets (Mnemonic, ledger, etc.)
Don't let the line count fool you, this PR isn't that big. The bulk of it was because of
common/vendor/trezor-connect.js. It's not on NPM, so it has to be loaded into the project directly. Currently in talks with them about the future of Trezor Connect.The wallet modal was modeled after #129, not v3 directly, so there are some noteworthy differences.
What This Doesn't Do
Basic Technical Overview
This is how Trezor does everything, and presumably the other HW wallets will have a similar flow:
components/WalletDecrypt/Trezor.jsxvia TrezorConnectcomponents/WalletDecrypt/DeterministicWalletsModal.jsxdeterministicWalletsreducer / sagaTrezor.jsxTrezor.jsxcallsonUnlockand creates a newTrezorWalletinstanceTrezorWalletextends theDeterministicWallet, and only differs forsignRawTransaction(And presumablysignMessagein the future)Trezor in Action
GIFs were recorded at reduced sizes, and addresses / amounts blurred, so don't worry if things look a little odd
Connecting
Token Values
Paging to More Addresses
Path Selection
Unlocking