diff --git a/contracts/exchange-libs/contracts/src/LibFillResults.sol b/contracts/exchange-libs/contracts/src/LibFillResults.sol index 7b7aa2e0b5..66185745ac 100644 --- a/contracts/exchange-libs/contracts/src/LibFillResults.sol +++ b/contracts/exchange-libs/contracts/src/LibFillResults.sol @@ -52,13 +52,18 @@ library LibFillResults { /// @dev Calculates amounts filled and fees paid by maker and taker. /// @param order to be filled. /// @param takerAssetFilledAmount Amount of takerAsset that will be filled. + /// @param protocolFeeMultiplier The current protocol fee of the exchange contract. + /// @param gasPrice The gasprice of the transaction. This is provided so that the function call can continue + /// to be pure rather than view. /// @return fillResults Amounts filled and fees paid by maker and taker. function calculateFillResults( LibOrder.Order memory order, - uint256 takerAssetFilledAmount + uint256 takerAssetFilledAmount, + uint256 protocolFeeMultiplier, + uint256 gasPrice ) internal - view + pure returns (FillResults memory fillResults) { // Compute proportional transfer amounts @@ -79,6 +84,9 @@ library LibFillResults { order.takerFee ); + // Compute the protocol fee that should be paid for a single fill. + fillResults.protocolFeePaid = gasPrice.safeMul(protocolFeeMultiplier); + return fillResults; } @@ -90,6 +98,9 @@ library LibFillResults { /// @param rightOrder Second order to match. /// @param leftOrderTakerAssetFilledAmount Amount of left order already filled. /// @param rightOrderTakerAssetFilledAmount Amount of right order already filled. + /// @param protocolFeeMultiplier The current protocol fee of the exchange contract. + /// @param gasPrice The gasprice of the transaction. This is provided so that the function call can continue + /// to be pure rather than view. /// @param shouldMaximallyFillOrders A value that indicates whether or not this calculation should use /// the maximal fill order matching strategy. /// @param matchedFillResults Amounts to fill and fees to pay by maker and taker of matched orders. @@ -98,10 +109,12 @@ library LibFillResults { LibOrder.Order memory rightOrder, uint256 leftOrderTakerAssetFilledAmount, uint256 rightOrderTakerAssetFilledAmount, + uint256 protocolFeeMultiplier, + uint256 gasPrice, bool shouldMaximallyFillOrders ) internal - view + pure returns (MatchedFillResults memory matchedFillResults) { // Derive maker asset amounts for left & right orders, given store taker assert amounts @@ -163,6 +176,12 @@ library LibFillResults { rightOrder.takerFee ); + // Compute the protocol fee that should be paid for a single fill. In this + // case this should be made the protocol fee for both the left and right orders. + uint256 protocolFee = gasPrice.safeMul(protocolFeeMultiplier); + matchedFillResults.left.protocolFeePaid = protocolFee; + matchedFillResults.right.protocolFeePaid = protocolFee; + // Return fill results return matchedFillResults; } diff --git a/contracts/exchange-libs/contracts/test/TestLibFillResults.sol b/contracts/exchange-libs/contracts/test/TestLibFillResults.sol index 4afa67de6a..bca710057d 100644 --- a/contracts/exchange-libs/contracts/test/TestLibFillResults.sol +++ b/contracts/exchange-libs/contracts/test/TestLibFillResults.sol @@ -29,13 +29,20 @@ contract TestLibFillResults { function calculateFillResults( LibOrder.Order memory order, - uint256 takerAssetFilledAmount + uint256 takerAssetFilledAmount, + uint256 protocolFeeMultiplier, + uint256 gasPrice ) public - view + pure returns (LibFillResults.FillResults memory fillResults) { - fillResults = LibFillResults.calculateFillResults(order, takerAssetFilledAmount); + fillResults = LibFillResults.calculateFillResults( + order, + takerAssetFilledAmount, + protocolFeeMultiplier, + gasPrice + ); return fillResults; } @@ -44,10 +51,12 @@ contract TestLibFillResults { LibOrder.Order memory rightOrder, uint256 leftOrderTakerAssetFilledAmount, uint256 rightOrderTakerAssetFilledAmount, + uint256 protocolFeeMultiplier, + uint256 gasPrice, bool shouldMaximallyFillOrders ) public - view + pure returns (LibFillResults.MatchedFillResults memory matchedFillResults) { matchedFillResults = LibFillResults.calculateMatchedFillResults( @@ -55,6 +64,8 @@ contract TestLibFillResults { rightOrder, leftOrderTakerAssetFilledAmount, rightOrderTakerAssetFilledAmount, + protocolFeeMultiplier, + gasPrice, shouldMaximallyFillOrders ); return matchedFillResults; diff --git a/contracts/exchange-libs/src/reference_functions.ts b/contracts/exchange-libs/src/reference_functions.ts index 346a5cd578..4a79c413e4 100644 --- a/contracts/exchange-libs/src/reference_functions.ts +++ b/contracts/exchange-libs/src/reference_functions.ts @@ -1,4 +1,3 @@ -import { constants } from '@0x/contracts-test-utils'; import { ReferenceFunctions } from '@0x/contracts-utils'; import { LibMathRevertErrors } from '@0x/order-utils'; import { FillResults, OrderWithoutDomain } from '@0x/types'; @@ -94,7 +93,12 @@ export function addFillResults(a: FillResults, b: FillResults): FillResults { /** * Calculates amounts filled and fees paid by maker and taker. */ -export function calculateFillResults(order: OrderWithoutDomain, takerAssetFilledAmount: BigNumber): FillResults { +export function calculateFillResults( + order: OrderWithoutDomain, + takerAssetFilledAmount: BigNumber, + protocolFeeMultiplier: BigNumber, + gasPrice: BigNumber, +): FillResults { const makerAssetFilledAmount = safeGetPartialAmountFloor( takerAssetFilledAmount, order.takerAssetAmount, @@ -107,6 +111,6 @@ export function calculateFillResults(order: OrderWithoutDomain, takerAssetFilled takerAssetFilledAmount, makerFeePaid, takerFeePaid, - protocolFeePaid: constants.ZERO_AMOUNT, // This field is not calculated in `calculateFillResults` as a gas optimization. + protocolFeePaid: safeMul(protocolFeeMultiplier, gasPrice), }; } diff --git a/contracts/exchange-libs/test/lib_fill_results.ts b/contracts/exchange-libs/test/lib_fill_results.ts index 160a4ed766..7f263e5bb3 100644 --- a/contracts/exchange-libs/test/lib_fill_results.ts +++ b/contracts/exchange-libs/test/lib_fill_results.ts @@ -93,6 +93,8 @@ blockchainTests('LibFillResults', env => { return ReferenceFunctions.calculateFillResults( makeOrder(otherAmount, orderTakerAssetAmount, otherAmount, otherAmount), takerAssetFilledAmount, + takerAssetFilledAmount, // Using this so that the gas price is distinct from protocolFeeMultiplier + otherAmount, ); } @@ -102,7 +104,12 @@ blockchainTests('LibFillResults', env => { otherAmount: BigNumber, ): Promise { const order = makeOrder(otherAmount, orderTakerAssetAmount, otherAmount, otherAmount); - return libsContract.calculateFillResults.callAsync(order, takerAssetFilledAmount); + return libsContract.calculateFillResults.callAsync( + order, + takerAssetFilledAmount, + takerAssetFilledAmount, // Using this so that the gas price is distinct from protocolFeeMultiplier + otherAmount, + ); } testCombinatoriallyWithReferenceFunc( @@ -115,6 +122,9 @@ blockchainTests('LibFillResults', env => { describe('explicit tests', () => { const MAX_UINT256_ROOT = constants.MAX_UINT256_ROOT; + const DEFAULT_GAS_PRICE = new BigNumber(200000); + const DEFAULT_PROTOCOL_FEE_MULTIPLIER = new BigNumber(150000); + function makeOrder(details?: Partial): Order { return _.assign({}, EMPTY_ORDER, details); } @@ -127,8 +137,18 @@ blockchainTests('LibFillResults', env => { takerFee: ONE_ETHER.times(0.0025), }); const takerAssetFilledAmount = ONE_ETHER.dividedToIntegerBy(3); - const expected = ReferenceFunctions.calculateFillResults(order, takerAssetFilledAmount); - const actual = await libsContract.calculateFillResults.callAsync(order, takerAssetFilledAmount); + const expected = ReferenceFunctions.calculateFillResults( + order, + takerAssetFilledAmount, + DEFAULT_PROTOCOL_FEE_MULTIPLIER, + DEFAULT_GAS_PRICE, + ); + const actual = await libsContract.calculateFillResults.callAsync( + order, + takerAssetFilledAmount, + DEFAULT_PROTOCOL_FEE_MULTIPLIER, + DEFAULT_GAS_PRICE, + ); expect(actual).to.deep.eq(expected); }); @@ -144,9 +164,14 @@ blockchainTests('LibFillResults', env => { takerAssetFilledAmount, order.makerAssetAmount, ); - return expect(libsContract.calculateFillResults.callAsync(order, takerAssetFilledAmount)).to.revertWith( - expectedError, - ); + return expect( + libsContract.calculateFillResults.callAsync( + order, + takerAssetFilledAmount, + DEFAULT_PROTOCOL_FEE_MULTIPLIER, + DEFAULT_GAS_PRICE, + ), + ).to.revertWith(expectedError); }); it('reverts if computing `fillResults.makerFeePaid` overflows', async () => { @@ -167,9 +192,14 @@ blockchainTests('LibFillResults', env => { makerAssetFilledAmount, order.makerFee, ); - return expect(libsContract.calculateFillResults.callAsync(order, takerAssetFilledAmount)).to.revertWith( - expectedError, - ); + return expect( + libsContract.calculateFillResults.callAsync( + order, + takerAssetFilledAmount, + DEFAULT_PROTOCOL_FEE_MULTIPLIER, + DEFAULT_GAS_PRICE, + ), + ).to.revertWith(expectedError); }); it('reverts if computing `fillResults.takerFeePaid` overflows', async () => { @@ -185,9 +215,14 @@ blockchainTests('LibFillResults', env => { takerAssetFilledAmount, order.takerFee, ); - return expect(libsContract.calculateFillResults.callAsync(order, takerAssetFilledAmount)).to.revertWith( - expectedError, - ); + return expect( + libsContract.calculateFillResults.callAsync( + order, + takerAssetFilledAmount, + DEFAULT_PROTOCOL_FEE_MULTIPLIER, + DEFAULT_GAS_PRICE, + ), + ).to.revertWith(expectedError); }); it('reverts if `order.makerAssetAmount` is 0', async () => { @@ -197,9 +232,14 @@ blockchainTests('LibFillResults', env => { }); const takerAssetFilledAmount = ONE_ETHER; const expectedError = new LibMathRevertErrors.DivisionByZeroError(); - return expect(libsContract.calculateFillResults.callAsync(order, takerAssetFilledAmount)).to.revertWith( - expectedError, - ); + return expect( + libsContract.calculateFillResults.callAsync( + order, + takerAssetFilledAmount, + DEFAULT_PROTOCOL_FEE_MULTIPLIER, + DEFAULT_GAS_PRICE, + ), + ).to.revertWith(expectedError); }); it('reverts if `order.takerAssetAmount` is 0', async () => { @@ -209,9 +249,14 @@ blockchainTests('LibFillResults', env => { }); const takerAssetFilledAmount = ONE_ETHER; const expectedError = new LibMathRevertErrors.DivisionByZeroError(); - return expect(libsContract.calculateFillResults.callAsync(order, takerAssetFilledAmount)).to.revertWith( - expectedError, - ); + return expect( + libsContract.calculateFillResults.callAsync( + order, + takerAssetFilledAmount, + DEFAULT_PROTOCOL_FEE_MULTIPLIER, + DEFAULT_GAS_PRICE, + ), + ).to.revertWith(expectedError); }); it('reverts if there is a rounding error computing `makerAsssetFilledAmount`', async () => { @@ -225,9 +270,14 @@ blockchainTests('LibFillResults', env => { order.takerAssetAmount, order.makerAssetAmount, ); - return expect(libsContract.calculateFillResults.callAsync(order, takerAssetFilledAmount)).to.revertWith( - expectedError, - ); + return expect( + libsContract.calculateFillResults.callAsync( + order, + takerAssetFilledAmount, + DEFAULT_PROTOCOL_FEE_MULTIPLIER, + DEFAULT_GAS_PRICE, + ), + ).to.revertWith(expectedError); }); it('reverts if there is a rounding error computing `makerFeePaid`', async () => { @@ -247,9 +297,91 @@ blockchainTests('LibFillResults', env => { order.makerAssetAmount, order.makerFee, ); - return expect(libsContract.calculateFillResults.callAsync(order, takerAssetFilledAmount)).to.revertWith( - expectedError, + return expect( + libsContract.calculateFillResults.callAsync( + order, + takerAssetFilledAmount, + DEFAULT_PROTOCOL_FEE_MULTIPLIER, + DEFAULT_GAS_PRICE, + ), + ).to.revertWith(expectedError); + }); + + it('reverts if there is a rounding error computing `takerFeePaid`', async () => { + const order = makeOrder({ + makerAssetAmount: ONE_ETHER, + takerAssetAmount: ONE_ETHER, + takerFee: new BigNumber(100), + }); + const takerAssetFilledAmount = order.takerAssetAmount.dividedToIntegerBy(3); + const makerAssetFilledAmount = ReferenceFunctions.getPartialAmountFloor( + takerAssetFilledAmount, + order.takerAssetAmount, + order.makerAssetAmount, ); + const expectedError = new LibMathRevertErrors.RoundingError( + makerAssetFilledAmount, + order.makerAssetAmount, + order.takerFee, + ); + return expect( + libsContract.calculateFillResults.callAsync( + order, + takerAssetFilledAmount, + DEFAULT_PROTOCOL_FEE_MULTIPLIER, + DEFAULT_GAS_PRICE, + ), + ).to.revertWith(expectedError); + }); + + it('reverts if computing `fillResults.protocolFeePaid` overflows', async () => { + const order = makeOrder({ + makerAssetAmount: ONE_ETHER, + takerAssetAmount: ONE_ETHER.times(2), + makerFee: ONE_ETHER.times(0.0023), + takerFee: ONE_ETHER.times(0.0025), + }); + const takerAssetFilledAmount = ONE_ETHER.dividedToIntegerBy(3); + const expectedError = new SafeMathRevertErrors.Uint256BinOpError( + SafeMathRevertErrors.BinOpErrorCodes.MultiplicationOverflow, + DEFAULT_GAS_PRICE, + MAX_UINT256, + ); + return expect( + libsContract.calculateFillResults.callAsync( + order, + takerAssetFilledAmount, + MAX_UINT256, + DEFAULT_GAS_PRICE, + ), + ).to.revertWith(expectedError); + }); + + it('reverts if there is a rounding error computing `makerFeePaid`', async () => { + const order = makeOrder({ + makerAssetAmount: ONE_ETHER, + takerAssetAmount: ONE_ETHER, + makerFee: new BigNumber(100), + }); + const takerAssetFilledAmount = order.takerAssetAmount.dividedToIntegerBy(3); + const makerAssetFilledAmount = ReferenceFunctions.getPartialAmountFloor( + takerAssetFilledAmount, + order.takerAssetAmount, + order.makerAssetAmount, + ); + const expectedError = new LibMathRevertErrors.RoundingError( + makerAssetFilledAmount, + order.makerAssetAmount, + order.makerFee, + ); + return expect( + libsContract.calculateFillResults.callAsync( + order, + takerAssetFilledAmount, + DEFAULT_PROTOCOL_FEE_MULTIPLIER, + DEFAULT_GAS_PRICE, + ), + ).to.revertWith(expectedError); }); }); }); @@ -358,14 +490,14 @@ blockchainTests('LibFillResults', env => { takerAssetFilledAmount: Web3Wrapper.toBaseUnitAmount(10, 18), makerFeePaid: Web3Wrapper.toBaseUnitAmount(100, 16), takerFeePaid: Web3Wrapper.toBaseUnitAmount(100, 16), - protocolFeePaid: constants.ZERO_AMOUNT, + protocolFeePaid: Web3Wrapper.toBaseUnitAmount(15, 4), }, right: { makerAssetFilledAmount: Web3Wrapper.toBaseUnitAmount(10, 18), takerAssetFilledAmount: Web3Wrapper.toBaseUnitAmount(2, 18), makerFeePaid: Web3Wrapper.toBaseUnitAmount(100, 16), takerFeePaid: Web3Wrapper.toBaseUnitAmount(100, 16), - protocolFeePaid: constants.ZERO_AMOUNT, + protocolFeePaid: Web3Wrapper.toBaseUnitAmount(15, 4), }, profitInLeftMakerAsset: Web3Wrapper.toBaseUnitAmount(3, 18), profitInRightMakerAsset: constants.ZERO_AMOUNT, @@ -401,6 +533,8 @@ blockchainTests('LibFillResults', env => { rightOrder, leftOrderTakerAssetFilledAmount, rightOrderTakerAssetFilledAmount, + protocolFeeMultiplier, + gasPrice, false, { from }, ); @@ -1072,6 +1206,8 @@ blockchainTests('LibFillResults', env => { rightOrder, leftOrderTakerAssetFilledAmount, rightOrderTakerAssetFilledAmount, + protocolFeeMultiplier, + gasPrice, true, { from }, ); diff --git a/contracts/exchange/contracts/src/MixinExchangeCore.sol b/contracts/exchange/contracts/src/MixinExchangeCore.sol index a5a828e1e7..157380cd3b 100644 --- a/contracts/exchange/contracts/src/MixinExchangeCore.sol +++ b/contracts/exchange/contracts/src/MixinExchangeCore.sol @@ -217,18 +217,10 @@ contract MixinExchangeCore is uint256 takerAssetFilledAmount = LibSafeMath.min256(takerAssetFillAmount, remainingTakerAssetAmount); // Compute proportional fill amounts - fillResults = LibFillResults.calculateFillResults(order, takerAssetFilledAmount); + fillResults = LibFillResults.calculateFillResults(order, takerAssetFilledAmount, protocolFeeMultiplier, tx.gasprice); bytes32 orderHash = orderInfo.orderHash; - // Settle order - _settleOrder( - orderHash, - order, - takerAddress, - fillResults - ); - // Update exchange internal state _updateFilledState( order, @@ -238,6 +230,14 @@ contract MixinExchangeCore is fillResults ); + // Settle order + _settleOrder( + orderHash, + order, + takerAddress, + fillResults + ); + return fillResults; } @@ -292,8 +292,7 @@ contract MixinExchangeCore is fillResults.takerAssetFilledAmount, fillResults.makerFeePaid, fillResults.takerFeePaid, - fillResults.protocolFeePaid, - msg.value >= fillResults.protocolFeePaid + fillResults.protocolFeePaid ); } @@ -471,8 +470,7 @@ contract MixinExchangeCore is // Calculate the protocol fee that should be paid and populate the `protocolFeePaid` field in `fillResults`. // It's worth noting that we leave this calculation until now so that work isn't wasted if a fee collector // is not registered in the exchange. - uint256 protocolFee = tx.gasprice.safeMul(protocolFeeMultiplier); - fillResults.protocolFeePaid = protocolFee; + uint256 protocolFee = fillResults.protocolFeePaid; // If sufficient ether was sent to the contract, the protocol fee should be paid in ETH. // Otherwise the fee should be paid in WETH. Since the exchange doesn't actually handle @@ -482,6 +480,8 @@ contract MixinExchangeCore is valuePaid = protocolFee; } IStaking(feeCollector).payProtocolFee.value(valuePaid)(order.makerAddress, takerAddress, protocolFee); + } else { + fillResults.protocolFeePaid = 0; } } } diff --git a/contracts/exchange/contracts/src/MixinMatchOrders.sol b/contracts/exchange/contracts/src/MixinMatchOrders.sol index 50a0a30d67..4bc6bd5db2 100644 --- a/contracts/exchange/contracts/src/MixinMatchOrders.sol +++ b/contracts/exchange/contracts/src/MixinMatchOrders.sol @@ -385,19 +385,11 @@ contract MixinMatchOrders is rightOrder, leftOrderInfo.orderTakerAssetFilledAmount, rightOrderInfo.orderTakerAssetFilledAmount, + protocolFeeMultiplier, + tx.gasprice, shouldMaximallyFillOrders ); - // Settle matched orders. Succeeds or throws. - _settleMatchedOrders( - leftOrderInfo.orderHash, - rightOrderInfo.orderHash, - leftOrder, - rightOrder, - takerAddress, - matchedFillResults - ); - // Update exchange state _updateFilledState( leftOrder, @@ -414,6 +406,16 @@ contract MixinMatchOrders is matchedFillResults.right ); + // Settle matched orders. Succeeds or throws. + _settleMatchedOrders( + leftOrderInfo.orderHash, + rightOrderInfo.orderHash, + leftOrder, + rightOrder, + takerAddress, + matchedFillResults + ); + return matchedFillResults; } @@ -492,12 +494,8 @@ contract MixinMatchOrders is // Pay the protocol fees if there is a registered `protocolFeeCollector` address. address feeCollector = protocolFeeCollector; if (feeCollector != address(0)) { - // Calculate the protocol fee that should be paid and populate the `protocolFeePaid` field in the left and - // right `fillResults` of `matchedFillResults`. It's worth noting that we leave this calculation until now - // so that work isn't wasted if a fee collector is not registered in the exchange. - uint256 protocolFee = tx.gasprice.safeMul(protocolFeeMultiplier); - matchedFillResults.left.protocolFeePaid = protocolFee; - matchedFillResults.right.protocolFeePaid = protocolFee; + // Only one of the protocol fees is used because they are identical. + uint256 protocolFee = matchedFillResults.left.protocolFeePaid; // Create a stack variable for the value that will be sent to the feeCollector when `payProtocolFee` is called. // This allows a gas optimization where the `leftOrder.makerAddress` only needs be loaded onto the stack once AND @@ -510,7 +508,7 @@ contract MixinMatchOrders is } IStaking(feeCollector).payProtocolFee.value(valuePaid)(leftOrder.makerAddress, takerAddress, protocolFee); - // Clear value paid for the next call to `payProtocolFee()`. + // Clear the value paid for the next calculation. valuePaid = 0; // Pay the right order's protocol fee. @@ -518,6 +516,9 @@ contract MixinMatchOrders is valuePaid = protocolFee; } IStaking(feeCollector).payProtocolFee.value(valuePaid)(rightOrder.makerAddress, takerAddress, protocolFee); + } else { + matchedFillResults.left.protocolFeePaid = 0; + matchedFillResults.right.protocolFeePaid = 0; } // Settle taker fees. diff --git a/contracts/exchange/contracts/src/MixinProtocolFees.sol b/contracts/exchange/contracts/src/MixinProtocolFees.sol index 0830c2c01a..266d8262a1 100644 --- a/contracts/exchange/contracts/src/MixinProtocolFees.sol +++ b/contracts/exchange/contracts/src/MixinProtocolFees.sol @@ -34,21 +34,21 @@ contract MixinProtocolFees is /// @dev Allows the owner to update the protocol fee multiplier. /// @param updatedProtocolFeeMultiplier The updated protocol fee multiplier. - function updateProtocolFeeMultiplier(uint256 updatedProtocolFeeMultiplier) + function setProtocolFeeMultiplier(uint256 updatedProtocolFeeMultiplier) external onlyOwner { - emit UpdatedProtocolFeeMultiplier(protocolFeeMultiplier, updatedProtocolFeeMultiplier); + emit ProtocolFeeMultiplier(protocolFeeMultiplier, updatedProtocolFeeMultiplier); protocolFeeMultiplier = updatedProtocolFeeMultiplier; } /// @dev Allows the owner to update the protocolFeeCollector address. /// @param updatedProtocolFeeCollector The updated protocolFeeCollector contract address. - function updateProtocolFeeCollectorAddress(address updatedProtocolFeeCollector) + function setProtocolFeeCollectorAddress(address updatedProtocolFeeCollector) external onlyOwner { - emit UpdatedProtocolFeeCollectorAddress(protocolFeeCollector, updatedProtocolFeeCollector); + emit ProtocolFeeCollectorAddress(protocolFeeCollector, updatedProtocolFeeCollector); protocolFeeCollector = updatedProtocolFeeCollector; } } diff --git a/contracts/exchange/contracts/src/MixinWrapperFunctions.sol b/contracts/exchange/contracts/src/MixinWrapperFunctions.sol index 83e15de2dd..377c8e1177 100644 --- a/contracts/exchange/contracts/src/MixinWrapperFunctions.sol +++ b/contracts/exchange/contracts/src/MixinWrapperFunctions.sol @@ -109,7 +109,7 @@ contract MixinWrapperFunctions is public payable nonReentrant - disableRefundUntilEnd + refundFinalBalance returns (LibFillResults.FillResults[] memory fillResults) { uint256 ordersLength = orders.length; @@ -137,7 +137,7 @@ contract MixinWrapperFunctions is public payable nonReentrant - disableRefundUntilEnd + refundFinalBalance returns (LibFillResults.FillResults[] memory fillResults) { uint256 ordersLength = orders.length; @@ -291,7 +291,6 @@ contract MixinWrapperFunctions is ) public payable - disableRefundUntilEnd returns (LibFillResults.FillResults memory fillResults) { fillResults = marketSellOrdersNoThrow(orders, takerAssetFillAmount, signatures); @@ -316,7 +315,6 @@ contract MixinWrapperFunctions is ) public payable - disableRefundUntilEnd returns (LibFillResults.FillResults memory fillResults) { fillResults = marketBuyOrdersNoThrow(orders, makerAssetFillAmount, signatures); @@ -341,6 +339,22 @@ contract MixinWrapperFunctions is } } + /// @dev Fetches information for all passed in orders. + /// @param orders Array of order specifications. + /// @return Array of OrderInfo instances that correspond to each order. + function getOrdersInfo(LibOrder.Order[] memory orders) + public + view + returns (LibOrder.OrderInfo[] memory) + { + uint256 ordersLength = orders.length; + LibOrder.OrderInfo[] memory ordersInfo = new LibOrder.OrderInfo[](ordersLength); + for (uint256 i = 0; i != ordersLength; i++) { + ordersInfo[i] = getOrderInfo(orders[i]); + } + return ordersInfo; + } + /// @dev Fills the input order. Reverts if exact takerAssetFillAmount not filled. /// @param order Order struct containing order specifications. /// @param takerAssetFillAmount Desired amount of takerAsset to sell. @@ -367,20 +381,4 @@ contract MixinWrapperFunctions is } return fillResults; } - - /// @dev Fetches information for all passed in orders. - /// @param orders Array of order specifications. - /// @return Array of OrderInfo instances that correspond to each order. - function getOrdersInfo(LibOrder.Order[] memory orders) - public - view - returns (LibOrder.OrderInfo[] memory) - { - uint256 ordersLength = orders.length; - LibOrder.OrderInfo[] memory ordersInfo = new LibOrder.OrderInfo[](ordersLength); - for (uint256 i = 0; i != ordersLength; i++) { - ordersInfo[i] = getOrderInfo(orders[i]); - } - return ordersInfo; - } } diff --git a/contracts/exchange/contracts/src/interfaces/IExchangeCore.sol b/contracts/exchange/contracts/src/interfaces/IExchangeCore.sol index 3ad5e822f5..9c59ab8ca3 100644 --- a/contracts/exchange/contracts/src/interfaces/IExchangeCore.sol +++ b/contracts/exchange/contracts/src/interfaces/IExchangeCore.sol @@ -25,9 +25,6 @@ import "@0x/contracts-exchange-libs/contracts/src/LibFillResults.sol"; contract IExchangeCore { - // keccak256("Fill(address,address,bytes32,address,address,uint256,uint256,uint256,uint256,uint256,bool,bytes,bytes,bytes,bytes)") - bytes32 internal constant FILL_EVENT_TOPIC = 0x266de417a663e51231ccdf89b2794cea06fde5e2c433d76473160b32d31fd867; - // Fill event is emitted whenever an order is filled. event Fill( address indexed makerAddress, // Address that created the order. @@ -43,8 +40,7 @@ contract IExchangeCore { uint256 takerAssetFilledAmount, // Amount of takerAsset sold by taker and bought by maker. uint256 makerFeePaid, // Amount of makerFeeAssetData paid to feeRecipient by maker. uint256 takerFeePaid, // Amount of takerFeeAssetData paid to feeRecipient by taker. - uint256 protocolFeePaid, // Amount of eth or weth paid to the staking contract. - bool isProtocolFeePaidInWei // Indicates whether the protocol fee is paid in Wei (ETH) or WETH. + uint256 protocolFeePaid // Amount of eth or weth paid to the staking contract. ); // Cancel event is emitted whenever an individual order is cancelled. diff --git a/contracts/exchange/contracts/src/interfaces/IProtocolFees.sol b/contracts/exchange/contracts/src/interfaces/IProtocolFees.sol index ea845c9827..10bd0e2cb4 100644 --- a/contracts/exchange/contracts/src/interfaces/IProtocolFees.sol +++ b/contracts/exchange/contracts/src/interfaces/IProtocolFees.sol @@ -25,18 +25,18 @@ contract IProtocolFees { bytes internal constant WETH_ASSET_DATA = hex"f47261b0"; // Logs updates to the protocol fee multiplier. - event UpdatedProtocolFeeMultiplier(uint256 oldProtocolFeeMultiplier, uint256 updatedProtocolFeeMultiplier); + event ProtocolFeeMultiplier(uint256 oldProtocolFeeMultiplier, uint256 updatedProtocolFeeMultiplier); // Logs updates to the protocolFeeCollector address. - event UpdatedProtocolFeeCollectorAddress(address oldProtocolFeeCollector, address updatedProtocolFeeCollector); + event ProtocolFeeCollectorAddress(address oldProtocolFeeCollector, address updatedProtocolFeeCollector); /// @dev Allows the owner to update the protocol fee multiplier. /// @param updatedProtocolFeeMultiplier The updated protocol fee multiplier. - function updateProtocolFeeMultiplier(uint256 updatedProtocolFeeMultiplier) + function setProtocolFeeMultiplier(uint256 updatedProtocolFeeMultiplier) external; /// @dev Allows the owner to update the protocolFeeCollector address. /// @param updatedProtocolFeeCollector The updated protocolFeeCollector contract address. - function updateProtocolFeeCollectorAddress(address updatedProtocolFeeCollector) + function setProtocolFeeCollectorAddress(address updatedProtocolFeeCollector) external; } diff --git a/contracts/exchange/contracts/src/interfaces/IStakingManager.sol b/contracts/exchange/contracts/src/interfaces/IStakingManager.sol deleted file mode 100644 index 4e0696647c..0000000000 --- a/contracts/exchange/contracts/src/interfaces/IStakingManager.sol +++ /dev/null @@ -1,50 +0,0 @@ -/* - - Copyright 2019 ZeroEx Intl. - - Licensed under the Apache License, Version 2.0 (the "License"); - you may not use this file except in compliance with the License. - You may obtain a copy of the License at - - http://www.apache.org/licenses/LICENSE-2.0 - - Unless required by applicable law or agreed to in writing, software - distributed under the License is distributed on an "AS IS" BASIS, - WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. - See the License for the specific language governing permissions and - limitations under the License. - -*/ - -pragma solidity ^0.5.9; - - -contract IStakingManager { - - // The proxy id of the weth asset proxy. - bytes internal constant WETH_ASSET_DATA = hex"f47261b0"; - - // Logs updates to the protocol fee multiplier. - event UpdatedProtocolFeeMultiplier(uint256 oldProtocolFeeMultiplier, uint256 updatedProtocolFeeMultiplier); - - // Logs updates to the staking address. - event UpdatedStakingAddress(address oldStaking, address updatedStaking); - - // Logs updates to the weth address. - event UpdatedWethAddress(address oldWeth, address updatedWeth); - - /// @dev Allows the owner to update the protocol fee multiplier. - /// @param updatedProtocolFeeMultiplier The updated protocol fee multiplier. - function updateProtocolFeeMultiplier(uint256 updatedProtocolFeeMultiplier) - external; - - /// @dev Allows the owner to update the staking address. - /// @param updatedStaking The updated staking contract address. - function updateStakingAddress(address updatedStaking) - external; - - /// @dev Allows the owner to update the WETH address. - /// @param updatedWeth The updated WETH contract address. - function updateWethAddress(address updatedWeth) - external; -} diff --git a/contracts/exchange/contracts/src/interfaces/IWrapperFunctions.sol b/contracts/exchange/contracts/src/interfaces/IWrapperFunctions.sol index 199a092cae..58bac17db9 100644 --- a/contracts/exchange/contracts/src/interfaces/IWrapperFunctions.sol +++ b/contracts/exchange/contracts/src/interfaces/IWrapperFunctions.sol @@ -156,4 +156,12 @@ contract IWrapperFunctions { /// @param orders Array of order specifications. function batchCancelOrders(LibOrder.Order[] memory orders) public; + + /// @dev Fetches information for all passed in orders + /// @param orders Array of order specifications. + /// @return Array of OrderInfo instances that correspond to each order. + function getOrdersInfo(LibOrder.Order[] memory orders) + public + view + returns (LibOrder.OrderInfo[] memory); } diff --git a/contracts/exchange/contracts/test/TestProtocolFeesReceiver.sol b/contracts/exchange/contracts/test/TestProtocolFeesReceiver.sol index a2e5ca468e..a908f4670f 100644 --- a/contracts/exchange/contracts/test/TestProtocolFeesReceiver.sol +++ b/contracts/exchange/contracts/test/TestProtocolFeesReceiver.sol @@ -15,7 +15,6 @@ limitations under the License. */ - pragma solidity ^0.5.9; pragma experimental ABIEncoderV2; @@ -300,14 +299,6 @@ contract TestProtocolFeesReceiver { // Execute the test. _; - - // Clear exchange state - testProtocolFees.setProtocolFeeCollector(address(0)); - testProtocolFees.setProtocolFeeMultiplier(0); - msg.sender.transfer(address(this).balance); - - // Clear this contract's state - delete testLogs; } /// @dev Constructs an order with a specified maker address. diff --git a/contracts/exchange/package.json b/contracts/exchange/package.json index 566061111a..f5edabd3fd 100644 --- a/contracts/exchange/package.json +++ b/contracts/exchange/package.json @@ -35,7 +35,7 @@ "compile:truffle": "truffle compile" }, "config": { - "abis": "./generated-artifacts/@(Exchange|ExchangeWrapper|IAssetProxy|IAssetProxyDispatcher|IEIP1271Wallet|IExchange|IExchangeCore|IMatchOrders|IProtocolFees|ISignatureValidator|IStakingManager|ITransactions|ITransferSimulator|IWallet|IWrapperFunctions|IsolatedExchange|LibExchangeRichErrorDecoder|MixinAssetProxyDispatcher|MixinExchangeCore|MixinMatchOrders|MixinProtocolFees|MixinSignatureValidator|MixinTransactions|MixinTransferSimulator|MixinWrapperFunctions|ReentrancyTester|TestAssetProxyDispatcher|TestExchangeInternals|TestLibExchangeRichErrorDecoder|TestProtocolFees|TestProtocolFeesReceiver|TestSignatureValidator|TestTransactions|TestValidatorWallet|TestWrapperFunctions|Whitelist).json", + "abis": "./generated-artifacts/@(Exchange|ExchangeWrapper|IAssetProxy|IAssetProxyDispatcher|IEIP1271Wallet|IExchange|IExchangeCore|IMatchOrders|IProtocolFees|ISignatureValidator|ITransactions|ITransferSimulator|IWallet|IWrapperFunctions|IsolatedExchange|LibExchangeRichErrorDecoder|MixinAssetProxyDispatcher|MixinExchangeCore|MixinMatchOrders|MixinProtocolFees|MixinSignatureValidator|MixinTransactions|MixinTransferSimulator|MixinWrapperFunctions|ReentrancyTester|TestAssetProxyDispatcher|TestExchangeInternals|TestLibExchangeRichErrorDecoder|TestProtocolFees|TestProtocolFeesReceiver|TestSignatureValidator|TestTransactions|TestValidatorWallet|TestWrapperFunctions|Whitelist).json", "abis:comment": "This list is auto-generated by contracts-gen. Don't edit manually." }, "repository": { diff --git a/contracts/exchange/src/artifacts.ts b/contracts/exchange/src/artifacts.ts index 1d6f5d30ce..48edf21992 100644 --- a/contracts/exchange/src/artifacts.ts +++ b/contracts/exchange/src/artifacts.ts @@ -16,7 +16,6 @@ import * as IMatchOrders from '../generated-artifacts/IMatchOrders.json'; import * as IProtocolFees from '../generated-artifacts/IProtocolFees.json'; import * as ISignatureValidator from '../generated-artifacts/ISignatureValidator.json'; import * as IsolatedExchange from '../generated-artifacts/IsolatedExchange.json'; -import * as IStakingManager from '../generated-artifacts/IStakingManager.json'; import * as ITransactions from '../generated-artifacts/ITransactions.json'; import * as ITransferSimulator from '../generated-artifacts/ITransferSimulator.json'; import * as IWallet from '../generated-artifacts/IWallet.json'; @@ -61,7 +60,6 @@ export const artifacts = { IMatchOrders: IMatchOrders as ContractArtifact, IProtocolFees: IProtocolFees as ContractArtifact, ISignatureValidator: ISignatureValidator as ContractArtifact, - IStakingManager: IStakingManager as ContractArtifact, ITransactions: ITransactions as ContractArtifact, ITransferSimulator: ITransferSimulator as ContractArtifact, IWallet: IWallet as ContractArtifact, diff --git a/contracts/exchange/src/wrappers.ts b/contracts/exchange/src/wrappers.ts index 9c8b3dffb2..d30ba6386f 100644 --- a/contracts/exchange/src/wrappers.ts +++ b/contracts/exchange/src/wrappers.ts @@ -13,7 +13,6 @@ export * from '../generated-wrappers/i_exchange_core'; export * from '../generated-wrappers/i_match_orders'; export * from '../generated-wrappers/i_protocol_fees'; export * from '../generated-wrappers/i_signature_validator'; -export * from '../generated-wrappers/i_staking_manager'; export * from '../generated-wrappers/i_transactions'; export * from '../generated-wrappers/i_transfer_simulator'; export * from '../generated-wrappers/i_wallet'; diff --git a/contracts/exchange/test/assertion_wrappers/fill_order_wrapper.ts b/contracts/exchange/test/assertion_wrappers/fill_order_wrapper.ts index 1387065bfc..b3f75f84a8 100644 --- a/contracts/exchange/test/assertion_wrappers/fill_order_wrapper.ts +++ b/contracts/exchange/test/assertion_wrappers/fill_order_wrapper.ts @@ -8,6 +8,7 @@ import { artifacts as erc20Artifacts } from '@0x/contracts-erc20'; import { artifacts as erc721Artifacts } from '@0x/contracts-erc721'; import { ReferenceFunctions as LibReferenceFunctions } from '@0x/contracts-exchange-libs'; import { + constants, expect, FillEventArgs, filterLogsToArguments, @@ -58,7 +59,13 @@ export class FillOrderWrapper { const balanceStore = LocalBalanceStore.create(initBalanceStore); const takerAssetFillAmount = opts.takerAssetFillAmount !== undefined ? opts.takerAssetFillAmount : signedOrder.takerAssetAmount; - const fillResults = LibReferenceFunctions.calculateFillResults(signedOrder, takerAssetFillAmount); + // TODO(jalextowle): Change this if the integration tests take protocol fees into account. + const fillResults = LibReferenceFunctions.calculateFillResults( + signedOrder, + takerAssetFillAmount, + constants.ZERO_AMOUNT, + constants.ZERO_AMOUNT, + ); const fillEvent = FillOrderWrapper.simulateFillEvent(signedOrder, takerAddress, fillResults); // Taker -> Maker balanceStore.transferAsset( diff --git a/contracts/exchange/test/codesize.ts b/contracts/exchange/test/codesize.ts new file mode 100644 index 0000000000..e84f3b47a1 --- /dev/null +++ b/contracts/exchange/test/codesize.ts @@ -0,0 +1,18 @@ +import { chaiSetup, getCodesizeFromArtifact } from '@0x/contracts-test-utils'; +import * as chai from 'chai'; + +chaiSetup.configure(); +const expect = chai.expect; + +import { artifacts } from '../src'; + +describe('Contract Size Checks', () => { + const MAX_CODE_SIZE = 24576; + + describe('Exchange', () => { + it('should have a codesize less than the maximum', async () => { + const actualSize = getCodesizeFromArtifact(artifacts.Exchange); + expect(actualSize).to.be.lt(MAX_CODE_SIZE); + }); + }); +}); diff --git a/contracts/exchange/test/internal.ts b/contracts/exchange/test/internal.ts index 887e68e070..2e47b30834 100644 --- a/contracts/exchange/test/internal.ts +++ b/contracts/exchange/test/internal.ts @@ -23,6 +23,8 @@ blockchainTests('Exchange core internal functions', env => { let testExchange: TestExchangeInternalsContract; let logDecoder: LogDecoder; let senderAddress: string; + const DEFAULT_PROTOCOL_MULTIPLIER = new BigNumber(150000); + const DEFAULT_GAS_PRICE = new BigNumber(200000); before(async () => { const accounts = await env.getAccountAddressesAsync(); @@ -169,12 +171,16 @@ blockchainTests('Exchange core internal functions', env => { orderTakerAssetFilledAmount: BigNumber, takerAddress: string, takerAssetFillAmount: BigNumber, - // protocolFeeMultiplier: BigNumber, - // gasPrice: BigNumber, - // isProtocolFeePaidInEth: boolean, + protocolFeeMultiplier: BigNumber, + gasPrice: BigNumber, ): Promise { const orderHash = randomHash(); - const fillResults = LibReferenceFunctions.calculateFillResults(order, takerAssetFillAmount); + const fillResults = LibReferenceFunctions.calculateFillResults( + order, + takerAssetFillAmount, + protocolFeeMultiplier, + gasPrice, + ); const expectedFilledState = orderTakerAssetFilledAmount.plus(takerAssetFillAmount); // const opts = isProtocolFeePaidInEth // ? { value: fillResults.protocolFeePaid } @@ -189,7 +195,6 @@ blockchainTests('Exchange core internal functions', env => { orderHash, orderTakerAssetFilledAmount, fillResults, - // opts, ), ); // Grab the new `filled` state for this order. @@ -209,31 +214,22 @@ blockchainTests('Exchange core internal functions', env => { expect(fillEvent.args.takerAssetFilledAmount).to.bignumber.eq(fillResults.takerAssetFilledAmount); expect(fillEvent.args.makerFeePaid).to.bignumber.eq(fillResults.makerFeePaid); expect(fillEvent.args.takerFeePaid).to.bignumber.eq(fillResults.takerFeePaid); - // expect(fillEvent.args.protocolFeePaid).to.bignumber.eq(fillResults.protocolFeePaid); - // expect(fillEvent.args.isProtocolFeePaidInEth).to.eq(isProtocolFeePaidInEth); expect(fillEvent.args.makerAssetData).to.eq(order.makerAssetData); expect(fillEvent.args.takerAssetData).to.eq(order.takerAssetData); expect(fillEvent.args.makerFeeAssetData).to.eq(order.makerFeeAssetData); expect(fillEvent.args.takerFeeAssetData).to.eq(order.takerFeeAssetData); + expect(fillEvent.args.protocolFeePaid).to.bignumber.eq(fillResults.protocolFeePaid); } - it('emits a `Fill` event and updates `filled` state correctly if protocol fee is paid in ETH', async () => { - const order = makeOrder(); - return testUpdateFilledStateAsync( - order, - order.takerAssetAmount.times(0.1), - randomAddress(), - order.takerAssetAmount.times(0.25), - ); - }); - - it('emits a `Fill` event and updates `filled` state correctly if protocol fee is paid in WETH', async () => { + it('emits a `Fill` event and updates `filled` state correctly', async () => { const order = makeOrder(); return testUpdateFilledStateAsync( order, order.takerAssetAmount.times(0.1), randomAddress(), order.takerAssetAmount.times(0.25), + DEFAULT_PROTOCOL_MULTIPLIER, + DEFAULT_GAS_PRICE, ); }); diff --git a/contracts/exchange/test/isolated_fill_order.ts b/contracts/exchange/test/isolated_fill_order.ts index c60a79bac8..d3327c2d34 100644 --- a/contracts/exchange/test/isolated_fill_order.ts +++ b/contracts/exchange/test/isolated_fill_order.ts @@ -37,6 +37,8 @@ blockchainTests('Isolated fillOrder() tests', env => { makerFeeAssetData: createGoodAssetData(), takerFeeAssetData: createGoodAssetData(), }; + const DEFAULT_GAS_PRICE = new BigNumber(20000); + const DEFAULT_PROTOCOL_FEE_MULTIPLIER = new BigNumber(15000); let takerAddress: string; let notTakerAddress: string; let exchange: IsolatedExchangeWrapper; @@ -69,7 +71,13 @@ blockchainTests('Isolated fillOrder() tests', env => { signature?: string, ): Promise { const orderInfo = await exchange.getOrderInfoAsync(order); - const efr = calculateExpectedFillResults(order, orderInfo, takerAssetFillAmount); + const efr = calculateExpectedFillResults( + order, + orderInfo, + takerAssetFillAmount, + DEFAULT_PROTOCOL_FEE_MULTIPLIER, + DEFAULT_GAS_PRICE, + ); const eoi = calculateExpectedOrderInfo(order, orderInfo, efr); const efb = calculateExpectedFillBalances(order, efr); // Fill the order. @@ -106,11 +114,15 @@ blockchainTests('Isolated fillOrder() tests', env => { order: Order, orderInfo: OrderInfo, takerAssetFillAmount: BigNumber, + protocolFeeMultiplier: BigNumber, + gasPrice: BigNumber, ): FillResults { const remainingTakerAssetAmount = order.takerAssetAmount.minus(orderInfo.orderTakerAssetFilledAmount); return LibReferenceFunctions.calculateFillResults( order, BigNumber.min(takerAssetFillAmount, remainingTakerAssetAmount), + protocolFeeMultiplier, + gasPrice, ); } diff --git a/contracts/exchange/test/protocol_fees.ts b/contracts/exchange/test/protocol_fees.ts index c1709e21d1..5cba8686ff 100644 --- a/contracts/exchange/test/protocol_fees.ts +++ b/contracts/exchange/test/protocol_fees.ts @@ -42,7 +42,7 @@ blockchainTests('Protocol Fee Payments', env => { ); }); - it('should pay protocol fee in WETH when too little value is sent', async () => { + it('should not forward ETH when too little value is sent', async () => { await testProtocolFeesReceiver.testFillOrderProtocolFees.awaitTransactionSuccessAsync( testProtocolFees.address, DEFAULT_PROTOCOL_FEE_MULTIPLIER, @@ -92,7 +92,7 @@ blockchainTests('Protocol Fee Payments', env => { ); }); - it('should pay protocol fee in WETH twice when too little value is sent', async () => { + it('should not forward ETH twice when too little value is sent', async () => { await testProtocolFeesReceiver.testMatchOrdersProtocolFees.awaitTransactionSuccessAsync( testProtocolFees.address, DEFAULT_PROTOCOL_FEE_MULTIPLIER, @@ -104,7 +104,7 @@ blockchainTests('Protocol Fee Payments', env => { ); }); - it('should pay protocol fee in ETH and then WETH when the correct value is sent', async () => { + it('should pay protocol fee in ETH and then not forward ETH when exactly one protocol fee is sent', async () => { await testProtocolFeesReceiver.testMatchOrdersProtocolFees.awaitTransactionSuccessAsync( testProtocolFees.address, DEFAULT_PROTOCOL_FEE_MULTIPLIER, @@ -116,7 +116,7 @@ blockchainTests('Protocol Fee Payments', env => { ); }); - it('should pay protocol fee in ETH when extra value is sent and then pay in WETH', async () => { + it('should pay protocol fee in ETH and then not forward ETH when a bit more than one protocol fee is sent', async () => { await testProtocolFeesReceiver.testMatchOrdersProtocolFees.awaitTransactionSuccessAsync( testProtocolFees.address, DEFAULT_PROTOCOL_FEE_MULTIPLIER, @@ -167,7 +167,7 @@ blockchainTests('Protocol Fee Payments', env => { ); }); - it('should pay one protocol fee in WETH when too little ETH is sent and only one order is in the batch', async () => { + it('should not forward ETH when less than one protocol fee is sent and only one order is in the batch', async () => { await testProtocolFeesReceiver.testBatchFillOrdersProtocolFees.awaitTransactionSuccessAsync( testProtocolFees.address, DEFAULT_PROTOCOL_FEE_MULTIPLIER, @@ -206,7 +206,7 @@ blockchainTests('Protocol Fee Payments', env => { ); }); - it('should pay both protocol fees in WETH when an insuffiecent amount of ETH for one protocol fee is sent', async () => { + it('should not forward ETH twice when an insuffiecent amount of ETH for one protocol fee is sent', async () => { await testProtocolFeesReceiver.testBatchFillOrdersProtocolFees.awaitTransactionSuccessAsync( testProtocolFees.address, DEFAULT_PROTOCOL_FEE_MULTIPLIER, @@ -219,7 +219,7 @@ blockchainTests('Protocol Fee Payments', env => { ); }); - it('should pay a protocol in ETH and then a fee in WETH when exactly one protocol fee in ETH is sent', async () => { + it('should pay a protocol in ETH and not forward ETH for the second when exactly one protocol fee in ETH is sent', async () => { await testProtocolFeesReceiver.testBatchFillOrdersProtocolFees.awaitTransactionSuccessAsync( testProtocolFees.address, DEFAULT_PROTOCOL_FEE_MULTIPLIER, @@ -245,7 +245,7 @@ blockchainTests('Protocol Fee Payments', env => { ); }); - it('should pay two protocol fees in ETH and one in WETH when exactly two protocol fees in ETH is sent', async () => { + it('should pay two protocol fees in ETH and then not forward ETH for a third when exactly two protocol fees in ETH is sent', async () => { await testProtocolFeesReceiver.testBatchFillOrdersProtocolFees.awaitTransactionSuccessAsync( testProtocolFees.address, DEFAULT_PROTOCOL_FEE_MULTIPLIER, diff --git a/contracts/exchange/test/protocol_fees_manager.ts b/contracts/exchange/test/protocol_fees_manager.ts index 9b5af04ab7..d55bb29204 100644 --- a/contracts/exchange/test/protocol_fees_manager.ts +++ b/contracts/exchange/test/protocol_fees_manager.ts @@ -1,18 +1,17 @@ -import { blockchainTests, constants, expect, LogDecoder } from '@0x/contracts-test-utils'; +import { blockchainTests, constants, expect } from '@0x/contracts-test-utils'; import { BigNumber, OwnableRevertErrors } from '@0x/utils'; import { LogWithDecodedArgs } from 'ethereum-types'; import { artifacts, ExchangeContract, - ExchangeUpdatedProtocolFeeCollectorAddressEventArgs, - ExchangeUpdatedProtocolFeeMultiplierEventArgs, + ExchangeProtocolFeeCollectorAddressEventArgs, + ExchangeProtocolFeeMultiplierEventArgs, } from '../src'; blockchainTests.resets('MixinProtocolFees', env => { let accounts: string[]; let exchange: ExchangeContract; - let logDecoder: LogDecoder; let nonOwner: string; let owner: string; let protocolFeeCollector: string; @@ -37,60 +36,59 @@ blockchainTests.resets('MixinProtocolFees', env => { {}, new BigNumber(1337), ); - - // Configure the log decoder - logDecoder = new LogDecoder(env.web3Wrapper, artifacts); }); - blockchainTests.resets('updateProtocolFeeMultiplier', () => { + blockchainTests.resets('setProtocolFeeMultiplier', () => { it('should revert if msg.sender != owner', async () => { const expectedError = new OwnableRevertErrors.OnlyOwnerError(nonOwner, owner); // Ensure that the transaction reverts with the expected rich error. - const tx = exchange.updateProtocolFeeCollectorAddress.sendTransactionAsync(protocolFeeCollector, { + const tx = exchange.setProtocolFeeCollectorAddress.sendTransactionAsync(protocolFeeCollector, { from: nonOwner, }); return expect(tx).to.revertWith(expectedError); }); - it('should succeed and emit an UpdatedProtocolFeeMultiplier event if msg.sender == owner', async () => { - // Call the `updateProtocolFeeMultiplier()` function and get the receipt. - const receipt = await logDecoder.getTxWithDecodedLogsAsync( - await exchange.updateProtocolFeeMultiplier.sendTransactionAsync(protocolFeeMultiplier, { + it('should succeed and emit an ProtocolFeeMultiplier event if msg.sender == owner', async () => { + // Call the `setProtocolFeeMultiplier()` function and get the receipt. + const receipt = await exchange.setProtocolFeeMultiplier.awaitTransactionSuccessAsync( + protocolFeeMultiplier, + { from: owner, - }), + }, ); // Verify that the protocolFeeCollector address was actually updated to the correct address. const updated = await exchange.protocolFeeMultiplier.callAsync(); expect(updated).bignumber.to.be.eq(protocolFeeMultiplier); - // Ensure that the correct `UpdatedProtocolFeeCollectorAddress` event was logged. + // Ensure that the correct `ProtocolFeeCollectorAddress` event was logged. // tslint:disable:no-unnecessary-type-assertion - const updatedEvent = receipt.logs[0] as LogWithDecodedArgs; - expect(updatedEvent.event).to.be.eq('UpdatedProtocolFeeMultiplier'); + const updatedEvent = receipt.logs[0] as LogWithDecodedArgs; + expect(updatedEvent.event).to.be.eq('ProtocolFeeMultiplier'); expect(updatedEvent.args.oldProtocolFeeMultiplier).bignumber.to.be.eq(constants.ZERO_AMOUNT); expect(updatedEvent.args.updatedProtocolFeeMultiplier).bignumber.to.be.eq(protocolFeeMultiplier); }); }); - blockchainTests.resets('updateProtocolFeeCollectorAddress', () => { + blockchainTests.resets('setProtocolFeeCollectorAddress', () => { it('should revert if msg.sender != owner', async () => { const expectedError = new OwnableRevertErrors.OnlyOwnerError(nonOwner, owner); // Ensure that the transaction reverts with the expected rich error. - const tx = exchange.updateProtocolFeeCollectorAddress.sendTransactionAsync(protocolFeeCollector, { + const tx = exchange.setProtocolFeeCollectorAddress.sendTransactionAsync(protocolFeeCollector, { from: nonOwner, }); return expect(tx).to.revertWith(expectedError); }); - it('should succeed and emit an UpdatedProtocolFeeCollectorAddress event if msg.sender == owner', async () => { - // Call the `updateProtocolFeeCollectorAddress()` function and get the receipt. - const receipt = await logDecoder.getTxWithDecodedLogsAsync( - await exchange.updateProtocolFeeCollectorAddress.sendTransactionAsync(protocolFeeCollector, { + it('should succeed and emit an ProtocolFeeCollectorAddress event if msg.sender == owner', async () => { + // Call the `setProtocolFeeCollectorAddress()` function and get the receipt. + const receipt = await exchange.setProtocolFeeCollectorAddress.awaitTransactionSuccessAsync( + protocolFeeCollector, + { from: owner, - }), + }, ); // Verify that the protocolFeeCollector address was actually updated to the correct address. @@ -99,10 +97,8 @@ blockchainTests.resets('MixinProtocolFees', env => { // Ensure that the correct `UpdatedProtocolFeeCollectorAddress` event was logged. // tslint:disable:no-unnecessary-type-assertion - const updatedEvent = receipt.logs[0] as LogWithDecodedArgs< - ExchangeUpdatedProtocolFeeCollectorAddressEventArgs - >; - expect(updatedEvent.event).to.be.eq('UpdatedProtocolFeeCollectorAddress'); + const updatedEvent = receipt.logs[0] as LogWithDecodedArgs; + expect(updatedEvent.event).to.be.eq('ProtocolFeeCollectorAddress'); expect(updatedEvent.args.oldProtocolFeeCollector).to.be.eq(constants.NULL_ADDRESS); expect(updatedEvent.args.updatedProtocolFeeCollector).to.be.eq(protocolFeeCollector); }); diff --git a/contracts/exchange/test/reference_functions.ts b/contracts/exchange/test/reference_functions.ts index 1a82ed6d50..596d9aee88 100644 --- a/contracts/exchange/test/reference_functions.ts +++ b/contracts/exchange/test/reference_functions.ts @@ -7,6 +7,8 @@ import * as _ from 'lodash'; describe('Reference functions', () => { const ONE_ETHER = constants.ONE_ETHER; + const DEFAULT_GAS_PRICE = new BigNumber(2); + const DEFAULT_PROTOCOL_FEE_MULTIPLIER = new BigNumber(150); const EMPTY_ORDER: Order = { senderAddress: constants.NULL_ADDRESS, makerAddress: constants.NULL_ADDRESS, @@ -42,9 +44,14 @@ describe('Reference functions', () => { takerAssetFilledAmount, order.makerAssetAmount, ); - return expect(() => LibReferenceFunctions.calculateFillResults(order, takerAssetFilledAmount)).to.throw( - expectedError.message, - ); + return expect(() => + LibReferenceFunctions.calculateFillResults( + order, + takerAssetFilledAmount, + DEFAULT_PROTOCOL_FEE_MULTIPLIER, + DEFAULT_GAS_PRICE, + ), + ).to.throw(expectedError.message); }); it('reverts if computing `fillResults.makerFeePaid` overflows', () => { @@ -65,9 +72,14 @@ describe('Reference functions', () => { makerAssetFilledAmount, order.makerFee, ); - return expect(() => LibReferenceFunctions.calculateFillResults(order, takerAssetFilledAmount)).to.throw( - expectedError.message, - ); + return expect(() => + LibReferenceFunctions.calculateFillResults( + order, + takerAssetFilledAmount, + DEFAULT_PROTOCOL_FEE_MULTIPLIER, + DEFAULT_GAS_PRICE, + ), + ).to.throw(expectedError.message); }); it('reverts if computing `fillResults.takerFeePaid` overflows', () => { @@ -83,9 +95,14 @@ describe('Reference functions', () => { takerAssetFilledAmount, order.takerFee, ); - return expect(() => LibReferenceFunctions.calculateFillResults(order, takerAssetFilledAmount)).to.throw( - expectedError.message, - ); + return expect(() => + LibReferenceFunctions.calculateFillResults( + order, + takerAssetFilledAmount, + DEFAULT_PROTOCOL_FEE_MULTIPLIER, + DEFAULT_GAS_PRICE, + ), + ).to.throw(expectedError.message); }); it('reverts if `order.makerAssetAmount` is 0', () => { @@ -95,9 +112,14 @@ 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, + DEFAULT_PROTOCOL_FEE_MULTIPLIER, + DEFAULT_GAS_PRICE, + ), + ).to.throw(expectedError.message); }); it('reverts if `order.takerAssetAmount` is 0', () => { @@ -107,9 +129,14 @@ 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, + DEFAULT_PROTOCOL_FEE_MULTIPLIER, + DEFAULT_GAS_PRICE, + ), + ).to.throw(expectedError.message); }); it('reverts if there is a rounding error computing `makerAsssetFilledAmount`', () => { @@ -123,9 +150,14 @@ describe('Reference functions', () => { order.takerAssetAmount, order.makerAssetAmount, ); - return expect(() => LibReferenceFunctions.calculateFillResults(order, takerAssetFilledAmount)).to.throw( - expectedError.message, - ); + return expect(() => + LibReferenceFunctions.calculateFillResults( + order, + takerAssetFilledAmount, + DEFAULT_PROTOCOL_FEE_MULTIPLIER, + DEFAULT_GAS_PRICE, + ), + ).to.throw(expectedError.message); }); it('reverts if there is a rounding error computing `makerFeePaid`', () => { @@ -145,9 +177,14 @@ describe('Reference functions', () => { order.makerAssetAmount, order.makerFee, ); - return expect(() => LibReferenceFunctions.calculateFillResults(order, takerAssetFilledAmount)).to.throw( - expectedError.message, - ); + return expect(() => + LibReferenceFunctions.calculateFillResults( + order, + takerAssetFilledAmount, + DEFAULT_PROTOCOL_FEE_MULTIPLIER, + DEFAULT_GAS_PRICE, + ), + ).to.throw(expectedError.message); }); it('reverts if there is a rounding error computing `takerFeePaid`', () => { @@ -167,9 +204,36 @@ describe('Reference functions', () => { order.makerAssetAmount, order.takerFee, ); - return expect(() => LibReferenceFunctions.calculateFillResults(order, takerAssetFilledAmount)).to.throw( - expectedError.message, + return expect(() => + LibReferenceFunctions.calculateFillResults( + order, + takerAssetFilledAmount, + DEFAULT_PROTOCOL_FEE_MULTIPLIER, + DEFAULT_GAS_PRICE, + ), + ).to.throw(expectedError.message); + }); + + it('reverts if there is an overflow computing `protocolFeePaid`', () => { + const order = makeOrder({ + makerAssetAmount: ONE_ETHER, + takerAssetAmount: ONE_ETHER, + takerFee: constants.ZERO_AMOUNT, + }); + const takerAssetFilledAmount = order.takerAssetAmount.dividedToIntegerBy(3); + const expectedError = new SafeMathRevertErrors.Uint256BinOpError( + SafeMathRevertErrors.BinOpErrorCodes.MultiplicationOverflow, + MAX_UINT256_ROOT, + MAX_UINT256_ROOT, ); + return expect(() => + LibReferenceFunctions.calculateFillResults( + order, + takerAssetFilledAmount, + MAX_UINT256_ROOT, + MAX_UINT256_ROOT, + ), + ).to.throw(expectedError.message); }); }); }); diff --git a/contracts/exchange/test/utils/constants.ts b/contracts/exchange/test/utils/constants.ts index cd566974f3..8a1948a8d9 100644 --- a/contracts/exchange/test/utils/constants.ts +++ b/contracts/exchange/test/utils/constants.ts @@ -9,8 +9,8 @@ export const constants = { ExchangeFunctionName.RegisterAssetProxy, ExchangeFunctionName.SimulateDispatchTransferFromCalls, ExchangeFunctionName.TransferOwnership, - ExchangeFunctionName.UpdateProtocolFeeMultiplier, - ExchangeFunctionName.UpdateProtocolFeeCollectorAddress, + ExchangeFunctionName.SetProtocolFeeMultiplier, + ExchangeFunctionName.SetProtocolFeeCollectorAddress, ], SINGLE_FILL_FN_NAMES: [ ExchangeFunctionName.FillOrder, diff --git a/contracts/exchange/test/utils/types.ts b/contracts/exchange/test/utils/types.ts index 92d65116b6..b8a4827109 100644 --- a/contracts/exchange/test/utils/types.ts +++ b/contracts/exchange/test/utils/types.ts @@ -32,6 +32,6 @@ export enum ExchangeFunctionName { SetSignatureValidatorApproval = 'setSignatureValidatorApproval', SimulateDispatchTransferFromCalls = 'simulateDispatchTransferFromCalls', TransferOwnership = 'transferOwnership', - UpdateProtocolFeeMultiplier = 'updateProtocolFeeMultiplier', - UpdateProtocolFeeCollectorAddress = 'updateProtocolFeeCollectorAddress', + SetProtocolFeeMultiplier = 'setProtocolFeeMultiplier', + SetProtocolFeeCollectorAddress = 'setProtocolFeeCollectorAddress', } diff --git a/contracts/exchange/test/wrapper.ts b/contracts/exchange/test/wrapper.ts index a4e4af9b00..4e76b24caf 100644 --- a/contracts/exchange/test/wrapper.ts +++ b/contracts/exchange/test/wrapper.ts @@ -91,7 +91,7 @@ blockchainTests.resets('Exchange wrappers', env => { ); // Set the protocol fee multiplier of the exchange - await exchange.updateProtocolFeeMultiplier.awaitTransactionSuccessAsync(PROTOCOL_FEE_MULTIPLIER, { + await exchange.setProtocolFeeMultiplier.awaitTransactionSuccessAsync(PROTOCOL_FEE_MULTIPLIER, { from: owner, }); diff --git a/contracts/exchange/test/wrapper_unit_tests.ts b/contracts/exchange/test/wrapper_unit_tests.ts index 5d0d995e33..6eab3939eb 100644 --- a/contracts/exchange/test/wrapper_unit_tests.ts +++ b/contracts/exchange/test/wrapper_unit_tests.ts @@ -62,7 +62,7 @@ blockchainTests('Exchange wrapper functions unit tests.', env => { ); // Set the protocol fee multiplier. - await testContract.updateProtocolFeeMultiplier.awaitTransactionSuccessAsync(protocolFeeMultiplier, { + await testContract.setProtocolFeeMultiplier.awaitTransactionSuccessAsync(protocolFeeMultiplier, { from: owner, }); }); @@ -1320,50 +1320,50 @@ blockchainTests('Exchange wrapper functions unit tests.', env => { const tx = txHelper.getResultAndReceiptAsync(testContract.batchCancelOrders, orders); return expect(tx).to.revertWith(expectedError); }); + }); - describe('getOrdersInfo', () => { - // Computes the expected (fake) order info generated by the `TestWrapperFunctions` contract. - function getExpectedOrderInfo(order: Order): OrderInfo { - const MAX_ORDER_STATUS = OrderStatus.Cancelled as number; - return { - orderHash: getExpectedOrderHash(order), - // Lower uint128 of `order.salt` is the `orderTakerAssetFilledAmount`. - orderTakerAssetFilledAmount: order.salt.mod(new BigNumber(2).pow(128)), - // High byte of `order.salt` is the `orderStatus`. - orderStatus: - order.salt.dividedToIntegerBy(new BigNumber(2).pow(248)).toNumber() % (MAX_ORDER_STATUS + 1), - }; - } + describe('getOrdersInfo', () => { + // Computes the expected (fake) order info generated by the `TestWrapperFunctions` contract. + function getExpectedOrderInfo(order: Order): OrderInfo { + const MAX_ORDER_STATUS = OrderStatus.Cancelled as number; + return { + orderHash: getExpectedOrderHash(order), + // Lower uint128 of `order.salt` is the `orderTakerAssetFilledAmount`. + orderTakerAssetFilledAmount: order.salt.mod(new BigNumber(2).pow(128)), + // High byte of `order.salt` is the `orderStatus`. + orderStatus: + order.salt.dividedToIntegerBy(new BigNumber(2).pow(248)).toNumber() % (MAX_ORDER_STATUS + 1), + }; + } - it('works with no orders', async () => { - const infos = await testContract.getOrdersInfo.callAsync([]); - expect(infos.length).to.eq(0); - }); + it('works with no orders', async () => { + const infos = await testContract.getOrdersInfo.callAsync([]); + expect(infos.length).to.eq(0); + }); - it('works with one order', async () => { - const orders = [randomOrder()]; - const expectedResult = orders.map(getExpectedOrderInfo); - const actualResult = await testContract.getOrdersInfo.callAsync(orders); - expect(actualResult).to.deep.eq(expectedResult); - }); + it('works with one order', async () => { + const orders = [randomOrder()]; + const expectedResult = orders.map(getExpectedOrderInfo); + const actualResult = await testContract.getOrdersInfo.callAsync(orders); + expect(actualResult).to.deep.eq(expectedResult); + }); - it('works with many orders', async () => { - const NUM_ORDERS = 16; - const orders = _.times(NUM_ORDERS, () => randomOrder()); - const expectedResult = orders.map(getExpectedOrderInfo); - const actualResult = await testContract.getOrdersInfo.callAsync(orders); - expect(actualResult).to.deep.eq(expectedResult); - }); + it('works with many orders', async () => { + const NUM_ORDERS = 16; + const orders = _.times(NUM_ORDERS, () => randomOrder()); + const expectedResult = orders.map(getExpectedOrderInfo); + const actualResult = await testContract.getOrdersInfo.callAsync(orders); + expect(actualResult).to.deep.eq(expectedResult); + }); - it('works with duplicate orders', async () => { - const NUM_UNIQUE_ORDERS = 4; - const CLONE_COUNT = 2; - const uniqueOrders = _.times(NUM_UNIQUE_ORDERS, () => randomOrder()); - const orders = _.shuffle(_.flatten(_.times(CLONE_COUNT, () => uniqueOrders))); - const expectedResult = orders.map(getExpectedOrderInfo); - const actualResult = await testContract.getOrdersInfo.callAsync(orders); - expect(actualResult).to.deep.eq(expectedResult); - }); + it('works with duplicate orders', async () => { + const NUM_UNIQUE_ORDERS = 4; + const CLONE_COUNT = 2; + const uniqueOrders = _.times(NUM_UNIQUE_ORDERS, () => randomOrder()); + const orders = _.shuffle(_.flatten(_.times(CLONE_COUNT, () => uniqueOrders))); + const expectedResult = orders.map(getExpectedOrderInfo); + const actualResult = await testContract.getOrdersInfo.callAsync(orders); + expect(actualResult).to.deep.eq(expectedResult); }); }); }); diff --git a/contracts/exchange/tsconfig.json b/contracts/exchange/tsconfig.json index bc05581348..a7b8ad4daf 100644 --- a/contracts/exchange/tsconfig.json +++ b/contracts/exchange/tsconfig.json @@ -13,7 +13,6 @@ "generated-artifacts/IMatchOrders.json", "generated-artifacts/IProtocolFees.json", "generated-artifacts/ISignatureValidator.json", - "generated-artifacts/IStakingManager.json", "generated-artifacts/ITransactions.json", "generated-artifacts/ITransferSimulator.json", "generated-artifacts/IWallet.json", diff --git a/contracts/staking/contracts/src/fees/MixinExchangeFees.sol b/contracts/staking/contracts/src/fees/MixinExchangeFees.sol index 1835d4ccda..f0ddd560c0 100644 --- a/contracts/staking/contracts/src/fees/MixinExchangeFees.sol +++ b/contracts/staking/contracts/src/fees/MixinExchangeFees.sol @@ -56,6 +56,7 @@ contract MixinExchangeFees is { using LibSafeMath for uint256; + /// TODO(jalextowle): Add WETH to protocol fees. Should this be unwrapped? /// @dev Pays a protocol fee in ETH. /// Only a known 0x exchange can call this method. See (MixinExchangeManager). /// @param makerAddress The address of the order's maker. diff --git a/contracts/staking/test/utils/Simulation.ts b/contracts/staking/test/utils/Simulation.ts index 316c986e5c..1db33021ef 100644 --- a/contracts/staking/test/utils/Simulation.ts +++ b/contracts/staking/test/utils/Simulation.ts @@ -191,7 +191,13 @@ export class Simulation { const feeAmount = p.protocolFeesByMaker[i]; // TODO(jalextowle): I'll need to fix this once I make my PR on protocol fees. The arguments // I'm adding are just placeholders for now. - await this._stakingWrapper.payProtocolFeeAsync(makerAddress, makerAddress, feeAmount, feeAmount, p.exchangeAddress); + await this._stakingWrapper.payProtocolFeeAsync( + makerAddress, + makerAddress, + feeAmount, + feeAmount, + p.exchangeAddress, + ); } // validate fees per pool let expectedTotalFeesThisEpoch = new BigNumber(0); diff --git a/contracts/test-utils/src/codesize.ts b/contracts/test-utils/src/codesize.ts new file mode 100644 index 0000000000..1f0beb6681 --- /dev/null +++ b/contracts/test-utils/src/codesize.ts @@ -0,0 +1,8 @@ +import { ContractArtifact } from 'ethereum-types'; + +/** + * Get the codesize of a provided artifact. + */ +export function getCodesizeFromArtifact(artifact: ContractArtifact): number { + return (artifact.compilerOutput.evm.bytecode.object.length - 2) / 2; +} diff --git a/contracts/test-utils/src/index.ts b/contracts/test-utils/src/index.ts index f6eac0048d..06fa5cec92 100644 --- a/contracts/test-utils/src/index.ts +++ b/contracts/test-utils/src/index.ts @@ -48,3 +48,4 @@ export { } from './types'; export { blockchainTests, BlockchainTestsEnvironment, describe } from './mocha_blockchain'; export { chaiSetup, expect } from './chai_setup'; +export { getCodesizeFromArtifact } from './codesize'; diff --git a/contracts/utils/contracts/src/Refundable.sol b/contracts/utils/contracts/src/Refundable.sol index 15eea9e31e..091c62aed6 100644 --- a/contracts/utils/contracts/src/Refundable.sol +++ b/contracts/utils/contracts/src/Refundable.sol @@ -27,7 +27,7 @@ contract Refundable { modifier refundFinalBalance { _; if (!shouldNotRefund) { - refundNonzeroBalance(); + _refundNonzeroBalance(); } } @@ -38,11 +38,11 @@ contract Refundable { shouldNotRefund = true; _; shouldNotRefund = false; - refundNonzeroBalance(); + _refundNonzeroBalance(); } } - function refundNonzeroBalance() + function _refundNonzeroBalance() internal { uint256 balance = address(this).balance; diff --git a/contracts/utils/contracts/test/TestRefundable.sol b/contracts/utils/contracts/test/TestRefundable.sol index 86f31fd3d7..a86cd67cab 100644 --- a/contracts/utils/contracts/test/TestRefundable.sol +++ b/contracts/utils/contracts/test/TestRefundable.sol @@ -28,7 +28,7 @@ contract TestRefundable is external payable { - refundNonzeroBalance(); + _refundNonzeroBalance(); } function setShouldNotRefund(bool shouldNotRefundNew)