Remove protocol fees from RFQ orders (#45)

* `@0x/contracts-zero-ex`: Remove protocol fees from native orders in certain scenarios.

* `@0x/contracts-zero-ex`: update changelog

* `@0x/contracts-zero-ex`: Add `taker` field to RFQ orders.`
`@0x/contracts-zero-ex`: Introduce protocol fees to all limit orders again.

* `@0x/contracts-zero-ex`: Rebase.

Co-authored-by: Lawrence Forman <me@merklejerk.com>

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
             }
         ]
     },

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
+    ///      The taker will be the caller.
-    ///      protocol fee.
     /// @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
+    ///      The taker will be the caller.
-    ///      attached to this call. Any unspent ETH will be refunded to
-    ///      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
+    /// @dev Fill an RFQ order. Internal variant.
-    ///      attached to this call. Any unspent ETH will be refunded to
-    ///      `msg.sender` (not `sender`).
     /// @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.

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
         );
     }

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)))
+            mstore(add(mem, 0x140), mload(add(order, 0x120)))
-            structHash := keccak256(mem, 0x140)
+            structHash := keccak256(mem, 0x160)
         }
     }
 }

contracts/zero-ex/contracts/test/TestMetaTransactionsNativeOrdersFeature.sol

@@ -87,7 +87,6 @@ contract TestMetaTransactionsNativeOrdersFeature is
     )
         public
         override
-        payable
         returns (uint128 takerTokenFilledAmount, uint128 makerTokenFilledAmount)
     {
         emit FillRfqOrderCalled(

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),

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);

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,
+        opts: Partial<{
-        _taker: string = taker,
+            fillAmount: BigNumber | number;
+            taker: string;
+            protocolFee?: BigNumber | number;
+        }> = {},
     ): Promise<TransactionReceiptWithDecodedLogs> {
+        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
+            await zeroEx.fillRfqOrder(order, sig, order.takerAmount).awaitTransactionSuccessAsync({ from: taker });
-                .fillRfqOrder(order, sig, order.takerAmount)
-                .awaitTransactionSuccessAsync({ from: taker, value: SINGLE_PROTOCOL_FEE });
             // 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,
+        opts: Partial<{
-        takerTokenAlreadyFilledAmount: BigNumber = ZERO_AMOUNT,
+            takerTokenFillAmount: BigNumber;
+            takerTokenAlreadyFilledAmount: BigNumber;
+            receipt: TransactionReceiptWithDecodedLogs;
+        }> = {},
     ): Promise<void> {
+        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(
+                .fillRfqOrder(order, await order.getSignatureWithProviderAsync(env.provider), order.takerAmount)
-                    order,
+                .awaitTransactionSuccessAsync({ from: taker, value: 1 });
-                    await order.getSignatureWithProviderAsync(env.provider),
+            // This will revert at the language level because the fill function is not payable.
-                    new BigNumber(order.takerAmount),
+            return expect(tx).to.be.rejectedWith('revert');
-                )
-                .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());
         });
     });
 
@@ -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 () => {

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);