Merge pull request #793 from 0xProject/feature/contracts/transferFromFallback

Optimize transferFrom (saves ~2000 gas /fill)

2 files changed, 136 insertions, 92 deletions

diff --git a/packages/contracts/test/asset_proxy/proxies.ts b/packages/contracts/test/asset_proxy/proxies.ts
index 39121d95c..08026331f 100644
--- a/packages/contracts/test/asset_proxy/proxies.ts
+++ b/packages/contracts/test/asset_proxy/proxies.ts
@@ -15,6 +15,7 @@ import {
 import { DummyERC721TokenContract } from '../../src/generated_contract_wrappers/dummy_e_r_c721_token';
 import { ERC20ProxyContract } from '../../src/generated_contract_wrappers/e_r_c20_proxy';
 import { ERC721ProxyContract } from '../../src/generated_contract_wrappers/e_r_c721_proxy';
+import { IAssetProxyContract } from '../../src/generated_contract_wrappers/i_asset_proxy';
 import { artifacts } from '../../src/utils/artifacts';
 import { expectRevertReasonOrAlwaysFailingTransactionAsync } from '../../src/utils/assertions';
 import { chaiSetup } from '../../src/utils/chai_setup';
@@ -27,6 +28,11 @@ import { provider, txDefaults, web3Wrapper } from '../../src/utils/web3_wrapper'
 chaiSetup.configure();
 const expect = chai.expect;
 const blockchainLifecycle = new BlockchainLifecycle(web3Wrapper);
+const assetProxyInterface = new IAssetProxyContract(
+    artifacts.IAssetProxy.compilerOutput.abi,
+    constants.NULL_ADDRESS,
+    provider,
+);
 
 // tslint:disable:no-unnecessary-type-assertion
 describe('Asset Transfer Proxies', () => {
@@ -105,14 +111,18 @@ describe('Asset Transfer Proxies', () => {
                 // Perform a transfer from makerAddress to takerAddress
                 const erc20Balances = await erc20Wrapper.getBalancesAsync();
                 const amount = new BigNumber(10);
+                const data = assetProxyInterface.transferFrom.getABIEncodedTransactionData(
+                    encodedAssetData,
+                    makerAddress,
+                    takerAddress,
+                    amount,
+                );
                 await web3Wrapper.awaitTransactionSuccessAsync(
-                    await erc20Proxy.transferFrom.sendTransactionAsync(
-                        encodedAssetData,
-                        makerAddress,
-                        takerAddress,
-                        amount,
-                        { from: exchangeAddress },
-                    ),
+                    await web3Wrapper.sendTransactionAsync({
+                        to: erc20Proxy.address,
+                        data,
+                        from: exchangeAddress,
+                    }),
                     constants.AWAIT_TRANSACTION_MINED_MS,
                 );
                 // Verify transfer was successful
@@ -131,14 +141,18 @@ describe('Asset Transfer Proxies', () => {
                 // Perform a transfer from makerAddress to takerAddress
                 const erc20Balances = await erc20Wrapper.getBalancesAsync();
                 const amount = new BigNumber(0);
+                const data = assetProxyInterface.transferFrom.getABIEncodedTransactionData(
+                    encodedAssetData,
+                    makerAddress,
+                    takerAddress,
+                    amount,
+                );
                 await web3Wrapper.awaitTransactionSuccessAsync(
-                    await erc20Proxy.transferFrom.sendTransactionAsync(
-                        encodedAssetData,
-                        makerAddress,
-                        takerAddress,
-                        amount,
-                        { from: exchangeAddress },
-                    ),
+                    await web3Wrapper.sendTransactionAsync({
+                        to: erc20Proxy.address,
+                        data,
+                        from: exchangeAddress,
+                    }),
                     constants.AWAIT_TRANSACTION_MINED_MS,
                 );
                 // Verify transfer was successful
@@ -156,7 +170,13 @@ describe('Asset Transfer Proxies', () => {
                 const encodedAssetData = assetProxyUtils.encodeERC20AssetData(zrxToken.address);
                 // Create allowance less than transfer amount. Set allowance on proxy.
                 const allowance = new BigNumber(0);
-                const transferAmount = new BigNumber(10);
+                const amount = new BigNumber(10);
+                const data = assetProxyInterface.transferFrom.getABIEncodedTransactionData(
+                    encodedAssetData,
+                    makerAddress,
+                    takerAddress,
+                    amount,
+                );
                 await web3Wrapper.awaitTransactionSuccessAsync(
                     await zrxToken.approve.sendTransactionAsync(erc20Proxy.address, allowance, {
                         from: makerAddress,
@@ -165,13 +185,11 @@ describe('Asset Transfer Proxies', () => {
                 );
                 // Perform a transfer; expect this to fail.
                 return expectRevertReasonOrAlwaysFailingTransactionAsync(
-                    erc20Proxy.transferFrom.sendTransactionAsync(
-                        encodedAssetData,
-                        makerAddress,
-                        takerAddress,
-                        transferAmount,
-                        { from: exchangeAddress },
-                    ),
+                    web3Wrapper.sendTransactionAsync({
+                        to: erc20Proxy.address,
+                        data,
+                        from: exchangeAddress,
+                    }),
                     RevertReason.TransferFailed,
                 );
             });
@@ -179,11 +197,18 @@ describe('Asset Transfer Proxies', () => {
             it('should throw if requesting address is not authorized', async () => {
                 // Construct ERC20 asset data
                 const encodedAssetData = assetProxyUtils.encodeERC20AssetData(zrxToken.address);
-
                 // Perform a transfer from makerAddress to takerAddress
                 const amount = new BigNumber(10);
+                const data = assetProxyInterface.transferFrom.getABIEncodedTransactionData(
+                    encodedAssetData,
+                    makerAddress,
+                    takerAddress,
+                    amount,
+                );
                 return expectRevertReasonOrAlwaysFailingTransactionAsync(
-                    erc20Proxy.transferFrom.sendTransactionAsync(encodedAssetData, makerAddress, takerAddress, amount, {
+                    web3Wrapper.sendTransactionAsync({
+                        to: erc20Proxy.address,
+                        data,
                         from: notAuthorized,
                     }),
                     RevertReason.SenderNotAuthorized,
@@ -208,14 +233,18 @@ describe('Asset Transfer Proxies', () => {
                 expect(ownerMakerAsset).to.be.bignumber.equal(makerAddress);
                 // Perform a transfer from makerAddress to takerAddress
                 const amount = new BigNumber(1);
+                const data = assetProxyInterface.transferFrom.getABIEncodedTransactionData(
+                    encodedAssetData,
+                    makerAddress,
+                    takerAddress,
+                    amount,
+                );
                 await web3Wrapper.awaitTransactionSuccessAsync(
-                    await erc721Proxy.transferFrom.sendTransactionAsync(
-                        encodedAssetData,
-                        makerAddress,
-                        takerAddress,
-                        amount,
-                        { from: exchangeAddress },
-                    ),
+                    await web3Wrapper.sendTransactionAsync({
+                        to: erc721Proxy.address,
+                        data,
+                        from: exchangeAddress,
+                    }),
                     constants.AWAIT_TRANSACTION_MINED_MS,
                 );
                 // Verify transfer was successful
@@ -231,17 +260,21 @@ describe('Asset Transfer Proxies', () => {
                 expect(ownerMakerAsset).to.be.bignumber.equal(makerAddress);
                 // Perform a transfer from makerAddress to takerAddress
                 const amount = new BigNumber(1);
-                const txHash = await erc721Proxy.transferFrom.sendTransactionAsync(
+                const data = assetProxyInterface.transferFrom.getABIEncodedTransactionData(
                     encodedAssetData,
                     makerAddress,
                     erc721Receiver.address,
                     amount,
-                    { from: exchangeAddress },
                 );
-                await web3Wrapper.awaitTransactionSuccessAsync(txHash, constants.AWAIT_TRANSACTION_MINED_MS);
-                // Parse transaction logs
                 const logDecoder = new LogDecoder(web3Wrapper, erc721Receiver.address);
-                const tx = await logDecoder.getTxWithDecodedLogsAsync(txHash);
+                const tx = await logDecoder.getTxWithDecodedLogsAsync(
+                    await web3Wrapper.sendTransactionAsync({
+                        to: erc721Proxy.address,
+                        data,
+                        from: exchangeAddress,
+                        gas: constants.TRANSFER_FROM_GAS,
+                    }),
+                );
                 // Verify that no log was emitted by erc721 receiver
                 expect(tx.logs.length).to.be.equal(1);
                 const tokenReceivedLog = tx.logs[0] as LogWithDecodedArgs<TokenReceivedContractEventArgs>;
@@ -266,17 +299,21 @@ describe('Asset Transfer Proxies', () => {
                 expect(ownerMakerAsset).to.be.bignumber.equal(makerAddress);
                 // Perform a transfer from makerAddress to takerAddress
                 const amount = new BigNumber(1);
-                const txHash = await erc721Proxy.transferFrom.sendTransactionAsync(
+                const data = assetProxyInterface.transferFrom.getABIEncodedTransactionData(
                     encodedAssetData,
                     makerAddress,
                     erc721Receiver.address,
                     amount,
-                    { from: exchangeAddress },
                 );
-                await web3Wrapper.awaitTransactionSuccessAsync(txHash, constants.AWAIT_TRANSACTION_MINED_MS);
-                // Parse transaction logs
                 const logDecoder = new LogDecoder(web3Wrapper, erc721Receiver.address);
-                const tx = await logDecoder.getTxWithDecodedLogsAsync(txHash);
+                const tx = await logDecoder.getTxWithDecodedLogsAsync(
+                    await web3Wrapper.sendTransactionAsync({
+                        to: erc721Proxy.address,
+                        data,
+                        from: exchangeAddress,
+                        gas: constants.TRANSFER_FROM_GAS,
+                    }),
+                );
                 // Validate log emitted by erc721 receiver
                 expect(tx.logs.length).to.be.equal(1);
                 const tokenReceivedLog = tx.logs[0] as LogWithDecodedArgs<TokenReceivedContractEventArgs>;
@@ -301,14 +338,19 @@ describe('Asset Transfer Proxies', () => {
                 expect(ownerMakerAsset).to.be.bignumber.equal(makerAddress);
                 // Perform a transfer from makerAddress to takerAddress
                 const amount = new BigNumber(1);
+                const data = assetProxyInterface.transferFrom.getABIEncodedTransactionData(
+                    encodedAssetData,
+                    makerAddress,
+                    erc20Proxy.address, // the ERC20 proxy does not have an ERC721 receiver
+                    amount,
+                );
                 return expectRevertReasonOrAlwaysFailingTransactionAsync(
-                    erc721Proxy.transferFrom.sendTransactionAsync(
-                        encodedAssetData,
-                        makerAddress,
-                        erc20Proxy.address, // the ERC20 proxy does not have an ERC721 receiver
-                        amount,
-                        { from: exchangeAddress },
-                    ),
+                    web3Wrapper.sendTransactionAsync({
+                        to: erc721Proxy.address,
+                        data,
+                        from: exchangeAddress,
+                        gas: constants.TRANSFER_FROM_GAS,
+                    }),
                     RevertReason.TransferFailed,
                 );
             });
@@ -321,14 +363,18 @@ describe('Asset Transfer Proxies', () => {
                 expect(ownerMakerAsset).to.be.bignumber.equal(makerAddress);
                 // Perform a transfer from makerAddress to takerAddress
                 const amount = new BigNumber(0);
+                const data = assetProxyInterface.transferFrom.getABIEncodedTransactionData(
+                    encodedAssetData,
+                    makerAddress,
+                    takerAddress,
+                    amount,
+                );
                 return expectRevertReasonOrAlwaysFailingTransactionAsync(
-                    erc721Proxy.transferFrom.sendTransactionAsync(
-                        encodedAssetData,
-                        makerAddress,
-                        takerAddress,
-                        amount,
-                        { from: exchangeAddress },
-                    ),
+                    web3Wrapper.sendTransactionAsync({
+                        to: erc721Proxy.address,
+                        data,
+                        from: exchangeAddress,
+                    }),
                     RevertReason.InvalidAmount,
                 );
             });
@@ -341,14 +387,18 @@ describe('Asset Transfer Proxies', () => {
                 expect(ownerMakerAsset).to.be.bignumber.equal(makerAddress);
                 // Perform a transfer from makerAddress to takerAddress
                 const amount = new BigNumber(500);
+                const data = assetProxyInterface.transferFrom.getABIEncodedTransactionData(
+                    encodedAssetData,
+                    makerAddress,
+                    takerAddress,
+                    amount,
+                );
                 return expectRevertReasonOrAlwaysFailingTransactionAsync(
-                    erc721Proxy.transferFrom.sendTransactionAsync(
-                        encodedAssetData,
-                        makerAddress,
-                        takerAddress,
-                        amount,
-                        { from: exchangeAddress },
-                    ),
+                    web3Wrapper.sendTransactionAsync({
+                        to: erc721Proxy.address,
+                        data,
+                        from: exchangeAddress,
+                    }),
                     RevertReason.InvalidAmount,
                 );
             });
@@ -358,15 +408,23 @@ describe('Asset Transfer Proxies', () => {
                 const encodedAssetData = assetProxyUtils.encodeERC721AssetData(erc721Token.address, erc721MakerTokenId);
                 // Remove transfer approval for makerAddress.
                 await web3Wrapper.awaitTransactionSuccessAsync(
-                    await erc721Token.setApprovalForAll.sendTransactionAsync(erc721Proxy.address, false, {
+                    await erc721Token.approve.sendTransactionAsync(constants.NULL_ADDRESS, erc721MakerTokenId, {
                         from: makerAddress,
                     }),
                     constants.AWAIT_TRANSACTION_MINED_MS,
                 );
                 // Perform a transfer; expect this to fail.
                 const amount = new BigNumber(1);
+                const data = assetProxyInterface.transferFrom.getABIEncodedTransactionData(
+                    encodedAssetData,
+                    makerAddress,
+                    takerAddress,
+                    amount,
+                );
                 return expectRevertReasonOrAlwaysFailingTransactionAsync(
-                    erc20Proxy.transferFrom.sendTransactionAsync(encodedAssetData, makerAddress, takerAddress, amount, {
+                    web3Wrapper.sendTransactionAsync({
+                        to: erc721Proxy.address,
+                        data,
                         from: exchangeAddress,
                     }),
                     RevertReason.TransferFailed,
@@ -378,14 +436,18 @@ describe('Asset Transfer Proxies', () => {
                 const encodedAssetData = assetProxyUtils.encodeERC721AssetData(erc721Token.address, erc721MakerTokenId);
                 // Perform a transfer from makerAddress to takerAddress
                 const amount = new BigNumber(1);
+                const data = assetProxyInterface.transferFrom.getABIEncodedTransactionData(
+                    encodedAssetData,
+                    makerAddress,
+                    takerAddress,
+                    amount,
+                );
                 return expectRevertReasonOrAlwaysFailingTransactionAsync(
-                    erc721Proxy.transferFrom.sendTransactionAsync(
-                        encodedAssetData,
-                        makerAddress,
-                        takerAddress,
-                        amount,
-                        { from: notAuthorized },
-                    ),
+                    web3Wrapper.sendTransactionAsync({
+                        to: erc721Proxy.address,
+                        data,
+                        from: notAuthorized,
+                    }),
                     RevertReason.SenderNotAuthorized,
                 );
             });
diff --git a/packages/contracts/test/exchange/match_orders.ts b/packages/contracts/test/exchange/match_orders.ts
index e23ab9851..7bdb9c385 100644
--- a/packages/contracts/test/exchange/match_orders.ts
+++ b/packages/contracts/test/exchange/match_orders.ts
@@ -651,13 +651,7 @@ describe('matchOrders', () => {
             });
             // Match orders
             return expectRevertReasonOrAlwaysFailingTransactionAsync(
-                matchOrderTester.matchOrdersAndVerifyBalancesAsync(
-                    signedOrderLeft,
-                    signedOrderRight,
-                    takerAddress,
-                    erc20BalancesByOwner,
-                    erc721TokenIdsByOwner,
-                ),
+                exchangeWrapper.matchOrdersAsync(signedOrderLeft, signedOrderRight, takerAddress),
                 RevertReason.NegativeSpreadRequired,
             );
         });
@@ -680,13 +674,7 @@ describe('matchOrders', () => {
             });
             // Match orders
             return expectRevertReasonOrAlwaysFailingTransactionAsync(
-                matchOrderTester.matchOrdersAndVerifyBalancesAsync(
-                    signedOrderLeft,
-                    signedOrderRight,
-                    takerAddress,
-                    erc20BalancesByOwner,
-                    erc721TokenIdsByOwner,
-                ),
+                exchangeWrapper.matchOrdersAsync(signedOrderLeft, signedOrderRight, takerAddress),
                 // We are assuming assetData fields of the right order are the
                 // reverse of the left order, rather than checking equality. This
                 // saves a bunch of gas, but as a result if the assetData fields are
@@ -715,13 +703,7 @@ describe('matchOrders', () => {
             });
             // Match orders
             return expectRevertReasonOrAlwaysFailingTransactionAsync(
-                matchOrderTester.matchOrdersAndVerifyBalancesAsync(
-                    signedOrderLeft,
-                    signedOrderRight,
-                    takerAddress,
-                    erc20BalancesByOwner,
-                    erc721TokenIdsByOwner,
-                ),
+                exchangeWrapper.matchOrdersAsync(signedOrderLeft, signedOrderRight, takerAddress),
                 RevertReason.InvalidOrderSignature,
             );
         });