From 8496c1cdd3bc477fcfe584adf8200f4ed35da2b0 Mon Sep 17 00:00:00 2001 From: Greg Hysen Date: Thu, 31 May 2018 18:59:02 -0700 Subject: Call safeTransferFrom only when there is receiver data present --- .../current/protocol/AssetProxy/ERC721Proxy.sol | 7 ++++++- packages/contracts/test/asset_proxy/proxies.ts | 24 ++++++++++++---------- packages/contracts/test/libraries/lib_mem.ts | 2 +- 3 files changed, 20 insertions(+), 13 deletions(-) diff --git a/packages/contracts/src/contracts/current/protocol/AssetProxy/ERC721Proxy.sol b/packages/contracts/src/contracts/current/protocol/AssetProxy/ERC721Proxy.sol index eb23736a0..9dac02d87 100644 --- a/packages/contracts/src/contracts/current/protocol/AssetProxy/ERC721Proxy.sol +++ b/packages/contracts/src/contracts/current/protocol/AssetProxy/ERC721Proxy.sol @@ -77,8 +77,13 @@ contract ERC721Proxy is ); // Transfer token. + // Save gas by calling safeTransferFrom only when there is data present. // Either succeeds or throws. - ERC721Token(token).safeTransferFrom(from, to, tokenId, data); + if(data.length > 0) { + ERC721Token(token).safeTransferFrom(from, to, tokenId, data); + } else { + ERC721Token(token).transferFrom(from, to, tokenId); + } } /// @dev Gets the proxy id associated with the proxy address. diff --git a/packages/contracts/test/asset_proxy/proxies.ts b/packages/contracts/test/asset_proxy/proxies.ts index d6bc8163b..f44c44045 100644 --- a/packages/contracts/test/asset_proxy/proxies.ts +++ b/packages/contracts/test/asset_proxy/proxies.ts @@ -280,7 +280,7 @@ describe('Asset Transfer Proxies', () => { expect(newOwnerMakerAsset).to.be.bignumber.equal(takerAddress); }); - it('should call onERC721Received when transferring to a smart contract', async () => { + it('should not call onERC721Received when transferring to a smart contract without receiver data', async () => { // Construct metadata for ERC721 proxy const encodedAssetData = assetProxyUtils.encodeERC721AssetData(erc721Token.address, erc721MakerTokenId); // Verify pre-condition @@ -296,23 +296,20 @@ describe('Asset Transfer Proxies', () => { amount, { from: exchangeAddress }, ); + // Parse transaction logs const tx = await zeroEx.awaitTransactionMinedAsync(txHash); tx.logs = _.filter(tx.logs, log => log.address === erc721Receiver.address); const logDecoder = new LogDecoder(constants.TESTRPC_NETWORK_ID); tx.logs = _.map(tx.logs, log => logDecoder.decodeLogOrThrow(log)); - // Validate log emitted by erc721 receiver - expect(tx.logs.length).to.be.equal(1); - const tokenReceivedLog = tx.logs[0] as LogWithDecodedArgs; - expect(tokenReceivedLog.args.from).to.be.equal(makerAddress); - expect(tokenReceivedLog.args.tokenId).to.be.bignumber.equal(erc721MakerTokenId); - expect(tokenReceivedLog.args.data).to.be.equal(nullDataHex); + // Verify that no log was emitted by erc721 receiver + expect(tx.logs.length).to.be.equal(0); // Verify transfer was successful const newOwnerMakerAsset = await erc721Token.ownerOf.callAsync(erc721MakerTokenId); expect(newOwnerMakerAsset).to.be.bignumber.equal(erc721Receiver.address); }); - it('should call onERC721Received when transferring to a smart contract and receive extra data', async () => { + it('should call onERC721Received when transferring to a smart contract with receiver data', async () => { // Construct metadata for ERC721 proxy const data = ethUtil.bufferToHex(assetProxyUtils.encodeUint256(ZeroEx.generatePseudoRandomSalt())); const encodedAssetData = assetProxyUtils.encodeERC721AssetData( @@ -338,7 +335,7 @@ describe('Asset Transfer Proxies', () => { tx.logs = _.filter(tx.logs, log => log.address === erc721Receiver.address); const logDecoder = new LogDecoder(constants.TESTRPC_NETWORK_ID); tx.logs = _.map(tx.logs, log => logDecoder.decodeLogOrThrow(log)); - // Validate log emitted by erc721 receiver + // Validate log emitted by erc721 receiver expect(tx.logs.length).to.be.equal(1); const tokenReceivedLog = tx.logs[0] as LogWithDecodedArgs; expect(tokenReceivedLog.args.from).to.be.equal(makerAddress); @@ -349,9 +346,14 @@ describe('Asset Transfer Proxies', () => { expect(newOwnerMakerAsset).to.be.bignumber.equal(erc721Receiver.address); }); - it('should throw if receiving contract does not have onERC721Received', async () => { + it('should throw if there is receiver data but contract does not have onERC721Received', async () => { // Construct metadata for ERC721 proxy - const encodedAssetData = assetProxyUtils.encodeERC721AssetData(erc721Token.address, erc721MakerTokenId); + const data = ethUtil.bufferToHex(assetProxyUtils.encodeUint256(ZeroEx.generatePseudoRandomSalt())); + const encodedAssetData = assetProxyUtils.encodeERC721AssetData( + erc721Token.address, + erc721MakerTokenId, + data, + ); // Verify pre-condition const ownerMakerAsset = await erc721Token.ownerOf.callAsync(erc721MakerTokenId); expect(ownerMakerAsset).to.be.bignumber.equal(makerAddress); diff --git a/packages/contracts/test/libraries/lib_mem.ts b/packages/contracts/test/libraries/lib_mem.ts index dd6f46742..efe719255 100644 --- a/packages/contracts/test/libraries/lib_mem.ts +++ b/packages/contracts/test/libraries/lib_mem.ts @@ -15,7 +15,7 @@ const toHex = (buf: Uint8Array): string => buf.reduce((a, v) => a + ('00' + v.to const fromHex = (str: string): Uint8Array => Uint8Array.from(Buffer.from(str.slice(2), 'hex')); -describe.only('LibMem', () => { +describe('LibMem', () => { let owner: string; let testLibMem: TestLibMemContract; -- cgit