From 342432dc76d1bfc2531fb2edef77afc523eacefe Mon Sep 17 00:00:00 2001 From: Amir Bandeali Date: Tue, 29 May 2018 22:09:57 -0700 Subject: Update Exchange statuses, revert instead of emmitting event on fill/cancel failures, and remove redundant logic in matchOrders --- packages/contracts/test/exchange/core.ts | 89 ++++++++++++------------ packages/contracts/test/exchange/match_orders.ts | 43 ++++++------ packages/contracts/test/exchange/transactions.ts | 6 +- packages/contracts/test/exchange/wrapper.ts | 2 +- 4 files changed, 69 insertions(+), 71 deletions(-) (limited to 'packages/contracts/test/exchange') diff --git a/packages/contracts/test/exchange/core.ts b/packages/contracts/test/exchange/core.ts index 8320e97d6..16492b8ac 100644 --- a/packages/contracts/test/exchange/core.ts +++ b/packages/contracts/test/exchange/core.ts @@ -15,7 +15,6 @@ import { ERC721ProxyContract } from '../../src/contract_wrappers/generated/e_r_c import { CancelContractEventArgs, ExchangeContract, - ExchangeStatusContractEventArgs, FillContractEventArgs, } from '../../src/contract_wrappers/generated/exchange'; import { artifacts } from '../../src/utils/artifacts'; @@ -25,6 +24,7 @@ import { ERC20Wrapper } from '../../src/utils/erc20_wrapper'; import { ERC721Wrapper } from '../../src/utils/erc721_wrapper'; import { ExchangeWrapper } from '../../src/utils/exchange_wrapper'; import { OrderFactory } from '../../src/utils/order_factory'; +import { orderUtils } from '../../src/utils/order_utils'; import { ContractName, ERC20BalancesByOwner, ExchangeStatus } from '../../src/utils/types'; import { provider, txDefaults, web3Wrapper } from '../../src/utils/web3_wrapper'; @@ -132,6 +132,7 @@ describe('Exchange core', () => { signedOrder = orderFactory.newSignedOrder(); }); +<<<<<<< HEAD it('should create an unfillable order', async () => { signedOrder = orderFactory.newSignedOrder({ makerAssetAmount: new BigNumber(1001), @@ -164,6 +165,8 @@ describe('Exchange core', () => { expect(takerAssetFilledAmountAfter2).to.be.bignumber.equal(takerAssetFilledAmountAfter1); }); +======= +>>>>>>> Update Exchange statuses, revert instead of emmitting event on fill/cancel failures, and remove redundant logic in matchOrders it('should transfer the correct amounts when makerAssetAmount === takerAssetAmount', async () => { signedOrder = orderFactory.newSignedOrder({ makerAssetAmount: Web3Wrapper.toBaseUnitAmount(new BigNumber(100), 18), @@ -550,37 +553,21 @@ describe('Exchange core', () => { ); }); - it('should not change erc20Balances if an order is expired', async () => { + it('should throw if an order is expired', async () => { signedOrder = orderFactory.newSignedOrder({ expirationTimeSeconds: new BigNumber(Math.floor((Date.now() - 10000) / 1000)), }); - await exchangeWrapper.fillOrderAsync(signedOrder, takerAddress); - - const newBalances = await erc20Wrapper.getBalancesAsync(); - expect(newBalances).to.be.deep.equal(erc20Balances); - }); - - it('should log an error event if an order is expired', async () => { - signedOrder = orderFactory.newSignedOrder({ - expirationTimeSeconds: new BigNumber(Math.floor((Date.now() - 10000) / 1000)), - }); - - const res = await exchangeWrapper.fillOrderAsync(signedOrder, takerAddress); - expect(res.logs).to.have.length(1); - const log = res.logs[0] as LogWithDecodedArgs; - const statusCode = log.args.statusId; - expect(statusCode).to.be.equal(ExchangeStatus.ORDER_EXPIRED); + return expect(exchangeWrapper.fillOrderAsync(signedOrder, takerAddress)).to.be.rejectedWith( + constants.REVERT, + ); }); - it('should log an error event if no value is filled', async () => { - signedOrder = orderFactory.newSignedOrder({}); + it('should throw if no value is filled', async () => { + signedOrder = orderFactory.newSignedOrder(); await exchangeWrapper.fillOrderAsync(signedOrder, takerAddress); - - const res = await exchangeWrapper.fillOrderAsync(signedOrder, takerAddress); - expect(res.logs).to.have.length(1); - const log = res.logs[0] as LogWithDecodedArgs; - const statusCode = log.args.statusId; - expect(statusCode).to.be.equal(ExchangeStatus.ORDER_FULLY_FILLED); + return expect(exchangeWrapper.fillOrderAsync(signedOrder, takerAddress)).to.be.rejectedWith( + constants.REVERT, + ); }); }); @@ -618,12 +605,11 @@ describe('Exchange core', () => { it('should be able to cancel a full order', async () => { await exchangeWrapper.cancelOrderAsync(signedOrder, makerAddress); - await exchangeWrapper.fillOrderAsync(signedOrder, takerAddress, { - takerAssetFillAmount: signedOrder.takerAssetAmount.div(2), - }); - - const newBalances = await erc20Wrapper.getBalancesAsync(); - expect(newBalances).to.be.deep.equal(erc20Balances); + return expect( + exchangeWrapper.fillOrderAsync(signedOrder, takerAddress, { + takerAssetFillAmount: signedOrder.takerAssetAmount.div(2), + }), + ).to.be.rejectedWith(constants.REVERT); }); it('should log 1 event with correct arguments', async () => { @@ -641,26 +627,39 @@ describe('Exchange core', () => { expect(orderHashUtils.getOrderHashHex(signedOrder)).to.be.equal(logArgs.orderHash); }); - it('should log an error if already cancelled', async () => { + it('should throw if already cancelled', async () => { await exchangeWrapper.cancelOrderAsync(signedOrder, makerAddress); - - const res = await exchangeWrapper.cancelOrderAsync(signedOrder, makerAddress); - expect(res.logs).to.have.length(1); - const log = res.logs[0] as LogWithDecodedArgs; - const statusCode = log.args.statusId; - expect(statusCode).to.be.equal(ExchangeStatus.ORDER_CANCELLED); + return expect(exchangeWrapper.cancelOrderAsync(signedOrder, makerAddress)).to.be.rejectedWith( + constants.REVERT, + ); }); - it('should log error if order is expired', async () => { + it('should throw if order is expired', async () => { signedOrder = orderFactory.newSignedOrder({ expirationTimeSeconds: new BigNumber(Math.floor((Date.now() - 10000) / 1000)), }); + return expect(exchangeWrapper.cancelOrderAsync(signedOrder, makerAddress)).to.be.rejectedWith( + constants.REVERT, + ); + }); - const res = await exchangeWrapper.cancelOrderAsync(signedOrder, makerAddress); - expect(res.logs).to.have.length(1); - const log = res.logs[0] as LogWithDecodedArgs; - const statusCode = log.args.statusId; - expect(statusCode).to.be.equal(ExchangeStatus.ORDER_EXPIRED); + it('should throw if rounding error is greater than 0.1%', async () => { + signedOrder = orderFactory.newSignedOrder({ + makerAssetAmount: new BigNumber(1001), + takerAssetAmount: new BigNumber(3), + }); + + const fillTakerAssetAmount1 = new BigNumber(2); + await exchangeWrapper.fillOrderAsync(signedOrder, takerAddress, { + takerAssetFillAmount: fillTakerAssetAmount1, + }); + + const fillTakerAssetAmount2 = new BigNumber(1); + return expect( + exchangeWrapper.fillOrderAsync(signedOrder, takerAddress, { + takerAssetFillAmount: fillTakerAssetAmount2, + }), + ).to.be.rejectedWith(constants.REVERT); }); }); diff --git a/packages/contracts/test/exchange/match_orders.ts b/packages/contracts/test/exchange/match_orders.ts index 07295d78a..24ee794bc 100644 --- a/packages/contracts/test/exchange/match_orders.ts +++ b/packages/contracts/test/exchange/match_orders.ts @@ -15,7 +15,6 @@ import { ERC721ProxyContract } from '../../src/contract_wrappers/generated/e_r_c import { CancelContractEventArgs, ExchangeContract, - ExchangeStatusContractEventArgs, FillContractEventArgs, } from '../../src/contract_wrappers/generated/exchange'; import { artifacts } from '../../src/utils/artifacts'; @@ -30,8 +29,8 @@ import { ContractName, ERC20BalancesByOwner, ERC721TokenIdsByOwner, - ExchangeStatus, OrderInfo, + OrderStatus, } from '../../src/utils/types'; import { provider, txDefaults, web3Wrapper } from '../../src/utils/web3_wrapper'; @@ -186,10 +185,10 @@ describe('matchOrders', () => { ); // Verify left order was fully filled const leftOrderInfo: OrderInfo = await exchangeWrapper.getOrderInfoAsync(signedOrderLeft); - expect(leftOrderInfo.orderStatus as ExchangeStatus).to.be.equal(ExchangeStatus.ORDER_FULLY_FILLED); + expect(leftOrderInfo.orderStatus as OrderStatus).to.be.equal(OrderStatus.FULLY_FILLED); // Verify right order was fully filled const rightOrderInfo: OrderInfo = await exchangeWrapper.getOrderInfoAsync(signedOrderRight); - expect(rightOrderInfo.orderStatus as ExchangeStatus).to.be.equal(ExchangeStatus.ORDER_FULLY_FILLED); + expect(rightOrderInfo.orderStatus as OrderStatus).to.be.equal(OrderStatus.FULLY_FILLED); }); it('should transfer the correct amounts when orders completely fill each other and taker doesnt take a profit', async () => { @@ -227,10 +226,10 @@ describe('matchOrders', () => { ); // Verify left order was fully filled const leftOrderInfo: OrderInfo = await exchangeWrapper.getOrderInfoAsync(signedOrderLeft); - expect(leftOrderInfo.orderStatus as ExchangeStatus).to.be.equal(ExchangeStatus.ORDER_FULLY_FILLED); + expect(leftOrderInfo.orderStatus as OrderStatus).to.be.equal(OrderStatus.FULLY_FILLED); // Verify right order was fully filled const rightOrderInfo: OrderInfo = await exchangeWrapper.getOrderInfoAsync(signedOrderRight); - expect(rightOrderInfo.orderStatus as ExchangeStatus).to.be.equal(ExchangeStatus.ORDER_FULLY_FILLED); + expect(rightOrderInfo.orderStatus as OrderStatus).to.be.equal(OrderStatus.FULLY_FILLED); // Verify taker did not take a profit expect(takerInitialBalances).to.be.deep.equal( newERC20BalancesByOwner[takerAddress][defaultERC20MakerAssetAddress], @@ -265,10 +264,10 @@ describe('matchOrders', () => { ); // Verify left order was fully filled const leftOrderInfo: OrderInfo = await exchangeWrapper.getOrderInfoAsync(signedOrderLeft); - expect(leftOrderInfo.orderStatus as ExchangeStatus).to.be.equal(ExchangeStatus.ORDER_FULLY_FILLED); + expect(leftOrderInfo.orderStatus as OrderStatus).to.be.equal(OrderStatus.FULLY_FILLED); // Verify right order was partially filled const rightOrderInfo: OrderInfo = await exchangeWrapper.getOrderInfoAsync(signedOrderRight); - expect(rightOrderInfo.orderStatus as ExchangeStatus).to.be.equal(ExchangeStatus.ORDER_FILLABLE); + expect(rightOrderInfo.orderStatus as OrderStatus).to.be.equal(OrderStatus.FILLABLE); }); it('should transfer the correct amounts when right order is completely filled and left order is partially filled', async () => { @@ -299,10 +298,10 @@ describe('matchOrders', () => { ); // Verify left order was partially filled const leftOrderInfo: OrderInfo = await exchangeWrapper.getOrderInfoAsync(signedOrderLeft); - expect(leftOrderInfo.orderStatus as ExchangeStatus).to.be.equal(ExchangeStatus.ORDER_FILLABLE); + expect(leftOrderInfo.orderStatus as OrderStatus).to.be.equal(OrderStatus.FILLABLE); // Verify right order was fully filled const rightOrderInfo: OrderInfo = await exchangeWrapper.getOrderInfoAsync(signedOrderRight); - expect(rightOrderInfo.orderStatus as ExchangeStatus).to.be.equal(ExchangeStatus.ORDER_FULLY_FILLED); + expect(rightOrderInfo.orderStatus as OrderStatus).to.be.equal(OrderStatus.FULLY_FILLED); }); it('should transfer the correct amounts when consecutive calls are used to completely fill the left order', async () => { @@ -338,10 +337,10 @@ describe('matchOrders', () => { ); // Verify left order was partially filled const leftOrderInfo: OrderInfo = await exchangeWrapper.getOrderInfoAsync(signedOrderLeft); - expect(leftOrderInfo.orderStatus as ExchangeStatus).to.be.equal(ExchangeStatus.ORDER_FILLABLE); + expect(leftOrderInfo.orderStatus as OrderStatus).to.be.equal(OrderStatus.FILLABLE); // Verify right order was fully filled const rightOrderInfo: OrderInfo = await exchangeWrapper.getOrderInfoAsync(signedOrderRight); - expect(rightOrderInfo.orderStatus as ExchangeStatus).to.be.equal(ExchangeStatus.ORDER_FULLY_FILLED); + expect(rightOrderInfo.orderStatus as OrderStatus).to.be.equal(OrderStatus.FULLY_FILLED); // Construct second right order // Note: This order needs makerAssetAmount=90/takerAssetAmount=[anything <= 45] to fully fill the right order. // However, we use 100/50 to ensure a partial fill as we want to go down the "left fill" @@ -368,10 +367,10 @@ describe('matchOrders', () => { ); // Verify left order was fully filled const leftOrderInfo2: OrderInfo = await exchangeWrapper.getOrderInfoAsync(signedOrderLeft); - expect(leftOrderInfo2.orderStatus as ExchangeStatus).to.be.equal(ExchangeStatus.ORDER_FULLY_FILLED); + expect(leftOrderInfo2.orderStatus as OrderStatus).to.be.equal(OrderStatus.FULLY_FILLED); // Verify second right order was partially filled const rightOrderInfo2: OrderInfo = await exchangeWrapper.getOrderInfoAsync(signedOrderRight2); - expect(rightOrderInfo2.orderStatus as ExchangeStatus).to.be.equal(ExchangeStatus.ORDER_FILLABLE); + expect(rightOrderInfo2.orderStatus as OrderStatus).to.be.equal(OrderStatus.FILLABLE); }); it('should transfer the correct amounts when consecutive calls are used to completely fill the right order', async () => { @@ -408,10 +407,10 @@ describe('matchOrders', () => { ); // Verify left order was partially filled const leftOrderInfo: OrderInfo = await exchangeWrapper.getOrderInfoAsync(signedOrderLeft); - expect(leftOrderInfo.orderStatus as ExchangeStatus).to.be.equal(ExchangeStatus.ORDER_FULLY_FILLED); + expect(leftOrderInfo.orderStatus as OrderStatus).to.be.equal(OrderStatus.FULLY_FILLED); // Verify right order was fully filled const rightOrderInfo: OrderInfo = await exchangeWrapper.getOrderInfoAsync(signedOrderRight); - expect(rightOrderInfo.orderStatus as ExchangeStatus).to.be.equal(ExchangeStatus.ORDER_FILLABLE); + expect(rightOrderInfo.orderStatus as OrderStatus).to.be.equal(OrderStatus.FILLABLE); // Create second left order // Note: This order needs makerAssetAmount=96/takerAssetAmount=48 to fully fill the right order. // However, we use 100/50 to ensure a partial fill as we want to go down the "right fill" @@ -441,10 +440,10 @@ describe('matchOrders', () => { ); // Verify second left order was partially filled const leftOrderInfo2: OrderInfo = await exchangeWrapper.getOrderInfoAsync(signedOrderLeft2); - expect(leftOrderInfo2.orderStatus as ExchangeStatus).to.be.equal(ExchangeStatus.ORDER_FILLABLE); + expect(leftOrderInfo2.orderStatus as OrderStatus).to.be.equal(OrderStatus.FILLABLE); // Verify right order was fully filled const rightOrderInfo2: OrderInfo = await exchangeWrapper.getOrderInfoAsync(signedOrderRight); - expect(rightOrderInfo2.orderStatus as ExchangeStatus).to.be.equal(ExchangeStatus.ORDER_FULLY_FILLED); + expect(rightOrderInfo2.orderStatus as OrderStatus).to.be.equal(OrderStatus.FULLY_FILLED); }); it('should transfer the correct amounts if fee recipient is the same across both matched orders', async () => { @@ -790,10 +789,10 @@ describe('matchOrders', () => { ); // Verify left order was fully filled const leftOrderInfo: OrderInfo = await exchangeWrapper.getOrderInfoAsync(signedOrderLeft); - expect(leftOrderInfo.orderStatus as ExchangeStatus).to.be.equal(ExchangeStatus.ORDER_FULLY_FILLED); + expect(leftOrderInfo.orderStatus as OrderStatus).to.be.equal(OrderStatus.FULLY_FILLED); // Verify right order was fully filled const rightOrderInfo: OrderInfo = await exchangeWrapper.getOrderInfoAsync(signedOrderRight); - expect(rightOrderInfo.orderStatus as ExchangeStatus).to.be.equal(ExchangeStatus.ORDER_FULLY_FILLED); + expect(rightOrderInfo.orderStatus as OrderStatus).to.be.equal(OrderStatus.FULLY_FILLED); }); it('should transfer correct amounts when right order maker asset is an ERC721 token', async () => { @@ -825,10 +824,10 @@ describe('matchOrders', () => { ); // Verify left order was fully filled const leftOrderInfo: OrderInfo = await exchangeWrapper.getOrderInfoAsync(signedOrderLeft); - expect(leftOrderInfo.orderStatus as ExchangeStatus).to.be.equal(ExchangeStatus.ORDER_FULLY_FILLED); + expect(leftOrderInfo.orderStatus as OrderStatus).to.be.equal(OrderStatus.FULLY_FILLED); // Verify right order was fully filled const rightOrderInfo: OrderInfo = await exchangeWrapper.getOrderInfoAsync(signedOrderRight); - expect(rightOrderInfo.orderStatus as ExchangeStatus).to.be.equal(ExchangeStatus.ORDER_FULLY_FILLED); + expect(rightOrderInfo.orderStatus as OrderStatus).to.be.equal(OrderStatus.FULLY_FILLED); }); }); }); // tslint:disable-line:max-file-line-count diff --git a/packages/contracts/test/exchange/transactions.ts b/packages/contracts/test/exchange/transactions.ts index 9af8b522b..6480b7e0b 100644 --- a/packages/contracts/test/exchange/transactions.ts +++ b/packages/contracts/test/exchange/transactions.ts @@ -194,9 +194,9 @@ describe('Exchange transactions', () => { it('should cancel the order when signed by maker and called by sender', async () => { await exchangeWrapper.executeTransactionAsync(signedTx, senderAddress); - const res = await exchangeWrapper.fillOrderAsync(signedOrder, senderAddress); - const newBalances = await erc20Wrapper.getBalancesAsync(); - expect(newBalances).to.deep.equal(erc20Balances); + return expect(exchangeWrapper.fillOrderAsync(signedOrder, senderAddress)).to.be.rejectedWith( + constants.REVERT, + ); }); }); }); diff --git a/packages/contracts/test/exchange/wrapper.ts b/packages/contracts/test/exchange/wrapper.ts index a158ba8f3..583ec9f91 100644 --- a/packages/contracts/test/exchange/wrapper.ts +++ b/packages/contracts/test/exchange/wrapper.ts @@ -959,7 +959,7 @@ describe('Exchange wrappers', () => { const takerAssetCancelAmounts = _.map(signedOrders, signedOrder => signedOrder.takerAssetAmount); await exchangeWrapper.batchCancelOrdersAsync(signedOrders, makerAddress); - await exchangeWrapper.batchFillOrdersAsync(signedOrders, takerAddress, { + await exchangeWrapper.batchFillOrdersNoThrowAsync(signedOrders, takerAddress, { takerAssetFillAmounts: takerAssetCancelAmounts, }); const newBalances = await erc20Wrapper.getBalancesAsync(); -- cgit v1.2.3 From 351173e554b4259f6a95f066b15d97317835aa8d Mon Sep 17 00:00:00 2001 From: Jacob Evans Date: Mon, 4 Jun 2018 16:55:22 -0700 Subject: Rebase from v2-prototype --- packages/contracts/test/exchange/core.ts | 38 +----------------------- packages/contracts/test/exchange/transactions.ts | 2 +- 2 files changed, 2 insertions(+), 38 deletions(-) (limited to 'packages/contracts/test/exchange') diff --git a/packages/contracts/test/exchange/core.ts b/packages/contracts/test/exchange/core.ts index 16492b8ac..91ead93f0 100644 --- a/packages/contracts/test/exchange/core.ts +++ b/packages/contracts/test/exchange/core.ts @@ -25,7 +25,7 @@ import { ERC721Wrapper } from '../../src/utils/erc721_wrapper'; import { ExchangeWrapper } from '../../src/utils/exchange_wrapper'; import { OrderFactory } from '../../src/utils/order_factory'; import { orderUtils } from '../../src/utils/order_utils'; -import { ContractName, ERC20BalancesByOwner, ExchangeStatus } from '../../src/utils/types'; +import { ContractName, ERC20BalancesByOwner, OrderStatus } from '../../src/utils/types'; import { provider, txDefaults, web3Wrapper } from '../../src/utils/web3_wrapper'; chaiSetup.configure(); @@ -131,42 +131,6 @@ describe('Exchange core', () => { erc20Balances = await erc20Wrapper.getBalancesAsync(); signedOrder = orderFactory.newSignedOrder(); }); - -<<<<<<< HEAD - it('should create an unfillable order', async () => { - signedOrder = orderFactory.newSignedOrder({ - makerAssetAmount: new BigNumber(1001), - takerAssetAmount: new BigNumber(3), - }); - - const takerAssetFilledAmountBefore = await exchangeWrapper.getTakerAssetFilledAmountAsync( - orderHashUtils.getOrderHashHex(signedOrder), - ); - expect(takerAssetFilledAmountBefore).to.be.bignumber.equal(0); - - const fillTakerAssetAmount1 = new BigNumber(2); - await exchangeWrapper.fillOrderAsync(signedOrder, takerAddress, { - takerAssetFillAmount: fillTakerAssetAmount1, - }); - - const takerAssetFilledAmountAfter1 = await exchangeWrapper.getTakerAssetFilledAmountAsync( - orderHashUtils.getOrderHashHex(signedOrder), - ); - expect(takerAssetFilledAmountAfter1).to.be.bignumber.equal(fillTakerAssetAmount1); - - const fillTakerAssetAmount2 = new BigNumber(1); - await exchangeWrapper.fillOrderAsync(signedOrder, takerAddress, { - takerAssetFillAmount: fillTakerAssetAmount2, - }); - - const takerAssetFilledAmountAfter2 = await exchangeWrapper.getTakerAssetFilledAmountAsync( - orderHashUtils.getOrderHashHex(signedOrder), - ); - expect(takerAssetFilledAmountAfter2).to.be.bignumber.equal(takerAssetFilledAmountAfter1); - }); - -======= ->>>>>>> Update Exchange statuses, revert instead of emmitting event on fill/cancel failures, and remove redundant logic in matchOrders it('should transfer the correct amounts when makerAssetAmount === takerAssetAmount', async () => { signedOrder = orderFactory.newSignedOrder({ makerAssetAmount: Web3Wrapper.toBaseUnitAmount(new BigNumber(100), 18), diff --git a/packages/contracts/test/exchange/transactions.ts b/packages/contracts/test/exchange/transactions.ts index 6480b7e0b..f31053ad3 100644 --- a/packages/contracts/test/exchange/transactions.ts +++ b/packages/contracts/test/exchange/transactions.ts @@ -18,7 +18,7 @@ import { ExchangeWrapper } from '../../src/utils/exchange_wrapper'; import { OrderFactory } from '../../src/utils/order_factory'; import { orderUtils } from '../../src/utils/order_utils'; import { TransactionFactory } from '../../src/utils/transaction_factory'; -import { ERC20BalancesByOwner, ExchangeStatus, SignedTransaction } from '../../src/utils/types'; +import { ERC20BalancesByOwner, OrderStatus, SignedTransaction } from '../../src/utils/types'; import { provider, txDefaults, web3Wrapper } from '../../src/utils/web3_wrapper'; chaiSetup.configure(); -- cgit v1.2.3