v0.0.3-ac.2

First pass of the review to v0.0.3-ac.1

Author: GNSPS

README.md

@@ -6,7 +6,7 @@ Bytes tightly packed arrays utility library for ethereum contracts written.
 
 The library lets you concatenate, slice and type cast bytes arrays both in memory and storage.
 
-Given this library has an all-internal collection of methods it doesn't make sense in having it reside in the mainnet. Instead it will only be available in EPM as an installable package.
+Given this library has an all-internal collection of methods it doesn't make sense having it reside in the mainnet. Instead it will only be available in EPM as an installable package.
 
 ## Usage
 
@@ -49,6 +49,15 @@ TODOs:
 * Slicing directly from storage
 * Implement inline assembly functions for better readability
 
+### Testing
+
+This project uses Truffle for tests. Truffle's version of `solc` needs to be at least 0.4.19 for the contracts to compile. If you encounter compilation errors, try:
+
+    $ cd /usr/local/lib/node_modules/truffle
+    $ npm install solc@latest
+
+To run the tests, start a `testrpc` instance, then run `truffle test`.
+
 ## API
 
 * `function concat(bytes memory _preBytes, bytes memory _postBytes) internal pure returns (bytes)`

contracts/AssertBytes.sol

@@ -1,4 +1,4 @@
-pragma solidity 0.4.19;
+pragma solidity ^0.4.19;
 
 
 library AssertBytes {

contracts/BytesLib.sol

@@ -1,34 +1,55 @@
-pragma solidity 0.4.19;
+pragma solidity ^0.4.19;
 
 
 library BytesLib {
     function concat(bytes memory _preBytes, bytes memory _postBytes) internal pure returns (bytes) {
         bytes memory tempBytes;
-        
+
         assembly {
+            // Get a location of some free memory and store it in tempBytes as
+            // Solidity does for memory variables.
             tempBytes := mload(0x40)
 
+            // Store the length of the first bytes array at the beginning of
+            // the memory for tempBytes.
             let length := mload(_preBytes)
             mstore(tempBytes, length)
-            
+
+            // Maintain a memory counter for the current write location in the
+            // temp bytes array by adding the 32 bytes for the array length to
+            // the starting location.
             let mc := add(tempBytes, 0x20)
+            // Stop copying when the memory counter reaches the length of the
+            // first bytes array.
             let end := add(mc, length)
-            
+
             for {
+                // Initialize a copy counter to the start of the _preBytes data,
+                // 32 bytes into its memory.
                 let cc := add(_preBytes, 0x20)
             } lt(mc, end) {
+                // Increase both counters by 32 bytes each iteration.
                 mc := add(mc, 0x20)
                 cc := add(cc, 0x20)
             } {
+                // Write the _preBytes data into the tempBytes memory 32 bytes
+                // at a time.
                 mstore(mc, mload(cc))
             }
-            
+
+            // Add the length of _postBytes to the current length of tempBytes
+            // and store it as the new length in the first 32 bytes of the
+            // tempBytes memory.
             length := mload(_postBytes)
             mstore(tempBytes, add(length, mload(tempBytes)))
-            
+
+            // Move the memory counter back from a multiple of 0x20 to the
+            // actual end of the _preBytes data.
             mc := end
+            // Stop copying when the memory counter reaches the new combined
+            // length of the arrays.
             end := add(mc, length)
-            
+
             for {
                 let cc := add(_postBytes, 0x20)
             } lt(mc, end) {
@@ -37,21 +58,34 @@ library BytesLib {
             } {
                 mstore(mc, mload(cc))
             }
-            
-            //update free-memory pointer
-            //allocating the array padded to 32 bytes like the compiler does now
-            //make an additional check for a resulting zero-length array:
-            //  if (sub - end == 0) then end = end + 1
-            mstore(0x40, and(add(add(end, iszero(sub(mc, end))), 31), not(31)))
+
+            // Update the free-memory pointer by padding our last write location
+            // to 32 bytes: add 31 bytes to the end of tempBytes to move to the
+            // next 32 byte block, then round down to the nearest multiple of
+            // 32. If the sum of the length of the two arrays is zero then add 
+            // one before rounding down to leave a blank 32 bytes (the length block with 0).
+            mstore(0x40, and(
+              add(add(end, iszero(add(length, mload(_preBytes)))), 31),
+              not(31) // Round down to the nearest 32 bytes.
+            ))
         }
-        
+
         return tempBytes;
     }
-    
+
     function concatStorage(bytes storage _preBytes, bytes memory _postBytes) internal {
         assembly {
-            // we know _preBytes_offset is 0
+            // Read the first 32 bytes of _preBytes storage, which is the length
+            // of the array. (We don't need to use the offset into the slot
+            // because arrays use the entire slot.)
             let fslot := sload(_preBytes_slot)
+            // Arrays of 31 bytes or less have an even value in their slot,
+            // while longer arrays have an odd value. The actual length is
+            // the slot divided by two for odd values, and the lowest order
+            // byte divided by two for even values.
+            // If the slot is even, bitwise and the slot with 255 and divide by
+            // two to get the length. If the slot is odd, bitwise and the slot
+            // with -1 and divide by two.
             let slength := div(and(fslot, sub(mul(0x100, iszero(and(fslot, 1))), 1)), 2)
             let mlength := mload(_postBytes)
             let newlength := add(slength, mlength)
@@ -60,13 +94,15 @@ library BytesLib {
             // v. http://solidity.readthedocs.io/en/latest/miscellaneous.html#layout-of-state-variables-in-storage
             switch add(lt(slength, 32), lt(newlength, 32))
             case 2 {
+                // Since the new array still fits in the slot, we just need to
+                // update the contents of the slot.
                 // uint256(bytes_storage) = uint256(bytes_storage) + uint256(bytes_memory) + new_length
                 sstore(
                     _preBytes_slot,
                     // all the modifications to the slot are inside this
                     // next block
                     add(
-                        // we can just add to the slot contents because the 
+                        // we can just add to the slot contents because the
                         // bytes we want to change are the LSBs
                         fslot,
                         add(
@@ -89,18 +125,29 @@ library BytesLib {
                 )
             }
             case 1 {
+                // The stored value fits in the slot, but the combined value
+                // will exceed it.
                 // get the keccak hash to get the contents of the array
                 mstore(0x0, _preBytes_slot)
                 let sc := add(keccak256(0x0, 0x20), div(slength, 32))
-                
+
                 // save new length
                 sstore(_preBytes_slot, add(mul(newlength, 2), 1))
-                
+
+                // The contents of the _postBytes array start 32 bytes into
+                // the structure. Our first read should obtain the `submod`
+                // bytes that can fit into the unused space in the last word
+                // of the stored array. To get this, we read 32 bytes starting
+                // from `submod`, so the data we read overlaps with the array
+                // contents by `submod` bytes. Masking the lowest-order
+                // `submod` bytes allows us to add that value directly to the
+                // stored value.
+
                 let submod := sub(32, slength)
                 let mc := add(_postBytes, submod)
-                let end := add(add(_postBytes, 0x20), mlength)
+                let end := add(_postBytes, mlength)
                 let mask := sub(exp(0x100, submod), 1)
-                
+
                 sstore(
                     sc,
                     add(
@@ -111,8 +158,8 @@ library BytesLib {
                         and(mload(mc), mask)
                     )
                 )
-                
-                for { 
+
+                for {
                     mc := add(mc, 0x20)
                     sc := add(sc, 1)
                 } lt(mc, end) {
@@ -121,50 +168,73 @@ library BytesLib {
                 } {
                     sstore(sc, mload(mc))
                 }
+
+                mask := exp(0x100, sub(mc, end))
+
+                sstore(sc, mul(div(mload(mc), mask), mask))
             }
             default {
                 // get the keccak hash to get the contents of the array
                 mstore(0x0, _preBytes_slot)
+                // Start copying to the last used word of the stored array.
                 let sc := add(keccak256(0x0, 0x20), div(slength, 32))
-                
+
                 // save new length
                 sstore(_preBytes_slot, add(mul(newlength, 2), 1))
-                
+
+                // Copy over the first `submod` bytes of the new data as in
+                // case 1 above.
                 let slengthmod := mod(slength, 32)
+                let mlengthmod := mod(mlength, 32)
                 let submod := sub(32, slengthmod)
                 let mc := add(_postBytes, submod)
-                let end := add(mc, mlength)
+                let end := add(_postBytes, mlength)
                 let mask := sub(exp(0x100, submod), 1)
-                
+
                 sstore(sc, add(sload(sc), and(mload(mc), mask)))
 
                 for { 
+                    sc := add(sc, 1)
                     mc := add(mc, 0x20)
                 } lt(mc, end) {
                     sc := add(sc, 1)
                     mc := add(mc, 0x20)
                 } {
                     sstore(sc, mload(mc))
                 }
+
+                mask := exp(0x100, sub(mc, end))
+
+                sstore(sc, mul(div(mload(mc), mask), mask))
             }
         }
     }
-    
+
     function slice(bytes _bytes, uint _start, uint _length) internal  pure returns (bytes) {
         require(_bytes.length >= (_start + _length));
-        
+
         bytes memory tempBytes;
-        
+
         assembly {
             switch iszero(_length)
             case 0 {
+                // Get a location of some free memory and store it in tempBytes as
+                // Solidity does for memory variables.
                 tempBytes := mload(0x40)
-                
+
+                // The first word of the slice result is potentially a partial
+                // word read from the original array. To read it, we calculate
+                // the length of that partial word and start copying that many
+                // bytes into the array. The first word we copy will start with
+                // data we don't care about, but the last `lengthmod` bytes will
+                // land at the beginning of the contents of the new array. When
+                // we're done copying, we overwrite the full first word with
+                // the actual length of the slice.
                 let lengthmod := and(_length, 31)
-                
+
                 let mc := add(tempBytes, lengthmod)
                 let end := add(mc, _length)
-                
+
                 for {
                     let cc := add(add(_bytes, lengthmod), _start)
                 } lt(mc, end) {
@@ -173,9 +243,9 @@ library BytesLib {
                 } {
                     mstore(mc, mload(cc))
                 }
-                
+
                 mstore(tempBytes, _length)
-                
+
                 //update free-memory pointer
                 //allocating the array padded to 32 bytes like the compiler does now
                 mstore(0x40, and(add(mc, 31), not(31)))
@@ -187,29 +257,29 @@ library BytesLib {
                 mstore(0x40, add(tempBytes, 0x20))
             }
         }
-        
+
         return tempBytes;
     }
-    
+
     function toAddress(bytes _bytes, uint _start) internal  pure returns (address) {
         require(_bytes.length >= (_start + 20));
         address tempAddress;
-        
+
         assembly {
             tempAddress := div(mload(add(add(_bytes, 0x20), _start)), 0x1000000000000000000000000)
         }
-        
+
         return tempAddress;
     }
-    
+
     function toUint(bytes _bytes, uint _start) internal  pure returns (uint256) {
         require(_bytes.length >= (_start + 32));
         uint256 tempUint;
-        
+
         assembly {
             tempUint := mload(add(add(_bytes, 0x20), _start))
         }
-        
+
         return tempUint;
     }
 
@@ -222,7 +292,7 @@ library BytesLib {
             // if lengths don't match the arrays are not equal
             switch eq(length, mload(_postBytes))
             case 1 {
-                // cb is a circuit breaker in the for loop since there's 
+                // cb is a circuit breaker in the for loop since there's
                 //  no said feature for inline assembly loops
                 // cb = 1 - don't breaker
                 // cb = 0 - break
@@ -262,6 +332,7 @@ library BytesLib {
         assembly {
             // we know _preBytes_offset is 0
             let fslot := sload(_preBytes_slot)
+            // Decode the length of the stored array like in concatStorage().
             let slength := div(and(fslot, sub(mul(0x100, iszero(and(fslot, 1))), 1)), 2)
             let mlength := mload(_postBytes)
 
@@ -283,7 +354,7 @@ library BytesLib {
                         }
                     }
                     default {
-                        // cb is a circuit breaker in the for loop since there's 
+                        // cb is a circuit breaker in the for loop since there's
                         //  no said feature for inline assembly loops
                         // cb = 1 - don't breaker
                         // cb = 0 - break
@@ -292,7 +363,7 @@ library BytesLib {
                         // get the keccak hash to get the contents of the array
                         mstore(0x0, _preBytes_slot)
                         let sc := keccak256(0x0, 0x20)
-                        
+
                         let mc := add(_postBytes, 0x20)
                         let end := add(mc, mlength)
 

ethpm.json

@@ -1,6 +1,6 @@
 {
   "package_name": "bytes",
-  "version": "0.0.3",
+  "version": "0.0.3-ac.2",
   "description": "Solidity bytes tightly packed arrays utility library.",
   "authors": [
     "Gonçalo Sá <[email protected]>"

package.json

@@ -1,6 +1,6 @@
 {
   "name": "solidity-bytes-utils",
-  "version": "0.0.3",
+  "version": "0.0.3-ac.2",
   "description": "Solidity bytes tightly packed arrays utility library.",
   "main": "truffle.js",
   "repository": {