Address feedback

3 files changed, 41 insertions, 26 deletions

diff --git a/src/ts/0x.js.ts b/src/ts/0x.js.ts
index 82dbce22d..893d95de6 100644
--- a/src/ts/0x.js.ts
+++ b/src/ts/0x.js.ts
@@ -20,41 +20,47 @@ export interface ECSignature {
 const MAX_DIGITS_IN_UNSIGNED_256_INT = 78;
 
 export class ZeroEx {
-    public static getOrderHash(exchangeContractAddr: string, makerAddr: string, takerAddr: string,
-                               depositTokenAddr: string, receiveTokenAddr: string, feeRecipient: string,
-                               depositAmt: BigNumber.BigNumber, receiveAmt: BigNumber.BigNumber,
-                               makerFee: BigNumber.BigNumber, takerFee: BigNumber.BigNumber,
-                               expiration: BigNumber.BigNumber, salt: BigNumber.BigNumber): string {
-        takerAddr = takerAddr !== '' ? takerAddr : constants.NULL_ADDRESS;
+    /**
+     * Computes the orderHash given the order parameters and returns it as a hex encoded string.
+     */
+    public static getOrderHashHex(exchangeContractAddr: string, makerAddr: string, takerAddr: string,
+                                  tokenMAddress: string, tokenTAddress: string, feeRecipient: string,
+                                  valueM: BigNumber.BigNumber, valueT: BigNumber.BigNumber,
+                                  makerFee: BigNumber.BigNumber, takerFee: BigNumber.BigNumber,
+                                  expiration: BigNumber.BigNumber, salt: BigNumber.BigNumber): string {
+        takerAddr = _.isEmpty(takerAddr) ? constants.NULL_ADDRESS : takerAddr ;
         assert.isETHAddressHex('exchangeContractAddr', exchangeContractAddr);
         assert.isETHAddressHex('makerAddr', makerAddr);
         assert.isETHAddressHex('takerAddr', takerAddr);
-        assert.isETHAddressHex('depositTokenAddr', depositTokenAddr);
-        assert.isETHAddressHex('receiveTokenAddr', receiveTokenAddr);
+        assert.isETHAddressHex('tokenMAddress', tokenMAddress);
+        assert.isETHAddressHex('tokenTAddress', tokenTAddress);
         assert.isETHAddressHex('feeRecipient', feeRecipient);
-        assert.isBigNumber('depositAmt', depositAmt);
-        assert.isBigNumber('receiveAmt', receiveAmt);
+        assert.isBigNumber('valueM', valueM);
+        assert.isBigNumber('valueT', valueT);
         assert.isBigNumber('makerFee', makerFee);
         assert.isBigNumber('takerFee', takerFee);
         assert.isBigNumber('expiration', expiration);
         assert.isBigNumber('salt', salt);
+
         const orderParts = [
             {value: exchangeContractAddr, type: SolidityTypes.address},
             {value: makerAddr, type: SolidityTypes.address},
             {value: takerAddr, type: SolidityTypes.address},
-            {value: depositTokenAddr, type: SolidityTypes.address},
-            {value: receiveTokenAddr, type: SolidityTypes.address},
+            {value: tokenMAddress, type: SolidityTypes.address},
+            {value: tokenTAddress, type: SolidityTypes.address},
             {value: feeRecipient, type: SolidityTypes.address},
-            {value: new BN(depositAmt.toString(), 10), type: SolidityTypes.uint256},
-            {value: new BN(receiveAmt.toString(), 10), type: SolidityTypes.uint256},
-            {value: new BN(makerFee.toString(), 10), type: SolidityTypes.uint256},
-            {value: new BN(takerFee.toString(), 10), type: SolidityTypes.uint256},
-            {value: new BN(expiration.toString(), 10), type: SolidityTypes.uint256},
-            {value: new BN(salt.toString(), 10), type: SolidityTypes.uint256},
+            {value: this.bigNumberToBN(valueM), type: SolidityTypes.uint256},
+            {value: this.bigNumberToBN(valueT), type: SolidityTypes.uint256},
+            {value: this.bigNumberToBN(makerFee), type: SolidityTypes.uint256},
+            {value: this.bigNumberToBN(takerFee), type: SolidityTypes.uint256},
+            {value: this.bigNumberToBN(expiration), type: SolidityTypes.uint256},
+            {value: this.bigNumberToBN(salt), type: SolidityTypes.uint256},
         ];
-        const hashBuff = ethABI.soliditySHA3(_.map(orderParts, 'type'), _.map(orderParts, 'value'));
-        const buffHashHex = ethUtil.bufferToHex(hashBuff);
-        return buffHashHex;
+        const types = _.map(orderParts, o => o.type);
+        const values = _.map(orderParts, o => o.value);
+        const hashBuff = ethABI.soliditySHA3(types, values);
+        const hashHex = ethUtil.bufferToHex(hashBuff);
+        return hashHex;
     }
     /**
      * Verifies that the elliptic curve signature `signature` was generated
@@ -123,4 +129,13 @@ export class ZeroEx {
         const baseUnitAmount = amount.times(unit);
         return baseUnitAmount;
     }
+
+    /**
+     * Converts BigNumber instance to BN
+     * We do it because ethABI accepts only BN's
+     * We should be consistent about using BigNumbers in our codebase and not use BN anywhere else
+     */
+    private static bigNumberToBN(value: BigNumber.BigNumber) {
+        return new BN(value.toString(), 10);
+    }
 }
diff --git a/src/ts/types.ts b/src/ts/types.ts
index c3da8d81e..c49ef2331 100644
--- a/src/ts/types.ts
+++ b/src/ts/types.ts
@@ -17,5 +17,4 @@ export const SolidityTypes = strEnum([
     'string',
     'bool',
 ]);
-
 export type SolidityTypes = keyof typeof SolidityTypes;
diff --git a/test/0x.js.ts b/test/0x.js.ts
index 07c58f1a5..d5b2015fb 100644
--- a/test/0x.js.ts
+++ b/test/0x.js.ts
@@ -11,8 +11,9 @@ const expect = chai.expect;
 
 describe('ZeroEx library', () => {
     describe('#getOrderHash', () => {
+        const ORDER_HASH = '0x103a5e97dab5dbeb8f385636f86a7d1e458a7ccbe1bd194727f0b2f85ab116c7';
         it('defaults takerAddress to NULL address', () => {
-            const orderHash = ZeroEx.getOrderHash(
+            const orderHash = ZeroEx.getOrderHashHex(
                 constants.NULL_ADDRESS,
                 constants.NULL_ADDRESS,
                 '',
@@ -26,10 +27,10 @@ describe('ZeroEx library', () => {
                 new BigNumber(0),
                 new BigNumber(0),
             );
-            expect(orderHash).to.be.equal('0x103a5e97dab5dbeb8f385636f86a7d1e458a7ccbe1bd194727f0b2f85ab116c7');
+            expect(orderHash).to.be.equal(ORDER_HASH);
         });
         it('calculates the order hash', () => {
-            const orderHash = ZeroEx.getOrderHash(
+            const orderHash = ZeroEx.getOrderHashHex(
                 constants.NULL_ADDRESS,
                 constants.NULL_ADDRESS,
                 constants.NULL_ADDRESS,
@@ -43,7 +44,7 @@ describe('ZeroEx library', () => {
                 new BigNumber(0),
                 new BigNumber(0),
             );
-            expect(orderHash).to.be.equal('0x103a5e97dab5dbeb8f385636f86a7d1e458a7ccbe1bd194727f0b2f85ab116c7');
+            expect(orderHash).to.be.equal(ORDER_HASH);
         });
     });
     describe('#isValidSignature', () => {