Merge pull request #324 from 0xProject/fix/0x.js/negativeFillValues

Reject negative amounts in public methods

4 files changed, 67 insertions, 0 deletions

diff --git a/packages/0x.js/CHANGELOG.md b/packages/0x.js/CHANGELOG.md
index 84997389a..1a09eaf28 100644
--- a/packages/0x.js/CHANGELOG.md
+++ b/packages/0x.js/CHANGELOG.md
@@ -1,5 +1,9 @@
 # CHANGELOG
 
+## v0.30.1 - _January 18, 2018_
+
+    * Fix a bug allowing negative fill values  (#212)
+
 ## v0.30.0 - _January 17, 2018_
 
     * Add an error parameter to the order watcher callback (#312)
diff --git a/packages/0x.js/test/exchange_wrapper_test.ts b/packages/0x.js/test/exchange_wrapper_test.ts
index d2a2149a0..7e0ffd818 100644
--- a/packages/0x.js/test/exchange_wrapper_test.ts
+++ b/packages/0x.js/test/exchange_wrapper_test.ts
@@ -390,6 +390,29 @@ describe('ExchangeWrapper', () => {
                     ).to.not.be.rejectedWith(ExchangeContractErrs.OrderFillAmountZero);
                 });
             });
+            describe('negative fill amount', async () => {
+                let signedOrder: SignedOrder;
+                const negativeFillTakerAmount = new BigNumber(-100);
+                beforeEach(async () => {
+                    signedOrder = await fillScenarios.createFillableSignedOrderAsync(
+                        makerTokenAddress,
+                        takerTokenAddress,
+                        makerAddress,
+                        takerAddress,
+                        fillableAmount,
+                    );
+                });
+                it('should not allow the exchange wrapper to fill if amount is negative', async () => {
+                    return expect(
+                        zeroEx.exchange.fillOrderAsync(
+                            signedOrder,
+                            negativeFillTakerAmount,
+                            shouldThrowOnInsufficientBalanceOrAllowance,
+                            takerAddress,
+                        ),
+                    ).to.be.rejected();
+                });
+            });
         });
         describe('#batchFillOrdersAsync', () => {
             let signedOrder: SignedOrder;
@@ -498,6 +521,30 @@ describe('ExchangeWrapper', () => {
                     ).to.not.be.rejectedWith(ExchangeContractErrs.OrderFillAmountZero);
                 });
             });
+            describe('negative batch fill amount', async () => {
+                beforeEach(async () => {
+                    const negativeFillTakerAmount = new BigNumber(-100);
+                    orderFillBatch = [
+                        {
+                            signedOrder,
+                            takerTokenFillAmount,
+                        },
+                        {
+                            signedOrder: anotherSignedOrder,
+                            takerTokenFillAmount: negativeFillTakerAmount,
+                        },
+                    ];
+                });
+                it('should not allow the exchange wrapper to batch fill if any amount is negative', async () => {
+                    return expect(
+                        zeroEx.exchange.batchFillOrdersAsync(
+                            orderFillBatch,
+                            shouldThrowOnInsufficientBalanceOrAllowance,
+                            takerAddress,
+                        ),
+                    ).to.be.rejected();
+                });
+            });
         });
         describe('#fillOrdersUpTo', () => {
             let signedOrder: SignedOrder;
diff --git a/packages/assert/src/index.ts b/packages/assert/src/index.ts
index 56f663780..7ad574ec7 100644
--- a/packages/assert/src/index.ts
+++ b/packages/assert/src/index.ts
@@ -12,6 +12,8 @@ export const assert = {
     },
     isValidBaseUnitAmount(variableName: string, value: BigNumber) {
         assert.isBigNumber(variableName, value);
+        const isNegative = value.lessThan(0);
+        this.assert(!isNegative, `${variableName} cannot be a negative number, found value: ${value.toNumber()}`);
         const hasDecimals = value.decimalPlaces() !== 0;
         this.assert(
             !hasDecimals,
diff --git a/packages/assert/test/assert_test.ts b/packages/assert/test/assert_test.ts
index ff337196d..b0fa398d6 100644
--- a/packages/assert/test/assert_test.ts
+++ b/packages/assert/test/assert_test.ts
@@ -22,6 +22,20 @@ describe('Assertions', () => {
             invalidInputs.forEach(input => expect(assert.isBigNumber.bind(assert, variableName, input)).to.throw());
         });
     });
+    describe('#isValidBaseUnitAmount', () => {
+        it('should not throw for valid input', () => {
+            const validInputs = [new BigNumber(23), new BigNumber('45000000')];
+            validInputs.forEach(input =>
+                expect(assert.isValidBaseUnitAmount.bind(assert, variableName, input)).to.not.throw(),
+            );
+        });
+        it('should throw for invalid input', () => {
+            const invalidInputs = [0, undefined, new BigNumber(3.145), 3.145, new BigNumber(-400)];
+            invalidInputs.forEach(input =>
+                expect(assert.isValidBaseUnitAmount.bind(assert, variableName, input)).to.throw(),
+            );
+        });
+    });
     describe('#isString', () => {
         it('should not throw for valid input', () => {
             const validInputs = ['hello', 'goodbye'];