Merge pull request #802 from 0xProject/fix/five_decimal_scenario

FillOrder Combinatorial Testing Fixes

4 files changed, 116 insertions, 40 deletions

diff --git a/packages/contracts/test/exchange/fill_order.ts b/packages/contracts/test/exchange/fill_order.ts
index 22eb401d9..029bd66e2 100644
--- a/packages/contracts/test/exchange/fill_order.ts
+++ b/packages/contracts/test/exchange/fill_order.ts
@@ -65,14 +65,7 @@ describe('FillOrder Tests', () => {
     describe('fillOrder', () => {
         const test = (fillScenarios: FillScenario[]) => {
             _.forEach(fillScenarios, fillScenario => {
-                const orderScenario = fillScenario.orderScenario;
-                const description = `Combinatorial OrderFill: ${orderScenario.feeRecipientScenario} ${
-                    orderScenario.makerAssetAmountScenario
-                } ${orderScenario.takerAssetAmountScenario} ${orderScenario.makerFeeScenario} ${
-                    orderScenario.takerFeeScenario
-                } ${orderScenario.expirationTimeSecondsScenario} ${orderScenario.makerAssetDataScenario} ${
-                    orderScenario.takerAssetDataScenario
-                }`;
+                const description = `Combinatorial OrderFill: ${JSON.stringify(fillScenario)}`;
                 it(description, async () => {
                     await coreCombinatorialUtils.testFillOrderScenarioAsync(provider, fillScenario);
                 });
diff --git a/packages/contracts/test/utils/asset_wrapper.ts b/packages/contracts/test/utils/asset_wrapper.ts
index 402a7ab28..f291170a2 100644
--- a/packages/contracts/test/utils/asset_wrapper.ts
+++ b/packages/contracts/test/utils/asset_wrapper.ts
@@ -84,8 +84,15 @@ export class AssetWrapper {
                         userAddress,
                     );
                 } else if (tokenOwner === userAddress && desiredBalance.eq(0)) {
-                    // Burn token
-                    await erc721Wrapper.burnAsync(assetProxyData.tokenAddress, assetProxyData.tokenId, userAddress);
+                    // Transfer token to someone else
+                    const userAddresses = await (erc721Wrapper as any)._web3Wrapper.getAvailableAddressesAsync();
+                    const nonOwner = _.find(userAddresses, a => a !== userAddress);
+                    await erc721Wrapper.transferFromAsync(
+                        assetProxyData.tokenAddress,
+                        assetProxyData.tokenId,
+                        tokenOwner,
+                        nonOwner,
+                    );
                     return;
                 } else if (
                     (userAddress !== tokenOwner && desiredBalance.eq(0)) ||
diff --git a/packages/contracts/test/utils/core_combinatorial_utils.ts b/packages/contracts/test/utils/core_combinatorial_utils.ts
index 718be17e0..03b09521d 100644
--- a/packages/contracts/test/utils/core_combinatorial_utils.ts
+++ b/packages/contracts/test/utils/core_combinatorial_utils.ts
@@ -146,13 +146,39 @@ export class CoreCombinatorialUtils {
     public exchangeWrapper: ExchangeWrapper;
     public assetWrapper: AssetWrapper;
     public static generateFillOrderCombinations(): FillScenario[] {
-        const takerScenarios = [TakerScenario.Unspecified];
-        const feeRecipientScenarios = [FeeRecipientAddressScenario.EthUserAddress];
-        const makerAssetAmountScenario = [OrderAssetAmountScenario.Large];
-        const takerAssetAmountScenario = [OrderAssetAmountScenario.Large];
-        const makerFeeScenario = [OrderAssetAmountScenario.Large];
-        const takerFeeScenario = [OrderAssetAmountScenario.Large];
-        const expirationTimeSecondsScenario = [ExpirationTimeSecondsScenario.InFuture];
+        const takerScenarios = [
+            TakerScenario.Unspecified,
+            // TakerScenario.CorrectlySpecified,
+            // TakerScenario.IncorrectlySpecified,
+        ];
+        const feeRecipientScenarios = [
+            FeeRecipientAddressScenario.EthUserAddress,
+            // FeeRecipientAddressScenario.BurnAddress,
+        ];
+        const makerAssetAmountScenario = [
+            OrderAssetAmountScenario.Large,
+            // OrderAssetAmountScenario.Zero,
+            // OrderAssetAmountScenario.Small,
+        ];
+        const takerAssetAmountScenario = [
+            OrderAssetAmountScenario.Large,
+            // OrderAssetAmountScenario.Zero,
+            // OrderAssetAmountScenario.Small,
+        ];
+        const makerFeeScenario = [
+            OrderAssetAmountScenario.Large,
+            // OrderAssetAmountScenario.Small,
+            // OrderAssetAmountScenario.Zero,
+        ];
+        const takerFeeScenario = [
+            OrderAssetAmountScenario.Large,
+            // OrderAssetAmountScenario.Small,
+            // OrderAssetAmountScenario.Zero,
+        ];
+        const expirationTimeSecondsScenario = [
+            ExpirationTimeSecondsScenario.InFuture,
+            ExpirationTimeSecondsScenario.InPast,
+        ];
         const makerAssetDataScenario = [
             AssetDataScenario.ERC20FiveDecimals,
             AssetDataScenario.ERC20NonZRXEighteenDecimals,
@@ -165,7 +191,55 @@ export class CoreCombinatorialUtils {
             AssetDataScenario.ERC721,
             AssetDataScenario.ZRXFeeToken,
         ];
-        const takerAssetFillAmountScenario = [TakerAssetFillAmountScenario.ExactlyRemainingFillableTakerAssetAmount];
+        const takerAssetFillAmountScenario = [
+            TakerAssetFillAmountScenario.ExactlyRemainingFillableTakerAssetAmount,
+            // TakerAssetFillAmountScenario.GreaterThanRemainingFillableTakerAssetAmount,
+            // TakerAssetFillAmountScenario.LessThanRemainingFillableTakerAssetAmount,
+        ];
+        const makerAssetBalanceScenario = [
+            BalanceAmountScenario.Higher,
+            // BalanceAmountScenario.Exact,
+            // BalanceAmountScenario.TooLow,
+        ];
+        const makerAssetAllowanceScenario = [
+            AllowanceAmountScenario.Higher,
+            // AllowanceAmountScenario.Exact,
+            // AllowanceAmountScenario.TooLow,
+            // AllowanceAmountScenario.Unlimited,
+        ];
+        const makerZRXBalanceScenario = [
+            BalanceAmountScenario.Higher,
+            // BalanceAmountScenario.Exact,
+            // BalanceAmountScenario.TooLow,
+        ];
+        const makerZRXAllowanceScenario = [
+            AllowanceAmountScenario.Higher,
+            // AllowanceAmountScenario.Exact,
+            // AllowanceAmountScenario.TooLow,
+            // AllowanceAmountScenario.Unlimited,
+        ];
+        const takerAssetBalanceScenario = [
+            BalanceAmountScenario.Higher,
+            // BalanceAmountScenario.Exact,
+            // BalanceAmountScenario.TooLow,
+        ];
+        const takerAssetAllowanceScenario = [
+            AllowanceAmountScenario.Higher,
+            // AllowanceAmountScenario.Exact,
+            // AllowanceAmountScenario.TooLow,
+            // AllowanceAmountScenario.Unlimited,
+        ];
+        const takerZRXBalanceScenario = [
+            BalanceAmountScenario.Higher,
+            // BalanceAmountScenario.Exact,
+            // BalanceAmountScenario.TooLow,
+        ];
+        const takerZRXAllowanceScenario = [
+            AllowanceAmountScenario.Higher,
+            // AllowanceAmountScenario.Exact,
+            // AllowanceAmountScenario.TooLow,
+            // AllowanceAmountScenario.Unlimited,
+        ];
         const fillScenarioArrays = CoreCombinatorialUtils._getAllCombinations([
             takerScenarios,
             feeRecipientScenarios,
@@ -177,9 +251,18 @@ export class CoreCombinatorialUtils {
             makerAssetDataScenario,
             takerAssetDataScenario,
             takerAssetFillAmountScenario,
+            makerAssetBalanceScenario,
+            makerAssetAllowanceScenario,
+            makerZRXBalanceScenario,
+            makerZRXAllowanceScenario,
+            takerAssetBalanceScenario,
+            takerAssetAllowanceScenario,
+            takerZRXBalanceScenario,
+            takerZRXAllowanceScenario,
         ]);
 
         const fillScenarios = _.map(fillScenarioArrays, fillScenarioArray => {
+            // tslint:disable:custom-no-magic-numbers
             const fillScenario: FillScenario = {
                 orderScenario: {
                     takerScenario: fillScenarioArray[0] as TakerScenario,
@@ -194,18 +277,19 @@ export class CoreCombinatorialUtils {
                 },
                 takerAssetFillAmountScenario: fillScenarioArray[9] as TakerAssetFillAmountScenario,
                 makerStateScenario: {
-                    traderAssetBalance: BalanceAmountScenario.Higher,
-                    traderAssetAllowance: AllowanceAmountScenario.Higher,
-                    zrxFeeBalance: BalanceAmountScenario.Higher,
-                    zrxFeeAllowance: AllowanceAmountScenario.Higher,
+                    traderAssetBalance: fillScenarioArray[10] as BalanceAmountScenario,
+                    traderAssetAllowance: fillScenarioArray[11] as AllowanceAmountScenario,
+                    zrxFeeBalance: fillScenarioArray[12] as BalanceAmountScenario,
+                    zrxFeeAllowance: fillScenarioArray[13] as AllowanceAmountScenario,
                 },
                 takerStateScenario: {
-                    traderAssetBalance: BalanceAmountScenario.Higher,
-                    traderAssetAllowance: AllowanceAmountScenario.Higher,
-                    zrxFeeBalance: BalanceAmountScenario.Higher,
-                    zrxFeeAllowance: AllowanceAmountScenario.Higher,
+                    traderAssetBalance: fillScenarioArray[14] as BalanceAmountScenario,
+                    traderAssetAllowance: fillScenarioArray[15] as AllowanceAmountScenario,
+                    zrxFeeBalance: fillScenarioArray[16] as BalanceAmountScenario,
+                    zrxFeeAllowance: fillScenarioArray[17] as AllowanceAmountScenario,
                 },
             };
+            // tslint:enable:custom-no-magic-numbers
             return fillScenario;
         });
 
@@ -768,25 +852,17 @@ export class CoreCombinatorialUtils {
 
             case AllowanceAmountScenario.TooLow:
                 const tooLowAllowance = takerFee.minus(1);
-                await this.assetWrapper.setProxyAllowanceAsync(
-                    signedOrder.takerAddress,
-                    this.zrxAssetData,
-                    tooLowAllowance,
-                );
+                await this.assetWrapper.setProxyAllowanceAsync(this.takerAddress, this.zrxAssetData, tooLowAllowance);
                 break;
 
             case AllowanceAmountScenario.Exact:
                 const exactAllowance = takerFee;
-                await this.assetWrapper.setProxyAllowanceAsync(
-                    signedOrder.takerAddress,
-                    this.zrxAssetData,
-                    exactAllowance,
-                );
+                await this.assetWrapper.setProxyAllowanceAsync(this.takerAddress, this.zrxAssetData, exactAllowance);
                 break;
 
             case AllowanceAmountScenario.Unlimited:
                 await this.assetWrapper.setProxyAllowanceAsync(
-                    signedOrder.takerAddress,
+                    this.takerAddress,
                     this.zrxAssetData,
                     constants.UNLIMITED_ALLOWANCE_IN_BASE_UNITS,
                 );
diff --git a/packages/order-utils/src/order_validation_utils.ts b/packages/order-utils/src/order_validation_utils.ts
index 54428f77d..94df3ef82 100644
--- a/packages/order-utils/src/order_validation_utils.ts
+++ b/packages/order-utils/src/order_validation_utils.ts
@@ -140,12 +140,12 @@ export class OrderValidationUtils {
         takerAddress: string,
         zrxAssetData: string,
     ): Promise<BigNumber> {
-        if (fillTakerAssetAmount.eq(0)) {
-            throw new Error(RevertReason.InvalidTakerAmount);
-        }
         if (signedOrder.makerAssetAmount.eq(0) || signedOrder.takerAssetAmount.eq(0)) {
             throw new Error(RevertReason.OrderUnfillable);
         }
+        if (fillTakerAssetAmount.eq(0)) {
+            throw new Error(RevertReason.InvalidTakerAmount);
+        }
         const orderHash = orderHashUtils.getOrderHashHex(signedOrder);
         const isValid = await isValidSignatureAsync(
             provider,