Merge pull request CoinAlpha#44 from yzhang1994/master

Change contract based on Hosho review

Author: Carlo P. Las Marias

.gitignore

@@ -9,4 +9,4 @@ archive/*
 coverage/*
 coverage.json
 coverageEnv/*
-.coveralls.yml
+.coveralls.yml

contracts/Basket.sol

@@ -103,7 +103,7 @@ contract Basket is StandardToken {
     for (uint i = 0; i < tokens.length; i++) {
       address t = tokens[i];
       uint w = weights[i];
-      assert(ERC20(t).transferFrom(msg.sender, this, w.mul(_quantity).div(10 ** decimals)));
+      require(ERC20(t).transferFrom(msg.sender, this, w.mul(_quantity).div(10 ** decimals)));
     }
 
     // charging suppliers a fee for every new basket minted
@@ -129,7 +129,7 @@ contract Basket is StandardToken {
   /// @param  _quantity                            Quantity of basket tokens to convert back to original tokens
   /// @return success                              Operation successful
   function debundleAndWithdraw(uint _quantity) public returns (bool success) {
-    assert(debundle(_quantity, msg.sender, msg.sender));
+    require(debundle(_quantity, msg.sender, msg.sender));
     emit LogDebundleAndWithdraw(msg.sender, _quantity);
     return true;
   }
@@ -185,7 +185,7 @@ contract Basket is StandardToken {
     uint bal = outstandingBalance[msg.sender][_token];
     require(bal > 0);
     outstandingBalance[msg.sender][_token] = 0;
-    assert(ERC20(_token).transfer(msg.sender, bal));
+    require(ERC20(_token).transfer(msg.sender, bal));
 
     emit LogWithdraw(msg.sender, _token, bal);
     return true;

contracts/BasketEscrow.sol

@@ -136,7 +136,7 @@ contract BasketEscrow {
     public
     returns (bool success)
   {
-    assert(ERC20(_basketAddress).transferFrom(msg.sender, this, _amountBasket));
+    require(ERC20(_basketAddress).transferFrom(msg.sender, this, _amountBasket));
     uint index = _createOrder(msg.sender, ETH_ADDRESS, _amountEth, _basketAddress, _amountBasket, _expiration, _nonce);
 
     emit LogSellOrderCreated(index, msg.sender, _basketAddress, _amountEth, _amountBasket, _expiration, _nonce);
@@ -164,6 +164,7 @@ contract BasketEscrow {
     internal
     returns (uint newOrderIndex)
   {
+    require(_expiration > now);
     require(_tokenGet == ETH_ADDRESS || basketRegistry.checkBasketExists(_tokenGet));   // Check: "Order not for ETH or invalid basket"
     require(_tokenGive == ETH_ADDRESS || basketRegistry.checkBasketExists(_tokenGive)); // Check: "Order not for ETH or invalid basket"
 
@@ -222,7 +223,7 @@ contract BasketEscrow {
   ) public returns (bool success) {
     uint cancelledOrderIndex = _cancelOrder(msg.sender, ETH_ADDRESS, _amountEth, _basketAddress, _amountBasket, _expiration, _nonce);
 
-    assert(ERC20(_basketAddress).transfer(msg.sender, _amountBasket));
+    require(ERC20(_basketAddress).transfer(msg.sender, _amountBasket));
 
     emit LogSellOrderCancelled(cancelledOrderIndex, msg.sender, _basketAddress, _amountEth, _amountBasket);
     return true;
@@ -277,7 +278,7 @@ contract BasketEscrow {
     uint      _nonce
   ) public returns (bool success) {
     uint filledOrderIndex = _fillOrder(_orderCreator, _basketAddress, _amountBasket, ETH_ADDRESS, _amountEth, _expiration, _nonce);
-    assert(ERC20(_basketAddress).transferFrom(msg.sender, _orderCreator, _amountBasket));
+    require(ERC20(_basketAddress).transferFrom(msg.sender, _orderCreator, _amountBasket));
 
     uint fee = _amountEth.mul(transactionFee).div(10 ** FEE_DECIMALS);
     msg.sender.transfer(_amountEth.sub(fee));
@@ -302,7 +303,7 @@ contract BasketEscrow {
     uint      _nonce
   ) public payable returns (bool success) {
     uint filledOrderIndex = _fillOrder(_orderCreator, ETH_ADDRESS, msg.value, _basketAddress, _amountBasket, _expiration, _nonce);
-    assert(ERC20(_basketAddress).transfer(msg.sender, _amountBasket));
+    require(ERC20(_basketAddress).transfer(msg.sender, _amountBasket));
 
     uint fee = msg.value.mul(transactionFee).div(10 ** FEE_DECIMALS);
     _orderCreator.transfer(msg.value.sub(fee));

contracts/BasketRegistry.sol

@@ -17,6 +17,8 @@
 
 pragma solidity 0.4.21;
 
+import "./zeppelin/SafeMath.sol";
+
 
 contract IBasketRegistry {
   // Called by BasketFactory
@@ -35,6 +37,8 @@ contract IBasketRegistry {
   * @author CoinAlpha, Inc. <[email protected]>
   */
 contract BasketRegistry {
+  using SafeMath for uint;
+
   // Constants set at contract inception
   address                           public admin;
   mapping(address => bool)          public basketFactoryMap;
@@ -123,13 +127,13 @@ contract BasketRegistry {
     if (arrangerBasketCount[_arranger] == 0) {
       arrangerList.push(_arranger);
       arrangerIndexFromAddress[_arranger] = arrangerIndex;
-      arrangerIndex += 1;
+      arrangerIndex = arrangerIndex.add(1);
     }
-    arrangerBasketCount[_arranger] += 1;
+    arrangerBasketCount[_arranger] = arrangerBasketCount[_arranger].add(1);
 
     emit LogBasketRegistration(_basketAddress, basketIndex);
-    basketIndex += 1;
-    return basketIndex - 1;
+    basketIndex = basketIndex.add(1);
+    return basketIndex.sub(1);
   }
 
   /// @dev Check if basket exists in registry
@@ -172,7 +176,7 @@ contract BasketRegistry {
   /// @param  _sender                              Address that bundled tokens
   /// @return success                              Operation successful
   function incrementBasketsMinted(uint _quantity, address _sender) public onlyBasket returns (bool) {
-    basketMap[msg.sender].totalMinted += _quantity;
+    basketMap[msg.sender].totalMinted = basketMap[msg.sender].totalMinted.add(_quantity);
     emit LogIncrementBasketsMinted(msg.sender, _quantity, _sender);
     return true;
   }
@@ -182,7 +186,7 @@ contract BasketRegistry {
   /// @param  _sender                              Address that debundled tokens
   /// @return success                              Operation successful
   function incrementBasketsBurned(uint _quantity, address _sender) public onlyBasket returns (bool) {
-    basketMap[msg.sender].totalBurned += _quantity;
+    basketMap[msg.sender].totalBurned = basketMap[msg.sender].totalBurned.add(_quantity);
     emit LogIncrementBasketsBurned(msg.sender, _quantity, _sender);
     return true;
   }

test/1_bundle_tokens.js

@@ -210,7 +210,7 @@ contract('TestToken | Basket', (accounts) => {
     it('should allow HOLDER_A to depositAndBundle', async () => {
       try {
         const fee = await basketAB.arrangerFee.call();
-        const insufficientFee = (amount1 / 2) * (Number(fee) / (10 ** FEE_DECIMALS));
+        const insufficientFee = (amount1 / 3) * (Number(fee) / (10 ** FEE_DECIMALS));
         await basketAB.depositAndBundlePromise(amount1, { from: HOLDER_A, value: insufficientFee, gas: 1e6 });
       } catch (err) { assert.equal(doesRevert(err), true, 'did not revert as expected'); }
     });
@@ -432,8 +432,24 @@ contract('TestToken | Basket', (accounts) => {
       try {
         await basketAB.withdrawPromise(tokenB.address, { from: HOLDER_A });
         const _balTokenB = await tokenB.balanceOf(HOLDER_A);
+        const _outstandingTokenB = Number(basketAB.outstandingBalance(HOLDER_A, tokenB.address));
         assert.strictEqual(Number(_balTokenB), Number(balTokenB) + amountToDebundle, 'tokenB balance did not increase');
+        assert.strictEqual(_outstandingTokenB, 0, 'tokenB balance did not decrease');
       } catch (err) { assert.throw(`after error: ${err.toString()}`); }
     });
+
+    it('HOLDER_A should revert transfers of locked tokens', async () => {
+      try {
+        await basketAB.withdrawPromise(tokenA.address, { from: HOLDER_A });
+      } catch (err) { assert.equal(doesRevert(err), true, 'did not revert as expected'); }
+    });
+  });
+
+  describe('Does not allow withdraw when balance is zero', async () => {
+    it('HOLDER_B should not be able to withdraw a single token', async () => {
+      try {
+        await basketAB.withdrawPromise(tokenB.address, { from: HOLDER_B });
+      } catch (err) { assert.equal(doesRevert(err), true, 'did not revert as expected'); }
+    });
   });
 });

test/3_exchange_baskets.js

@@ -233,14 +233,29 @@ contract('Basket Escrow', (accounts) => {
       } catch (err) { assert.equal(doesRevert(err), true, 'did not revert as expected'); }
     });
 
+    it('creates and logs buy orders ', async () => {
+      const instantExpiration = 0;
+      try {
+        // exact same params as last order
+        nonce = Math.random() * 1e7;
+        const buyOrderParams = [
+          basketABAddress, amountBasketsToBuy, instantExpiration, nonce,
+          { from: HOLDER_A, value: amountEthToSend, gas: 1e6 },
+        ];
+        await basketEscrow.createBuyOrder(...buyOrderParams);
+      } catch (err) { assert.equal(doesRevert(err), true, 'did not revert as expected'); }
+    });
+
     after('update nonce', () => { nonce = Math.random() * 1e7; });
   });
 
   describe('Holder_A cancels expired buy order', () => {
+    const timeDelta = 10;   // expires in 10 seconds
+
     before('create second buy order and check initial balance', async () => {
       try {
         // set expiration time to now to ensure the order will expire
-        expirationInSeconds = 0;
+        expirationInSeconds = (new Date().getTime() / 1000) + timeDelta;
         const buyOrderParams = [
           basketABAddress, amountBasketsToBuy, expirationInSeconds, nonce,
           { from: HOLDER_A, value: amountEthToSend, gas: 1e6 },
@@ -256,6 +271,12 @@ contract('Basket Escrow', (accounts) => {
       } catch (err) { assert.throw(`Error creating second buy order: ${err.toString()}`); }
     });
 
+    it('waits <timeDelta> seconds before proceeding', async () => {
+      await new Promise((resolve) => {
+        setTimeout(() => { resolve(); }, timeDelta * 1000);
+      });
+    });
+
     it('allows and logs cancellation of buy orders ', async () => {
       try {
         const cancelBuyParams = [
@@ -432,9 +453,11 @@ contract('Basket Escrow', (accounts) => {
     });
   });
 
-  const instantExpiration = 0;
 
   describe('MARKET_MAKER fails to fill expired orders', () => {
+    const timeDelta = 20;   // 10 seconds
+    const instantExpiration = (new Date().getTime() / 1000) + timeDelta;
+
     before('creates an order that expires instantly', async () => {
       try {
         nonce = Math.random() * 1e7;
@@ -443,17 +466,19 @@ contract('Basket Escrow', (accounts) => {
           { from: HOLDER_A, value: amountEthToSend, gas: 1e6 },
         ];
         await basketEscrow.createBuyOrder(...buyOrderParams);
-      } catch (err) { assert.throw(`Error in creating instantly expired order: ${err.toString()}`); }
+      } catch (err) { assert.throw(`Error in creating expired order: ${err.toString()}`); }
     });
 
     it('cannot fill an expired order', async () => {
-      try {
-        const fillBuyParams = [
-          HOLDER_B, basketABAddress, amountBasketsToBuy, amountEthToSend, instantExpiration, nonce,
-          { from: MARKET_MAKER, gas: 1e6 },
-        ];
-        const _fillBuyResults = await basketEscrow.fillBuyOrder(...fillBuyParams);
-      } catch (err) { assert.equal(doesRevert(err), true, 'did not revert as expected'); }
+      setTimeout(async () => {
+        try {
+          const fillBuyParams = [
+            HOLDER_B, basketABAddress, amountBasketsToBuy, amountEthToSend, instantExpiration, nonce,
+            { from: MARKET_MAKER, gas: 1e6 },
+          ];
+          const _fillBuyResults = await basketEscrow.fillBuyOrder(...fillBuyParams);
+        } catch (err) { assert.equal(doesRevert(err), true, 'did not revert as expected'); }
+      }, timeDelta * 1000);
     });
   });