From 8f809e3a292c55e0f6440f9cf7c8746ede7ad583 Mon Sep 17 00:00:00 2001 From: Amir Bandeali Date: Fri, 23 Mar 2018 12:17:54 -0700 Subject: Remove partial cancels --- .../protocol/Exchange/MixinExchangeCore.sol | 57 +++++++----------- .../protocol/Exchange/MixinWrapperFunctions.sol | 10 +--- .../protocol/Exchange/mixins/MExchangeCore.sol | 6 +- packages/contracts/src/utils/exchange_wrapper.ts | 23 ++------ packages/contracts/src/utils/formatters.ts | 6 +- packages/contracts/src/utils/types.ts | 1 - packages/contracts/test/exchange/core.ts | 69 +--------------------- packages/contracts/test/exchange/wrapper.ts | 4 +- 8 files changed, 35 insertions(+), 141 deletions(-) (limited to 'packages/contracts') 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 { - 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 { + 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 { - 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; - 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; 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, -- cgit v1.2.3