Feedback

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

4 files changed, 29 insertions, 23 deletions

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;
     }
 }