From b5e4bd3c502ab117e977588776bf6cba04f65ac9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?James=20Lefr=C3=A8re?= Date: Wed, 17 Jul 2019 16:44:12 +0200 Subject: [PATCH 1/2] Minor normalizers refactor * Fix Flow errors for string normalizers, require a string value * Use a bound function for address/hex sequence normalizers --- .../@colony/purser-core/normalizers.js | 57 +++++++------------ 1 file changed, 21 insertions(+), 36 deletions(-) diff --git a/modules/node_modules/@colony/purser-core/normalizers.js b/modules/node_modules/@colony/purser-core/normalizers.js index 24b37e20..8c3184b4 100644 --- a/modules/node_modules/@colony/purser-core/normalizers.js +++ b/modules/node_modules/@colony/purser-core/normalizers.js @@ -75,37 +75,37 @@ export const derivationPathNormalizer = (derivationPath: string): string => { export const multipleOfTwoHexValueNormalizer = (hexValue: string): string => String(hexValue).padStart(Math.ceil(hexValue.length / 2) * 2, '0'); +const stringPrefixNormalizer = ( + pattern: RegExp, + str: string, + prefix: boolean = true, +) => { + /* + * Index 1 is the prefix (if it exists), index 2 is the value without a prefix + */ + const matchedAddress = str.match(pattern) || []; + return prefix + ? `0x${matchedAddress[2]}` + : matchedAddress[2]; +}; + /** - * Nomalize an Ethereum address + * Normalize an Ethereum address * * This method assumes the address is already validatated and is in the correct format. * * @method addressNormalizer * - * @param {string} adddress The address to normalize + * @param {string} address The address to normalize * @param {boolean} prefix Should the final value have a prefix? * * @return {string} The normalized string */ -export const addressNormalizer = ( - adddress: string | void, - prefix: boolean = true, -): string => { - /* - * Index 1 is the prefix (if it exists), index 2 is the address without a prefix - * - * Flow doesn't trust that we are actually capable of validating a string - */ - /* $FlowFixMe */ - const matchedAddress: Array<*> = adddress.match(MATCH.ADDRESS); - if (prefix) { - return `0x${matchedAddress[2]}`; - } - return matchedAddress[2]; -}; +export const addressNormalizer = + stringPrefixNormalizer.bind(null, MATCH.ADDRESS); /** - * Nomalize a hex string sequence. + * Normalize a hex string sequence. * * Transforms it to lower case, and, depending on the prefix argument, * either add it (`0x`) or remove it @@ -120,24 +120,9 @@ export const addressNormalizer = ( * @return {string} The normalized string */ export const hexSequenceNormalizer = ( - hexString: string | void, + hexString: string, prefix: boolean = true, -): string => { - /* - * Flow doesn't trust that we are actually capable of validating a string - */ - /* $FlowFixMe */ - const lowecaseSequence = hexString.toLowerCase(); - /* - * Index 1 is the prefix (if it exists), index 2 is the rest of the string sequence - */ - /* $FlowFixMe */ - const matchedString: Array<*> = lowecaseSequence.match(MATCH.HEX_STRING); - if (prefix) { - return `0x${matchedString[2]}`; - } - return matchedString[2]; -}; +) => stringPrefixNormalizer(MATCH.HEX_STRING, hexString.toLowerCase(), prefix); /** * Normalize the recovery param of an Ethereum ECDSA signature. From eea53980cafa31f2cdf9b42a32023521583f9fa3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?James=20Lefr=C3=A8re?= Date: Wed, 17 Jul 2019 16:48:26 +0200 Subject: [PATCH 2/2] Adjust transaction params * Adjust parameters for `ethereumjs-tx` transactions (recent breaking changes) and ensure that the chain ID is supplied; this fixes a bug where the recovery param wasn't validated correctly * Fix logic for supplying a `to` property for transactions (it can also be `null`, not just `undefined`) * Fix some Flow types * Fix some comments * Update/fix tests --- .../@colony/purser-core/helpers.js | 7 +- .../node_modules/@colony/purser-core/utils.js | 2 +- .../@colony/purser-ledger/flowtypes.js | 18 +-- .../@colony/purser-ledger/staticMethods.js | 14 +- .../@colony/purser-metamask/helpers.js | 2 +- .../@colony/purser-metamask/staticMethods.js | 41 +++-- .../@colony/purser-software/class.js | 7 - .../@colony/purser-software/staticMethods.js | 4 +- .../@colony/purser-trezor/class.js | 2 +- .../@colony/purser-trezor/staticMethods.js | 150 +++++++++--------- .../staticMethods/signTransaction.test.js | 28 ++-- .../staticMethods/signTransaction.test.js | 31 ++-- 12 files changed, 158 insertions(+), 148 deletions(-) diff --git a/modules/node_modules/@colony/purser-core/helpers.js b/modules/node_modules/@colony/purser-core/helpers.js index f11a6f40..a68aafea 100644 --- a/modules/node_modules/@colony/purser-core/helpers.js +++ b/modules/node_modules/@colony/purser-core/helpers.js @@ -248,7 +248,7 @@ export const transactionObjectValidator = ({ * Only check if the address (`to` prop) is in the correct * format, if one was provided in the initial transaction object */ - if (typeof to !== 'undefined') { + if (to) { addressValidator(to); } /* @@ -403,7 +403,7 @@ export const messageOrDataValidator = ( if (message) { messageValidator(message); return message; - } + } messageDataValidator(messageData); return typeof messageData === 'string' ? new Uint8Array(Buffer.from( @@ -411,8 +411,7 @@ export const messageOrDataValidator = ( 'hex', )) : messageData; - -} +}; /* * This default export is only here to help us with testing, otherwise diff --git a/modules/node_modules/@colony/purser-core/utils.js b/modules/node_modules/@colony/purser-core/utils.js index 705aab9c..f8a1ce1c 100644 --- a/modules/node_modules/@colony/purser-core/utils.js +++ b/modules/node_modules/@colony/purser-core/utils.js @@ -53,7 +53,7 @@ export const warning = (...args: Array<*>): void => { let level: string = 'low'; const lastArgIndex: number = args.length - 1; const options: * = args[lastArgIndex]; - const [message: string] = args; + const [message]: [string] = args; const literalTemplates: Array<*> = args.slice(1); /* * We're being very specific with object testing here, since we don't want to diff --git a/modules/node_modules/@colony/purser-ledger/flowtypes.js b/modules/node_modules/@colony/purser-ledger/flowtypes.js index 47b00917..3768a826 100644 --- a/modules/node_modules/@colony/purser-ledger/flowtypes.js +++ b/modules/node_modules/@colony/purser-ledger/flowtypes.js @@ -9,30 +9,22 @@ type GetAddressReturnType = { address: string, }; /* - * This is the same (in terms of types) for both a transaction signature and - * a message signature. + * The message signature return type defines the recovery param as a `Number` */ -type SignatureReturnType = { +type MessageSignatureReturnType = { r: string, s: string, v: number, }; /* - * But the transaction signature returns the recovery param as a `String`. + * But the transaction signature returns the recovery param as a `String`. * See: http://ledgerhq.github.io/ledgerjs/docs/#ethsigntransaction */ type TransactionSignatureReturnType = { - ...SignatureReturnType, + r: string, + s: string, v: string, }; -/* - * Whereas the message signature returns the recovery param as a `Number`. - * See: http://ledgerhq.github.io/ledgerjs/docs/#ethsignpersonalmessage - */ -type MessageSignatureReturnType = { - ...SignatureReturnType, - v: number, -}; export type LedgerInstanceType = { getAddress: ( diff --git a/modules/node_modules/@colony/purser-ledger/staticMethods.js b/modules/node_modules/@colony/purser-ledger/staticMethods.js index 2f00901a..862de139 100644 --- a/modules/node_modules/@colony/purser-ledger/staticMethods.js +++ b/modules/node_modules/@colony/purser-ledger/staticMethods.js @@ -100,7 +100,6 @@ export const signTransaction = async ({ /* $FlowFixMe */ multipleOfTwoHexValueNormalizer(gasLimit.toString(16)), ), - chainId, /* * Nonces needs to be sent in as a hex string, and to be padded as a multiple of two. * Eg: '3' to be '03', `12c` to be `012c` @@ -151,11 +150,14 @@ export const signTransaction = async ({ multipleOfTwoHexValueNormalizer(chainId.toString(16)), ), }, - /* - * Only send (and normalize) the destingation address if one was - * provided in the initial transaction object. - */ - typeof to !== 'undefined' ? { to: addressNormalizer(to) } : {}, + { + chain: chainId, + /* + * Only send (and normalize) the destination address if one was + * provided in the initial transaction object. + */ + ...(to ? { to: addressNormalizer(to) } : {}), + }, ), ); /* diff --git a/modules/node_modules/@colony/purser-metamask/helpers.js b/modules/node_modules/@colony/purser-metamask/helpers.js index ebfc8c8d..e047cbcc 100644 --- a/modules/node_modules/@colony/purser-metamask/helpers.js +++ b/modules/node_modules/@colony/purser-metamask/helpers.js @@ -147,7 +147,7 @@ export const setStateEventObserver = ( ): void => { const { publicConfigStore: { _events: stateEvents }, - }: () => MetamaskInpageProviderType = + }: MetamaskInpageProviderType = /* * We need this little go-around trick to mock just one export of * the module, while leaving the rest of the module intact so we can test it diff --git a/modules/node_modules/@colony/purser-metamask/staticMethods.js b/modules/node_modules/@colony/purser-metamask/staticMethods.js index 42310dfa..bf4924c8 100644 --- a/modules/node_modules/@colony/purser-metamask/staticMethods.js +++ b/modules/node_modules/@colony/purser-metamask/staticMethods.js @@ -33,6 +33,7 @@ import { STD_ERRORS } from './defaults'; import { staticMethods as messages } from './messages'; export const signTransactionCallback = ( + chainId: number, resolve: (string) => void, reject: (Error) => void, ) => @@ -78,17 +79,27 @@ export const signTransactionCallback = ( * RLP encode (to hex string) with ethereumjs-tx, prefix with * `0x` and return. Convert to BN all the numbers-as-strings. */ - const signedTransaction = new EthereumTx({ - data: signedData, - gasLimit: new BigNumber(gas), - gasPrice: new BigNumber(signedGasPrice), - nonce: new BigNumber(nonce), - r, - s, - to: signedTo, - v, - value: new BigNumber(signedValue), - }); + const signedTransaction = new EthereumTx( + { + data: signedData, + gasLimit: new BigNumber(gas), + gasPrice: new BigNumber(signedGasPrice), + nonce: new BigNumber(nonce), + r, + s, + to: signedTo, + v, + value: new BigNumber(signedValue), + }, + { + chain: chainId, + /* + * Only send (and normalize) the destination address if one was + * provided in the initial transaction object. + */ + ...(signedTo ? { to: addressNormalizer(signedTo) } : {}), + }, + ); const serializedSignedTransaction = signedTransaction .serialize() .toString(HEX_HASH_TYPE); @@ -131,6 +142,7 @@ export const signTransaction = async ({ ...transactionObject }: Object = {}): Promise => { const { + chainId, gasPrice, gasLimit, to, @@ -176,18 +188,19 @@ export const signTransaction = async ({ gas: gasLimit.toString(), gasPrice: gasPrice.toString(), data: hexSequenceNormalizer(inputData), + chainId, /* * Most likely this value is `undefined`, but that is good (see above) */ nonce: manualNonce, }, /* - * Only send (and normalize) the destingation address if one was + * Only send (and normalize) the destination address if one was * provided in the initial transaction object. */ - typeof to !== 'undefined' ? { to: addressNormalizer(to) } : {}, + to ? { to: addressNormalizer(to) } : {}, ), - signTransactionCallback(resolve, reject), + signTransactionCallback(chainId, resolve, reject), ), ), messages.cannotSendTransaction, diff --git a/modules/node_modules/@colony/purser-software/class.js b/modules/node_modules/@colony/purser-software/class.js index 839c8d8a..1c010b1e 100644 --- a/modules/node_modules/@colony/purser-software/class.js +++ b/modules/node_modules/@colony/purser-software/class.js @@ -55,17 +55,10 @@ export default class SoftwareWallet { privateKey: string; - publicKey: string; - originalMnemonic: string; derivationPath: string; - /* - * Encrypted JSON Keystore - */ - keystore: string; - type: string; subtype: string; diff --git a/modules/node_modules/@colony/purser-software/staticMethods.js b/modules/node_modules/@colony/purser-software/staticMethods.js index cbf4dd83..eee9c3a0 100644 --- a/modules/node_modules/@colony/purser-software/staticMethods.js +++ b/modules/node_modules/@colony/purser-software/staticMethods.js @@ -74,10 +74,10 @@ export const signTransaction = async ({ data: hexSequenceNormalizer(inputData), }, /* - * Only send (and normalize) the destingation address if one was + * Only send (and normalize) the destination address if one was * provided in the initial transaction object. */ - typeof to !== 'undefined' ? { to: addressNormalizer(to) } : {}, + to ? { to: addressNormalizer(to) } : {}, ), ); return hexSequenceNormalizer(signedTransaction); diff --git a/modules/node_modules/@colony/purser-trezor/class.js b/modules/node_modules/@colony/purser-trezor/class.js index 47594f6f..b8e5b01a 100644 --- a/modules/node_modules/@colony/purser-trezor/class.js +++ b/modules/node_modules/@colony/purser-trezor/class.js @@ -42,7 +42,7 @@ export default class TrezorWallet extends GenericWallet { * * Otherwise, a `to` address *must* be set. */ - if (typeof to === 'undefined') { + if (!to) { requiredSignProps = REQUIRED_TREZOR_PROPS.SIGN_TRANSACTION_CONTRACT; /* diff --git a/modules/node_modules/@colony/purser-trezor/staticMethods.js b/modules/node_modules/@colony/purser-trezor/staticMethods.js index cf5703e1..cd6f97ea 100644 --- a/modules/node_modules/@colony/purser-trezor/staticMethods.js +++ b/modules/node_modules/@colony/purser-trezor/staticMethods.js @@ -70,89 +70,88 @@ export const signTransaction = async ({ * Between the unsigned EthereumTx signature object values and the values * sent to the Trezor server */ - const unsignedTransaction = await new EthereumTx( - Object.assign( - {}, - { + const unsignedTransaction = new EthereumTx( + { + /* + * We could really do with some BN.js flow types declarations :( + */ + gasPrice: hexSequenceNormalizer( /* - * We could really do with some BN.js flow types declarations :( + * @TODO Add `bigNumber` `toHexString` wrapper method + * + * Flow confuses bigNumber's `toString` with the String object + * prototype `toString` method */ - gasPrice: hexSequenceNormalizer( - /* - * @TODO Add `bigNumber` `toHexString` wrapper method - * - * Flow confuses bigNumber's `toString` with the String object - * prototype `toString` method - */ - /* $FlowFixMe */ - multipleOfTwoHexValueNormalizer(gasPrice.toString(16)), - ), - gasLimit: hexSequenceNormalizer( - /* - * @TODO Add `bigNumber` `toHexString` wrapper method - * - * Flow confuses bigNumber's `toString` with the String object - * prototype `toString` method - */ - /* $FlowFixMe */ - multipleOfTwoHexValueNormalizer(gasLimit.toString(16)), - ), - chainId, + /* $FlowFixMe */ + multipleOfTwoHexValueNormalizer(gasPrice.toString(16)), + ), + gasLimit: hexSequenceNormalizer( /* - * Nonces needs to be sent in as a hex string, and to be padded as a multiple of two. - * Eg: '3' to be '03', `12c` to be `012c` + * @TODO Add `bigNumber` `toHexString` wrapper method + * + * Flow confuses bigNumber's `toString` with the String object + * prototype `toString` method */ - nonce: hexSequenceNormalizer( - /* - * @TODO Add `bigNumber` `toHexString` wrapper method - * - * Flow confuses bigNumber's `toString` with the String object - * prototype `toString` method - */ - /* $FlowFixMe */ - multipleOfTwoHexValueNormalizer(nonce.toString(16)), - ), - value: hexSequenceNormalizer( - /* - * @TODO Add `bigNumber` `toHexString` wrapper method - * - * Flow confuses bigNumber's `toString` with the String object - * prototype `toString` method - */ - /* $FlowFixMe */ - multipleOfTwoHexValueNormalizer(value.toString(16)), - ), - data: hexSequenceNormalizer(inputData), + /* $FlowFixMe */ + multipleOfTwoHexValueNormalizer(gasLimit.toString(16)), + ), + /* + * Nonces needs to be sent in as a hex string, and to be padded as a multiple of two. + * Eg: '3' to be '03', `12c` to be `012c` + */ + nonce: hexSequenceNormalizer( /* - * The transaction object needs to be seeded with the (R) and (S) signature components with - * empty data, and the Reco(V)ery param as the chain id (all, im hex string format). + * @TODO Add `bigNumber` `toHexString` wrapper method * - * See this issue for context: - * https://github.com/LedgerHQ/ledgerjs/issues/43 + * Flow confuses bigNumber's `toString` with the String object + * prototype `toString` method */ - r: hexSequenceNormalizer( - multipleOfTwoHexValueNormalizer(String(SIGNATURE.R)), - ), - s: hexSequenceNormalizer( - multipleOfTwoHexValueNormalizer(String(SIGNATURE.S)), - ), - v: hexSequenceNormalizer( - /* - * @TODO Add `bigNumber` `toHexString` wrapper method - * - * Flow confuses bigNumber's `toString` with the String object - * prototype `toString` method - */ - /* $FlowFixMe */ - multipleOfTwoHexValueNormalizer(chainId.toString(16)), - ), - }, + /* $FlowFixMe */ + multipleOfTwoHexValueNormalizer(nonce.toString(16)), + ), + value: hexSequenceNormalizer( + /* + * @TODO Add `bigNumber` `toHexString` wrapper method + * + * Flow confuses bigNumber's `toString` with the String object + * prototype `toString` method + */ + /* $FlowFixMe */ + multipleOfTwoHexValueNormalizer(value.toString(16)), + ), + data: hexSequenceNormalizer(inputData), + /* + * The transaction object needs to be seeded with the (R) and (S) signature components with + * empty data, and the Reco(V)ery param as the chain id (all, im hex string format). + * + * See this issue for context: + * https://github.com/LedgerHQ/ledgerjs/issues/43 + */ + r: hexSequenceNormalizer( + multipleOfTwoHexValueNormalizer(String(SIGNATURE.R)), + ), + s: hexSequenceNormalizer( + multipleOfTwoHexValueNormalizer(String(SIGNATURE.S)), + ), + v: hexSequenceNormalizer( + /* + * @TODO Add `bigNumber` `toHexString` wrapper method + * + * Flow confuses bigNumber's `toString` with the String object + * prototype `toString` method + */ + /* $FlowFixMe */ + multipleOfTwoHexValueNormalizer(chainId.toString(16)), + ), + }, + { + chain: chainId, /* - * Only set (and normalize) the destingation address if one was + * Only send (and normalize) the destination address if one was * provided in the initial transaction object. */ - typeof to !== 'undefined' ? { to: addressNormalizer(to) } : {}, - ), + ...(to ? { to: addressNormalizer(to) } : {}), + }, ); /* * Modify the default payload to set the transaction details @@ -215,7 +214,7 @@ export const signTransaction = async ({ * * Trezor service requires the prefix from the address to be stripped */ - typeof to !== 'undefined' ? { to: addressNormalizer(to, false) } : {}, + to ? { to: addressNormalizer(to, false) } : {}, ); /* * We need to catch the cancelled error since it's part of a normal user workflow @@ -280,6 +279,7 @@ export const signTransaction = async ({ * * @param {string} derivationPath the derivation path for the account with which to sign the message * @param {string} message the message you want to sign + * @param {string} messageData the message data you want to sign * * All the above params are sent in as props of an object. * @@ -297,7 +297,7 @@ export const signMessage = async ({ const toSign = messageOrDataValidator({ message, messageData }); warning(messages.messageSignatureOnlyTrezor); try { - const { signature: signedMessage } = await payloadListener({ + const { signature: signedMessage = '' } = await payloadListener({ payload: Object.assign({}, PAYLOAD_SIGNMSG, { /* * Path needs to be sent in as an derivation path array diff --git a/modules/tests/purser-ledger/staticMethods/signTransaction.test.js b/modules/tests/purser-ledger/staticMethods/signTransaction.test.js index f241d03b..633e081e 100644 --- a/modules/tests/purser-ledger/staticMethods/signTransaction.test.js +++ b/modules/tests/purser-ledger/staticMethods/signTransaction.test.js @@ -170,18 +170,22 @@ describe('`Ledger` Hardware Wallet Module Static Methods', () => { */ expect(EthereumTx).toHaveBeenCalled(); expect(EthereumTx).toHaveBeenCalledWith( - expect.objectContaining({ - gasPrice, - gasLimit, - chainId, - nonce, - to, - value, - data: inputData, - r: String(SIGNATURE.R), - s: String(SIGNATURE.S), - v: chainId, - }), + expect.objectContaining( + { + gasPrice, + gasLimit, + nonce, + value, + data: inputData, + r: String(SIGNATURE.R), + s: String(SIGNATURE.S), + v: chainId, + }, + { + chain: chainId, + to, + }, + ), ); }); test('Normalizes the signed transaction signature components', async () => { diff --git a/modules/tests/purser-trezor/staticMethods/signTransaction.test.js b/modules/tests/purser-trezor/staticMethods/signTransaction.test.js index 947bd473..871a91fc 100644 --- a/modules/tests/purser-trezor/staticMethods/signTransaction.test.js +++ b/modules/tests/purser-trezor/staticMethods/signTransaction.test.js @@ -79,18 +79,22 @@ describe('`Trezor` Hardware Wallet Module Static Methods', () => { test('Creates the initial, unsigned signature', async () => { await signTransaction(mockedArgumentsObject); expect(EthereumTx).toHaveBeenCalled(); - expect(EthereumTx).toHaveBeenCalledWith({ - gasPrice, - gasLimit, - chainId, - nonce, - to, - value, - data: inputData, - r: '0', - s: '0', - v: chainId, - }); + expect(EthereumTx).toHaveBeenCalledWith( + { + gasPrice, + gasLimit, + nonce, + value, + data: inputData, + r: '0', + s: '0', + v: chainId, + }, + { + to, + chain: chainId, + }, + ); }); test('Uses the correct trezor service payload type', async () => { const { type, requiredFirmware } = PAYLOAD_SIGNTX; @@ -189,6 +193,9 @@ describe('`Trezor` Hardware Wallet Module Static Methods', () => { ); }); test('Signs a transaction without a destination address', async () => { + payloadListener.mockImplementation(() => ({ + signature: 'mocked-signature', + })); expect( signTransaction({ gasPrice,