Remove partial cancels

8 files changed, 35 insertions, 141 deletions

diff --git a/packages/contracts/src/contracts/current/protocol/Exchange/MixinExchangeCore.sol b/packages/contracts/src/contracts/current/protocol/Exchange/MixinExchangeCore.sol
index acbed1ad7..a0603ec78 100644
--- a/packages/contracts/src/contracts/current/protocol/Exchange/MixinExchangeCore.sol
+++ b/packages/contracts/src/contracts/current/protocol/Exchange/MixinExchangeCore.sol
@@ -41,7 +41,7 @@ contract MixinExchangeCore is
 {
     // Mappings of orderHash => amounts of takerTokenAmount filled or cancelled.
     mapping (bytes32 => uint256) public filled;
-    mapping (bytes32 => uint256) public cancelled;
+    mapping (bytes32 => bool) public cancelled;
 
     // Mapping of makerAddress => lowest salt an order can have in order to be fillable
     // Orders with a salt less than their maker's epoch are considered cancelled
@@ -65,8 +65,6 @@ contract MixinExchangeCore is
         address indexed feeRecipientAddress,
         address makerTokenAddress,
         address takerTokenAddress,
-        uint256 makerTokenCancelledAmount,
-        uint256 takerTokenCancelledAmount,
         bytes32 indexed orderHash
     );
 
@@ -94,9 +92,15 @@ contract MixinExchangeCore is
         // Compute the order hash
         bytes32 orderHash = getOrderHash(order);
 
+        // Check if order has been cancelled
+        if (cancelled[orderHash]) {
+            LogError(uint8(Errors.ORDER_FULLY_FILLED_OR_CANCELLED), orderHash);
+            return 0;
+        }
+
         // Validate order and maker only if first time seen
         // TODO: Read filled and cancelled only once
-        if (filled[orderHash] == 0 && cancelled[orderHash] == 0) {
+        if (filled[orderHash] == 0) {
             require(order.makerTokenAmount > 0);
             require(order.takerTokenAmount > 0);
             require(isValidSignature(orderHash, order.makerAddress, signature));
@@ -115,14 +119,14 @@ contract MixinExchangeCore is
         }
 
         // Validate order availability
-        uint256 remainingTakerTokenAmount = safeSub(order.takerTokenAmount, getUnavailableTakerTokenAmount(orderHash));
-        takerTokenFilledAmount = min256(takerTokenFillAmount, remainingTakerTokenAmount);
-        if (takerTokenFilledAmount == 0) {
-            LogError(uint8(Errors.ORDER_FULLY_FILLED_OR_CANCELLED), orderHash);
+        uint256 remainingTakerTokenAmount = safeSub(order.takerTokenAmount, filled[orderHash]);
+        if (remainingTakerTokenAmount == 0) {
+            LogError(uint8(Errors.ORDER_FULLY_FILLED), orderHash);
             return 0;
         }
 
         // Validate fill order rounding
+        takerTokenFilledAmount = min256(takerTokenFillAmount, remainingTakerTokenAmount);
         if (isRoundingError(takerTokenFilledAmount, order.takerTokenAmount, order.makerTokenAmount)) {
             LogError(uint8(Errors.ROUNDING_ERROR_TOO_LARGE), orderHash);
             return 0;
@@ -157,15 +161,12 @@ contract MixinExchangeCore is
         return takerTokenFilledAmount;
     }
 
-    /// @dev Cancels the input order.
+    /// @dev After calling, the order can not be filled anymore.
     /// @param order Order struct containing order specifications.
-    /// @param takerTokenCancelAmount Desired amount of takerToken to cancel in order.
-    /// @return Amount of takerToken cancelled.
-    function cancelOrder(
-        Order order,
-        uint256 takerTokenCancelAmount)
+    /// @return True if the order state changed to cancelled. False if the transaction was already cancelled or expired.
+    function cancelOrder(Order order)
         public
-        returns (uint256 takerTokenCancelledAmount)
+        returns (bool)
     {
         // Compute the order hash
         bytes32 orderHash = getOrderHash(order);
@@ -173,33 +174,28 @@ contract MixinExchangeCore is
         // Validate the order
         require(order.makerTokenAmount > 0);
         require(order.takerTokenAmount > 0);
-        require(takerTokenCancelAmount > 0);
         require(order.makerAddress == msg.sender);
 
         if (block.timestamp >= order.expirationTimeSeconds) {
             LogError(uint8(Errors.ORDER_EXPIRED), orderHash);
-            return 0;
+            return false;
         }
 
-        uint256 remainingTakerTokenAmount = safeSub(order.takerTokenAmount, getUnavailableTakerTokenAmount(orderHash));
-        takerTokenCancelledAmount = min256(takerTokenCancelAmount, remainingTakerTokenAmount);
-        if (takerTokenCancelledAmount == 0) {
+        if (cancelled[orderHash]) {
             LogError(uint8(Errors.ORDER_FULLY_FILLED_OR_CANCELLED), orderHash);
-            return 0;
+            return false;
         }
 
-        cancelled[orderHash] = safeAdd(cancelled[orderHash], takerTokenCancelledAmount);
+        cancelled[orderHash] = true;
 
         LogCancel(
             order.makerAddress,
             order.feeRecipientAddress,
             order.makerTokenAddress,
             order.takerTokenAddress,
-            getPartialAmount(takerTokenCancelledAmount, order.takerTokenAmount, order.makerTokenAmount),
-            takerTokenCancelledAmount,
             orderHash
         );
-        return takerTokenCancelledAmount;
+        return true;
     }
 
     /// @param salt Orders created with a salt less or equal to this value will be cancelled.
@@ -233,15 +229,4 @@ contract MixinExchangeCore is
         isError = errPercentageTimes1000000 > 1000;
         return isError;
     }
-
-    /// @dev Calculates the sum of values already filled and cancelled for a given order.
-    /// @param orderHash The Keccak-256 hash of the given order.
-    /// @return Sum of values already filled and cancelled.
-    function getUnavailableTakerTokenAmount(bytes32 orderHash)
-        public view
-        returns (uint256 unavailableTakerTokenAmount)
-    {
-        unavailableTakerTokenAmount = safeAdd(filled[orderHash], cancelled[orderHash]);
-        return unavailableTakerTokenAmount;
-    }
 }
diff --git a/packages/contracts/src/contracts/current/protocol/Exchange/MixinWrapperFunctions.sol b/packages/contracts/src/contracts/current/protocol/Exchange/MixinWrapperFunctions.sol
index 358649aa1..39f4784cb 100644
--- a/packages/contracts/src/contracts/current/protocol/Exchange/MixinWrapperFunctions.sol
+++ b/packages/contracts/src/contracts/current/protocol/Exchange/MixinWrapperFunctions.sol
@@ -262,17 +262,11 @@ contract MixinWrapperFunctions is
 
     /// @dev Synchronously cancels multiple orders in a single transaction.
     /// @param orders Array of orders.
-    /// @param takerTokenCancelAmounts Array of desired amounts of takerToken to cancel in orders.
-    function batchCancelOrders(
-        Order[] orders,
-        uint256[] takerTokenCancelAmounts)
+    function batchCancelOrders(Order[] orders)
         public
     {
         for (uint256 i = 0; i < orders.length; i++) {
-            cancelOrder(
-                orders[i],
-                takerTokenCancelAmounts[i]
-            );
+            cancelOrder(orders[i]);
         }
     }
     
diff --git a/packages/contracts/src/contracts/current/protocol/Exchange/mixins/MExchangeCore.sol b/packages/contracts/src/contracts/current/protocol/Exchange/mixins/MExchangeCore.sol
index 6768e2376..a495434e6 100644
--- a/packages/contracts/src/contracts/current/protocol/Exchange/mixins/MExchangeCore.sol
+++ b/packages/contracts/src/contracts/current/protocol/Exchange/mixins/MExchangeCore.sol
@@ -30,11 +30,9 @@ contract MExchangeCore is LibOrder {
         public
         returns (uint256 takerTokenFilledAmount);
 
-    function cancelOrder(
-        Order order,
-        uint256 takerTokenCancelAmount)
+    function cancelOrder(Order order)
         public
-        returns (uint256 takerTokenCancelledAmount);
+        returns (bool);
 
  function cancelOrdersUpTo(uint256 salt)
         external;
diff --git a/packages/contracts/src/utils/exchange_wrapper.ts b/packages/contracts/src/utils/exchange_wrapper.ts
index 3f56fd52f..bbe9a1baf 100644
--- a/packages/contracts/src/utils/exchange_wrapper.ts
+++ b/packages/contracts/src/utils/exchange_wrapper.ts
@@ -34,17 +34,9 @@ export class ExchangeWrapper {
         const tx = await this._getTxWithDecodedExchangeLogsAsync(txHash);
         return tx;
     }
-    public async cancelOrderAsync(
-        signedOrder: SignedOrder,
-        from: string,
-        opts: { takerTokenCancelAmount?: BigNumber } = {},
-    ): Promise<TransactionReceiptWithDecodedLogs> {
-        const params = orderUtils.createCancel(signedOrder, opts.takerTokenCancelAmount);
-        const txHash = await this._exchange.cancelOrder.sendTransactionAsync(
-            params.order,
-            params.takerTokenCancelAmount,
-            { from },
-        );
+    public async cancelOrderAsync(signedOrder: SignedOrder, from: string): Promise<TransactionReceiptWithDecodedLogs> {
+        const params = orderUtils.createCancel(signedOrder);
+        const txHash = await this._exchange.cancelOrder.sendTransactionAsync(params.order, { from });
         const tx = await this._getTxWithDecodedExchangeLogsAsync(txHash);
         return tx;
     }
@@ -156,14 +148,9 @@ export class ExchangeWrapper {
     public async batchCancelOrdersAsync(
         orders: SignedOrder[],
         from: string,
-        opts: { takerTokenCancelAmounts?: BigNumber[] } = {},
     ): Promise<TransactionReceiptWithDecodedLogs> {
-        const params = formatters.createBatchCancel(orders, opts.takerTokenCancelAmounts);
-        const txHash = await this._exchange.batchCancelOrders.sendTransactionAsync(
-            params.orders,
-            params.takerTokenCancelAmounts,
-            { from },
-        );
+        const params = formatters.createBatchCancel(orders);
+        const txHash = await this._exchange.batchCancelOrders.sendTransactionAsync(params.orders, { from });
         const tx = await this._getTxWithDecodedExchangeLogsAsync(txHash);
         return tx;
     }
diff --git a/packages/contracts/src/utils/formatters.ts b/packages/contracts/src/utils/formatters.ts
index 4f6116f06..2b261f967 100644
--- a/packages/contracts/src/utils/formatters.ts
+++ b/packages/contracts/src/utils/formatters.ts
@@ -55,10 +55,9 @@ export const formatters = {
         });
         return marketFillOrders;
     },
-    createBatchCancel(signedOrders: SignedOrder[], takerTokenCancelAmounts: BigNumber[] = []) {
+    createBatchCancel(signedOrders: SignedOrder[]) {
         const batchCancel: BatchCancelOrders = {
             orders: [],
-            takerTokenCancelAmounts,
         };
         _.forEach(signedOrders, signedOrder => {
             batchCancel.orders.push({
@@ -74,9 +73,6 @@ export const formatters = {
                 expirationTimeSeconds: signedOrder.expirationTimeSeconds,
                 salt: signedOrder.salt,
             });
-            if (takerTokenCancelAmounts.length < signedOrders.length) {
-                batchCancel.takerTokenCancelAmounts.push(signedOrder.takerTokenAmount);
-            }
         });
         return batchCancel;
     },
diff --git a/packages/contracts/src/utils/types.ts b/packages/contracts/src/utils/types.ts
index 06fe23824..460d0a58a 100644
--- a/packages/contracts/src/utils/types.ts
+++ b/packages/contracts/src/utils/types.ts
@@ -25,7 +25,6 @@ export interface MarketFillOrders {
 
 export interface BatchCancelOrders {
     orders: OrderStruct[];
-    takerTokenCancelAmounts: BigNumber[];
 }
 
 export interface CancelOrdersBefore {
diff --git a/packages/contracts/test/exchange/core.ts b/packages/contracts/test/exchange/core.ts
index 69d059256..3919d43d8 100644
--- a/packages/contracts/test/exchange/core.ts
+++ b/packages/contracts/test/exchange/core.ts
@@ -631,16 +631,6 @@ describe('Exchange', () => {
             return expect(exWrapper.cancelOrderAsync(signedOrder, makerAddress)).to.be.rejectedWith(constants.REVERT);
         });
 
-        it('should throw if takerTokenCancelAmount is 0', async () => {
-            signedOrder = orderFactory.newSignedOrder();
-
-            return expect(
-                exWrapper.cancelOrderAsync(signedOrder, makerAddress, {
-                    takerTokenCancelAmount: new BigNumber(0),
-                }),
-            ).to.be.rejectedWith(constants.REVERT);
-        });
-
         it('should be able to cancel a full order', async () => {
             await exWrapper.cancelOrderAsync(signedOrder, makerAddress);
             await exWrapper.fillOrderAsync(signedOrder, takerAddress, {
@@ -651,75 +641,22 @@ describe('Exchange', () => {
             expect(newBalances).to.be.deep.equal(balances);
         });
 
-        it('should be able to cancel part of an order', async () => {
-            const takerTokenCancelAmount = signedOrder.takerTokenAmount.div(2);
-            await exWrapper.cancelOrderAsync(signedOrder, makerAddress, {
-                takerTokenCancelAmount,
-            });
-
-            const res = await exWrapper.fillOrderAsync(signedOrder, takerAddress, {
-                takerTokenFillAmount: signedOrder.takerTokenAmount,
-            });
-            const log = logDecoder.decodeLogOrThrow(res.logs[0]) as LogWithDecodedArgs<LogFillContractEventArgs>;
-            expect(log.args.takerTokenFilledAmount).to.be.bignumber.equal(
-                signedOrder.takerTokenAmount.minus(takerTokenCancelAmount),
-            );
-
-            const newBalances = await dmyBalances.getAsync();
-            const cancelMakerTokenAmount = takerTokenCancelAmount
-                .times(signedOrder.makerTokenAmount)
-                .dividedToIntegerBy(signedOrder.takerTokenAmount);
-            const makerFeePaid = signedOrder.makerFeeAmount
-                .times(cancelMakerTokenAmount)
-                .dividedToIntegerBy(signedOrder.makerTokenAmount);
-            const takerFeePaid = signedOrder.takerFeeAmount
-                .times(cancelMakerTokenAmount)
-                .dividedToIntegerBy(signedOrder.makerTokenAmount);
-            expect(newBalances[makerAddress][signedOrder.makerTokenAddress]).to.be.bignumber.equal(
-                balances[makerAddress][signedOrder.makerTokenAddress].minus(cancelMakerTokenAmount),
-            );
-            expect(newBalances[makerAddress][signedOrder.takerTokenAddress]).to.be.bignumber.equal(
-                balances[makerAddress][signedOrder.takerTokenAddress].add(takerTokenCancelAmount),
-            );
-            expect(newBalances[makerAddress][zrx.address]).to.be.bignumber.equal(
-                balances[makerAddress][zrx.address].minus(makerFeePaid),
-            );
-            expect(newBalances[takerAddress][signedOrder.takerTokenAddress]).to.be.bignumber.equal(
-                balances[takerAddress][signedOrder.takerTokenAddress].minus(takerTokenCancelAmount),
-            );
-            expect(newBalances[takerAddress][signedOrder.makerTokenAddress]).to.be.bignumber.equal(
-                balances[takerAddress][signedOrder.makerTokenAddress].add(cancelMakerTokenAmount),
-            );
-            expect(newBalances[takerAddress][zrx.address]).to.be.bignumber.equal(
-                balances[takerAddress][zrx.address].minus(takerFeePaid),
-            );
-            expect(newBalances[feeRecipientAddress][zrx.address]).to.be.bignumber.equal(
-                balances[feeRecipientAddress][zrx.address].add(makerFeePaid.add(takerFeePaid)),
-            );
-        });
-
         it('should log 1 event with correct arguments', async () => {
             const divisor = 2;
-            const res = await exWrapper.cancelOrderAsync(signedOrder, makerAddress, {
-                takerTokenCancelAmount: signedOrder.takerTokenAmount.div(divisor),
-            });
+            const res = await exWrapper.cancelOrderAsync(signedOrder, makerAddress);
             expect(res.logs).to.have.length(1);
 
             const log = logDecoder.decodeLogOrThrow(res.logs[0]) as LogWithDecodedArgs<LogCancelContractEventArgs>;
             const logArgs = log.args;
-            const expectedCancelledMakerTokenAmount = signedOrder.makerTokenAmount.div(divisor);
-            const expectedCancelledTakerTokenAmount = signedOrder.takerTokenAmount.div(divisor);
 
             expect(signedOrder.makerAddress).to.be.equal(logArgs.makerAddress);
             expect(signedOrder.feeRecipientAddress).to.be.equal(logArgs.feeRecipientAddress);
             expect(signedOrder.makerTokenAddress).to.be.equal(logArgs.makerTokenAddress);
             expect(signedOrder.takerTokenAddress).to.be.equal(logArgs.takerTokenAddress);
-            expect(expectedCancelledMakerTokenAmount).to.be.bignumber.equal(logArgs.makerTokenCancelledAmount);
-            expect(expectedCancelledTakerTokenAmount).to.be.bignumber.equal(logArgs.takerTokenCancelledAmount);
             expect(orderUtils.getOrderHashHex(signedOrder)).to.be.equal(logArgs.orderHash);
         });
 
-        it('should not log events if no value is cancelled', async () => {
+        it('should log an error if already cancelled', async () => {
             await exWrapper.cancelOrderAsync(signedOrder, makerAddress);
 
             const res = await exWrapper.cancelOrderAsync(signedOrder, makerAddress);
@@ -729,7 +666,7 @@ describe('Exchange', () => {
             expect(errCode).to.be.equal(ExchangeContractErrs.ERROR_ORDER_FULLY_FILLED_OR_CANCELLED);
         });
 
-        it('should not log events if order is expired', async () => {
+        it('should log error if order is expired', async () => {
             signedOrder = orderFactory.newSignedOrder({
                 expirationTimeSeconds: new BigNumber(Math.floor((Date.now() - 10000) / 1000)),
             });
diff --git a/packages/contracts/test/exchange/wrapper.ts b/packages/contracts/test/exchange/wrapper.ts
index 02e57b922..b87f45387 100644
--- a/packages/contracts/test/exchange/wrapper.ts
+++ b/packages/contracts/test/exchange/wrapper.ts
@@ -724,9 +724,7 @@ describe('Exchange', () => {
         describe('batchCancelOrders', () => {
             it('should be able to cancel multiple signedOrders', async () => {
                 const takerTokenCancelAmounts = _.map(signedOrders, signedOrder => signedOrder.takerTokenAmount);
-                await exWrapper.batchCancelOrdersAsync(signedOrders, makerAddress, {
-                    takerTokenCancelAmounts,
-                });
+                await exWrapper.batchCancelOrdersAsync(signedOrders, makerAddress);
 
                 await exWrapper.batchFillOrdersAsync(signedOrders, takerAddress, {
                     takerTokenFillAmounts: takerTokenCancelAmounts,