diff --git a/contracts/zero-ex/CHANGELOG.json b/contracts/zero-ex/CHANGELOG.json index 24658a09de..db20cf6126 100644 --- a/contracts/zero-ex/CHANGELOG.json +++ b/contracts/zero-ex/CHANGELOG.json @@ -37,6 +37,10 @@ { "note": "Do not try to pull all tokens if selling all ETH in `TransformERC20Feature`", "pr": 46 + }, + { + "note": "Remove protocol fees from all RFQ orders and add `taker` field to RFQ orders", + "pr": 45 } ] }, diff --git a/contracts/zero-ex/contracts/src/features/INativeOrdersFeature.sol b/contracts/zero-ex/contracts/src/features/INativeOrdersFeature.sol index 8b83f9e414..b5c239adcf 100644 --- a/contracts/zero-ex/contracts/src/features/INativeOrdersFeature.sol +++ b/contracts/zero-ex/contracts/src/features/INativeOrdersFeature.sol @@ -56,7 +56,6 @@ interface INativeOrdersFeature { /// @param taker The taker of the order. /// @param takerTokenFilledAmount How much taker token was filled. /// @param makerTokenFilledAmount How much maker token was filled. - /// @param protocolFeePaid How much protocol fee was paid. /// @param pool The fee pool associated with this order. event RfqOrderFilled( bytes32 orderHash, @@ -66,7 +65,6 @@ interface INativeOrdersFeature { address takerToken, uint128 takerTokenFilledAmount, uint128 makerTokenFilledAmount, - uint256 protocolFeePaid, bytes32 pool ); @@ -126,8 +124,7 @@ interface INativeOrdersFeature { returns (uint128 takerTokenFilledAmount, uint128 makerTokenFilledAmount); /// @dev Fill an RFQ order for up to `takerTokenFillAmount` taker tokens. - /// The taker will be the caller. ETH should be attached to pay the - /// protocol fee. + /// The taker will be the caller. /// @param order The RFQ order. /// @param signature The order signature. /// @param takerTokenFillAmount Maximum taker token amount to fill this order with. @@ -139,7 +136,6 @@ interface INativeOrdersFeature { uint128 takerTokenFillAmount ) external - payable returns (uint128 takerTokenFilledAmount, uint128 makerTokenFilledAmount); /// @dev Fill an RFQ order for exactly `takerTokenFillAmount` taker tokens. @@ -160,9 +156,7 @@ interface INativeOrdersFeature { returns (uint128 makerTokenFilledAmount); /// @dev Fill an RFQ order for exactly `takerTokenFillAmount` taker tokens. - /// The taker will be the caller. ETH protocol fees can be - /// attached to this call. Any unspent ETH will be refunded to - /// the caller. + /// The taker will be the caller. /// @param order The RFQ order. /// @param signature The order signature. /// @param takerTokenFillAmount How much taker token to fill this order with. @@ -173,7 +167,6 @@ interface INativeOrdersFeature { uint128 takerTokenFillAmount ) external - payable returns (uint128 makerTokenFilledAmount); /// @dev Fill a limit order. Internal variant. ETH protocol fees can be @@ -197,9 +190,7 @@ interface INativeOrdersFeature { payable returns (uint128 takerTokenFilledAmount, uint128 makerTokenFilledAmount); - /// @dev Fill an RFQ order. Internal variant. ETH protocol fees can be - /// attached to this call. Any unspent ETH will be refunded to - /// `msg.sender` (not `sender`). + /// @dev Fill an RFQ order. Internal variant. /// @param order The RFQ order. /// @param signature The order signature. /// @param takerTokenFillAmount Maximum taker token to fill this order with. @@ -213,7 +204,6 @@ interface INativeOrdersFeature { address taker ) external - payable returns (uint128 takerTokenFilledAmount, uint128 makerTokenFilledAmount); /// @dev Cancel a single limit order. The caller must be the maker. diff --git a/contracts/zero-ex/contracts/src/features/NativeOrdersFeature.sol b/contracts/zero-ex/contracts/src/features/NativeOrdersFeature.sol index 0108aef96f..a75e2d576d 100644 --- a/contracts/zero-ex/contracts/src/features/NativeOrdersFeature.sol +++ b/contracts/zero-ex/contracts/src/features/NativeOrdersFeature.sol @@ -210,7 +210,6 @@ contract NativeOrdersFeature is ) public override - payable returns (uint128 takerTokenFilledAmount, uint128 makerTokenFilledAmount) { FillNativeOrderResults memory results = @@ -220,7 +219,6 @@ contract NativeOrdersFeature is takerTokenFillAmount, msg.sender ); - _refundExcessProtocolFeeToSender(results.ethProtocolFeePaid); (takerTokenFilledAmount, makerTokenFilledAmount) = ( results.takerTokenFilledAmount, results.makerTokenFilledAmount @@ -280,7 +278,6 @@ contract NativeOrdersFeature is ) public override - payable returns (uint128 makerTokenFilledAmount) { FillNativeOrderResults memory results = @@ -298,7 +295,6 @@ contract NativeOrdersFeature is takerTokenFillAmount ).rrevert(); } - _refundExcessProtocolFeeToSender(results.ethProtocolFeePaid); makerTokenFilledAmount = results.makerTokenFilledAmount; } @@ -359,7 +355,6 @@ contract NativeOrdersFeature is public virtual override - payable onlySelf returns (uint128 takerTokenFilledAmount, uint128 makerTokenFilledAmount) { @@ -370,7 +365,6 @@ contract NativeOrdersFeature is takerTokenFillAmount, taker ); - _refundExcessProtocolFeeToSender(results.ethProtocolFeePaid); (takerTokenFilledAmount, makerTokenFilledAmount) = ( results.takerTokenFilledAmount, results.makerTokenFilledAmount @@ -898,6 +892,15 @@ contract NativeOrdersFeature is } } + // Must be fillable by the taker. + if (order.taker != address(0) && order.taker != taker) { + LibNativeOrdersRichErrors.OrderNotFillableByTakerError( + orderInfo.orderHash, + taker, + order.taker + ).rrevert(); + } + // Signature must be valid for the order. { address signer = LibSignature.getSignerOfHash(orderInfo.orderHash, signature); @@ -910,9 +913,6 @@ contract NativeOrdersFeature is } } - // Pay the protocol fee. - results.ethProtocolFeePaid = _collectProtocolFee(order.pool); - // Settle between the maker and taker. (results.takerTokenFilledAmount, results.makerTokenFilledAmount) = _settleOrder( SettleOrderInfo({ @@ -936,7 +936,6 @@ contract NativeOrdersFeature is address(order.takerToken), results.takerTokenFilledAmount, results.makerTokenFilledAmount, - results.ethProtocolFeePaid, order.pool ); } diff --git a/contracts/zero-ex/contracts/src/features/libs/LibNativeOrder.sol b/contracts/zero-ex/contracts/src/features/libs/LibNativeOrder.sol index d1298e6744..d5924e069c 100644 --- a/contracts/zero-ex/contracts/src/features/libs/LibNativeOrder.sol +++ b/contracts/zero-ex/contracts/src/features/libs/LibNativeOrder.sol @@ -56,6 +56,7 @@ library LibNativeOrder { uint128 makerAmount; uint128 takerAmount; address maker; + address taker; address txOrigin; bytes32 pool; uint64 expiry; @@ -102,13 +103,14 @@ library LibNativeOrder { // "uint128 takerAmount,", // "address maker,", // "address txOrigin,", + // "address taker,", // "bytes32 pool,", // "uint64 expiry,", // "uint256 salt" // ")" // )) uint256 private constant _RFQ_ORDER_TYPEHASH = - 0xc6b3034376598bc7f28b05e81db7ed88486dcdb6b4a6c7300353fffc5f31f382; + 0xe593d3fdfa8b60e5e17a1b2204662ecbe15c23f2084b9ad5bae40359540a7da9; /// @dev Get the struct hash of a limit order. /// @param order The limit order. @@ -181,6 +183,7 @@ library LibNativeOrder { // order.makerAmount, // order.takerAmount, // order.maker, + // order.taker, // order.txOrigin, // order.pool, // order.expiry, @@ -199,15 +202,17 @@ library LibNativeOrder { mstore(add(mem, 0x80), and(UINT_128_MASK, mload(add(order, 0x60)))) // order.maker; mstore(add(mem, 0xA0), and(ADDRESS_MASK, mload(add(order, 0x80)))) - // order.txOrigin; + // order.taker; mstore(add(mem, 0xC0), and(ADDRESS_MASK, mload(add(order, 0xA0)))) + // order.txOrigin; + mstore(add(mem, 0xE0), and(ADDRESS_MASK, mload(add(order, 0xC0)))) // order.pool; - mstore(add(mem, 0xE0), mload(add(order, 0xC0))) + mstore(add(mem, 0x100), mload(add(order, 0xE0))) // order.expiry; - mstore(add(mem, 0x100), and(UINT_64_MASK, mload(add(order, 0xE0)))) + mstore(add(mem, 0x120), and(UINT_64_MASK, mload(add(order, 0x100)))) // order.salt; - mstore(add(mem, 0x120), mload(add(order, 0x100))) - structHash := keccak256(mem, 0x140) + mstore(add(mem, 0x140), mload(add(order, 0x120))) + structHash := keccak256(mem, 0x160) } } } diff --git a/contracts/zero-ex/contracts/test/TestMetaTransactionsNativeOrdersFeature.sol b/contracts/zero-ex/contracts/test/TestMetaTransactionsNativeOrdersFeature.sol index f60ff429c4..6ba6992e7e 100644 --- a/contracts/zero-ex/contracts/test/TestMetaTransactionsNativeOrdersFeature.sol +++ b/contracts/zero-ex/contracts/test/TestMetaTransactionsNativeOrdersFeature.sol @@ -87,7 +87,6 @@ contract TestMetaTransactionsNativeOrdersFeature is ) public override - payable returns (uint128 takerTokenFilledAmount, uint128 makerTokenFilledAmount) { emit FillRfqOrderCalled( diff --git a/contracts/zero-ex/src/orders.ts b/contracts/zero-ex/src/orders.ts index 6a57e37a51..76531248eb 100644 --- a/contracts/zero-ex/src/orders.ts +++ b/contracts/zero-ex/src/orders.ts @@ -20,6 +20,7 @@ const COMMON_ORDER_DEFAULT_VALUES = { makerAmount: ZERO, takerAmount: ZERO, maker: NULL_ADDRESS, + taker: NULL_ADDRESS, pool: hexUtils.leftPad(0), expiry: ZERO, salt: ZERO, @@ -29,7 +30,6 @@ const COMMON_ORDER_DEFAULT_VALUES = { const LIMIT_ORDER_DEFAULT_VALUES = { ...COMMON_ORDER_DEFAULT_VALUES, takerTokenFeeAmount: ZERO, - taker: NULL_ADDRESS, sender: NULL_ADDRESS, feeRecipient: NULL_ADDRESS, }; @@ -63,6 +63,7 @@ export abstract class OrderBase { public takerAmount: BigNumber; public maker: string; public pool: string; + public taker: string; public expiry: BigNumber; public salt: BigNumber; public chainId: number; @@ -75,6 +76,7 @@ export abstract class OrderBase { this.makerAmount = _fields.makerAmount; this.takerAmount = _fields.takerAmount; this.maker = _fields.maker; + this.taker = _fields.taker; this.pool = _fields.pool; this.expiry = _fields.expiry; this.salt = _fields.salt; @@ -142,7 +144,6 @@ export class LimitOrder extends OrderBase { ); public takerTokenFeeAmount: BigNumber; - public taker: string; public sender: string; public feeRecipient: string; @@ -150,7 +151,6 @@ export class LimitOrder extends OrderBase { const _fields = { ...LIMIT_ORDER_DEFAULT_VALUES, ...fields }; super(_fields); this.takerTokenFeeAmount = _fields.takerTokenFeeAmount; - this.taker = _fields.taker; this.sender = _fields.sender; this.feeRecipient = _fields.feeRecipient; } @@ -246,6 +246,7 @@ export class RfqOrder extends OrderBase { 'uint128 makerAmount', 'uint128 takerAmount', 'address maker', + 'address taker', 'address txOrigin', 'bytes32 pool', 'uint64 expiry', @@ -272,6 +273,7 @@ export class RfqOrder extends OrderBase { makerAmount: this.makerAmount, takerAmount: this.takerAmount, maker: this.maker, + taker: this.taker, txOrigin: this.txOrigin, pool: this.pool, expiry: this.expiry, @@ -291,6 +293,7 @@ export class RfqOrder extends OrderBase { hexUtils.leftPad(this.makerAmount), hexUtils.leftPad(this.takerAmount), hexUtils.leftPad(this.maker), + hexUtils.leftPad(this.taker), hexUtils.leftPad(this.txOrigin), hexUtils.leftPad(this.pool), hexUtils.leftPad(this.expiry), @@ -309,6 +312,7 @@ export class RfqOrder extends OrderBase { { type: 'uint128', name: 'makerAmount' }, { type: 'uint128', name: 'takerAmount' }, { type: 'address', name: 'maker' }, + { type: 'address', name: 'taker' }, { type: 'address', name: 'txOrigin' }, { type: 'bytes32', name: 'pool' }, { type: 'uint64', name: 'expiry' }, @@ -323,6 +327,7 @@ export class RfqOrder extends OrderBase { makerAmount: this.makerAmount.toString(10), takerAmount: this.takerAmount.toString(10), maker: this.maker, + taker: this.taker, txOrigin: this.txOrigin, pool: this.pool, expiry: this.expiry.toString(10), diff --git a/contracts/zero-ex/test/features/meta_transactions_test.ts b/contracts/zero-ex/test/features/meta_transactions_test.ts index da1bd5488f..f937ab2e95 100644 --- a/contracts/zero-ex/test/features/meta_transactions_test.ts +++ b/contracts/zero-ex/test/features/meta_transactions_test.ts @@ -197,11 +197,12 @@ blockchainTests.resets('MetaTransactions feature', env => { const fillAmount = new BigNumber(23456); const mtx = getRandomMetaTransaction({ callData: nativeOrdersFeature.fillRfqOrder(order, sig, fillAmount).getABIEncodedTransactionData(), + value: ZERO_AMOUNT, }); const signature = await signMetaTransactionAsync(mtx); const callOpts = { gasPrice: mtx.minGasPrice, - value: mtx.value, + value: 0, }; const rawResult = await feature.executeMetaTransaction(mtx, signature).callAsync(callOpts); expect(rawResult).to.eq(RAW_ORDER_SUCCESS_RESULT); diff --git a/contracts/zero-ex/test/features/native_orders_feature_test.ts b/contracts/zero-ex/test/features/native_orders_feature_test.ts index ba04feb165..11ce6ea181 100644 --- a/contracts/zero-ex/test/features/native_orders_feature_test.ts +++ b/contracts/zero-ex/test/features/native_orders_feature_test.ts @@ -128,13 +128,22 @@ blockchainTests.resets('NativeOrdersFeature', env => { async function fillLimitOrderAsync( order: LimitOrder, - fillAmount: BigNumber | number = order.takerAmount, - _taker: string = taker, + opts: Partial<{ + fillAmount: BigNumber | number; + taker: string; + protocolFee?: BigNumber | number; + }> = {}, ): Promise { + const { fillAmount, taker: _taker, protocolFee } = { + taker, + fillAmount: order.takerAmount, + ...opts, + }; await prepareBalancesForOrderAsync(order, _taker); + const _protocolFee = protocolFee === undefined ? SINGLE_PROTOCOL_FEE : protocolFee; return zeroEx .fillLimitOrder(order, await order.getSignatureWithProviderAsync(env.provider), new BigNumber(fillAmount)) - .awaitTransactionSuccessAsync({ from: _taker, value: SINGLE_PROTOCOL_FEE }); + .awaitTransactionSuccessAsync({ from: _taker, value: _protocolFee }); } describe('getLimitOrderInfo()', () => { @@ -200,7 +209,7 @@ blockchainTests.resets('NativeOrdersFeature', env => { const order = getTestLimitOrder(); const fillAmount = order.takerAmount.minus(1); // Fill the order first. - await fillLimitOrderAsync(order, fillAmount); + await fillLimitOrderAsync(order, { fillAmount }); const info = await zeroEx.getLimitOrderInfo(order).callAsync(); assertOrderInfoEquals(info, { status: OrderStatus.Fillable, @@ -226,7 +235,7 @@ blockchainTests.resets('NativeOrdersFeature', env => { const order = getTestLimitOrder(); const fillAmount = order.takerAmount.minus(1); // Fill the order first. - await fillLimitOrderAsync(order, fillAmount); + await fillLimitOrderAsync(order, { fillAmount }); await zeroEx.cancelLimitOrder(order).awaitTransactionSuccessAsync({ from: maker }); const info = await zeroEx.getLimitOrderInfo(order).callAsync(); assertOrderInfoEquals(info, { @@ -245,7 +254,7 @@ blockchainTests.resets('NativeOrdersFeature', env => { await prepareBalancesForOrderAsync(order, _taker); return zeroEx .fillRfqOrder(order, await order.getSignatureWithProviderAsync(env.provider), new BigNumber(fillAmount)) - .awaitTransactionSuccessAsync({ from: _taker, value: SINGLE_PROTOCOL_FEE }); + .awaitTransactionSuccessAsync({ from: _taker }); } describe('getRfqOrderInfo()', () => { @@ -287,9 +296,7 @@ blockchainTests.resets('NativeOrdersFeature', env => { await prepareBalancesForOrderAsync(order); const sig = await order.getSignatureWithProviderAsync(env.provider); // Fill the order first. - await zeroEx - .fillRfqOrder(order, sig, order.takerAmount) - .awaitTransactionSuccessAsync({ from: taker, value: SINGLE_PROTOCOL_FEE }); + await zeroEx.fillRfqOrder(order, sig, order.takerAmount).awaitTransactionSuccessAsync({ from: taker }); // Advance time to expire the order. await env.web3Wrapper.increaseTimeAsync(61); const info = await zeroEx.getRfqOrderInfo(order).callAsync(); @@ -381,7 +388,7 @@ blockchainTests.resets('NativeOrdersFeature', env => { it('can cancel a partially filled order', async () => { const order = getTestLimitOrder(); - await fillLimitOrderAsync(order, order.takerAmount.minus(1)); + await fillLimitOrderAsync(order, { fillAmount: order.takerAmount.minus(1) }); const receipt = await zeroEx.cancelLimitOrder(order).awaitTransactionSuccessAsync({ from: maker }); verifyEventsFromLogs( receipt.logs, @@ -748,6 +755,7 @@ blockchainTests.resets('NativeOrdersFeature', env => { takerTokenFilledAmount, takerTokenFeeFilledAmount, } = computeLimitOrderFilledAmounts(order, takerTokenFillAmount, takerTokenAlreadyFilledAmount); + const protocolFee = order.taker !== NULL_ADDRESS ? ZERO_AMOUNT : SINGLE_PROTOCOL_FEE; return { taker, takerTokenFilledAmount, @@ -758,16 +766,25 @@ blockchainTests.resets('NativeOrdersFeature', env => { feeRecipient: order.feeRecipient, makerToken: order.makerToken, takerToken: order.takerToken, - protocolFeePaid: SINGLE_PROTOCOL_FEE, + protocolFeePaid: protocolFee, pool: order.pool, }; } async function assertExpectedFinalBalancesFromLimitOrderFillAsync( order: LimitOrder, - takerTokenFillAmount: BigNumber = order.takerAmount, - takerTokenAlreadyFilledAmount: BigNumber = ZERO_AMOUNT, + opts: Partial<{ + takerTokenFillAmount: BigNumber; + takerTokenAlreadyFilledAmount: BigNumber; + receipt: TransactionReceiptWithDecodedLogs; + }> = {}, ): Promise { + const { takerTokenFillAmount, takerTokenAlreadyFilledAmount, receipt } = { + takerTokenFillAmount: order.takerAmount, + takerTokenAlreadyFilledAmount: ZERO_AMOUNT, + receipt: undefined, + ...opts, + }; const { makerTokenFilledAmount, takerTokenFilledAmount, @@ -779,6 +796,13 @@ blockchainTests.resets('NativeOrdersFeature', env => { expect(makerBalance).to.bignumber.eq(takerTokenFilledAmount); expect(takerBalance).to.bignumber.eq(makerTokenFilledAmount); expect(feeRecipientBalance).to.bignumber.eq(takerTokenFeeFilledAmount); + if (receipt) { + const balanceOfTakerNow = await env.web3Wrapper.getBalanceInWeiAsync(taker); + const balanceOfTakerBefore = await env.web3Wrapper.getBalanceInWeiAsync(taker, receipt.blockNumber - 1); + const protocolFee = order.taker === NULL_ADDRESS ? SINGLE_PROTOCOL_FEE : 0; + const totalCost = GAS_PRICE.times(receipt.gasUsed).plus(protocolFee); + expect(balanceOfTakerBefore.minus(totalCost)).to.bignumber.eq(balanceOfTakerNow); + } } describe('fillLimitOrder()', () => { @@ -795,13 +819,13 @@ blockchainTests.resets('NativeOrdersFeature', env => { status: OrderStatus.Filled, takerTokenFilledAmount: order.takerAmount, }); - await assertExpectedFinalBalancesFromLimitOrderFillAsync(order); + await assertExpectedFinalBalancesFromLimitOrderFillAsync(order, { receipt }); }); it('can partially fill an order', async () => { const order = getTestLimitOrder(); const fillAmount = order.takerAmount.minus(1); - const receipt = await fillLimitOrderAsync(order, fillAmount); + const receipt = await fillLimitOrderAsync(order, { fillAmount }); verifyEventsFromLogs( receipt.logs, [createLimitOrderFilledEventArgs(order, fillAmount)], @@ -812,13 +836,13 @@ blockchainTests.resets('NativeOrdersFeature', env => { status: OrderStatus.Fillable, takerTokenFilledAmount: fillAmount, }); - await assertExpectedFinalBalancesFromLimitOrderFillAsync(order, fillAmount); + await assertExpectedFinalBalancesFromLimitOrderFillAsync(order, { takerTokenFillAmount: fillAmount }); }); it('can fully fill an order in two steps', async () => { const order = getTestLimitOrder(); let fillAmount = order.takerAmount.dividedToIntegerBy(2); - let receipt = await fillLimitOrderAsync(order, fillAmount); + let receipt = await fillLimitOrderAsync(order, { fillAmount }); verifyEventsFromLogs( receipt.logs, [createLimitOrderFilledEventArgs(order, fillAmount)], @@ -826,7 +850,7 @@ blockchainTests.resets('NativeOrdersFeature', env => { ); const alreadyFilledAmount = fillAmount; fillAmount = order.takerAmount.minus(fillAmount); - receipt = await fillLimitOrderAsync(order, fillAmount); + receipt = await fillLimitOrderAsync(order, { fillAmount }); verifyEventsFromLogs( receipt.logs, [createLimitOrderFilledEventArgs(order, fillAmount, alreadyFilledAmount)], @@ -842,7 +866,7 @@ blockchainTests.resets('NativeOrdersFeature', env => { it('clamps fill amount to remaining available', async () => { const order = getTestLimitOrder(); const fillAmount = order.takerAmount.plus(1); - const receipt = await fillLimitOrderAsync(order, fillAmount); + const receipt = await fillLimitOrderAsync(order, { fillAmount }); verifyEventsFromLogs( receipt.logs, [createLimitOrderFilledEventArgs(order, fillAmount)], @@ -853,13 +877,13 @@ blockchainTests.resets('NativeOrdersFeature', env => { status: OrderStatus.Filled, takerTokenFilledAmount: order.takerAmount, }); - await assertExpectedFinalBalancesFromLimitOrderFillAsync(order, fillAmount); + await assertExpectedFinalBalancesFromLimitOrderFillAsync(order, { takerTokenFillAmount: fillAmount }); }); it('clamps fill amount to remaining available in partial filled order', async () => { const order = getTestLimitOrder(); let fillAmount = order.takerAmount.dividedToIntegerBy(2); - let receipt = await fillLimitOrderAsync(order, fillAmount); + let receipt = await fillLimitOrderAsync(order, { fillAmount }); verifyEventsFromLogs( receipt.logs, [createLimitOrderFilledEventArgs(order, fillAmount)], @@ -867,7 +891,7 @@ blockchainTests.resets('NativeOrdersFeature', env => { ); const alreadyFilledAmount = fillAmount; fillAmount = order.takerAmount.minus(fillAmount).plus(1); - receipt = await fillLimitOrderAsync(order, fillAmount); + receipt = await fillLimitOrderAsync(order, { fillAmount }); verifyEventsFromLogs( receipt.logs, [createLimitOrderFilledEventArgs(order, fillAmount, alreadyFilledAmount)], @@ -910,7 +934,7 @@ blockchainTests.resets('NativeOrdersFeature', env => { it('non-taker cannot fill order', async () => { const order = getTestLimitOrder({ taker }); - const tx = fillLimitOrderAsync(order, order.takerAmount, notTaker); + const tx = fillLimitOrderAsync(order, { fillAmount: order.takerAmount, taker: notTaker }); return expect(tx).to.revertWith( new RevertErrors.OrderNotFillableByTakerError(order.getHash(), notTaker, order.taker), ); @@ -918,7 +942,7 @@ blockchainTests.resets('NativeOrdersFeature', env => { it('non-sender cannot fill order', async () => { const order = getTestLimitOrder({ sender: taker }); - const tx = fillLimitOrderAsync(order, order.takerAmount, notTaker); + const tx = fillLimitOrderAsync(order, { fillAmount: order.takerAmount, taker: notTaker }); return expect(tx).to.revertWith( new RevertErrors.OrderNotFillableBySenderError(order.getHash(), notTaker, order.sender), ); @@ -934,7 +958,7 @@ blockchainTests.resets('NativeOrdersFeature', env => { ); }); - it('fails if no protocol fee attached (and no weth allowance)', async () => { + it('fails if no protocol fee attached', async () => { const order = getTestLimitOrder(); await prepareBalancesForOrderAsync(order); const tx = zeroEx @@ -948,6 +972,17 @@ blockchainTests.resets('NativeOrdersFeature', env => { // token spender fallthroigh, so we won't get too specific. return expect(tx).to.revertWith(new AnyRevertError()); }); + + it('refunds excess protocol fee', async () => { + const order = getTestLimitOrder(); + const receipt = await fillLimitOrderAsync(order, { protocolFee: SINGLE_PROTOCOL_FEE.plus(1) }); + verifyEventsFromLogs( + receipt.logs, + [createLimitOrderFilledEventArgs(order)], + IZeroExEvents.LimitOrderFilled, + ); + await assertExpectedFinalBalancesFromLimitOrderFillAsync(order, { receipt }); + }); }); interface RfqOrderFilledAmounts { @@ -993,7 +1028,6 @@ blockchainTests.resets('NativeOrdersFeature', env => { maker: order.maker, makerToken: order.makerToken, takerToken: order.takerToken, - protocolFeePaid: SINGLE_PROTOCOL_FEE, pool: order.pool, }; } @@ -1170,6 +1204,14 @@ blockchainTests.resets('NativeOrdersFeature', env => { ); }); + it('non-taker cannot fill order', async () => { + const order = getTestRfqOrder({ taker, txOrigin: notTaker }); + const tx = fillRfqOrderAsync(order, order.takerAmount, notTaker); + return expect(tx).to.revertWith( + new RevertErrors.OrderNotFillableByTakerError(order.getHash(), notTaker, order.taker), + ); + }); + it('cannot fill an expired order', async () => { const order = getTestRfqOrder({ expiry: createExpiry(-60) }); const tx = fillRfqOrderAsync(order); @@ -1208,19 +1250,14 @@ blockchainTests.resets('NativeOrdersFeature', env => { ); }); - it('fails if no protocol fee attached (and no weth allowance)', async () => { + it('fails if ETH is attached', async () => { const order = getTestRfqOrder(); - await prepareBalancesForOrderAsync(order); + await prepareBalancesForOrderAsync(order, taker); const tx = zeroEx - .fillRfqOrder( - order, - await order.getSignatureWithProviderAsync(env.provider), - new BigNumber(order.takerAmount), - ) - .awaitTransactionSuccessAsync({ from: taker, value: ZERO_AMOUNT }); - // The exact revert error depends on whether we are still doing a - // token spender fallthrough, so we won't get too specific. - return expect(tx).to.revertWith(new AnyRevertError()); + .fillRfqOrder(order, await order.getSignatureWithProviderAsync(env.provider), order.takerAmount) + .awaitTransactionSuccessAsync({ from: taker, value: 1 }); + // This will revert at the language level because the fill function is not payable. + return expect(tx).to.be.rejectedWith('revert'); }); }); @@ -1249,6 +1286,18 @@ blockchainTests.resets('NativeOrdersFeature', env => { new RevertErrors.FillOrKillFailedError(order.getHash(), order.takerAmount, fillAmount), ); }); + + it('refunds excess protocol fee', async () => { + const order = getTestLimitOrder(); + await prepareBalancesForOrderAsync(order); + const takerBalanceBefore = await env.web3Wrapper.getBalanceInWeiAsync(taker); + const receipt = await zeroEx + .fillOrKillLimitOrder(order, await order.getSignatureWithProviderAsync(env.provider), order.takerAmount) + .awaitTransactionSuccessAsync({ from: taker, value: SINGLE_PROTOCOL_FEE.plus(1) }); + const takerBalanceAfter = await env.web3Wrapper.getBalanceInWeiAsync(taker); + const totalCost = GAS_PRICE.times(receipt.gasUsed).plus(SINGLE_PROTOCOL_FEE); + expect(takerBalanceBefore.minus(totalCost)).to.bignumber.eq(takerBalanceAfter); + }); }); describe('fillOrKillRfqOrder()', () => { @@ -1257,7 +1306,7 @@ blockchainTests.resets('NativeOrdersFeature', env => { await prepareBalancesForOrderAsync(order); const receipt = await zeroEx .fillOrKillRfqOrder(order, await order.getSignatureWithProviderAsync(env.provider), order.takerAmount) - .awaitTransactionSuccessAsync({ from: taker, value: SINGLE_PROTOCOL_FEE }); + .awaitTransactionSuccessAsync({ from: taker }); verifyEventsFromLogs(receipt.logs, [createRfqOrderFilledEventArgs(order)], IZeroExEvents.RfqOrderFilled); }); @@ -1267,11 +1316,21 @@ blockchainTests.resets('NativeOrdersFeature', env => { const fillAmount = order.takerAmount.plus(1); const tx = zeroEx .fillOrKillRfqOrder(order, await order.getSignatureWithProviderAsync(env.provider), fillAmount) - .awaitTransactionSuccessAsync({ from: taker, value: SINGLE_PROTOCOL_FEE }); + .awaitTransactionSuccessAsync({ from: taker }); return expect(tx).to.revertWith( new RevertErrors.FillOrKillFailedError(order.getHash(), order.takerAmount, fillAmount), ); }); + + it('fails if ETH is attached', async () => { + const order = getTestRfqOrder(); + await prepareBalancesForOrderAsync(order); + const tx = zeroEx + .fillOrKillRfqOrder(order, await order.getSignatureWithProviderAsync(env.provider), order.takerAmount) + .awaitTransactionSuccessAsync({ from: taker, value: 1 }); + // This will revert at the language level because the fill function is not payable. + return expect(tx).to.be.rejectedWith('revert'); + }); }); it.skip('RFQ gas benchmark', async () => { diff --git a/docs/basics/orders.rst b/docs/basics/orders.rst index c595665304..53434873ee 100644 --- a/docs/basics/orders.rst +++ b/docs/basics/orders.rst @@ -282,7 +282,7 @@ RFQ orders are a stripped down version of standard limit orders, supporting fewe Some notable differences from regular limit orders are: -* The only fill restrictions that can be placed on an RFQ order is on the ``tx.origin`` of the transaction. +* The only fill restrictions that can be placed on an RFQ order is on the ``tx.origin`` and ``taker`` of the transaction. * There are no taker token fees. Structure @@ -303,6 +303,8 @@ The ``RFQOrder`` struct has the following fields: +-----------------+-------------+-----------------------------------------------------------------------------+ | ``maker`` | ``address`` | The address of the maker, and signer, of this order. | +-----------------+-------------+-----------------------------------------------------------------------------+ +| ``taker`` | ``address`` | Allowed taker address. Set to zero to allow any taker. | ++-----------------+-------------+-----------------------------------------------------------------------------+ | ``txOrigin`` | ``address`` | The allowed address of the EOA that submitted the Ethereum transaction. | +-----------------+-------------+-----------------------------------------------------------------------------+ | ``pool`` | ``bytes32`` | The staking pool to attribute the 0x protocol fee from this order. | @@ -348,6 +350,7 @@ The hash of the order is used to uniquely identify an order inside the protocol. 'uint128 makerAmount,', 'uint128 takerAmount,', 'address maker,' + 'address taker,' 'address txOrigin,' 'bytes32 pool,', 'uint64 expiry,', @@ -359,6 +362,7 @@ The hash of the order is used to uniquely identify an order inside the protocol. order.makerAmount, order.takerAmount, order.maker, + order.taker, order.txOrigin, order.pool, order.expiry, @@ -421,7 +425,6 @@ RFQ orders can be filled with the ``fillRfqOrder()`` or ``fillOrKillRfqOrder()`` uint128 takerTokenFillAmount ) external - payable // How much maker token from the order the taker received. returns (uint128 takerTokenFillAmount, uint128 makerTokenFillAmount); @@ -438,7 +441,6 @@ RFQ orders can be filled with the ``fillRfqOrder()`` or ``fillOrKillRfqOrder()`` uint128 takerTokenFillAmount ) external - payable // How much maker token from the order the taker received. returns (uint128 makerTokenFillAmount);