Feedback

remove id management from testnet faucet
spread over txParams rather than modify in place

10 files changed, 61 insertions, 96 deletions

diff --git a/packages/subproviders/CHANGELOG.json b/packages/subproviders/CHANGELOG.json
index 0c299e90a..f0702cd5d 100644
--- a/packages/subproviders/CHANGELOG.json
+++ b/packages/subproviders/CHANGELOG.json
@@ -1,10 +1,9 @@
 [
     {
-        "timestamp": 1522904386,
         "version": "0.8.5",
         "changes": [
             {
-                "note": "Add Prive Key Subprovider and refactor Provider Engine usage into Base Wallet Subprovider",
+                "note": "Add private key subprovider and refactor shared functionality into a base wallet subprovider",
                 "pr": 506
             }
         ]
diff --git a/packages/subproviders/src/index.ts b/packages/subproviders/src/index.ts
index 3541ac6f5..dd553fde4 100644
--- a/packages/subproviders/src/index.ts
+++ b/packages/subproviders/src/index.ts
@@ -12,7 +12,7 @@ export { LedgerSubprovider } from './subproviders/ledger';
 export { GanacheSubprovider } from './subproviders/ganache';
 export { Subprovider } from './subproviders/subprovider';
 export { NonceTrackerSubprovider } from './subproviders/nonce_tracker';
-export { PKWalletSubprovider } from './subproviders/pk_wallet_subprovider';
+export { PrivateKeyWalletSubprovider } from './subproviders/private_key_wallet_subprovider';
 export {
     Callback,
     ErrorCallback,
diff --git a/packages/subproviders/src/subproviders/base_wallet_subprovider.ts b/packages/subproviders/src/subproviders/base_wallet_subprovider.ts
index 83b0da52f..034f83e7f 100644
--- a/packages/subproviders/src/subproviders/base_wallet_subprovider.ts
+++ b/packages/subproviders/src/subproviders/base_wallet_subprovider.ts
@@ -1,13 +1,19 @@
+import { assert } from '@0xproject/assert';
 import { JSONRPCRequestPayload, JSONRPCResponsePayload } from '@0xproject/types';
 import { addressUtils } from '@0xproject/utils';
 import * as _ from 'lodash';
 
-import { Callback, PartialTxParams, ResponseWithTxParams, WalletSubproviderErrors } from '../types';
+import { Callback, ErrorCallback, PartialTxParams, ResponseWithTxParams, WalletSubproviderErrors } from '../types';
 
 import { Subprovider } from './subprovider';
 
 export abstract class BaseWalletSubprovider extends Subprovider {
-    protected static _validateSender(sender: string) {
+    protected static _validateTxParams(txParams: PartialTxParams) {
+        assert.isETHAddressHex('to', txParams.to);
+        assert.isHexString('nonce', txParams.nonce);
+        assert.isHexString('gas', txParams.gas);
+    }
+    private static _validateSender(sender: string) {
         if (_.isUndefined(sender) || !addressUtils.isAddress(sender)) {
             throw new Error(WalletSubproviderErrors.SenderInvalidOrNotSupplied);
         }
@@ -15,7 +21,7 @@ export abstract class BaseWalletSubprovider extends Subprovider {
 
     public abstract async getAccountsAsync(): Promise<string[]>;
     public abstract async signTransactionAsync(txParams: PartialTxParams): Promise<string>;
-    public abstract async signPersonalMessageAsync(dataIfExists: string): Promise<string>;
+    public abstract async signPersonalMessageAsync(data: string): Promise<string>;
 
     /**
      * This method conforms to the web3-provider-engine interface.
@@ -26,11 +32,7 @@ export abstract class BaseWalletSubprovider extends Subprovider {
      * @param end Callback to call if subprovider handled the request and wants to pass back the request.
      */
     // tslint:disable-next-line:async-suffix
-    public async handleRequest(
-        payload: JSONRPCRequestPayload,
-        next: Callback,
-        end: (err: Error | null, result?: any) => void,
-    ) {
+    public async handleRequest(payload: JSONRPCRequestPayload, next: Callback, end: ErrorCallback) {
         let accounts;
         let txParams;
         switch (payload.method) {
@@ -104,30 +106,31 @@ export abstract class BaseWalletSubprovider extends Subprovider {
         const result = await this.emitPayloadAsync(payload);
         return result;
     }
-    private async _populateMissingTxParamsAsync(txParams: PartialTxParams): Promise<PartialTxParams> {
-        if (_.isUndefined(txParams.gasPrice)) {
+    private async _populateMissingTxParamsAsync(partialTxParams: PartialTxParams): Promise<PartialTxParams> {
+        let txParams = partialTxParams;
+        if (_.isUndefined(partialTxParams.gasPrice)) {
             const gasPriceResult = await this.emitPayloadAsync({
                 method: 'eth_gasPrice',
                 params: [],
             });
             const gasPrice = gasPriceResult.result.toString();
-            txParams.gasPrice = gasPrice;
+            txParams = { ...txParams, gasPrice };
         }
-        if (_.isUndefined(txParams.nonce)) {
+        if (_.isUndefined(partialTxParams.nonce)) {
             const nonceResult = await this.emitPayloadAsync({
                 method: 'eth_getTransactionCount',
-                params: [txParams.from, 'pending'],
+                params: [partialTxParams.from, 'pending'],
             });
             const nonce = nonceResult.result;
-            txParams.nonce = nonce;
+            txParams = { ...txParams, nonce };
         }
-        if (_.isUndefined(txParams.gas)) {
+        if (_.isUndefined(partialTxParams.gas)) {
             const gasResult = await this.emitPayloadAsync({
                 method: 'eth_estimateGas',
-                params: [txParams],
+                params: [partialTxParams],
             });
             const gas = gasResult.result.toString();
-            txParams.gas = gas;
+            txParams = { ...txParams, gas };
         }
         return txParams;
     }
diff --git a/packages/subproviders/src/subproviders/ledger.ts b/packages/subproviders/src/subproviders/ledger.ts
index 71864f19c..aa86bf6c0 100644
--- a/packages/subproviders/src/subproviders/ledger.ts
+++ b/packages/subproviders/src/subproviders/ledger.ts
@@ -129,6 +129,7 @@ export class LedgerSubprovider extends BaseWalletSubprovider {
      * @return Signed transaction hex string
      */
     public async signTransactionAsync(txParams: PartialTxParams): Promise<string> {
+        LedgerSubprovider._validateTxParams(txParams);
         this._ledgerClientIfExists = await this._createLedgerClientAsync();
 
         const tx = new EthereumTx(txParams);
diff --git a/packages/subproviders/src/subproviders/pk_wallet_subprovider.ts b/packages/subproviders/src/subproviders/private_key_wallet_subprovider.ts
index 06dc39237..c3a53773a 100644
--- a/packages/subproviders/src/subproviders/pk_wallet_subprovider.ts
+++ b/packages/subproviders/src/subproviders/private_key_wallet_subprovider.ts
@@ -11,19 +11,21 @@ import { Subprovider } from './subprovider';
 
 /**
  * This class implements the [web3-provider-engine](https://github.com/MetaMask/provider-engine) subprovider interface.
- * This subprovider intercepts all account related RPC requests (e.g message/transaction signing, etc...)
+ * This subprovider intercepts all account related RPC requests (e.g message/transaction signing, etc...) and handles
+ * all requests with the supplied Ethereum private key.
  */
-export class PKWalletSubprovider extends BaseWalletSubprovider {
+export class PrivateKeyWalletSubprovider extends BaseWalletSubprovider {
     private _address: string;
     private _privateKeyBuffer: Buffer;
     constructor(privateKey: string) {
+        assert.isString('privateKey', privateKey);
         super();
         this._privateKeyBuffer = new Buffer(privateKey, 'hex');
         this._address = `0x${ethUtil.privateToAddress(this._privateKeyBuffer).toString('hex')}`;
     }
     /**
-     * Retrieve the account calcuated from the private key.
-     * This method is automatically called when issuing a `eth_accounts` JSON RPC request
+     * Retrieve the account associated with the supplied private key.
+     * This method is implicitly called when issuing a `eth_accounts` JSON RPC request
      * via your providerEngine instance.
      * @return An array of accounts
      */
@@ -39,6 +41,7 @@ export class PKWalletSubprovider extends BaseWalletSubprovider {
      * @return Signed transaction hex string
      */
     public async signTransactionAsync(txParams: PartialTxParams): Promise<string> {
+        PrivateKeyWalletSubprovider._validateTxParams(txParams);
         const tx = new EthereumTx(txParams);
         tx.sign(this._privateKeyBuffer);
         const rawTx = `0x${tx.serialize().toString('hex')}`;
@@ -62,7 +65,6 @@ export class PKWalletSubprovider extends BaseWalletSubprovider {
         const msgHashBuff = ethUtil.hashPersonalMessage(dataBuff);
         const sig = ethUtil.ecsign(msgHashBuff, this._privateKeyBuffer);
         const rpcSig = ethUtil.toRpcSig(sig.v, sig.r, sig.s);
-
         return rpcSig;
     }
 }
diff --git a/packages/subproviders/test/integration/ledger_subprovider_test.ts b/packages/subproviders/test/integration/ledger_subprovider_test.ts
index 3039bd560..da858b6b3 100644
--- a/packages/subproviders/test/integration/ledger_subprovider_test.ts
+++ b/packages/subproviders/test/integration/ledger_subprovider_test.ts
@@ -14,6 +14,7 @@ import RpcSubprovider = require('web3-provider-engine/subproviders/rpc');
 import { LedgerSubprovider } from '../../src';
 import { DoneCallback, LedgerEthereumClient } from '../../src/types';
 import { chaiSetup } from '../chai_setup';
+import { fixtureData } from '../utils/fixture_data';
 import { reportCallbackErrors } from '../utils/report_callback_errors';
 
 chaiSetup.configure();
@@ -25,9 +26,6 @@ async function ledgerEthereumNodeJsClientFactoryAsync(): Promise<LedgerEthereumC
     return ledgerEthClient;
 }
 
-const TESTRPC_DERIVATION_PATH = `m/44'/60'/0'/0`;
-const TEST_RPC_ACCOUNT_0 = '0x5409ed021d9299bf6814279a6a1411a7e866a631';
-
 describe('LedgerSubprovider', () => {
     let ledgerSubprovider: LedgerSubprovider;
     const networkId: number = 42;
@@ -35,7 +33,7 @@ describe('LedgerSubprovider', () => {
         ledgerSubprovider = new LedgerSubprovider({
             networkId,
             ledgerEthereumClientFactoryAsync: ledgerEthereumNodeJsClientFactoryAsync,
-            derivationPath: TESTRPC_DERIVATION_PATH,
+            derivationPath: fixtureData.TESTRPC_DERIVATION_PATH,
         });
     });
     describe('direct method calls', () => {
@@ -46,7 +44,7 @@ describe('LedgerSubprovider', () => {
         });
         it('returns the expected first account from a ledger set up with the test mnemonic', async () => {
             const accounts = await ledgerSubprovider.getAccountsAsync();
-            expect(accounts[0]).to.be.equal(TEST_RPC_ACCOUNT_0);
+            expect(accounts[0]).to.be.equal(fixtureData.TEST_RPC_ACCOUNT_0);
         });
         it('returns requested number of accounts', async () => {
             const numberOfAccounts = 20;
@@ -55,12 +53,10 @@ describe('LedgerSubprovider', () => {
             expect(accounts.length).to.be.equal(numberOfAccounts);
         });
         it('signs a personal message', async () => {
-            const data = ethUtils.bufferToHex(ethUtils.toBuffer('hello world'));
+            const data = ethUtils.bufferToHex(ethUtils.toBuffer(fixtureData.PERSONAL_MESSAGE_STRING));
             const ecSignatureHex = await ledgerSubprovider.signPersonalMessageAsync(data);
             expect(ecSignatureHex.length).to.be.equal(132);
-            expect(ecSignatureHex).to.be.equal(
-                '0x1b0ec5e2908e993d0c8ab6b46da46be2688fdf03c7ea6686075de37392e50a7d7fcc531446699132fbda915bd989882e0064d417018773a315fb8d43ed063c9b00',
-            );
+            expect(ecSignatureHex).to.be.equal(fixtureData.PERSONAL_MESSAGE_SIGNED_RESULT);
         });
         it('signs a transaction', async () => {
             const tx = {
@@ -69,7 +65,7 @@ describe('LedgerSubprovider', () => {
                 to: '0x0000000000000000000000000000000000000000',
                 value: '0x00',
                 chainId: 3,
-                from: TEST_RPC_ACCOUNT_0,
+                from: fixtureData.TEST_RPC_ACCOUNT_0,
             };
             const txHex = await ledgerSubprovider.signTransactionAsync(tx);
             expect(txHex).to.be.equal(
@@ -173,7 +169,7 @@ describe('LedgerSubprovider', () => {
                 // Give first account on Ledger sufficient ETH to complete tx send
                 let tx = {
                     to: accounts[0],
-                    from: TEST_RPC_ACCOUNT_0,
+                    from: fixtureData.TEST_RPC_ACCOUNT_0,
                     value: '0x8ac7230489e80000', // 10 ETH
                 };
                 let payload = {
diff --git a/packages/subproviders/test/unit/pk_wallet_subprovider_test.ts b/packages/subproviders/test/unit/private_key_wallet_subprovider_test.ts
index 6dd96399a..32650b3a0 100644
--- a/packages/subproviders/test/unit/pk_wallet_subprovider_test.ts
+++ b/packages/subproviders/test/unit/private_key_wallet_subprovider_test.ts
@@ -4,7 +4,7 @@ import * as ethUtils from 'ethereumjs-util';
 import * as _ from 'lodash';
 import Web3ProviderEngine = require('web3-provider-engine');
 
-import { GanacheSubprovider, PKWalletSubprovider } from '../../src/';
+import { GanacheSubprovider, PrivateKeyWalletSubprovider } from '../../src/';
 import {
     DoneCallback,
     LedgerCommunicationClient,
@@ -12,31 +12,28 @@ import {
     WalletSubproviderErrors,
 } from '../../src/types';
 import { chaiSetup } from '../chai_setup';
+import { fixtureData } from '../utils/fixture_data';
 import { reportCallbackErrors } from '../utils/report_callback_errors';
 
 chaiSetup.configure();
 const expect = chai.expect;
-const TEST_RPC_ACCOUNT_0 = '0x5409ed021d9299bf6814279a6a1411a7e866a631';
-const TEST_ACCOUNT_PRIVATE_KEY = 'F2F48EE19680706196E2E339E5DA3491186E0C4C5030670656B0E0164837257D';
 
-describe('PKWalletSubprovider', () => {
-    let subprovider: PKWalletSubprovider;
+describe('PrivateKeyWalletSubprovider', () => {
+    let subprovider: PrivateKeyWalletSubprovider;
     before(async () => {
-        subprovider = new PKWalletSubprovider(TEST_ACCOUNT_PRIVATE_KEY);
+        subprovider = new PrivateKeyWalletSubprovider(fixtureData.TEST_ACCOUNT_PRIVATE_KEY);
     });
     describe('direct method calls', () => {
         describe('success cases', () => {
             it('returns the account', async () => {
                 const accounts = await subprovider.getAccountsAsync();
-                expect(accounts[0]).to.be.equal(TEST_RPC_ACCOUNT_0);
+                expect(accounts[0]).to.be.equal(fixtureData.TEST_RPC_ACCOUNT_0);
                 expect(accounts.length).to.be.equal(1);
             });
             it('signs a personal message', async () => {
-                const data = ethUtils.bufferToHex(ethUtils.toBuffer('hello world'));
+                const data = ethUtils.bufferToHex(ethUtils.toBuffer(fixtureData.PERSONAL_MESSAGE_STRING));
                 const ecSignatureHex = await subprovider.signPersonalMessageAsync(data);
-                expect(ecSignatureHex).to.be.equal(
-                    '0x1b0ec5e2908e993d0c8ab6b46da46be2688fdf03c7ea6686075de37392e50a7d7fcc531446699132fbda915bd989882e0064d417018773a315fb8d43ed063c9b00',
-                );
+                expect(ecSignatureHex).to.be.equal(fixtureData.PERSONAL_MESSAGE_SIGNED_RESULT);
             });
             it('signs a transaction', async () => {
                 const tx = {
@@ -46,7 +43,7 @@ describe('PKWalletSubprovider', () => {
                     to: '0x0000000000000000000000000000000000000000',
                     value: '0x00',
                     chainId: 3,
-                    from: TEST_RPC_ACCOUNT_0,
+                    from: fixtureData.TEST_RPC_ACCOUNT_0,
                 };
                 const txHex = await subprovider.signTransactionAsync(tx);
                 expect(txHex).to.be.equal(
@@ -74,14 +71,14 @@ describe('PKWalletSubprovider', () => {
                 };
                 const callback = reportCallbackErrors(done)((err: Error, response: JSONRPCResponsePayload) => {
                     expect(err).to.be.a('null');
-                    expect(response.result[0]).to.be.equal(TEST_RPC_ACCOUNT_0);
+                    expect(response.result[0]).to.be.equal(fixtureData.TEST_RPC_ACCOUNT_0);
                     expect(response.result.length).to.be.equal(1);
                     done();
                 });
                 provider.sendAsync(payload, callback);
             });
             it('signs a personal message with eth_sign', (done: DoneCallback) => {
-                const messageHex = ethUtils.bufferToHex(ethUtils.toBuffer('hello world'));
+                const messageHex = ethUtils.bufferToHex(ethUtils.toBuffer(fixtureData.PERSONAL_MESSAGE_STRING));
                 const payload = {
                     jsonrpc: '2.0',
                     method: 'eth_sign',
@@ -90,15 +87,13 @@ describe('PKWalletSubprovider', () => {
                 };
                 const callback = reportCallbackErrors(done)((err: Error, response: JSONRPCResponsePayload) => {
                     expect(err).to.be.a('null');
-                    expect(response.result).to.be.equal(
-                        '0x1b0ec5e2908e993d0c8ab6b46da46be2688fdf03c7ea6686075de37392e50a7d7fcc531446699132fbda915bd989882e0064d417018773a315fb8d43ed063c9b00',
-                    );
+                    expect(response.result).to.be.equal(fixtureData.PERSONAL_MESSAGE_SIGNED_RESULT);
                     done();
                 });
                 provider.sendAsync(payload, callback);
             });
             it('signs a personal message with personal_sign', (done: DoneCallback) => {
-                const messageHex = ethUtils.bufferToHex(ethUtils.toBuffer('hello world'));
+                const messageHex = ethUtils.bufferToHex(ethUtils.toBuffer(fixtureData.PERSONAL_MESSAGE_STRING));
                 const payload = {
                     jsonrpc: '2.0',
                     method: 'personal_sign',
@@ -107,9 +102,7 @@ describe('PKWalletSubprovider', () => {
                 };
                 const callback = reportCallbackErrors(done)((err: Error, response: JSONRPCResponsePayload) => {
                     expect(err).to.be.a('null');
-                    expect(response.result).to.be.equal(
-                        '0x1b0ec5e2908e993d0c8ab6b46da46be2688fdf03c7ea6686075de37392e50a7d7fcc531446699132fbda915bd989882e0064d417018773a315fb8d43ed063c9b00',
-                    );
+                    expect(response.result).to.be.equal(fixtureData.PERSONAL_MESSAGE_SIGNED_RESULT);
                     done();
                 });
                 provider.sendAsync(payload, callback);
diff --git a/packages/subproviders/test/utils/fixture_data.ts b/packages/subproviders/test/utils/fixture_data.ts
new file mode 100644
index 000000000..3b6ab123e
--- /dev/null
+++ b/packages/subproviders/test/utils/fixture_data.ts
@@ -0,0 +1,8 @@
+export const fixtureData = {
+    TEST_RPC_ACCOUNT_0: '0x5409ed021d9299bf6814279a6a1411a7e866a631',
+    TEST_ACCOUNT_PRIVATE_KEY: 'F2F48EE19680706196E2E339E5DA3491186E0C4C5030670656B0E0164837257D',
+    PERSONAL_MESSAGE_STRING: 'hello world',
+    PERSONAL_MESSAGE_SIGNED_RESULT:
+        '0x1b0ec5e2908e993d0c8ab6b46da46be2688fdf03c7ea6686075de37392e50a7d7fcc531446699132fbda915bd989882e0064d417018773a315fb8d43ed063c9b00',
+    TESTRPC_DERIVATION_PATH: `m/44'/60'/0'/0`,
+};
diff --git a/packages/testnet-faucets/src/ts/handler.ts b/packages/testnet-faucets/src/ts/handler.ts
index 9c8e59248..a6e786552 100644
--- a/packages/testnet-faucets/src/ts/handler.ts
+++ b/packages/testnet-faucets/src/ts/handler.ts
@@ -9,15 +9,13 @@ import * as Web3 from 'web3';
 // we are not running in a browser env.
 // Filed issue: https://github.com/ethereum/web3.js/issues/844
 (global as any).XMLHttpRequest = undefined;
-import { NonceTrackerSubprovider, PKWalletSubprovider } from '@0xproject/subproviders';
+import { NonceTrackerSubprovider, PrivateKeyWalletSubprovider } from '@0xproject/subproviders';
 import ProviderEngine = require('web3-provider-engine');
-import HookedWalletSubprovider = require('web3-provider-engine/subproviders/hooked-wallet');
 import RpcSubprovider = require('web3-provider-engine/subproviders/rpc');
 
 import { configs } from './configs';
 import { DispatchQueue } from './dispatch_queue';
 import { dispenseAssetTasks } from './dispense_asset_tasks';
-import { idManagement } from './id_management';
 import { rpcUrls } from './rpc_urls';
 
 interface NetworkConfig {
@@ -46,7 +44,7 @@ export class Handler {
         }
         const engine = new ProviderEngine();
         engine.addProvider(new NonceTrackerSubprovider());
-        engine.addProvider(new PKWalletSubprovider(configs.DISPENSER_PRIVATE_KEY));
+        engine.addProvider(new PrivateKeyWalletSubprovider(configs.DISPENSER_PRIVATE_KEY));
         engine.addProvider(
             new RpcSubprovider({
                 rpcUrl,
diff --git a/packages/testnet-faucets/src/ts/id_management.ts b/packages/testnet-faucets/src/ts/id_management.ts
deleted file mode 100644
index 7c598f91c..000000000
--- a/packages/testnet-faucets/src/ts/id_management.ts
+++ /dev/null
@@ -1,35 +0,0 @@
-import EthereumTx = require('ethereumjs-tx');
-import * as ethUtil from 'ethereumjs-util';
-import * as _ from 'lodash';
-
-import { configs } from './configs';
-
-type Callback = (err: Error | null, result: any) => void;
-
-export const idManagement = {
-    getAccounts(callback: Callback) {
-        callback(null, [configs.DISPENSER_ADDRESS]);
-    },
-    approveTransaction(txData: object, callback: Callback) {
-        callback(null, true);
-    },
-    signTransaction(txData: object, callback: Callback) {
-        const tx = new EthereumTx(txData);
-        const privateKeyBuffer = new Buffer(configs.DISPENSER_PRIVATE_KEY as string, 'hex');
-        tx.sign(privateKeyBuffer);
-        const rawTx = `0x${tx.serialize().toString('hex')}`;
-        callback(null, rawTx);
-    },
-    signMessage(message: object, callback: Callback) {
-        const dataIfExists = _.get(message, 'data');
-        if (_.isUndefined(dataIfExists)) {
-            callback(new Error('NO_DATA_TO_SIGN'), null);
-        }
-        const privateKeyBuffer = new Buffer(configs.DISPENSER_PRIVATE_KEY as string, 'hex');
-        const dataBuff = ethUtil.toBuffer(dataIfExists);
-        const msgHashBuff = ethUtil.hashPersonalMessage(dataBuff);
-        const sig = ethUtil.ecsign(msgHashBuff, privateKeyBuffer);
-        const rpcSig = ethUtil.toRpcSig(sig.v, sig.r, sig.s);
-        callback(null, rpcSig);
-    },
-};