Tons of tests around nested byte arrays and ERC721 receiver

6 files changed, 223 insertions, 32 deletions

diff --git a/packages/contracts/src/contracts/current/test/TestLibBytes/TestLibBytes.sol b/packages/contracts/src/contracts/current/test/TestLibBytes/TestLibBytes.sol
index 69554605d..0bf11b03b 100644
--- a/packages/contracts/src/contracts/current/test/TestLibBytes/TestLibBytes.sol
+++ b/packages/contracts/src/contracts/current/test/TestLibBytes/TestLibBytes.sol
@@ -155,6 +155,7 @@ contract TestLibBytes is
         return b;
     }
 
+=======
     /// @dev Reads the first 4 bytes from a byte array of arbitrary length.
     /// @param b Byte array to read first 4 bytes from.
     /// @return First 4 bytes of data.
@@ -166,4 +167,28 @@ contract TestLibBytes is
         result = readFirst4(b);
         return result;
     }
+
+    function publicReadBytes(
+        bytes memory b,
+        uint256 index)
+        public
+        pure
+        returns (bytes memory result)
+    {
+        result = readBytes(b, index);
+        return result;
+    }
+
+
+    function publicWriteBytes(
+        bytes memory b,
+        uint256 index,
+        bytes memory input)
+        public
+        pure
+        returns (bytes memory)
+    {
+        writeBytes(b, index, input);
+        return b;
+    }
 }
diff --git a/packages/contracts/src/contracts/current/utils/LibBytes/LibBytes.sol b/packages/contracts/src/contracts/current/utils/LibBytes/LibBytes.sol
index fb8359462..6351f1a46 100644
--- a/packages/contracts/src/contracts/current/utils/LibBytes/LibBytes.sol
+++ b/packages/contracts/src/contracts/current/utils/LibBytes/LibBytes.sol
@@ -300,24 +300,21 @@ contract LibBytes is
         returns (bytes memory result)
     {
         // Read length of nested bytes
-        require(
-            b.length >= index + 32,
-            GTE_32_LENGTH_REQUIRED
-        );
         uint256 nestedBytesLength = readUint256(b, index);
+        index += 32;
 
         // Assert length of <b> is valid, given
         // length of nested bytes
         require(
-            b.length >= index + 32 + nestedBytesLength,
+            b.length >= index + nestedBytesLength,
             GTE_32_LENGTH_REQUIRED
         );
 
         // Allocate memory and copy value to result
         result = new bytes(nestedBytesLength);
         memcpy(
-            getMemAddress(result) + 32,    // +32 skips array length
-            getMemAddress(b) + index + 32, // +32 skips array length
+            getMemAddress(result) + 32, // +32 skips array length
+            getMemAddress(b) + index + 32,
             nestedBytesLength
         );
 
@@ -344,9 +341,9 @@ contract LibBytes is
 
         // Copy <input> into <b>
         memcpy(
-            getMemAddress(b) + index,
-            getMemAddress(input),
-            input.length + 32 /* 32 bytes to store length */
+            getMemAddress(b) + index + 32,  // +32 to skip length of <b>
+            getMemAddress(input),           // include length of byte array
+            input.length + 32               // +32 bytes to store length
         );
     }
 }
diff --git a/packages/contracts/test/asset_proxy/decoder.ts b/packages/contracts/test/asset_proxy/decoder.ts
index 4416334d1..0f1413ff1 100644
--- a/packages/contracts/test/asset_proxy/decoder.ts
+++ b/packages/contracts/test/asset_proxy/decoder.ts
@@ -56,21 +56,45 @@ describe('LibAssetProxyDecoder', () => {
             expect(decodedTokenAddress).to.be.equal(expectedDecodedProxyData.tokenAddress);
         });
 
-        it('should correctly decode ERC721 proxy data)', async () => {
+        it('should correctly decode ERC721 proxy data', async () => {
             const tokenId = ZeroEx.generatePseudoRandomSalt();
             const encodedProxyData = assetProxyUtils.encodeERC721ProxyData(testAddress, tokenId);
             const expectedDecodedProxyData = assetProxyUtils.decodeERC721ProxyData(encodedProxyData);
             let decodedAssetProxyId: number;
             let decodedTokenAddress: string;
             let decodedTokenId: BigNumber;
+            let decodedData: string;
             [
                 decodedAssetProxyId,
                 decodedTokenAddress,
                 decodedTokenId,
+                decodedData,
             ] = await testAssetProxyDecoder.publicDecodeERC721Data.callAsync(encodedProxyData);
             expect(decodedAssetProxyId).to.be.equal(expectedDecodedProxyData.assetProxyId);
             expect(decodedTokenAddress).to.be.equal(expectedDecodedProxyData.tokenAddress);
             expect(decodedTokenId).to.be.bignumber.equal(expectedDecodedProxyData.tokenId);
+            expect(decodedData).to.be.equal(expectedDecodedProxyData.data);
+        });
+
+        it('should correctly decode ERC721 proxy data with receiver data', async () => {
+            const tokenId = ZeroEx.generatePseudoRandomSalt();
+            const data = ethUtil.bufferToHex(assetProxyUtils.encodeUint256(ZeroEx.generatePseudoRandomSalt())) + 'FFFF';
+            const encodedProxyData = assetProxyUtils.encodeERC721ProxyData(testAddress, tokenId, data);
+            const expectedDecodedProxyData = assetProxyUtils.decodeERC721ProxyData(encodedProxyData);
+            let decodedAssetProxyId: number;
+            let decodedTokenAddress: string;
+            let decodedTokenId: BigNumber;
+            let decodedData: string;
+            [
+                decodedAssetProxyId,
+                decodedTokenAddress,
+                decodedTokenId,
+                decodedData,
+            ] = await testAssetProxyDecoder.publicDecodeERC721Data.callAsync(encodedProxyData);
+            expect(decodedAssetProxyId).to.be.equal(expectedDecodedProxyData.assetProxyId);
+            expect(decodedTokenAddress).to.be.equal(expectedDecodedProxyData.tokenAddress);
+            expect(decodedTokenId).to.be.bignumber.equal(expectedDecodedProxyData.tokenId);
+            expect(decodedData).to.be.equal(expectedDecodedProxyData.data);
         });
     });
 });
diff --git a/packages/contracts/test/asset_proxy/proxies.ts b/packages/contracts/test/asset_proxy/proxies.ts
index d7f27deb4..4995e95a0 100644
--- a/packages/contracts/test/asset_proxy/proxies.ts
+++ b/packages/contracts/test/asset_proxy/proxies.ts
@@ -261,7 +261,7 @@ describe('Asset Transfer Proxies', () => {
         });
     });
 
-    describe.only('Transfer Proxy - ERC721', () => {
+    describe('Transfer Proxy - ERC721', () => {
         describe('transferFrom', () => {
             it('should successfully transfer tokens', async () => {
                 // Construct metadata for ERC721 proxy
@@ -289,7 +289,7 @@ describe('Asset Transfer Proxies', () => {
                 expect(newOwnerMakerAsset).to.be.bignumber.equal(takerAddress);
             });
 
-            it.only('should call onERC721Received when transferring to a smart contract', async () => {
+            it('should call onERC721Received when transferring to a smart contract', async () => {
                 // Construct metadata for ERC721 proxy
                 const encodedProxyMetadata = assetProxyUtils.encodeERC721ProxyData(
                     erc721Token.address,
@@ -326,9 +326,11 @@ describe('Asset Transfer Proxies', () => {
 
             it('should call onERC721Received when transferring to a smart contract and receive extra data', async () => {
                 // Construct metadata for ERC721 proxy
+                const data = ethUtil.bufferToHex(assetProxyUtils.encodeUint256(ZeroEx.generatePseudoRandomSalt()));
                 const encodedProxyMetadata = assetProxyUtils.encodeERC721ProxyData(
                     erc721Token.address,
                     erc721MakerTokenId,
+                    data,
                 );
                 // Verify pre-condition
                 const ownerMakerAsset = await erc721Token.ownerOf.callAsync(erc721MakerTokenId);
@@ -348,14 +350,17 @@ 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));
-                // Verify erc721 receiver log emitted
-                console.log(tx.logs);
+                // Validate log emitted  by erc721 receiver
+                expect(tx.logs.length).to.be.equal(1);
+                const tokenReceivedLog = tx.logs[0] as LogWithDecodedArgs<TokenReceivedContractEventArgs>;
+                expect(tokenReceivedLog.args.from).to.be.equal(makerAddress);
+                expect(tokenReceivedLog.args.tokenId).to.be.bignumber.equal(erc721MakerTokenId);
+                expect(tokenReceivedLog.args.data).to.be.equal(data);
                 // Verify transfer was successful
                 const newOwnerMakerAsset = await erc721Token.ownerOf.callAsync(erc721MakerTokenId);
                 expect(newOwnerMakerAsset).to.be.bignumber.equal(erc721Receiver.address);
             });
 
-            /*
             it('should throw if receiving contract does not have onERC721Received', async () => {
                 // Construct metadata for ERC721 proxy
                 const encodedProxyMetadata = assetProxyUtils.encodeERC721ProxyData(
@@ -368,17 +373,16 @@ describe('Asset Transfer Proxies', () => {
                 // Perform a transfer from makerAddress to takerAddress
                 const erc20Balances = await erc20Wrapper.getBalancesAsync();
                 const amount = new BigNumber(1);
-                await erc721Proxy.transferFrom.sendTransactionAsync(
-                    encodedProxyMetadata,
-                    makerAddress,
-                    takerAddress,
-                    amount,
-                    { from: exchangeAddress },
-                );
-                // Verify transfer was successful
-                const newOwnerMakerAsset = await erc721Token.ownerOf.callAsync(erc721MakerTokenId);
-                expect(newOwnerMakerAsset).to.be.bignumber.equal(takerAddress);
-            });*/
+                return expect(
+                    erc721Proxy.transferFrom.sendTransactionAsync(
+                        encodedProxyMetadata,
+                        makerAddress,
+                        erc20Proxy.address, // the ERC20 proxy does not have an ERC721 receiver
+                        amount,
+                        { from: exchangeAddress },
+                    ),
+                ).to.be.rejectedWith(constants.REVERT);
+            });
 
             it('should throw if transferring 0 amount of a token', async () => {
                 // Construct metadata for ERC721 proxy
diff --git a/packages/contracts/test/libraries/lib_bytes.ts b/packages/contracts/test/libraries/lib_bytes.ts
index 0996cdc84..a6e3c7d37 100644
--- a/packages/contracts/test/libraries/lib_bytes.ts
+++ b/packages/contracts/test/libraries/lib_bytes.ts
@@ -6,7 +6,8 @@ import ethUtil = require('ethereumjs-util');
 
 import { TestLibBytesContract } from '../../src/generated_contract_wrappers/test_lib_bytes';
 import { artifacts } from '../../src/utils/artifacts';
-import { expectRevertOrOtherErrorAsync } from '../../src/utils/assertions';
+import { expectRevertOrOtherErrorAsync, expectRevertOrOtherErrorAsync } from '../../src/utils/assertions';
+import { assetProxyUtils } from '../../src/utils/asset_proxy_utils';
 import { chaiSetup } from '../../src/utils/chai_setup';
 import { constants } from '../../src/utils/constants';
 import { provider, txDefaults, web3Wrapper } from '../../src/utils/web3_wrapper';
@@ -28,6 +29,15 @@ describe('LibBytes', () => {
     let testAddress: string;
     const testBytes32 = '0x102030405060708090a0b0c0d0e0f0102030405060708090a0b0c0d0e0f01020';
     const testUint256 = new BigNumber(testBytes32, 16);
+    let shortData: string;
+    let shortTestBytes: string;
+    let shortTestBytesAsBuffer: Buffer;
+    let wordOfData: string;
+    let wordOfTestBytes: string;
+    let wordOfTestBytesAsBuffer: Buffer;
+    let longData: string;
+    let longTestBytes: string;
+    let longTestBytesAsBuffer: Buffer;
 
     before(async () => {
         await blockchainLifecycle.startAsync();
@@ -48,6 +58,26 @@ describe('LibBytes', () => {
         expect(byteArrayLongerThan32BytesLength).to.be.greaterThan(32);
         const testBytes32Length = ethUtil.toBuffer(testBytes32).byteLength;
         expect(testBytes32Length).to.be.equal(32);
+        // Create short test bytes
+        shortData = '0xffffaa';
+        const encodedShortData = ethUtil.toBuffer(shortData);
+        const shortDataLength = new BigNumber(encodedShortData.byteLength);
+        const encodedShortDataLength = assetProxyUtils.encodeUint256(shortDataLength);
+        shortTestBytesAsBuffer = Buffer.concat([encodedShortDataLength, encodedShortData]);
+        shortTestBytes = ethUtil.bufferToHex(shortTestBytesAsBuffer);
+        // Create test bytes one word in length
+        wordOfData = ethUtil.bufferToHex(assetProxyUtils.encodeUint256(ZeroEx.generatePseudoRandomSalt()));
+        const encodedWordOfData = ethUtil.toBuffer(wordOfData);
+        const wordOfDataLength = new BigNumber(encodedWordOfData.byteLength);
+        const encodedWordOfDataLength = assetProxyUtils.encodeUint256(wordOfDataLength);
+        wordOfTestBytesAsBuffer = Buffer.concat([encodedWordOfDataLength, encodedWordOfData]);
+        wordOfTestBytes = ethUtil.bufferToHex(wordOfTestBytesAsBuffer);
+        // Create long test bytes (combines short test bytes with word of test bytes)
+        longData = ethUtil.bufferToHex(Buffer.concat([encodedShortData, encodedWordOfData]));
+        const longDataLength = new BigNumber(encodedShortData.byteLength + encodedWordOfData.byteLength);
+        const encodedLongDataLength = assetProxyUtils.encodeUint256(longDataLength);
+        longTestBytesAsBuffer = Buffer.concat([encodedLongDataLength, encodedShortData, encodedWordOfData]);
+        longTestBytes = ethUtil.bufferToHex(longTestBytesAsBuffer);
     });
     beforeEach(async () => {
         await blockchainLifecycle.startAsync();
@@ -285,6 +315,7 @@ describe('LibBytes', () => {
     });
     */
 
+=======
     describe('readFirst4', () => {
         // AssertionError: expected promise to be rejected with an error including 'revert' but it was fulfilled with '0x08c379a0'
         it('should revert if byte array has a length < 4', async () => {
@@ -300,4 +331,92 @@ describe('LibBytes', () => {
             expect(first4Bytes).to.equal(expectedFirst4Bytes);
         });
     });
+
+    describe('readBytes', () => {
+        it('should successfully read short, nested array of bytes when it takes up the whole array', async () => {
+            const testBytesOffset = new BigNumber(0);
+            const bytes = await libBytes.publicReadBytes.callAsync(shortTestBytes, testBytesOffset);
+            return expect(bytes).to.be.equal(shortData);
+        });
+
+        it('should successfully read short, nested array of bytes when it is offset in the array', async () => {
+            const prefixByteArrayBuffer = ethUtil.toBuffer('0xabcdef');
+            const shortDataAsBuffer = ethUtil.toBuffer(shortData);
+            const combinedByteArrayBuffer = Buffer.concat([prefixByteArrayBuffer, shortTestBytesAsBuffer]);
+            const combinedByteArray = ethUtil.bufferToHex(combinedByteArrayBuffer);
+            const testUint256Offset = new BigNumber(prefixByteArrayBuffer.byteLength);
+            const bytes = await libBytes.publicReadBytes.callAsync(combinedByteArray, testUint256Offset);
+            return expect(bytes).to.be.equal(shortData);
+        });
+
+        it('should successfully read a nested array of bytes - one word in length - when it takes up the whole array', async () => {
+            const testBytesOffset = new BigNumber(0);
+            const bytes = await libBytes.publicReadBytes.callAsync(wordOfTestBytes, testBytesOffset);
+            return expect(bytes).to.be.equal(wordOfData);
+        });
+
+        it('should successfully read a nested array of bytes - one word in length - when it is offset in the array', async () => {
+            const prefixByteArrayBuffer = ethUtil.toBuffer('0xabcdef');
+            const wordOfDataAsBuffer = ethUtil.toBuffer(wordOfData);
+            const combinedByteArrayBuffer = Buffer.concat([prefixByteArrayBuffer, wordOfTestBytesAsBuffer]);
+            const combinedByteArray = ethUtil.bufferToHex(combinedByteArrayBuffer);
+            const testUint256Offset = new BigNumber(prefixByteArrayBuffer.byteLength);
+            const bytes = await libBytes.publicReadBytes.callAsync(combinedByteArray, testUint256Offset);
+            return expect(bytes).to.be.equal(wordOfData);
+        });
+
+        it('should successfully read long, nested array of bytes when it takes up the whole array', async () => {
+            const testBytesOffset = new BigNumber(0);
+            const bytes = await libBytes.publicReadBytes.callAsync(longTestBytes, testBytesOffset);
+            return expect(bytes).to.be.equal(longData);
+        });
+
+        it('should successfully read long, nested array of bytes when it is offset in the array', async () => {
+            const prefixByteArrayBuffer = ethUtil.toBuffer('0xabcdef');
+            const longDataAsBuffer = ethUtil.toBuffer(longData);
+            const combinedByteArrayBuffer = Buffer.concat([prefixByteArrayBuffer, longTestBytesAsBuffer]);
+            const combinedByteArray = ethUtil.bufferToHex(combinedByteArrayBuffer);
+            const testUint256Offset = new BigNumber(prefixByteArrayBuffer.byteLength);
+            const bytes = await libBytes.publicReadBytes.callAsync(combinedByteArray, testUint256Offset);
+            return expect(bytes).to.be.equal(longData);
+        });
+
+        it('should fail if the byte array is too short to hold the length of a nested byte array)', async () => {
+            // The length of the nested array is 32 bytes. By storing less than 32 bytes, a length cannot be read.
+            const offset = new BigNumber(0);
+            return expect(libBytes.publicReadBytes.callAsync(byteArrayShorterThan32Bytes, offset)).to.be.rejectedWith(
+                constants.REVERT,
+            );
+        });
+
+        it('should fail if we store a nested byte array length, without a nested byte array)', async () => {
+            const offset = new BigNumber(0);
+            return expect(libBytes.publicReadBytes.callAsync(testBytes32, offset)).to.be.rejectedWith(constants.REVERT);
+        });
+
+        it('should fail if the length between the offset and end of the byte array is too short to hold the length of a nested byte array)', async () => {
+            const badOffset = new BigNumber(ethUtil.toBuffer(byteArrayShorterThan32Bytes).byteLength);
+            return expect(
+                libBytes.publicReadBytes.callAsync(byteArrayShorterThan32Bytes, badOffset),
+            ).to.be.rejectedWith(constants.REVERT);
+        });
+
+        it('should fail if the length between the offset and end of the byte array is too short to hold the nested byte array)', async () => {
+            const badOffset = new BigNumber(ethUtil.toBuffer(testBytes32).byteLength);
+            return expect(libBytes.publicReadBytes.callAsync(testBytes32, badOffset)).to.be.rejectedWith(
+                constants.REVERT,
+            );
+        });
+    });
+
+    /// @TODO Implement test cases for writeUint256. Test template below.
+    ///       Currently, the generated contract wrappers do not support this library's write methods.
+    /*
+    describe('writeBytes', () => {
+        it('should successfully write bytes when it takes up the whole array)', async () => {});
+        it('should successfully write bytes when it is offset in the array)', async () => {});
+        it('should fail if the byte array is too short to hold the nested bytes)', async () => {});
+        it('should fail if the length between the offset and end of the byte array is too short to hold the nested bytes)', async () => {});
+    });
+    */
 });
diff --git a/packages/order-utils/src/asset_proxy_utils.ts b/packages/order-utils/src/asset_proxy_utils.ts
index 55f2d56df..8255376a1 100644
--- a/packages/order-utils/src/asset_proxy_utils.ts
+++ b/packages/order-utils/src/asset_proxy_utils.ts
@@ -2,6 +2,7 @@ import { AssetProxyId, ERC20ProxyData, ERC721ProxyData, ProxyData } from '@0xpro
 import { BigNumber } from '@0xproject/utils';
 import BN = require('bn.js');
 import ethUtil = require('ethereumjs-util');
+import * as _ from 'lodash';
 
 const ERC20_PROXY_METADATA_BYTE_LENGTH = 21;
 const ERC721_PROXY_METADATA_BYTE_LENGTH = 53;
@@ -74,19 +75,25 @@ export const assetProxyUtils = {
         };
         return erc20ProxyData;
     },
-    encodeERC721ProxyData(tokenAddress: string, tokenId: BigNumber): string {
+    encodeERC721ProxyData(tokenAddress: string, tokenId: BigNumber, data?: string): string {
         const encodedAssetProxyId = assetProxyUtils.encodeAssetProxyId(AssetProxyId.ERC721);
         const encodedAddress = assetProxyUtils.encodeAddress(tokenAddress);
         const encodedTokenId = assetProxyUtils.encodeUint256(tokenId);
         const encodedMetadata = Buffer.concat([encodedAddress, encodedTokenId, encodedAssetProxyId]);
+        if (!_.isUndefined(data)) {
+            const encodedData = ethUtil.toBuffer(data);
+            const dataLength = new BigNumber(encodedData.byteLength);
+            const encodedDataLength = assetProxyUtils.encodeUint256(dataLength);
+            encodedMetadata = Buffer.concat([encodedMetadata, encodedDataLength, encodedData]);
+        }
         const encodedMetadataHex = ethUtil.bufferToHex(encodedMetadata);
         return encodedMetadataHex;
     },
     decodeERC721ProxyData(proxyData: string): ERC721ProxyData {
         const encodedProxyMetadata = ethUtil.toBuffer(proxyData);
-        if (encodedProxyMetadata.byteLength !== ERC721_PROXY_METADATA_BYTE_LENGTH) {
+        if (encodedProxyMetadata.byteLength < ERC721_PROXY_METADATA_BYTE_LENGTH) {
             throw new Error(
-                `Could not decode ERC20 Proxy Data. Expected length of encoded data to be 53. Got ${
+                `Could not decode ERC20 Proxy Data. Expected length of encoded data to be at least 53. Got ${
                     encodedProxyMetadata.byteLength
                 }`,
             );
@@ -106,10 +113,25 @@ export const assetProxyUtils = {
         const tokenIdOffset = ERC721_PROXY_METADATA_BYTE_LENGTH - 1;
         const encodedTokenId = encodedProxyMetadata.slice(addressOffset, tokenIdOffset);
         const tokenId = assetProxyUtils.decodeUint256(encodedTokenId);
-        const erc721ProxyData = {
+        const nullData = '0x';
+        let data = nullData;
+        if (encodedProxyMetadata.byteLength > 53) {
+            const encodedDataLength = encodedProxyMetadata.slice(53, 85);
+            const dataLength = assetProxyUtils.decodeUint256(encodedDataLength);
+            const expectedDataLength = new BigNumber(encodedProxyMetadata.byteLength - 85);
+            if (!dataLength.equals(expectedDataLength)) {
+                throw new Error(
+                    `Data length (${dataLength}) does not match actual length of data (${expectedDataLength})`,
+                );
+            }
+            const encodedData = encodedProxyMetadata.slice(85);
+            data = ethUtil.bufferToHex(encodedData);
+        }
+        const erc721ProxyData: ERC721ProxyData = {
             assetProxyId,
             tokenAddress,
             tokenId,
+            data,
         };
         return erc721ProxyData;
     },