Style audit for proxies + libmem + libbytes

8 files changed, 72 insertions, 50 deletions

diff --git a/packages/contracts/src/contracts/current/protocol/AssetProxy/ERC20Proxy.sol b/packages/contracts/src/contracts/current/protocol/AssetProxy/ERC20Proxy.sol
index 54cbeb963..96950f1cd 100644
--- a/packages/contracts/src/contracts/current/protocol/AssetProxy/ERC20Proxy.sol
+++ b/packages/contracts/src/contracts/current/protocol/AssetProxy/ERC20Proxy.sol
@@ -47,7 +47,7 @@ contract ERC20Proxy is
     )
         internal
     {
-        // Decode proxy data.
+        // Decode asset data.
         (
             uint8 proxyId,
             address token
diff --git a/packages/contracts/src/contracts/current/protocol/AssetProxy/ERC721Proxy.sol b/packages/contracts/src/contracts/current/protocol/AssetProxy/ERC721Proxy.sol
index 58c23b03b..102064c15 100644
--- a/packages/contracts/src/contracts/current/protocol/AssetProxy/ERC721Proxy.sol
+++ b/packages/contracts/src/contracts/current/protocol/AssetProxy/ERC721Proxy.sol
@@ -48,7 +48,7 @@ contract ERC721Proxy is
     )
         internal
     {
-        // Decode proxy data.
+        // Decode asset data.
         (
             uint8 proxyId,
             address token,
@@ -118,7 +118,7 @@ contract ERC721Proxy is
             INVALID_ASSET_DATA_LENGTH
         );
 
-        // Decode asset data
+        // Decode asset data.
         proxyId = uint8(assetData[0]);
         token = readAddress(assetData, 1);
         tokenId = readUint256(assetData, 21);
diff --git a/packages/contracts/src/contracts/current/test/TestLibBytes/TestLibBytes.sol b/packages/contracts/src/contracts/current/test/TestLibBytes/TestLibBytes.sol
index 0bf11b03b..f009a6a71 100644
--- a/packages/contracts/src/contracts/current/test/TestLibBytes/TestLibBytes.sol
+++ b/packages/contracts/src/contracts/current/test/TestLibBytes/TestLibBytes.sol
@@ -155,7 +155,6 @@ 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.
@@ -168,6 +167,10 @@ contract TestLibBytes is
         return result;
     }
 
+    /// @dev Reads nested bytes from a specific position.
+    /// @param b Byte array containing nested bytes.
+    /// @param index Index of nested bytes.
+    /// @return result Nested bytes.
     function publicReadBytes(
         bytes memory b,
         uint256 index)
@@ -179,7 +182,11 @@ contract TestLibBytes is
         return result;
     }
 
-
+    /// @dev Inserts bytes at a specific position in a byte array.
+    /// @param b Byte array to insert <input> into.
+    /// @param index Index in byte array of <input>.
+    /// @param input bytes to insert.
+    /// @return b Updated input byte array
     function publicWriteBytes(
         bytes memory b,
         uint256 index,
diff --git a/packages/contracts/src/contracts/current/test/TestLibMem/TestLibMem.sol b/packages/contracts/src/contracts/current/test/TestLibMem/TestLibMem.sol
index 64bc182f4..076bee24c 100644
--- a/packages/contracts/src/contracts/current/test/TestLibMem/TestLibMem.sol
+++ b/packages/contracts/src/contracts/current/test/TestLibMem/TestLibMem.sol
@@ -23,8 +23,15 @@ import "../../utils/LibMem/LibMem.sol";
 contract TestLibMem is
     LibMem
 {
+
+    /// @dev Copies a block of memory from one location to another.
+    /// @param mem Memory contents we want to apply memcpy to
+    /// @param dest Destination offset into <mem>.
+    /// @param source Source offset into <mem>.
+    /// @param length Length of bytes to copy from <source> to <dest>
+    /// @return mem Memory contents after calling memcpy.
     function testMemcpy(
-        bytes mem,       ///< Memory contents we want to apply memcpy to
+        bytes mem,
         uint256 dest,
         uint256 source,
         uint256 length
@@ -36,13 +43,13 @@ contract TestLibMem is
         // Sanity check. Overflows are not checked.
         require(source + length <= mem.length);
         require(dest + length <= mem.length);
-        
+
         // Get pointer to memory contents
         uint256 offset = getMemAddress(mem) + 32;
-        
+
         // Execute memcpy adjusted for memory array location
         memcpy(offset + dest, offset + source, length);
-        
+
         // Return modified memory contents
         return mem;
     }
diff --git a/packages/contracts/src/contracts/current/utils/LibBytes/LibBytes.sol b/packages/contracts/src/contracts/current/utils/LibBytes/LibBytes.sol
index 6351f1a46..5610c47b3 100644
--- a/packages/contracts/src/contracts/current/utils/LibBytes/LibBytes.sol
+++ b/packages/contracts/src/contracts/current/utils/LibBytes/LibBytes.sol
@@ -268,7 +268,6 @@ contract LibBytes is
         writeBytes32(b, index, bytes32(input));
     }
 
-=======
     /// @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.
@@ -287,10 +286,10 @@ contract LibBytes is
         return result;
     }
 
-    /// @dev Reads a uint256 value from a position in a byte array.
-    /// @param b Byte array containing a uint256 value.
-    /// @param index Index in byte array of uint256 value.
-    /// @return uint256 value from byte array.
+    /// @dev Reads nested bytes from a specific position.
+    /// @param b Byte array containing nested bytes.
+    /// @param index Index of nested bytes.
+    /// @return result Nested bytes.
     function readBytes(
         bytes memory b,
         uint256 index
@@ -321,10 +320,10 @@ contract LibBytes is
         return result;
     }
 
-    /// @dev Writes a uint256 into a specific position in a byte array.
+    /// @dev Inserts bytes at a specific position in a byte array.
     /// @param b Byte array to insert <input> into.
     /// @param index Index in byte array of <input>.
-    /// @param input uint256 to put into byte array.
+    /// @param input bytes to insert.
     function writeBytes(
         bytes memory b,
         uint256 index,
diff --git a/packages/contracts/src/contracts/current/utils/LibMem/LibMem.sol b/packages/contracts/src/contracts/current/utils/LibMem/LibMem.sol
index 500044500..960850725 100644
--- a/packages/contracts/src/contracts/current/utils/LibMem/LibMem.sol
+++ b/packages/contracts/src/contracts/current/utils/LibMem/LibMem.sol
@@ -18,23 +18,27 @@
 
 pragma solidity ^0.4.24;
 
-contract LibMem {
+contract LibMem
+{
 
+    /// @dev Gets the memory address for a byte array.
+    /// @param input Byte array to lookup.
+    /// @return memoryAddress Memory address of byte array.
     function getMemAddress(bytes memory input)
         internal
         pure
-        returns (uint256 address_)
+        returns (uint256 memoryAddress)
     {
         assembly {
-            address_ := input
+            memoryAddress := input
         }
-        return address_;
+        return memoryAddress;
     }
 
     /// @dev Copies `length` bytes from memory location `source` to `dest`.
-    /// @param dest memory address to copy bytes to
-    /// @param source memory address to copy bytes from
-    /// @param length number of bytes to copy
+    /// @param dest memory address to copy bytes to.
+    /// @param source memory address to copy bytes from.
+    /// @param length number of bytes to copy.
     function memcpy(
         uint256 dest,
         uint256 source,
diff --git a/packages/contracts/test/asset_proxy/decoder.ts b/packages/contracts/test/asset_proxy/decoder.ts
index 8c1253d5c..e395c04c1 100644
--- a/packages/contracts/test/asset_proxy/decoder.ts
+++ b/packages/contracts/test/asset_proxy/decoder.ts
@@ -19,7 +19,7 @@ chaiSetup.configure();
 const expect = chai.expect;
 const blockchainLifecycle = new BlockchainLifecycle(web3Wrapper);
 
-describe('LibAssetProxyDecoder', () => {
+describe('TestAssetDataDecoders', () => {
     let owner: string;
     let testAssetProxyDecoder: TestAssetDataDecodersContract;
     let testAddress: string;
@@ -43,8 +43,8 @@ describe('LibAssetProxyDecoder', () => {
         await blockchainLifecycle.revertAsync();
     });
 
-    describe('LibAssetProxyDecoder', () => {
-        it('should correctly decode ERC20 proxy data)', async () => {
+    describe('Asset Data Decoders', () => {
+        it('should correctly decode ERC20 asset data)', async () => {
             const encodedAssetData = assetProxyUtils.encodeERC20AssetData(testAddress);
             const expectedDecodedAssetData = assetProxyUtils.decodeERC20AssetData(encodedAssetData);
             let decodedAssetProxyId: number;
@@ -56,7 +56,7 @@ describe('LibAssetProxyDecoder', () => {
             expect(decodedTokenAddress).to.be.equal(expectedDecodedAssetData.tokenAddress);
         });
 
-        it('should correctly decode ERC721 proxy data', async () => {
+        it('should correctly decode ERC721 asset data', async () => {
             const tokenId = ZeroEx.generatePseudoRandomSalt();
             const encodedAssetData = assetProxyUtils.encodeERC721AssetData(testAddress, tokenId);
             const expectedDecodedAssetData = assetProxyUtils.decodeERC721AssetData(encodedAssetData);
@@ -76,25 +76,26 @@ describe('LibAssetProxyDecoder', () => {
             expect(decodedData).to.be.equal(expectedDecodedAssetData.data);
         });
 
-        it('should correctly decode ERC721 proxy data with receiver data', async () => {
+        it('should correctly decode ERC721 asset data with receiver data', async () => {
             const tokenId = ZeroEx.generatePseudoRandomSalt();
-            const data = ethUtil.bufferToHex(assetProxyUtils.encodeUint256(ZeroEx.generatePseudoRandomSalt())) + 'FFFF';
-            const encodedAssetData = assetProxyUtils.encodeERC721AssetData(testAddress, tokenId, data);
+            const receiverData =
+                ethUtil.bufferToHex(assetProxyUtils.encodeUint256(ZeroEx.generatePseudoRandomSalt())) + 'FFFF';
+            const encodedAssetData = assetProxyUtils.encodeERC721AssetData(testAddress, tokenId, receiverData);
             const expectedDecodedAssetData = assetProxyUtils.decodeERC721AssetData(encodedAssetData);
             let decodedAssetProxyId: number;
             let decodedTokenAddress: string;
             let decodedTokenId: BigNumber;
-            let decodedData: string;
+            let decodedReceiverData: string;
             [
                 decodedAssetProxyId,
                 decodedTokenAddress,
                 decodedTokenId,
-                decodedData,
+                decodedReceiverData,
             ] = await testAssetProxyDecoder.publicDecodeERC721Data.callAsync(encodedAssetData);
             expect(decodedAssetProxyId).to.be.equal(expectedDecodedAssetData.assetProxyId);
             expect(decodedTokenAddress).to.be.equal(expectedDecodedAssetData.tokenAddress);
             expect(decodedTokenId).to.be.bignumber.equal(expectedDecodedAssetData.tokenId);
-            expect(decodedData).to.be.equal(expectedDecodedAssetData.data);
+            expect(decodedReceiverData).to.be.equal(expectedDecodedAssetData.data);
         });
     });
 });
diff --git a/packages/contracts/test/asset_proxy/proxies.ts b/packages/contracts/test/asset_proxy/proxies.ts
index f44c44045..e8c598935 100644
--- a/packages/contracts/test/asset_proxy/proxies.ts
+++ b/packages/contracts/test/asset_proxy/proxies.ts
@@ -106,7 +106,7 @@ describe('Asset Transfer Proxies', () => {
     describe('Transfer Proxy - ERC20', () => {
         describe('transferFrom', () => {
             it('should successfully transfer tokens', async () => {
-                // Construct metadata for ERC20 proxy
+                // Construct ERC20 asset data
                 const encodedAssetData = assetProxyUtils.encodeERC20AssetData(zrxToken.address);
                 // Perform a transfer from makerAddress to takerAddress
                 const erc20Balances = await erc20Wrapper.getBalancesAsync();
@@ -132,7 +132,7 @@ describe('Asset Transfer Proxies', () => {
             });
 
             it('should do nothing if transferring 0 amount of a token', async () => {
-                // Construct metadata for ERC20 proxy
+                // Construct ERC20 asset data
                 const encodedAssetData = assetProxyUtils.encodeERC20AssetData(zrxToken.address);
                 // Perform a transfer from makerAddress to takerAddress
                 const erc20Balances = await erc20Wrapper.getBalancesAsync();
@@ -158,7 +158,7 @@ describe('Asset Transfer Proxies', () => {
             });
 
             it('should throw if allowances are too low', async () => {
-                // Construct metadata for ERC20 proxy
+                // Construct ERC20 asset data
                 const encodedAssetData = assetProxyUtils.encodeERC20AssetData(zrxToken.address);
                 // Create allowance less than transfer amount. Set allowance on proxy.
                 const allowance = new BigNumber(0);
@@ -182,7 +182,7 @@ describe('Asset Transfer Proxies', () => {
             });
 
             it('should throw if requesting address is not authorized', async () => {
-                // Construct metadata for ERC20 proxy
+                // Construct ERC20 asset data
                 const encodedAssetData = assetProxyUtils.encodeERC20AssetData(zrxToken.address);
                 // Perform a transfer from makerAddress to takerAddress
                 const amount = new BigNumber(10);
@@ -258,7 +258,7 @@ describe('Asset Transfer Proxies', () => {
     describe('Transfer Proxy - ERC721', () => {
         describe('transferFrom', () => {
             it('should successfully transfer tokens', async () => {
-                // Construct metadata for ERC721 proxy
+                // Construct ERC721 asset data
                 const encodedAssetData = assetProxyUtils.encodeERC721AssetData(erc721Token.address, erc721MakerTokenId);
                 // Verify pre-condition
                 const ownerMakerAsset = await erc721Token.ownerOf.callAsync(erc721MakerTokenId);
@@ -281,7 +281,7 @@ describe('Asset Transfer Proxies', () => {
             });
 
             it('should not call onERC721Received when transferring to a smart contract without receiver data', async () => {
-                // Construct metadata for ERC721 proxy
+                // Construct ERC721 asset data
                 const encodedAssetData = assetProxyUtils.encodeERC721AssetData(erc721Token.address, erc721MakerTokenId);
                 // Verify pre-condition
                 const ownerMakerAsset = await erc721Token.ownerOf.callAsync(erc721MakerTokenId);
@@ -310,12 +310,14 @@ describe('Asset Transfer Proxies', () => {
             });
 
             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()));
+                // Construct ERC721 asset data
+                const receiverData = ethUtil.bufferToHex(
+                    assetProxyUtils.encodeUint256(ZeroEx.generatePseudoRandomSalt()),
+                );
                 const encodedAssetData = assetProxyUtils.encodeERC721AssetData(
                     erc721Token.address,
                     erc721MakerTokenId,
-                    data,
+                    receiverData,
                 );
                 // Verify pre-condition
                 const ownerMakerAsset = await erc721Token.ownerOf.callAsync(erc721MakerTokenId);
@@ -340,19 +342,21 @@ describe('Asset Transfer Proxies', () => {
                 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);
+                expect(tokenReceivedLog.args.data).to.be.equal(receiverData);
                 // Verify transfer was successful
                 const newOwnerMakerAsset = await erc721Token.ownerOf.callAsync(erc721MakerTokenId);
                 expect(newOwnerMakerAsset).to.be.bignumber.equal(erc721Receiver.address);
             });
 
             it('should throw if there is receiver data but contract does not have onERC721Received', async () => {
-                // Construct metadata for ERC721 proxy
-                const data = ethUtil.bufferToHex(assetProxyUtils.encodeUint256(ZeroEx.generatePseudoRandomSalt()));
+                // Construct ERC721 asset data
+                const receiverData = ethUtil.bufferToHex(
+                    assetProxyUtils.encodeUint256(ZeroEx.generatePseudoRandomSalt()),
+                );
                 const encodedAssetData = assetProxyUtils.encodeERC721AssetData(
                     erc721Token.address,
                     erc721MakerTokenId,
-                    data,
+                    receiverData,
                 );
                 // Verify pre-condition
                 const ownerMakerAsset = await erc721Token.ownerOf.callAsync(erc721MakerTokenId);
@@ -372,7 +376,7 @@ describe('Asset Transfer Proxies', () => {
             });
 
             it('should throw if transferring 0 amount of a token', async () => {
-                // Construct metadata for ERC721 proxy
+                // Construct ERC721 asset data
                 const encodedAssetData = assetProxyUtils.encodeERC721AssetData(erc721Token.address, erc721MakerTokenId);
                 // Verify pre-condition
                 const ownerMakerAsset = await erc721Token.ownerOf.callAsync(erc721MakerTokenId);
@@ -391,7 +395,7 @@ describe('Asset Transfer Proxies', () => {
             });
 
             it('should throw if transferring > 1 amount of a token', async () => {
-                // Construct metadata for ERC721 proxy
+                // Construct ERC721 asset data
                 const encodedAssetData = assetProxyUtils.encodeERC721AssetData(erc721Token.address, erc721MakerTokenId);
                 // Verify pre-condition
                 const ownerMakerAsset = await erc721Token.ownerOf.callAsync(erc721MakerTokenId);
@@ -410,7 +414,7 @@ describe('Asset Transfer Proxies', () => {
             });
 
             it('should throw if allowances are too low', async () => {
-                // Construct metadata for ERC721 proxy
+                // Construct ERC721 asset data
                 const encodedAssetData = assetProxyUtils.encodeERC721AssetData(erc721Token.address, erc721MakerTokenId);
                 // Remove transfer approval for makerAddress.
                 await web3Wrapper.awaitTransactionSuccessAsync(
@@ -429,7 +433,7 @@ describe('Asset Transfer Proxies', () => {
             });
 
             it('should throw if requesting address is not authorized', async () => {
-                // Construct metadata for ERC721 proxy
+                // Construct ERC721 asset data
                 const encodedAssetData = assetProxyUtils.encodeERC721AssetData(erc721Token.address, erc721MakerTokenId);
                 // Perform a transfer from makerAddress to takerAddress
                 const amount = new BigNumber(1);