From 861aebb2e30ff7f38733759527a0383fc4d99930 Mon Sep 17 00:00:00 2001 From: Alex Towle Date: Wed, 28 Aug 2019 14:41:12 -0700 Subject: [PATCH] `@0x:contracts-exchange` Refactored the protocol fee tests and added tests for batchFillOrders --- contracts/exchange/8:p | 29 -- .../contracts/src/MixinMatchOrders.sol | 12 +- .../test/TestProtocolFeesReceiver.sol | 344 +++++++++++------- .../assertion_wrappers/fill_order_wrapper.ts | 18 - contracts/exchange/test/internal.ts | 21 +- contracts/exchange/test/protocol_fees.ts | 162 ++++++++- ...unit_tests.ts => protocol_fees_manager.ts} | 0 .../exchange/test/reference_functions.ts | 28 +- .../utils/fill_order_combinatorial_utils.ts | 1 - .../test/utils/fill_order_simulator.ts | 3 +- .../exchange/test/utils/match_order_tester.ts | 9 +- contracts/exchange/test/wrapper.ts | 18 +- 12 files changed, 422 insertions(+), 223 deletions(-) delete mode 100644 contracts/exchange/8:p rename contracts/exchange/test/{protocol_fees_unit_tests.ts => protocol_fees_manager.ts} (100%) diff --git a/contracts/exchange/8:p b/contracts/exchange/8:p deleted file mode 100644 index 571c250bed..0000000000 --- a/contracts/exchange/8:p +++ /dev/null @@ -1,29 +0,0 @@ -import { blockchainTests, constants } from '@0x/contracts-test-utils'; - -import { artifacts, ExchangeContract, TestProtocolFeesContract, TestProtocolFeesReceiverContract } from '../src'; - -blockchainTests('Protocol Fee Payments', env => { - let testProtocolFees: TestProtocolFeesContract; - let testProtocolFeesReceiver: TestProtocolFeesReceiverContract; - - before(async () => { - testProtocolFees = await TestProtocolFeesContract.deployFrom0xArtifactAsync( - artifacts.TestProtocolFees, - env.provider, - env.txDefaults, - {}, - ); - testProtocolFeesReceiver = await TestProtocolFeesReceiverContract.deployFrom0xArtifactAsync( - artifacts.TestProtocolFeesReceiver, - env.provider, - env.txDefaults, - {}, - ); - }); - - blockchainTests.resets('fillOrder Protocol Fees', () => { - it('should pay protocol fee in ETH when the correct value is sent', async () => { - await testProtocolFeesReceiver.testFillOrderProtocolFees.awaitTransactionSuccessAsync(; - }); - }); -}); diff --git a/contracts/exchange/contracts/src/MixinMatchOrders.sol b/contracts/exchange/contracts/src/MixinMatchOrders.sol index fa9f1ea0d2..50a0a30d67 100644 --- a/contracts/exchange/contracts/src/MixinMatchOrders.sol +++ b/contracts/exchange/contracts/src/MixinMatchOrders.sol @@ -505,10 +505,18 @@ contract MixinMatchOrders is uint256 valuePaid = 0; // Pay the left order's protocol fee. - if (address(this).balance >= 2 * protocolFee) { - valuePaid = 2 * protocolFee; + if (address(this).balance >= protocolFee) { + valuePaid = protocolFee; } IStaking(feeCollector).payProtocolFee.value(valuePaid)(leftOrder.makerAddress, takerAddress, protocolFee); + + // Clear value paid for the next call to `payProtocolFee()`. + valuePaid = 0; + + // Pay the right order's protocol fee. + if (address(this).balance >= protocolFee) { + valuePaid = protocolFee; + } IStaking(feeCollector).payProtocolFee.value(valuePaid)(rightOrder.makerAddress, takerAddress, protocolFee); } diff --git a/contracts/exchange/contracts/test/TestProtocolFeesReceiver.sol b/contracts/exchange/contracts/test/TestProtocolFeesReceiver.sol index b0b423269a..a2e5ca468e 100644 --- a/contracts/exchange/contracts/test/TestProtocolFeesReceiver.sol +++ b/contracts/exchange/contracts/test/TestProtocolFeesReceiver.sol @@ -42,6 +42,7 @@ contract TestProtocolFeesReceiver { /* Testing State */ + // A struct that provides a schema for test data that should be logged. struct TestLog { address loggedMaker; address loggedPayer; @@ -49,10 +50,83 @@ contract TestProtocolFeesReceiver { uint256 loggedValue; } + // The array of testLogs that will be added to by `payProtocolFee` and processed by the tests. TestLog[] testLogs; /* Testing Functions */ + /// @dev Tests the `batchFillOrders` function's payment of protocol fees. + /// @param testProtocolFees The TestProtocolFees that should be tested against. + /// @param protocolFeeMultiplier The protocol fee multiplier that should be registered + /// in the test suite before executing `batchFillOrders`. + /// @param numberOfOrders The number of orders that should be created and executed for this test. + /// @param shouldSetProtocolFeeCollector A boolean value indicating whether or not this contract + /// should be registered as the `protocolFeeCollector`. + function testBatchFillOrdersProtocolFees( + TestProtocolFees testProtocolFees, + uint256 protocolFeeMultiplier, + uint256 numberOfOrders, + bool shouldSetProtocolFeeCollector + ) + external + payable + handleState(testProtocolFees, protocolFeeMultiplier, shouldSetProtocolFeeCollector) + { + // Create empty arrays for taker asset filled amounts and signatures, which will suffice for this test. + uint256[] memory takerAssetFilledAmounts = new uint256[](numberOfOrders); + bytes[] memory signatures = new bytes[](numberOfOrders); + + // Construct an array of orders in which every even-indexed order has a makerAddress of makerAddress1 and + // every odd-indexed order has a makerAddress of makerAddress2. This is done to make sure that the correct + // makers are being logged. + LibOrder.Order[] memory orders = new LibOrder.Order[](numberOfOrders); + for (uint256 i = 0; i < numberOfOrders; i++) { + orders[i] = createOrder(i % 2 == 0 ? makerAddress1 : makerAddress2); + } + + // Forward all of the value sent to the contract to `batchFillOrders()`. + testProtocolFees.batchFillOrders.value(msg.value)(orders, takerAssetFilledAmounts, signatures); + + // If the `protocolFeeCollector` was set, ensure that the protocol fees were paid correctly. + // Otherwise, the protocol fees should not have been paid. + if (shouldSetProtocolFeeCollector) { + // Ensure that the correct number of test logs were recorded. + require(testLogs.length == numberOfOrders, "Incorrect number of test logs in batchFillOrders test"); + + // Calculate the expected protocol fee. + uint256 expectedProtocolFeePaid = tx.gasprice.safeMul(protocolFeeMultiplier); + + // Set the expected available balance for the first log. + uint256 expectedAvailableBalance = msg.value; + + // Verify all of the test logs. + for (uint256 i = 0; i < testLogs.length; i++) { + // Verify the logged data. + verifyTestLog( + testLogs[i], + expectedAvailableBalance, // expectedAvailableBalance + i % 2 == 0 ? makerAddress1 : makerAddress2, // expectedMakerAddress + address(this), // expectedPayerAddress + expectedProtocolFeePaid // expectedProtocolFeePaid + ); + + // Set the expected available balance for the next log. + expectedAvailableBalance = expectedAvailableBalance >= expectedProtocolFeePaid ? + expectedAvailableBalance - expectedProtocolFeePaid : + expectedAvailableBalance; + } + } else { + // Ensure that zero test logs were created. + require(testLogs.length == 0, "Incorrect number of test logs in batchFillOrders test"); + } + } + + /// @dev Tests the `fillOrder` function's payment of protocol fees. + /// @param testProtocolFees The TestProtocolFees that should be tested against. + /// @param protocolFeeMultiplier The protocol fee multiplier that should be registered + /// in the test suite before executing `fillOrder`. + /// @param shouldSetProtocolFeeCollector A boolean value indicating whether or not this contract + /// should be registered as the `protocolFeeCollector`. function testFillOrderProtocolFees( TestProtocolFees testProtocolFees, uint256 protocolFeeMultiplier, @@ -60,30 +134,48 @@ contract TestProtocolFeesReceiver { ) external payable + handleState(testProtocolFees, protocolFeeMultiplier, shouldSetProtocolFeeCollector) { - // Set up the exchange state for the test. - setExchangeState( - testProtocolFees, - protocolFeeMultiplier, - shouldSetProtocolFeeCollector - ); + // Create empty values for the takerAssetFilledAmount and the signature since these values don't + // matter for this test. + uint256 takerAssetFilledAmount = 0; + bytes memory signature = new bytes(0); // Construct an of order with distinguishing information. - LibOrder.Order memory order; - order.makerAddress = makerAddress1; - order.makerAssetAmount = 1; - order.takerAssetAmount = 1; + LibOrder.Order memory order = createOrder(makerAddress1); // Forward all of the value sent to the contract to `fillOrder()`. - testProtocolFees.fillOrder.value(msg.value)(order, 0, new bytes(0)); + testProtocolFees.fillOrder.value(msg.value)(order, takerAssetFilledAmount, signature); - // Ensure that the results of the test were correct. - verifyFillOrderTestResults(protocolFeeMultiplier, shouldSetProtocolFeeCollector); + // If the `protocolFeeCollector` was set, ensure that the protocol fee was paid correctly. + // Otherwise, the protocol fee should not have been paid. + if (shouldSetProtocolFeeCollector) { + // Ensure that only one test log was created by the call to `fillOrder()`. + require(testLogs.length == 1, "Incorrect number of test logs in fillOrder test"); - // Clear all state that was set to ensure that future tests are unaffected by this one. - clearState(testProtocolFees); + // Calculate the expected protocol fee. + uint256 expectedProtocolFeePaid = tx.gasprice.safeMul(protocolFeeMultiplier); + + // Verify that the test log that was created is correct. + verifyTestLog( + testLogs[0], + msg.value, // expectedAvailableBalance + makerAddress1, // expectedMakerAddress + address(this), // expectedPayerAddress + expectedProtocolFeePaid // expectedProtocolFeePaid + ); + } else { + // Ensure that zero test logs were created. + require(testLogs.length == 0, "Incorrect number of test logs in fillOrder test"); + } } + /// @dev Tests the `matchOrders` function's payment of protocol fees. + /// @param testProtocolFees The TestProtocolFees that should be tested against. + /// @param protocolFeeMultiplier The protocol fee multiplier that should be registered + /// in the test suite before executing `matchOrders`. + /// @param shouldSetProtocolFeeCollector A boolean value indicating whether or not this contract + /// should be registered as the `protocolFeeCollector`. function testMatchOrdersProtocolFees( TestProtocolFees testProtocolFees, uint256 protocolFeeMultiplier, @@ -91,144 +183,124 @@ contract TestProtocolFeesReceiver { ) external payable + handleState(testProtocolFees, protocolFeeMultiplier, shouldSetProtocolFeeCollector) { - // Set up the exchange state for the test. - setExchangeState( - testProtocolFees, - protocolFeeMultiplier, - shouldSetProtocolFeeCollector - ); + // Create two empty signatures since signatures are not used in this test. + bytes memory leftSignature = new bytes(0); + bytes memory rightSignature = new bytes(0); - // Construct a pair of orders with distinguishing information. - LibOrder.Order memory leftOrder; - leftOrder.makerAddress = makerAddress1; - leftOrder.makerAssetAmount = 1; - leftOrder.takerAssetAmount = 1; - LibOrder.Order memory rightOrder; - rightOrder.makerAddress = makerAddress2; - rightOrder.makerAssetAmount = 1; - rightOrder.takerAssetAmount = 1; + // Construct a distinguished left order. + LibOrder.Order memory leftOrder = createOrder(makerAddress1); + + // Construct a distinguished right order. + LibOrder.Order memory rightOrder = createOrder(makerAddress2); // Forward all of the value sent to the contract to `matchOrders()`. - testProtocolFees.matchOrders.value(msg.value)(leftOrder, rightOrder, new bytes(0), new bytes(0)); + testProtocolFees.matchOrders.value(msg.value)(leftOrder, rightOrder, leftSignature, rightSignature); - // Ensure that the results of the test were correct. - verifyMatchOrdersTestResults(protocolFeeMultiplier, shouldSetProtocolFeeCollector); + // If the `protocolFeeCollector` was set, ensure that the protocol fee was paid correctly. + // Otherwise, the protocol fee should not have been paid. + if (shouldSetProtocolFeeCollector) { + // Ensure that only one test log was created by the call to `fillOrder()`. + require(testLogs.length == 2, "Incorrect number of test logs in matchOrders test"); - // Clear all state that was set to ensure that future tests are unaffected by this one. - clearState(testProtocolFees); + // Calculate the expected protocol fee. + uint256 expectedProtocolFeePaid = tx.gasprice.safeMul(protocolFeeMultiplier); + + // Set the expected available balance for the first log. + uint256 expectedAvailableBalance = msg.value; + + // Verify that the first test log that was created is correct. + verifyTestLog( + testLogs[0], + expectedAvailableBalance, // expectedAvailableBalance + makerAddress1, // expectedMakerAddress + address(this), // expectedPayerAddress + expectedProtocolFeePaid // expectedProtocolFeePaid + ); + + // Set the expected available balance for the second log. + expectedAvailableBalance = expectedAvailableBalance >= expectedProtocolFeePaid ? + expectedAvailableBalance - expectedProtocolFeePaid : + expectedAvailableBalance; + + // Verify that the second test log that was created is correct. + verifyTestLog( + testLogs[1], + expectedAvailableBalance, // expectedAvailableBalance + makerAddress2, // expectedMakerAddress + address(this), // expectedPayerAddress + expectedProtocolFeePaid // expectedProtocolFeePaid + ); + } else { + // Ensure that zero test logs were created. + require(testLogs.length == 0, "Incorrect number of test logs in matchOrders test"); + } } /* Verification Functions */ - function verifyFillOrderTestResults( - uint256 protocolFeeMultiplier, - bool shouldSetProtocolFeeCollector + /// @dev Verifies a test log against expected values. + /// @param expectedAvailableBalance The balance that should be available when this call is made. + /// This is important especially for tests on wrapper functions. + /// @param expectedMakerAddress The expected maker address to be recorded in `payProtocolFee`. + /// @param expectedPayerAddress The expected payer address to be recorded in `payProtocolFee`. + /// @param expectedProtocolFeePaid The expected protocol fee paidto be recorded in `payProtocolFee`. + function verifyTestLog( + TestLog memory log, + uint256 expectedAvailableBalance, + address expectedMakerAddress, + address expectedPayerAddress, + uint256 expectedProtocolFeePaid ) internal { - // If the `protocolFeeCollector` was set, then this contract should have been called. - if (shouldSetProtocolFeeCollector) { - // Calculate the protocol fee that should be paid. - uint256 protocolFee = tx.gasprice.safeMul(protocolFeeMultiplier); - - // Ensure that one TestLog was recorded. - require(testLogs.length == 1, "Incorrect TestLog length for fillOrder test"); - - // Get an alias to the test log. - TestLog memory log = testLogs[0]; - - // If the forwarded value was greater than the protocol fee, the protocol fee should - // have been sent back to this contract. - if (msg.value >= protocolFee) { - // Ensure that the protocolFee was forwarded to this contract. - require( - log.loggedValue == protocolFee, - "Incorrect eth was received during fillOrder test when adequate ETH was sent" - ); - } else { - // Ensure that no ether was forwarded to this contract. - require( - log.loggedValue == 0, - "Incorrect eth was received during fillOrder test when inadequate ETH was sent" - ); - } - - // Ensure that the correct data was logged during this test. - require(log.loggedMaker == makerAddress1, "Incorrect maker address was logged for fillOrder test"); - require(log.loggedPayer == address(this), "Incorrect taker address was logged for fillOrder test"); - require(log.loggedProtocolFeePaid == protocolFee, "Incorrect protocol fee was logged for fillOrder test"); + // If the expected available balance was sufficient to pay the protocol fee, the protocol fee + // should have been paid in ether. Otherwise, no ether should be sent to pay the protocol fee. + if (expectedAvailableBalance >= expectedProtocolFeePaid) { + // Ensure that the protocol fee was paid in ether. + require( + log.loggedValue == expectedProtocolFeePaid, + "Incorrect eth was received during fillOrder test when adequate ETH was sent" + ); } else { - // Ensure that `protocolFeePaid()` was not called. - require(testLogs.length == 0, "protocolFeePaid was called"); + // Ensure that the protocol fee was not paid in ether. + require( + log.loggedValue == 0, + "Incorrect eth was received during fillOrder test when inadequate ETH was sent" + ); } + + // Ensure that the correct data was logged. + require(log.loggedMaker == expectedMakerAddress, "Incorrect maker address was logged"); + require(log.loggedPayer == expectedPayerAddress, "Incorrect taker address was logged"); + require(log.loggedProtocolFeePaid == expectedProtocolFeePaid, "Incorrect protocol fee was logged"); } - function verifyMatchOrdersTestResults( - uint256 protocolFeeMultiplier, - bool shouldSetProtocolFeeCollector - ) - internal - { - // If the `protocolFeeCollector` was set, then this contract should have been called. - if (shouldSetProtocolFeeCollector) { - // Calculate the protocol fee that should be paid. - uint256 protocolFee = tx.gasprice.safeMul(protocolFeeMultiplier); + /* Testing Convenience Functions */ - // Ensure that one TestLog was recorded. - require(testLogs.length == 2, "Incorrect TestLog length for matchOrders test"); - - // Get an alias to the test logs. - TestLog memory log1 = testLogs[0]; - TestLog memory log2 = testLogs[1]; - - // If the forwarded value was greater than the protocol fee, the protocol fee should - // have been sent back to this contract. - if (msg.value >= 2 * protocolFee) { - // Ensure that the protocolFee was forwarded to this contract. - require( - log1.loggedValue == protocolFee && log2.loggedValue == protocolFee, - "Incorrect eth was received during matchOrders test when adequate ETH was sent" - ); - } else { - // Ensure that no ether was forwarded to this contract. - require( - log1.loggedValue == 0 && log2.loggedValue == 0, - "Incorrect eth was received during fillOrder test when inadequate ETH was sent" - ); - } - - // Ensure that the correct data was logged during this test. - require(log1.loggedMaker == makerAddress1, "Incorrect maker address was logged for matchOrders test"); - require(log1.loggedPayer == address(this), "Incorrect taker address was logged for matchOrders test"); - require(log1.loggedProtocolFeePaid == protocolFee, "Incorrect protocol fee was logged for matchOrders test"); - require(log2.loggedMaker == makerAddress2, "Incorrect maker address was logged for matchOrders test"); - require(log2.loggedPayer == address(this), "Incorrect taker address was logged for matchOrders test"); - require(log2.loggedProtocolFeePaid == protocolFee, "Incorrect protocol fee was logged for matchOrders test"); - } else { - // Ensure that `protocolFeePaid()` was not called. - require(testLogs.length == 0, "protocolFeePaid was called"); - } - } - - /* State setup and teardown functions */ - - function setExchangeState( + /// @dev Sets up state that is necessary for tests and then cleans up the state that was written + /// to so that test cases can be thought of as atomic. + /// @param testProtocolFees The TestProtocolFees contract that is being used during testing. + /// @param protocolFeeMultiplier The protocolFeeMultiplier of this test case. + /// @param shouldSetProtocolFeeCollector A boolean value that indicates whether or not this address + /// should be made the protocol fee collector. + modifier handleState( TestProtocolFees testProtocolFees, uint256 protocolFeeMultiplier, bool shouldSetProtocolFeeCollector ) - internal { + // If necessary, set the protocol fee collector field in the exchange. if (shouldSetProtocolFeeCollector) { testProtocolFees.setProtocolFeeCollector(address(this)); } + // Set the protocol fee multiplier in the exchange. testProtocolFees.setProtocolFeeMultiplier(protocolFeeMultiplier); - } - function clearState(TestProtocolFees testProtocolFees) - internal - { + // Execute the test. + _; + // Clear exchange state testProtocolFees.setProtocolFeeCollector(address(0)); testProtocolFees.setProtocolFeeMultiplier(0); @@ -238,6 +310,25 @@ contract TestProtocolFeesReceiver { delete testLogs; } + /// @dev Constructs an order with a specified maker address. + /// @param makerAddress The maker address of the order. + function createOrder(address makerAddress) + internal + pure + returns (LibOrder.Order memory order) + { + order.makerAddress = makerAddress; + order.makerAssetAmount = 1; // This is 1 so that it doesn't trigger a `DivionByZero()` error. + order.takerAssetAmount = 1; // This is 1 so that it doesn't trigger a `DivionByZero()` error. + } + + /* Protocol Fee Receiver and Fallback */ + + /// @dev Receives payments of protocol fees from a TestProtocolFees contract + /// and records the data provided and the message value sent. + /// @param makerAddress The maker address that should be recorded. + /// @param payerAddress The payer address that should be recorded. + /// @param protocolFeePaid The protocol fee that should be recorded. function payProtocolFee( address makerAddress, address payerAddress, @@ -246,6 +337,7 @@ contract TestProtocolFeesReceiver { external payable { + // Push the collected data into `testLogs`. testLogs.push(TestLog({ loggedMaker: makerAddress, loggedPayer: payerAddress, @@ -254,6 +346,8 @@ contract TestProtocolFeesReceiver { })); } + /// @dev A payable fallback function that makes this contract "payable". This is necessary to allow + /// this contract to gracefully handle refunds from TestProtocolFees contracts. function () external payable diff --git a/contracts/exchange/test/assertion_wrappers/fill_order_wrapper.ts b/contracts/exchange/test/assertion_wrappers/fill_order_wrapper.ts index 34a663064f..1387065bfc 100644 --- a/contracts/exchange/test/assertion_wrappers/fill_order_wrapper.ts +++ b/contracts/exchange/test/assertion_wrappers/fill_order_wrapper.ts @@ -33,8 +33,6 @@ export class FillOrderWrapper { private readonly _blockchainBalanceStore: BlockchainBalanceStore; private readonly _web3Wrapper: Web3Wrapper; - // FIXME - Punting on these tests for now since no staking contract will be registered. This - // should be revisited when the protocol fee testing has been unit tested well. /** * Simulates matching two orders by transferring amounts defined in * `transferAmounts` and returns the results. @@ -90,20 +88,6 @@ export class FillOrderWrapper { fillResults.makerFeePaid, signedOrder.makerFeeAssetData, ); - // FIXME - Punting on these tests for now since no staking contract will be registered. This - // should be revisited when the protocol fee testing has been unit tested well. - // if (stakingOpts.messageValue.isGreaterThanOrEqualTo(fillResults.protocolFeePaid)) { - // // Pay the protocol fee in ETH. - // balanceStore.transferAsset(takerAddress, stakingOpts.stakingAddress, fillResults.protocolFeePaid, ''); - // } else { - // // Pay the protocol fee in WETH. - // balanceStore.transferAsset( - // takerAddress, - // stakingOpts.stakingAddress, - // fillResults.protocolFeePaid, - // AssetProxyId.ERC20, - // ); - // } return [fillResults, fillEvent, balanceStore]; } @@ -167,8 +151,6 @@ export class FillOrderWrapper { return this._blockchainBalanceStore; } - // FIXME - Punting on these tests for now since no staking contract will be registered. This - // should be revisited when the protocol fee testing has been unit tested well. /** * Fills an order and asserts the effects. This includes * 1. The order info (via `getOrderInfo`) diff --git a/contracts/exchange/test/internal.ts b/contracts/exchange/test/internal.ts index 8369990ffd..887e68e070 100644 --- a/contracts/exchange/test/internal.ts +++ b/contracts/exchange/test/internal.ts @@ -164,7 +164,6 @@ blockchainTests('Exchange core internal functions', env => { return _.assign({}, ORDER_DEFAULTS, details); } - // FIXME - This test definitely needs to be updated async function testUpdateFilledStateAsync( order: OrderWithoutDomain, orderTakerAssetFilledAmount: BigNumber, @@ -190,7 +189,7 @@ blockchainTests('Exchange core internal functions', env => { orderHash, orderTakerAssetFilledAmount, fillResults, - // opts, // FIXME + // opts, ), ); // Grab the new `filled` state for this order. @@ -242,7 +241,6 @@ blockchainTests('Exchange core internal functions', env => { const order = makeOrder(); const orderTakerAssetFilledAmount = constants.MAX_UINT256.dividedToIntegerBy(2); const takerAssetFillAmount = constants.MAX_UINT256.dividedToIntegerBy(2).plus(2); - // FIXME const fillResults = { makerAssetFilledAmount: constants.ZERO_AMOUNT, takerAssetFilledAmount: takerAssetFillAmount, @@ -289,7 +287,6 @@ blockchainTests('Exchange core internal functions', env => { const order = DEFAULT_ORDER; const orderHash = randomHash(); const takerAddress = randomAddress(); - // FIXME const fillResults = { makerAssetFilledAmount: ONE_ETHER.times(2), takerAssetFilledAmount: ONE_ETHER.times(10), @@ -365,14 +362,14 @@ blockchainTests('Exchange core internal functions', env => { takerAssetFilledAmount: ONE_ETHER.times(10), makerFeePaid: ONE_ETHER.times(0.01), takerFeePaid: constants.MAX_UINT256, - protocolFeePaid: constants.ZERO_AMOUNT, // FIXME + protocolFeePaid: constants.ZERO_AMOUNT, }, right: { takerAssetFilledAmount: ONE_ETHER.times(20), makerAssetFilledAmount: ONE_ETHER.times(4), makerFeePaid: ONE_ETHER.times(0.02), takerFeePaid: constants.MAX_UINT256_ROOT, - protocolFeePaid: constants.ZERO_AMOUNT, // FIXME + protocolFeePaid: constants.ZERO_AMOUNT, }, profitInLeftMakerAsset: ONE_ETHER, profitInRightMakerAsset: ONE_ETHER.times(2), @@ -414,14 +411,14 @@ blockchainTests('Exchange core internal functions', env => { takerAssetFilledAmount: ONE_ETHER.times(10), makerFeePaid: ONE_ETHER.times(0.01), takerFeePaid: constants.MAX_UINT256, - protocolFeePaid: constants.ZERO_AMOUNT, // FIXME + protocolFeePaid: constants.ZERO_AMOUNT, }, right: { takerAssetFilledAmount: ONE_ETHER.times(20), makerAssetFilledAmount: ONE_ETHER.times(4), makerFeePaid: ONE_ETHER.times(0.02), takerFeePaid: constants.MAX_UINT256_ROOT, - protocolFeePaid: constants.ZERO_AMOUNT, // FIXME + protocolFeePaid: constants.ZERO_AMOUNT, }, profitInLeftMakerAsset: ONE_ETHER, profitInRightMakerAsset: ONE_ETHER.times(2), @@ -452,14 +449,14 @@ blockchainTests('Exchange core internal functions', env => { takerAssetFilledAmount: ONE_ETHER.times(10), makerFeePaid: ONE_ETHER.times(0.01), takerFeePaid: ONE_ETHER.times(0.025), - protocolFeePaid: constants.ZERO_AMOUNT, // FIXME + protocolFeePaid: constants.ZERO_AMOUNT, }, right: { takerAssetFilledAmount: ONE_ETHER.times(20), makerAssetFilledAmount: ONE_ETHER.times(4), makerFeePaid: ONE_ETHER.times(0.02), takerFeePaid: ONE_ETHER.times(0.05), - protocolFeePaid: constants.ZERO_AMOUNT, // FIXME + protocolFeePaid: constants.ZERO_AMOUNT, }, profitInLeftMakerAsset: ONE_ETHER, profitInRightMakerAsset: ONE_ETHER.times(2), @@ -550,14 +547,14 @@ blockchainTests('Exchange core internal functions', env => { takerAssetFilledAmount: ONE_ETHER.times(10), makerFeePaid: ONE_ETHER.times(0.01), takerFeePaid: ONE_ETHER.times(0.025), - protocolFeePaid: constants.ZERO_AMOUNT, // FIXME + protocolFeePaid: constants.ZERO_AMOUNT, }, right: { takerAssetFilledAmount: ONE_ETHER.times(20), makerAssetFilledAmount: ONE_ETHER.times(4), makerFeePaid: ONE_ETHER.times(0.02), takerFeePaid: ONE_ETHER.times(0.05), - protocolFeePaid: constants.ZERO_AMOUNT, // FIXME + protocolFeePaid: constants.ZERO_AMOUNT, }, profitInLeftMakerAsset: ONE_ETHER, profitInRightMakerAsset: ONE_ETHER.times(2), diff --git a/contracts/exchange/test/protocol_fees.ts b/contracts/exchange/test/protocol_fees.ts index a29e3dba2c..c1709e21d1 100644 --- a/contracts/exchange/test/protocol_fees.ts +++ b/contracts/exchange/test/protocol_fees.ts @@ -3,7 +3,10 @@ import { BigNumber } from '@0x/utils'; import { artifacts, TestProtocolFeesContract, TestProtocolFeesReceiverContract } from '../src'; -blockchainTests.only('Protocol Fee Payments', env => { +// The contents of this test suite does not inform the reader about the assertions made in these +// tests. For more information and a more accurate view of the tests, check out +// "contracts/test/TestProtocolFeesReceiver.sol". +blockchainTests('Protocol Fee Payments', env => { let testProtocolFees: TestProtocolFeesContract; let testProtocolFeesReceiver: TestProtocolFeesReceiverContract; @@ -46,7 +49,7 @@ blockchainTests.only('Protocol Fee Payments', env => { true, { gasPrice: DEFAULT_GAS_PRICE, - value: DEFAULT_PROTOCOL_FEE.minus(new BigNumber(10)), + value: DEFAULT_PROTOCOL_FEE.minus(10), }, ); }); @@ -70,7 +73,7 @@ blockchainTests.only('Protocol Fee Payments', env => { true, { gasPrice: DEFAULT_GAS_PRICE, - value: DEFAULT_PROTOCOL_FEE.plus(new BigNumber(10)), + value: DEFAULT_PROTOCOL_FEE.plus(10), }, ); }); @@ -89,19 +92,19 @@ blockchainTests.only('Protocol Fee Payments', env => { ); }); - it('should pay protocol fee in WETH when too little value is sent', async () => { + it('should pay protocol fee in WETH twice when too little value is sent', async () => { await testProtocolFeesReceiver.testMatchOrdersProtocolFees.awaitTransactionSuccessAsync( testProtocolFees.address, DEFAULT_PROTOCOL_FEE_MULTIPLIER, true, { gasPrice: DEFAULT_GAS_PRICE, - value: DEFAULT_PROTOCOL_FEE.minus(new BigNumber(10)), + value: DEFAULT_PROTOCOL_FEE.minus(10), }, ); }); - it('should pay protocol fee in ETH when the correct value is sent', async () => { + it('should pay protocol fee in ETH and then WETH when the correct value is sent', async () => { await testProtocolFeesReceiver.testMatchOrdersProtocolFees.awaitTransactionSuccessAsync( testProtocolFees.address, DEFAULT_PROTOCOL_FEE_MULTIPLIER, @@ -113,14 +116,157 @@ blockchainTests.only('Protocol Fee Payments', env => { ); }); - it('should pay protocol fee in ETH when extra value is sent', async () => { + it('should pay protocol fee in ETH when extra value is sent and then pay in WETH', async () => { await testProtocolFeesReceiver.testMatchOrdersProtocolFees.awaitTransactionSuccessAsync( testProtocolFees.address, DEFAULT_PROTOCOL_FEE_MULTIPLIER, true, { gasPrice: DEFAULT_GAS_PRICE, - value: DEFAULT_PROTOCOL_FEE.plus(new BigNumber(10)), + value: DEFAULT_PROTOCOL_FEE.plus(10), + }, + ); + }); + + it('should pay protocol fee in ETH when exactly double the protocol fee is sent', async () => { + await testProtocolFeesReceiver.testMatchOrdersProtocolFees.awaitTransactionSuccessAsync( + testProtocolFees.address, + DEFAULT_PROTOCOL_FEE_MULTIPLIER, + true, + { + gasPrice: DEFAULT_GAS_PRICE, + value: DEFAULT_PROTOCOL_FEE.times(2), + }, + ); + }); + + it('should pay protocol fee in ETH when more than double the protocol fee is sent', async () => { + await testProtocolFeesReceiver.testMatchOrdersProtocolFees.awaitTransactionSuccessAsync( + testProtocolFees.address, + DEFAULT_PROTOCOL_FEE_MULTIPLIER, + true, + { + gasPrice: DEFAULT_GAS_PRICE, + value: DEFAULT_PROTOCOL_FEE.times(2).plus(10), + }, + ); + }); + }); + + blockchainTests.resets('batchFillOrder ProtocolFees', () => { + it('should not pay protocol fees when there is not a protocolFeeCollector registered', async () => { + await testProtocolFeesReceiver.testBatchFillOrdersProtocolFees.awaitTransactionSuccessAsync( + testProtocolFees.address, + DEFAULT_PROTOCOL_FEE_MULTIPLIER, + new BigNumber(2), // If successful, create a `batchFillOrders` with 2 orders. + false, + { + gasPrice: DEFAULT_GAS_PRICE, + value: DEFAULT_PROTOCOL_FEE, + }, + ); + }); + + it('should pay one protocol fee in WETH when too little ETH is sent and only one order is in the batch', async () => { + await testProtocolFeesReceiver.testBatchFillOrdersProtocolFees.awaitTransactionSuccessAsync( + testProtocolFees.address, + DEFAULT_PROTOCOL_FEE_MULTIPLIER, + new BigNumber(1), + true, + { + gasPrice: DEFAULT_GAS_PRICE, + value: DEFAULT_PROTOCOL_FEE.minus(10), + }, + ); + }); + + it('should pay one protocol fee in ETH when the exact protocol fee is sent and only one order is in the batch', async () => { + await testProtocolFeesReceiver.testBatchFillOrdersProtocolFees.awaitTransactionSuccessAsync( + testProtocolFees.address, + DEFAULT_PROTOCOL_FEE_MULTIPLIER, + new BigNumber(1), + true, + { + gasPrice: DEFAULT_GAS_PRICE, + value: DEFAULT_PROTOCOL_FEE, + }, + ); + }); + + it('should pay one protocol fee in ETH when more than the exact protocol fee is sent and only one order is in the batch', async () => { + await testProtocolFeesReceiver.testBatchFillOrdersProtocolFees.awaitTransactionSuccessAsync( + testProtocolFees.address, + DEFAULT_PROTOCOL_FEE_MULTIPLIER, + new BigNumber(1), + true, + { + gasPrice: DEFAULT_GAS_PRICE, + value: DEFAULT_PROTOCOL_FEE.plus(10), + }, + ); + }); + + it('should pay both protocol fees in WETH when an insuffiecent amount of ETH for one protocol fee is sent', async () => { + await testProtocolFeesReceiver.testBatchFillOrdersProtocolFees.awaitTransactionSuccessAsync( + testProtocolFees.address, + DEFAULT_PROTOCOL_FEE_MULTIPLIER, + new BigNumber(2), + true, + { + gasPrice: DEFAULT_GAS_PRICE, + value: DEFAULT_PROTOCOL_FEE.minus(10), + }, + ); + }); + + it('should pay a protocol in ETH and then a fee in WETH when exactly one protocol fee in ETH is sent', async () => { + await testProtocolFeesReceiver.testBatchFillOrdersProtocolFees.awaitTransactionSuccessAsync( + testProtocolFees.address, + DEFAULT_PROTOCOL_FEE_MULTIPLIER, + new BigNumber(2), + true, + { + gasPrice: DEFAULT_GAS_PRICE, + value: DEFAULT_PROTOCOL_FEE, + }, + ); + }); + + it('should pay both protocol fees in ETH when exactly two protocol fees in ETH is sent', async () => { + await testProtocolFeesReceiver.testBatchFillOrdersProtocolFees.awaitTransactionSuccessAsync( + testProtocolFees.address, + DEFAULT_PROTOCOL_FEE_MULTIPLIER, + new BigNumber(2), + true, + { + gasPrice: DEFAULT_GAS_PRICE, + value: DEFAULT_PROTOCOL_FEE.times(2), + }, + ); + }); + + it('should pay two protocol fees in ETH and one in WETH when exactly two protocol fees in ETH is sent', async () => { + await testProtocolFeesReceiver.testBatchFillOrdersProtocolFees.awaitTransactionSuccessAsync( + testProtocolFees.address, + DEFAULT_PROTOCOL_FEE_MULTIPLIER, + new BigNumber(3), + true, + { + gasPrice: DEFAULT_GAS_PRICE, + value: DEFAULT_PROTOCOL_FEE.times(2), + }, + ); + }); + + it('should pay three protocol fees in ETH when more than three protocol fees in ETH is sent', async () => { + await testProtocolFeesReceiver.testBatchFillOrdersProtocolFees.awaitTransactionSuccessAsync( + testProtocolFees.address, + DEFAULT_PROTOCOL_FEE_MULTIPLIER, + new BigNumber(3), + true, + { + gasPrice: DEFAULT_GAS_PRICE, + value: DEFAULT_PROTOCOL_FEE.times(3).plus(10), }, ); }); diff --git a/contracts/exchange/test/protocol_fees_unit_tests.ts b/contracts/exchange/test/protocol_fees_manager.ts similarity index 100% rename from contracts/exchange/test/protocol_fees_unit_tests.ts rename to contracts/exchange/test/protocol_fees_manager.ts diff --git a/contracts/exchange/test/reference_functions.ts b/contracts/exchange/test/reference_functions.ts index 9beca861fd..1a82ed6d50 100644 --- a/contracts/exchange/test/reference_functions.ts +++ b/contracts/exchange/test/reference_functions.ts @@ -65,7 +65,9 @@ describe('Reference functions', () => { makerAssetFilledAmount, order.makerFee, ); - return expect(() => LibReferenceFunctions.calculateFillResults(order, takerAssetFilledAmount)).to.throw(expectedError.message); + return expect(() => LibReferenceFunctions.calculateFillResults(order, takerAssetFilledAmount)).to.throw( + expectedError.message, + ); }); it('reverts if computing `fillResults.takerFeePaid` overflows', () => { @@ -81,7 +83,9 @@ describe('Reference functions', () => { takerAssetFilledAmount, order.takerFee, ); - return expect(() => LibReferenceFunctions.calculateFillResults(order, takerAssetFilledAmount)).to.throw(expectedError.message); + return expect(() => LibReferenceFunctions.calculateFillResults(order, takerAssetFilledAmount)).to.throw( + expectedError.message, + ); }); it('reverts if `order.makerAssetAmount` is 0', () => { @@ -91,7 +95,9 @@ describe('Reference functions', () => { }); const takerAssetFilledAmount = ONE_ETHER; const expectedError = new LibMathRevertErrors.DivisionByZeroError(); - return expect(() => LibReferenceFunctions.calculateFillResults(order, takerAssetFilledAmount)).to.throw(expectedError.message); + return expect(() => LibReferenceFunctions.calculateFillResults(order, takerAssetFilledAmount)).to.throw( + expectedError.message, + ); }); it('reverts if `order.takerAssetAmount` is 0', () => { @@ -101,7 +107,9 @@ describe('Reference functions', () => { }); const takerAssetFilledAmount = ONE_ETHER; const expectedError = new LibMathRevertErrors.DivisionByZeroError(); - return expect(() => LibReferenceFunctions.calculateFillResults(order, takerAssetFilledAmount)).to.throw(expectedError.message); + return expect(() => LibReferenceFunctions.calculateFillResults(order, takerAssetFilledAmount)).to.throw( + expectedError.message, + ); }); it('reverts if there is a rounding error computing `makerAsssetFilledAmount`', () => { @@ -115,7 +123,9 @@ describe('Reference functions', () => { order.takerAssetAmount, order.makerAssetAmount, ); - return expect(() => LibReferenceFunctions.calculateFillResults(order, takerAssetFilledAmount)).to.throw(expectedError.message); + return expect(() => LibReferenceFunctions.calculateFillResults(order, takerAssetFilledAmount)).to.throw( + expectedError.message, + ); }); it('reverts if there is a rounding error computing `makerFeePaid`', () => { @@ -135,7 +145,9 @@ describe('Reference functions', () => { order.makerAssetAmount, order.makerFee, ); - return expect(() => LibReferenceFunctions.calculateFillResults(order, takerAssetFilledAmount)).to.throw(expectedError.message); + return expect(() => LibReferenceFunctions.calculateFillResults(order, takerAssetFilledAmount)).to.throw( + expectedError.message, + ); }); it('reverts if there is a rounding error computing `takerFeePaid`', () => { @@ -155,7 +167,9 @@ describe('Reference functions', () => { order.makerAssetAmount, order.takerFee, ); - return expect(() => LibReferenceFunctions.calculateFillResults(order, takerAssetFilledAmount)).to.throw(expectedError.message); + return expect(() => LibReferenceFunctions.calculateFillResults(order, takerAssetFilledAmount)).to.throw( + expectedError.message, + ); }); }); }); diff --git a/contracts/exchange/test/utils/fill_order_combinatorial_utils.ts b/contracts/exchange/test/utils/fill_order_combinatorial_utils.ts index f69d642ec6..d3ce5693ae 100644 --- a/contracts/exchange/test/utils/fill_order_combinatorial_utils.ts +++ b/contracts/exchange/test/utils/fill_order_combinatorial_utils.ts @@ -49,7 +49,6 @@ enum TestOutlook { Failure, } -// FIXME - Really punting on this for now. It's possible that this won't need to be changed. /** * Instantiates a new instance of FillOrderCombinatorialUtils. Since this method has some * required async setup, a factory method is required. diff --git a/contracts/exchange/test/utils/fill_order_simulator.ts b/contracts/exchange/test/utils/fill_order_simulator.ts index 9a5feabc1d..024bcc05b6 100644 --- a/contracts/exchange/test/utils/fill_order_simulator.ts +++ b/contracts/exchange/test/utils/fill_order_simulator.ts @@ -20,7 +20,6 @@ export enum FillOrderError { TransferFailed = 'TRANSFER_FAILED', } -// FIXME - Punting on protocol fees for now /** * Simplified fill order simulator. */ @@ -122,7 +121,7 @@ export class FillOrderSimulator { makerAssetFilledAmount: makerAssetFillAmount, makerFeePaid, takerFeePaid, - protocolFeePaid: constants.ZERO_AMOUNT, // FIXME + protocolFeePaid: constants.ZERO_AMOUNT, }; } } diff --git a/contracts/exchange/test/utils/match_order_tester.ts b/contracts/exchange/test/utils/match_order_tester.ts index dd043c0ff1..1aa156ccb5 100644 --- a/contracts/exchange/test/utils/match_order_tester.ts +++ b/contracts/exchange/test/utils/match_order_tester.ts @@ -235,7 +235,6 @@ export class MatchOrderTester { return expectedBatchMatchResults; } - // FIXME - Punting on protocol fees until later /** * Matches two complementary orders and asserts results. * @param orders The matched orders and filled states. @@ -267,7 +266,7 @@ export class MatchOrderTester { orders.leftOrder, orders.rightOrder, takerAddress, - {}, // FIXME + {}, ); transactionReceipt = await this._executeMatchOrdersWithMaximalFillAsync( orders.leftOrder, @@ -1200,14 +1199,14 @@ function convertToMatchResults(result: MatchResults): MatchedFillResults { takerAssetFilledAmount: result.fills[0].takerAssetFilledAmount, makerFeePaid: result.fills[0].makerFeePaid, takerFeePaid: result.fills[0].takerFeePaid, - protocolFeePaid: constants.ZERO_AMOUNT, // FIXME + protocolFeePaid: constants.ZERO_AMOUNT, }, right: { makerAssetFilledAmount: result.fills[1].makerAssetFilledAmount, takerAssetFilledAmount: result.fills[1].takerAssetFilledAmount, makerFeePaid: result.fills[1].makerFeePaid, takerFeePaid: result.fills[1].takerFeePaid, - protocolFeePaid: constants.ZERO_AMOUNT, // FIXME + protocolFeePaid: constants.ZERO_AMOUNT, }, profitInLeftMakerAsset, profitInRightMakerAsset, @@ -1226,7 +1225,7 @@ function convertToFillResults(result: FillEventArgs): FillResults { takerAssetFilledAmount: result.takerAssetFilledAmount, makerFeePaid: result.makerFeePaid, takerFeePaid: result.takerFeePaid, - protocolFeePaid: constants.ZERO_AMOUNT, // FIXME + protocolFeePaid: constants.ZERO_AMOUNT, }; return fillResults; } diff --git a/contracts/exchange/test/wrapper.ts b/contracts/exchange/test/wrapper.ts index 65b18a09e9..b2e2cb54f3 100644 --- a/contracts/exchange/test/wrapper.ts +++ b/contracts/exchange/test/wrapper.ts @@ -501,8 +501,6 @@ blockchainTests.resets('Exchange wrappers', env => { .dividedToIntegerBy(signedOrder.makerAssetAmount); takerAssetFillAmounts.push(takerAssetFillAmount); - // FIXME - Punting on these tests for now since no staking contract will be registered. This - // should be revisited when the protocol fee testing has been unit tested well. expectedFillResults.push({ takerAssetFilledAmount: takerAssetFillAmount, makerAssetFilledAmount, @@ -534,8 +532,6 @@ blockchainTests.resets('Exchange wrappers', env => { ].plus(makerFee.plus(takerFee)); }); - // FIXME - Punting on these tests for now since no staking contract will be registered. This - // should be revisited when the protocol fee testing has been unit tested well. const fillResults = await exchange.batchFillOrders.callAsync( signedOrders, takerAssetFillAmounts, @@ -573,8 +569,6 @@ blockchainTests.resets('Exchange wrappers', env => { .dividedToIntegerBy(signedOrder.makerAssetAmount); takerAssetFillAmounts.push(takerAssetFillAmount); - // FIXME - Punting on these tests for now since no staking contract will be registered. This - // should be revisited when the protocol fee testing has been unit tested well. expectedFillResults.push({ takerAssetFilledAmount: takerAssetFillAmount, makerAssetFilledAmount, @@ -659,8 +653,6 @@ blockchainTests.resets('Exchange wrappers', env => { .dividedToIntegerBy(signedOrder.makerAssetAmount); takerAssetFillAmounts.push(takerAssetFillAmount); - // FIXME - Punting on these tests for now since no staking contract will be registered. This - // should be revisited when the protocol fee testing has been unit tested well. expectedFillResults.push({ takerAssetFilledAmount: takerAssetFillAmount, makerAssetFilledAmount, @@ -732,8 +724,6 @@ blockchainTests.resets('Exchange wrappers', env => { .dividedToIntegerBy(signedOrder.makerAssetAmount); takerAssetFillAmounts.push(takerAssetFillAmount); - // FIXME - Punting on these tests for now since no staking contract will be registered. This - // should be revisited when the protocol fee testing has been unit tested well. expectedFillResults.push({ takerAssetFilledAmount: takerAssetFillAmount, makerAssetFilledAmount, @@ -876,7 +866,7 @@ blockchainTests.resets('Exchange wrappers', env => { takerAssetFilledAmount: signedOrder.takerAssetAmount, makerFeePaid: signedOrder.makerFee, takerFeePaid: signedOrder.takerFee, - protocolFeePaid: constants.ZERO_AMOUNT, // FIXME - This is what is being used now. + protocolFeePaid: constants.ZERO_AMOUNT, })) .reduce( (totalFillResults, currentFillResults) => ({ @@ -948,7 +938,7 @@ blockchainTests.resets('Exchange wrappers', env => { takerAssetFilledAmount: signedOrder.takerAssetAmount, makerFeePaid: signedOrder.makerFee, takerFeePaid: signedOrder.takerFee, - protocolFeePaid: constants.ZERO_AMOUNT, // FIXME + protocolFeePaid: constants.ZERO_AMOUNT, })) .reduce( (totalFillResults, currentFillResults) => ({ @@ -1064,7 +1054,7 @@ blockchainTests.resets('Exchange wrappers', env => { takerAssetFilledAmount: signedOrder.takerAssetAmount, makerFeePaid: signedOrder.makerFee, takerFeePaid: signedOrder.takerFee, - protocolFeePaid: constants.ZERO_AMOUNT, // FIXME + protocolFeePaid: constants.ZERO_AMOUNT, })) .reduce( (totalFillResults, currentFillResults) => ({ @@ -1137,7 +1127,7 @@ blockchainTests.resets('Exchange wrappers', env => { takerAssetFilledAmount: signedOrder.takerAssetAmount, makerFeePaid: signedOrder.makerFee, takerFeePaid: signedOrder.takerFee, - protocolFeePaid: constants.ZERO_AMOUNT, // FIXME + protocolFeePaid: constants.ZERO_AMOUNT, })) .reduce( (totalFillResults, currentFillResults) => ({