Addressed comments and update 0xProject/types-ethereumjs-util deps

9 files changed, 74 insertions, 103 deletions

diff --git a/packages/testnet-faucets/README.md b/packages/testnet-faucets/README.md
index d0b9858a1..6cd842ea6 100644
--- a/packages/testnet-faucets/README.md
+++ b/packages/testnet-faucets/README.md
@@ -86,25 +86,24 @@ Returns a JSON payload describing the state of the queues for each network. For
 }
 ```
 
-`GET /ether/:recipient`
+`GET /ether/:recipient?networkId=:networkId`
 
-Schedules a transaction that sends 0.1 ETH to the `recipient` where `recipient` is a hex encoded Ethereum address prefixed with `0x`.
+Schedules a transaction that sends 0.1 ETH to the `recipient` on the network specified by `networkId` where `recipient` is a hex encoded Ethereum address prefixed with `0x`. If no `networkId` is provided via query parameters the faucet will default to network 42 (Kovan).
 
-`GET /zrx/:recipient`
 
-Schedules a transaction that sends 0.1 ZRX to the `recipient` where `recipient` is a hex encoded Ethereum address prefixed with `0x`.
+`GET /zrx/:recipient?networkId=:networkId`
 
-`GET /order/weth/:recipient`
+Schedules a transaction that sends 0.1 ZRX to the `recipient` on the network specified by `networkId` where `recipient` is a hex encoded Ethereum address prefixed with `0x`. If no `networkId` is provided via query parameters the faucet will default to network 42 (Kovan).
 
-Returns a JSON payload describing an order for 0.1 WETH in exchange for 0.1 ZRX signed by the dispenser address. The taker is specified by `recipient` where `recipient` is a hex encoded Ethereum address prefixed with `0x`.
+`GET /order/weth/:recipient?networkId=:networkId`
 
-`GET /order/zrx/:recipient`
+Returns a JSON payload describing an order for 0.1 WETH in exchange for 0.1 ZRX signed by the dispenser address on the network specified by `networkId`. The taker is specified by `recipient` where `recipient` is a hex encoded Ethereum address prefixed with `0x`. If no `networkId` is provided via query parameters the faucet will default to network 42 (Kovan).
 
-Returns a JSON payload describing an order for 0.1 ZRX in exchange for 0.1 WETH signed by the dispenser address. The taker is specified by `recipient` where `recipient` is a hex encoded Ethereum address prefixed with `0x`.
+`GET /order/zrx/:recipient?networkId=:networkId`
 
-#### Parameters
+Returns a JSON payload describing an order for 0.1 ZRX in exchange for 0.1 WETH signed by the dispenser address on the network specified by `networkId`. The taker is specified by `recipient` where `recipient` is a hex encoded Ethereum address prefixed with `0x`. If no `networkId` is provided via query parameters the faucet will default to network 42 (Kovan).
 
-The endpoints `/ether`, `/zrx`, `/order/weth/`, and `/order/zrx/` take a query parameter named `networkId` to specify the desired ethereum network
+#### Example request
 
 ```bash
 curl -i http://localhost:3000/ether/0x14e2F1F157E7DD4057D02817436D628A37120FD1\?networkId=3
@@ -112,8 +111,6 @@ curl -i http://localhost:3000/ether/0x14e2F1F157E7DD4057D02817436D628A37120FD1\?
 
 This command will request the local server to initiate a transfer of 0.1 ETH from the dispensing address to `0x14e2F1F157E7DD4057D02817436D628A37120FD1` on the Ropsten testnet.
 
-If no `networkId` is provided via query parameters the faucet will default to network 42 (Kovan)
-
 ### Docker configs
 
 ```
diff --git a/packages/testnet-faucets/package.json b/packages/testnet-faucets/package.json
index f73857f82..b7a55aeb0 100644
--- a/packages/testnet-faucets/package.json
+++ b/packages/testnet-faucets/package.json
@@ -37,7 +37,6 @@
         "shx": "^0.2.2",
         "source-map-loader": "^0.1.6",
         "tslint": "5.8.0",
-        "types-bn": "^0.0.1",
         "types-ethereumjs-util": "0xProject/types-ethereumjs-util",
         "typescript": "2.7.1",
         "web3-typescript-typings": "^0.9.8",
diff --git a/packages/testnet-faucets/src/ts/global.d.ts b/packages/testnet-faucets/src/ts/global.d.ts
index 50cd205a1..97cd35680 100644
--- a/packages/testnet-faucets/src/ts/global.d.ts
+++ b/packages/testnet-faucets/src/ts/global.d.ts
@@ -24,13 +24,3 @@ declare module 'ethereumjs-tx' {
     }
     export = EthereumTx;
 }
-
-// Define extra params on Request for parameter extraction in middleware
-/* tslint:disable */
-declare namespace Express {
-    export interface Request {
-        recipientAddress: string;
-        networkId: string;
-    }
-}
-/* tslint:enable */
diff --git a/packages/testnet-faucets/src/ts/handler.ts b/packages/testnet-faucets/src/ts/handler.ts
index 7883cefd8..1dae8a47f 100644
--- a/packages/testnet-faucets/src/ts/handler.ts
+++ b/packages/testnet-faucets/src/ts/handler.ts
@@ -2,13 +2,13 @@ import { Order, SignedOrder, ZeroEx } from '0x.js';
 import { BigNumber } from '@0xproject/utils';
 import * as express from 'express';
 import * as _ from 'lodash';
+import * as Web3 from 'web3';
 
 // HACK: web3 injects XMLHttpRequest into the global scope and ProviderEngine checks XMLHttpRequest
 // to know whether it is running in a browser or node environment. We need it to be undefined since
 // we are not running in a browser env.
 // Filed issue: https://github.com/ethereum/web3.js/issues/844
 (global as any).XMLHttpRequest = undefined;
-
 import ProviderEngine = require('web3-provider-engine');
 import HookedWalletSubprovider = require('web3-provider-engine/subproviders/hooked-wallet');
 import NonceSubprovider = require('web3-provider-engine/subproviders/nonce-tracker');
@@ -22,12 +22,6 @@ import { rpcUrls } from './rpc_urls';
 import { utils } from './utils';
 import { ZRXRequestQueue } from './zrx_request_queue';
 
-// HACK: web3 leaks XMLHttpRequest into the global scope and causes requests to hang
-// because they are using the wrong XHR package.
-// Filed issue: https://github.com/ethereum/web3.js/issues/844
-// tslint:disable-next-line:ordered-imports
-import * as Web3 from 'web3';
-
 interface ItemByNetworkId<T> {
     [networkId: string]: T;
 }
@@ -38,15 +32,46 @@ enum RequestedAssetType {
     ZRX = 'ZRX',
 }
 
-const TWO_DAYS_IN_MS = 1.728e8; // TODO: make this configurable
+const FIVE_DAYS_IN_MS = 4.32e8; // TODO: make this configurable
 
 export class Handler {
     private _zeroExByNetworkId: ItemByNetworkId<ZeroEx> = {};
     private _etherRequestQueueByNetworkId: ItemByNetworkId<RequestQueue> = {};
     private _zrxRequestQueueByNetworkId: ItemByNetworkId<RequestQueue> = {};
+    private static _dispenseAsset(
+        req: express.Request,
+        res: express.Response,
+        requestQueueByNetworkId: ItemByNetworkId<RequestQueue>,
+        requestedAssetType: RequestedAssetType,
+    ) {
+        const requestQueue = _.get(requestQueueByNetworkId, req.params.networkId);
+        if (_.isUndefined(requestQueue)) {
+            res.status(400).send('UNSUPPORTED_NETWORK_ID');
+            return;
+        }
+        const didAddToQueue = requestQueue.add(req.params.recipient);
+        if (!didAddToQueue) {
+            res.status(503).send('QUEUE_IS_FULL');
+            return;
+        }
+        utils.consoleLog(`Added ${req.params.recipient} to queue: ${requestedAssetType} networkId: ${req.params.networkId}`);
+        res.status(200).end();
+    }
+    private static _createProviderEngine(rpcUrl: string) {
+        const engine = new ProviderEngine();
+        engine.addProvider(new NonceSubprovider());
+        engine.addProvider(new HookedWalletSubprovider(idManagement));
+        engine.addProvider(
+            new RpcSubprovider({
+                rpcUrl,
+            }),
+        );
+        engine.start();
+        return engine;
+    }
     constructor() {
         _.forIn(rpcUrls, (rpcUrl: string, networkId: string) => {
-            const providerObj = this._createProviderEngine(rpcUrl);
+            const providerObj = Handler._createProviderEngine(rpcUrl);
             const web3 = new Web3(providerObj);
             const zeroExConfig = {
                 networkId: +networkId,
@@ -77,10 +102,10 @@ export class Handler {
         res.status(200).send(payload);
     }
     public dispenseEther(req: express.Request, res: express.Response) {
-        this._dispenseAsset(req, res, this._etherRequestQueueByNetworkId, RequestedAssetType.ETH);
+        Handler._dispenseAsset(req, res, this._etherRequestQueueByNetworkId, RequestedAssetType.ETH);
     }
     public dispenseZRX(req: express.Request, res: express.Response) {
-        this._dispenseAsset(req, res, this._zrxRequestQueueByNetworkId, RequestedAssetType.ZRX);
+        Handler._dispenseAsset(req, res, this._zrxRequestQueueByNetworkId, RequestedAssetType.ZRX);
     }
     public async dispenseWETHOrder(req: express.Request, res: express.Response) {
         await this._dispenseOrder(req, res, RequestedAssetType.WETH);
@@ -88,28 +113,8 @@ export class Handler {
     public async dispenseZRXOrder(req: express.Request, res: express.Response, next: express.NextFunction) {
         await this._dispenseOrder(req, res, RequestedAssetType.ZRX);
     }
-    // tslint:disable-next-line:prefer-function-over-method
-    private _dispenseAsset(
-        req: express.Request,
-        res: express.Response,
-        requestQueueByNetworkId: ItemByNetworkId<RequestQueue>,
-        requestedAssetType: RequestedAssetType,
-    ) {
-        const requestQueue = _.get(requestQueueByNetworkId, req.networkId);
-        if (_.isUndefined(requestQueue)) {
-            res.status(400).send('UNSUPPORTED_NETWORK_ID');
-            return;
-        }
-        const didAddToQueue = requestQueue.add(req.recipientAddress);
-        if (!didAddToQueue) {
-            res.status(503).send('QUEUE_IS_FULL');
-            return;
-        }
-        utils.consoleLog(`Added ${req.recipientAddress} to queue: ${requestedAssetType} networkId: ${req.networkId}`);
-        res.status(200).end();
-    }
     private async _dispenseOrder(req: express.Request, res: express.Response, requestedAssetType: RequestedAssetType) {
-        const zeroEx = _.get(this._zeroExByNetworkId, req.networkId);
+        const zeroEx = _.get(this._zeroExByNetworkId, req.params.networkId);
         if (_.isUndefined(zeroEx)) {
             res.status(400).send('UNSUPPORTED_NETWORK_ID');
             return;
@@ -117,21 +122,19 @@ export class Handler {
         res.setHeader('Content-Type', 'application/json');
         const makerTokenAddress = await zeroEx.tokenRegistry.getTokenAddressBySymbolIfExistsAsync(requestedAssetType);
         if (_.isUndefined(makerTokenAddress)) {
-            res.status(400).send('INVALID_TOKEN');
-            return;
+            throw new Error(`Unsupported asset type: ${requestedAssetType}`);
         }
         const takerTokenSymbol =
             requestedAssetType === RequestedAssetType.WETH ? RequestedAssetType.ZRX : RequestedAssetType.WETH;
         const takerTokenAddress = await zeroEx.tokenRegistry.getTokenAddressBySymbolIfExistsAsync(takerTokenSymbol);
         if (_.isUndefined(takerTokenAddress)) {
-            res.status(400).send('INVALID_TOKEN');
-            return;
+            throw new Error(`Unsupported asset type: ${requestedAssetType}`);
         }
         const makerTokenAmount = new BigNumber(0.1);
         const takerTokenAmount = new BigNumber(0.1);
         const order: Order = {
             maker: configs.DISPENSER_ADDRESS,
-            taker: req.recipientAddress,
+            taker: req.params.recipient,
             makerFee: new BigNumber(0),
             takerFee: new BigNumber(0),
             makerTokenAmount,
@@ -141,7 +144,7 @@ export class Handler {
             salt: ZeroEx.generatePseudoRandomSalt(),
             exchangeContractAddress: zeroEx.exchange.getContractAddress(),
             feeRecipient: ZeroEx.NULL_ADDRESS,
-            expirationUnixTimestampSec: new BigNumber(Date.now() + TWO_DAYS_IN_MS),
+            expirationUnixTimestampSec: new BigNumber(Date.now() + FIVE_DAYS_IN_MS),
         };
         const orderHash = ZeroEx.getOrderHashHex(order);
         const signature = await zeroEx.signOrderHashAsync(orderHash, configs.DISPENSER_ADDRESS, false);
@@ -154,17 +157,4 @@ export class Handler {
         utils.consoleLog(`Dispensed signed order: ${payload}`);
         res.status(200).send(payload);
     }
-    // tslint:disable-next-line:prefer-function-over-method
-    private _createProviderEngine(rpcUrl: string) {
-        const engine = new ProviderEngine();
-        engine.addProvider(new NonceSubprovider());
-        engine.addProvider(new HookedWalletSubprovider(idManagement));
-        engine.addProvider(
-            new RpcSubprovider({
-                rpcUrl,
-            }),
-        );
-        engine.start();
-        return engine;
-    }
 }
diff --git a/packages/testnet-faucets/src/ts/id_management.ts b/packages/testnet-faucets/src/ts/id_management.ts
index b088e6cf8..7c598f91c 100644
--- a/packages/testnet-faucets/src/ts/id_management.ts
+++ b/packages/testnet-faucets/src/ts/id_management.ts
@@ -21,12 +21,12 @@ export const idManagement = {
         callback(null, rawTx);
     },
     signMessage(message: object, callback: Callback) {
-        const data = _.get(message, 'data');
-        if (_.isUndefined(data)) {
-            callback(new Error('No data to sign'), null);
+        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(data);
+        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);
diff --git a/packages/testnet-faucets/src/ts/parameter_extractor.ts b/packages/testnet-faucets/src/ts/parameter_transformer.ts
index f3a26ae01..c5711d462 100644
--- a/packages/testnet-faucets/src/ts/parameter_extractor.ts
+++ b/packages/testnet-faucets/src/ts/parameter_transformer.ts
@@ -8,27 +8,22 @@ import { utils } from './utils';
 
 const DEFAULT_NETWORK_ID = 42; // kovan
 
-export const parameterExtractor = {
-    extract(req: Request, res: Response, next: NextFunction) {
+export const parameterTransformer = {
+    transform(req: Request, res: Response, next: NextFunction) {
         const recipientAddress = req.params.recipient;
-        if (_.isUndefined(recipientAddress) || !_isValidEthereumAddress(recipientAddress)) {
+        if (_.isUndefined(recipientAddress) || !addressUtils.isAddress(recipientAddress)) {
             res.status(400).send('INVALID_RECIPIENT_ADDRESS');
             return;
         }
         const lowerCaseRecipientAddress = recipientAddress.toLowerCase();
-        req.recipientAddress = lowerCaseRecipientAddress;
+        req.params.recipient = lowerCaseRecipientAddress;
         const networkId = _.get(req.query, 'networkId', DEFAULT_NETWORK_ID);
-        const rpcUrl = _.get(rpcUrls, networkId);
-        if (_.isUndefined(rpcUrl)) {
+        const rpcUrlIfExists = _.get(rpcUrls, networkId);
+        if (_.isUndefined(rpcUrlIfExists)) {
             res.status(400).send('UNSUPPORTED_NETWORK_ID');
             return;
         }
-        req.networkId = networkId;
+        req.params.networkId = networkId;
         next();
     },
 };
-
-function _isValidEthereumAddress(address: string): boolean {
-    const lowercaseAddress = address.toLowerCase();
-    return addressUtils.isAddress(lowercaseAddress);
-}
diff --git a/packages/testnet-faucets/src/ts/request_queue.ts b/packages/testnet-faucets/src/ts/request_queue.ts
index 7d685522a..718f8be0c 100644
--- a/packages/testnet-faucets/src/ts/request_queue.ts
+++ b/packages/testnet-faucets/src/ts/request_queue.ts
@@ -13,14 +13,12 @@ const DEFAULT_QUEUE_INTERVAL_MS = 1000;
 export class RequestQueue {
     protected _queueIntervalMs: number;
     protected _queue: string[];
-    protected _queueIntervalId: NodeJS.Timer;
+    protected _queueIntervalId?: NodeJS.Timer;
     protected _web3: Web3;
     constructor(web3: any) {
         this._queueIntervalMs = DEFAULT_QUEUE_INTERVAL_MS;
         this._queue = [];
-
         this._web3 = web3;
-
         this._start();
     }
     public add(recipientAddress: string): boolean {
@@ -47,7 +45,9 @@ export class RequestQueue {
         }, this._queueIntervalMs);
     }
     protected _stop() {
-        clearInterval(this._queueIntervalId);
+        if (!_.isUndefined(this._queueIntervalId)) {
+            clearInterval(this._queueIntervalId);
+        }
     }
     // tslint:disable-next-line:prefer-function-over-method
     protected async _processNextRequestFireAndForgetAsync(recipientAddress: string) {
diff --git a/packages/testnet-faucets/src/ts/rpc_urls.ts b/packages/testnet-faucets/src/ts/rpc_urls.ts
index 25a3b938f..c57e85eac 100644
--- a/packages/testnet-faucets/src/ts/rpc_urls.ts
+++ b/packages/testnet-faucets/src/ts/rpc_urls.ts
@@ -1,13 +1,13 @@
 import { configs } from './configs';
 
 const productionRpcUrls = {
-    '2': `https://ropsten.infura.io/${configs.INFURA_API_KEY}`,
-    '3': `https://rinkeby.infura.io/${configs.INFURA_API_KEY}`,
-    '42': `https://kovan.infura.io/${configs.INFURA_API_KEY}`,
+    2: `https://ropsten.infura.io/${configs.INFURA_API_KEY}`,
+    3: `https://rinkeby.infura.io/${configs.INFURA_API_KEY}`,
+    42: `https://kovan.infura.io/${configs.INFURA_API_KEY}`,
 };
 
 const developmentRpcUrls = {
-    '50': 'http://127.0.0.1:8545',
+    50: 'http://127.0.0.1:8545',
 };
 
 export const rpcUrls = configs.ENVIRONMENT === 'development' ? developmentRpcUrls : productionRpcUrls;
diff --git a/packages/testnet-faucets/src/ts/server.ts b/packages/testnet-faucets/src/ts/server.ts
index 38c2f1428..be1077756 100644
--- a/packages/testnet-faucets/src/ts/server.ts
+++ b/packages/testnet-faucets/src/ts/server.ts
@@ -3,7 +3,7 @@ import * as express from 'express';
 
 import { errorReporter } from './error_reporter';
 import { Handler } from './handler';
-import { parameterExtractor } from './parameter_extractor';
+import { parameterTransformer } from './parameter_transformer';
 
 // Setup the errorReporter to catch uncaught exceptions and unhandled rejections
 errorReporter.setup();
@@ -21,10 +21,10 @@ app.get('/ping', (req: express.Request, res: express.Response) => {
     res.status(200).send('pong');
 });
 app.get('/info', handler.getQueueInfo.bind(handler));
-app.get('/ether/:recipient', parameterExtractor.extract, handler.dispenseEther.bind(handler));
-app.get('/zrx/:recipient', parameterExtractor.extract, handler.dispenseZRX.bind(handler));
-app.get('/order/weth/:recipient', parameterExtractor.extract, handler.dispenseWETHOrder.bind(handler));
-app.get('/order/zrx/:recipient', parameterExtractor.extract, handler.dispenseZRXOrder.bind(handler));
+app.get('/ether/:recipient', parameterTransformer.transform, handler.dispenseEther.bind(handler));
+app.get('/zrx/:recipient', parameterTransformer.transform, handler.dispenseZRX.bind(handler));
+app.get('/order/weth/:recipient', parameterTransformer.transform, handler.dispenseWETHOrder.bind(handler));
+app.get('/order/zrx/:recipient', parameterTransformer.transform, handler.dispenseZRXOrder.bind(handler));
 
 // Log to rollbar any errors unhandled by handlers
 app.use(errorReporter.errorHandler());