From 751f9b9240fef07de5a604fa2345453a7656dfb2 Mon Sep 17 00:00:00 2001 From: Greg Hysen Date: Tue, 20 Mar 2018 17:34:58 -0700 Subject: Change from cancelOrdersBefore to cancelOrdersUpTo. The effect is that orders with salt <= to maker epoch will be cancelled (previously, it was salt < maker epoch) --- .../current/protocol/Exchange/IExchange.sol | 4 +- .../protocol/Exchange/MixinExchangeCore.sol | 15 ++++--- .../protocol/Exchange/mixins/MExchangeCore.sol | 2 +- packages/contracts/src/utils/exchange_wrapper.ts | 4 +- packages/contracts/test/exchange/core.ts | 50 ++++++++++++---------- 5 files changed, 40 insertions(+), 35 deletions(-) diff --git a/packages/contracts/src/contracts/current/protocol/Exchange/IExchange.sol b/packages/contracts/src/contracts/current/protocol/Exchange/IExchange.sol index 9e285e8bb..b7164a4e9 100644 --- a/packages/contracts/src/contracts/current/protocol/Exchange/IExchange.sol +++ b/packages/contracts/src/contracts/current/protocol/Exchange/IExchange.sol @@ -163,8 +163,8 @@ contract IExchange { returns (uint256 takerTokenCancelledAmount); /// @dev Cancels all orders for a specified maker up to a certain time. - /// @param salt Orders created with a lower salt value will be cancelled - function cancelOrdersBefore(uint256 salt) + /// @param salt Orders created with a salt less or equal to this value will be cancelled. + function cancelOrdersUpTo(uint256 salt) external; /// @dev Fills an order with specified parameters and ECDSA signature. Throws if specified amount not filled entirely. diff --git a/packages/contracts/src/contracts/current/protocol/Exchange/MixinExchangeCore.sol b/packages/contracts/src/contracts/current/protocol/Exchange/MixinExchangeCore.sol index 6d69b1787..acbed1ad7 100644 --- a/packages/contracts/src/contracts/current/protocol/Exchange/MixinExchangeCore.sol +++ b/packages/contracts/src/contracts/current/protocol/Exchange/MixinExchangeCore.sol @@ -70,9 +70,9 @@ contract MixinExchangeCore is bytes32 indexed orderHash ); - event LogCancelBefore( + event LogCancelUpTo( address indexed maker, - uint256 salt + uint256 makerEpoch ); /* @@ -202,13 +202,14 @@ contract MixinExchangeCore is return takerTokenCancelledAmount; } - /// @param salt Orders created with a salt less than this value will be cancelled. - function cancelOrdersBefore(uint256 salt) + /// @param salt Orders created with a salt less or equal to this value will be cancelled. + function cancelOrdersUpTo(uint256 salt) external { - require(salt > makerEpoch[msg.sender]); // epoch must be monotonically increasing - makerEpoch[msg.sender] = salt; - LogCancelBefore(msg.sender, salt); + uint256 newMakerEpoch = salt + 1; // makerEpoch is initialized to 0, so to cancelUpTo we need salt+1 + require(newMakerEpoch > makerEpoch[msg.sender]); // epoch must be monotonically increasing + makerEpoch[msg.sender] = newMakerEpoch; + LogCancelUpTo(msg.sender, newMakerEpoch); } /// @dev Checks if rounding error > 0.1%. 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 77300c0f8..6768e2376 100644 --- a/packages/contracts/src/contracts/current/protocol/Exchange/mixins/MExchangeCore.sol +++ b/packages/contracts/src/contracts/current/protocol/Exchange/mixins/MExchangeCore.sol @@ -36,6 +36,6 @@ contract MExchangeCore is LibOrder { public returns (uint256 takerTokenCancelledAmount); - function cancelOrdersBefore(uint256 salt) + function cancelOrdersUpTo(uint256 salt) external; } diff --git a/packages/contracts/src/utils/exchange_wrapper.ts b/packages/contracts/src/utils/exchange_wrapper.ts index 808ff0fd4..3f56fd52f 100644 --- a/packages/contracts/src/utils/exchange_wrapper.ts +++ b/packages/contracts/src/utils/exchange_wrapper.ts @@ -167,11 +167,11 @@ export class ExchangeWrapper { const tx = await this._getTxWithDecodedExchangeLogsAsync(txHash); return tx; } - public async cancelOrdersBeforeAsync( + public async cancelOrdersUpToAsync( salt: BigNumber, from: string, ): Promise { - const txHash = await this._exchange.cancelOrdersBefore.sendTransactionAsync( + const txHash = await this._exchange.cancelOrdersUpTo.sendTransactionAsync( salt, { from }, ); diff --git a/packages/contracts/test/exchange/core.ts b/packages/contracts/test/exchange/core.ts index 53863c5d8..f28bb1215 100644 --- a/packages/contracts/test/exchange/core.ts +++ b/packages/contracts/test/exchange/core.ts @@ -745,62 +745,66 @@ describe('Exchange', () => { describe('cancelOrdersBefore', () => { it('should fail to set makerEpoch less than current makerEpoch', async () => { const makerEpoch = new BigNumber(1); - await exWrapper.cancelOrdersBeforeAsync(makerEpoch, makerAddress); + await exWrapper.cancelOrdersUpToAsync(makerEpoch, makerAddress); const lesserMakerEpoch = new BigNumber(0); return expect( - exWrapper.cancelOrdersBeforeAsync(lesserMakerEpoch, makerAddress), + exWrapper.cancelOrdersUpToAsync(lesserMakerEpoch, makerAddress), ).to.be.rejectedWith(constants.REVERT); }); it('should fail to set makerEpoch equal to existing makerEpoch', async () => { const makerEpoch = new BigNumber(1); - await exWrapper.cancelOrdersBeforeAsync(makerEpoch, makerAddress); + await exWrapper.cancelOrdersUpToAsync(makerEpoch, makerAddress); return expect( - exWrapper.cancelOrdersBeforeAsync(makerEpoch, makerAddress), + exWrapper.cancelOrdersUpToAsync(makerEpoch, makerAddress), ).to.be.rejectedWith(constants.REVERT); }); it('should cancel only orders with a makerEpoch less than existing makerEpoch', async () => { // Cancel all transactions with a makerEpoch less than 1 const makerEpoch = new BigNumber(1); - await exWrapper.cancelOrdersBeforeAsync(makerEpoch, makerAddress); + await exWrapper.cancelOrdersUpToAsync(makerEpoch, makerAddress); - // Create 3 orders with makerEpoch values: 0,1,2 - // Since we cancelled with makerEpoch=1, orders with makerEpoch<1 will not be processed + // Create 3 orders with makerEpoch values: 0,1,2,3 + // Since we cancelled with makerEpoch=1, orders with makerEpoch<=1 will not be processed balances = await dmyBalances.getAsync(); const signedOrders = await Promise.all([ orderFactory.newSignedOrder({ - makerTokenAmount: ZeroEx.toBaseUnitAmount(new BigNumber(17), 18), - takerTokenAmount: ZeroEx.toBaseUnitAmount(new BigNumber(17), 18), + makerTokenAmount: ZeroEx.toBaseUnitAmount(new BigNumber(9), 18), + takerTokenAmount: ZeroEx.toBaseUnitAmount(new BigNumber(9), 18), salt: new BigNumber(0)}), orderFactory.newSignedOrder({ - makerTokenAmount: ZeroEx.toBaseUnitAmount(new BigNumber(97), 18), - takerTokenAmount: ZeroEx.toBaseUnitAmount(new BigNumber(97), 18), + makerTokenAmount: ZeroEx.toBaseUnitAmount(new BigNumber(79), 18), + takerTokenAmount: ZeroEx.toBaseUnitAmount(new BigNumber(79), 18), salt: new BigNumber(1)}), orderFactory.newSignedOrder({ makerTokenAmount: ZeroEx.toBaseUnitAmount(new BigNumber(979), 18), takerTokenAmount: ZeroEx.toBaseUnitAmount(new BigNumber(979), 18), salt: new BigNumber(2)}), + orderFactory.newSignedOrder({ + makerTokenAmount: ZeroEx.toBaseUnitAmount(new BigNumber(7979), 18), + takerTokenAmount: ZeroEx.toBaseUnitAmount(new BigNumber(7979), 18), + salt: new BigNumber(3)}), ]); await exWrapper.batchFillOrdersNoThrowAsync(signedOrders, takerAddress); const newBalances = await dmyBalances.getAsync(); - const fillMakerTokenAmount = signedOrders[1].makerTokenAmount.add(signedOrders[2].makerTokenAmount); - const fillTakerTokenAmount = signedOrders[1].takerTokenAmount.add(signedOrders[2].takerTokenAmount); - const makerFeeAmount = signedOrders[1].makerFeeAmount.add(signedOrders[2].makerFeeAmount); - const takerFeeAmount = signedOrders[1].takerFeeAmount.add(signedOrders[2].takerFeeAmount); - expect(newBalances[makerAddress][signedOrders[2].makerTokenAddress]).to.be.bignumber.equal( - balances[makerAddress][signedOrders[2].makerTokenAddress].minus(fillMakerTokenAmount), + const fillMakerTokenAmount = signedOrders[2].makerTokenAmount.add(signedOrders[3].makerTokenAmount); + const fillTakerTokenAmount = signedOrders[2].takerTokenAmount.add(signedOrders[3].takerTokenAmount); + const makerFeeAmount = signedOrders[2].makerFeeAmount.add(signedOrders[3].makerFeeAmount); + const takerFeeAmount = signedOrders[2].takerFeeAmount.add(signedOrders[3].takerFeeAmount); + expect(newBalances[makerAddress][signedOrders[3].makerTokenAddress]).to.be.bignumber.equal( + balances[makerAddress][signedOrders[3].makerTokenAddress].minus(fillMakerTokenAmount), ); - expect(newBalances[makerAddress][signedOrders[2].takerTokenAddress]).to.be.bignumber.equal( - balances[makerAddress][signedOrders[2].takerTokenAddress].add(fillTakerTokenAmount), + expect(newBalances[makerAddress][signedOrders[3].takerTokenAddress]).to.be.bignumber.equal( + balances[makerAddress][signedOrders[3].takerTokenAddress].add(fillTakerTokenAmount), ); expect(newBalances[makerAddress][zrx.address]).to.be.bignumber.equal(balances[makerAddress][zrx.address].minus(makerFeeAmount)); - expect(newBalances[takerAddress][signedOrders[2].takerTokenAddress]).to.be.bignumber.equal( - balances[takerAddress][signedOrders[2].takerTokenAddress].minus(fillTakerTokenAmount), + expect(newBalances[takerAddress][signedOrders[3].takerTokenAddress]).to.be.bignumber.equal( + balances[takerAddress][signedOrders[3].takerTokenAddress].minus(fillTakerTokenAmount), ); - expect(newBalances[takerAddress][signedOrders[2].makerTokenAddress]).to.be.bignumber.equal( - balances[takerAddress][signedOrders[2].makerTokenAddress].add(fillMakerTokenAmount), + expect(newBalances[takerAddress][signedOrders[3].makerTokenAddress]).to.be.bignumber.equal( + balances[takerAddress][signedOrders[3].makerTokenAddress].add(fillMakerTokenAmount), ); expect(newBalances[takerAddress][zrx.address]).to.be.bignumber.equal(balances[takerAddress][zrx.address].minus(takerFeeAmount)); expect(newBalances[feeRecipientAddress][zrx.address]).to.be.bignumber.equal( -- cgit v1.2.3