ETokenExplicitSender minor fixes (#79)

* Removed unused imported contract

* Finished incomplete comment

* Changed event UpgradeFinalized to only emit one address

* Removed FIXME comment

* Added senderIsProxy modifier to finalizeUpgrade

* Changed formatting on finalizeUpgrade

* Updated Event tests

* Modified incorrect comments

* Fixed tests

* Made requested changes to comments

* Added upgradeExists modifier to UpgradeFinalized

* Reinserted tests

contracts/token/ETokenExplicitSender.sol

@@ -8,7 +8,6 @@ import "./ERC20/ExternalERC20Mintable.sol";
 import "../lifecycle/Pausable.sol";
 import "openzeppelin-solidity/contracts/token/ERC20/ERC20Detailed.sol";
 import "../access/roles/BurnerRole.sol";
-import "../access/roles/MinterRole.sol";
 import "../access/Accesslist.sol";
 import "../access/AccesslistGuarded.sol";
 import "./IUpgradableEToken.sol";
@@ -26,13 +25,13 @@ contract ETokenExplicitSender is IUpgradableEToken,
 {
 
     /**
-     * @dev Holds the address of the
+     * @dev Holds the address of the contract that was upgraded from
      */
     address private _upgradedFrom;
 
     bool private enabled;
 
-    event UpgradeFinalized(address indexed upgradedFrom, address indexed sender);
+    event UpgradeFinalized(address indexed upgradedFrom);
 
     /**
      * @param name The name of the token
@@ -85,11 +84,18 @@ contract ETokenExplicitSender is IUpgradableEToken,
      * @dev Called by the upgraded contract in order to mark the finalization of
      * the upgrade and activate the new contract
      */
-    function finalizeUpgrade() external {
-        require(_upgradedFrom != address(0), "Must have a contract to upgrade from");
-        require(msg.sender == _upgradedFrom, "Sender is not old contract");
+    function finalizeUpgrade()
+        external
+        upgradeExists
+        senderIsProxy
+    {
         enabled = true;
-        emit UpgradeFinalized(_upgradedFrom, msg.sender);
+        emit UpgradeFinalized(msg.sender);
+    }
+
+    modifier upgradeExists() {
+        require(_upgradedFrom != address(0), "Must have a contract to upgrade from");
+        _;
     }
 
     /**
@@ -249,7 +255,6 @@ contract ETokenExplicitSender is IUpgradableEToken,
         isEnabled
         senderIsProxy
         whenNotPaused
-        // FIXME: This used to be spender spender. Why wasn't this caught in tests?
         requireHasAccess(spender)
         requireHasAccess(sender)
         returns (bool)
@@ -394,10 +399,10 @@ contract ETokenExplicitSender is IUpgradableEToken,
     }
 
     /**
-     * @dev Like EToken.transfer, but gets sender from
-     * explicit sender parameter rather than msg.sender. This function
-     * can only be called from the proxy contract (the contract that
-     * this contract upgraded).
+     * @dev Like EToken.transfer. Transfers tokens to a specified address using
+     * modifiers to ensure the caller is allowed to make the call.
+     * @param to The address to transfer to
+     * @param value the amount to be transferred
      */
     function transfer(address to, uint256 value)
         public
@@ -411,10 +416,10 @@ contract ETokenExplicitSender is IUpgradableEToken,
     }
 
     /**
-     * @dev Like EToken.approve, but gets sender from
-     * explicit sender parameter rather than msg.sender. This function
-     * can only be called from the proxy contract (the contract that
-     * this contract upgraded).
+     * @dev Like EToken.approve. Approves passed address to spend specified
+     * amount on their behalf
+     * @param spender The address which will spend the funds
+     * @param value The amount of tokens to be spent.
      */
     function approve(address spender, uint256 value)
         public
@@ -428,10 +433,10 @@ contract ETokenExplicitSender is IUpgradableEToken,
     }
 
     /**
-     * @dev Like EToken.transferFrom, but gets sender from
-     * explicit sender parameter rather than msg.sender. This function
-     * can only be called from the proxy contract (the contract that
-     * this contract upgraded).
+     * @dev Like EToken.transferFrom. Transfers tokens from one address to another
+     * @param from The address to send tokens from
+     * @param to The address to transfer to
+     * @param value the amount of tokens to be transferred 
      */
     function transferFrom(address from, address to, uint256 value)
         public
@@ -446,10 +451,10 @@ contract ETokenExplicitSender is IUpgradableEToken,
     }
 
     /**
-     * @dev Like EToken.increaseAllowance, but gets sender from
-     * explicit sender parameter rather than msg.sender. This function
-     * can only be called from the proxy contract (the contract that
-     * this contract upgraded).
+     * @dev Like EToken.increaseAllowance. Increase the amount of tokens spender
+     * @dev is allowed to spend
+     * @param spender The address which will spend the funds.
+     * @param addedValue The amount of tokens to increase the allowance by.
      */
     function increaseAllowance(address spender, uint256 addedValue)
         public
@@ -463,10 +468,10 @@ contract ETokenExplicitSender is IUpgradableEToken,
     }
 
     /**
-     * @dev Like EToken.decreaseAllowance, but gets sender from
-     * explicit sender parameter rather than msg.sender. This function
-     * can only be called from the proxy contract (the contract that
-     * this contract upgraded).
+     * @dev Like EToken.decreaseAllowance. Decrease the amount of tokens allowed
+     * @dev to spender
+     * @param spender The address which will spend the funds.
+     * @param subtractedValue The amount of tokens to decrease the allowance by.
      */
     function decreaseAllowance(address spender, uint256 subtractedValue)
         public

test/token/EToken.events.js

@@ -3,7 +3,6 @@ const utils = require('./../utils.js');
 module.exports = utils.makeEventMap({
   upgrade: (contract, oldContract) => [
     { eventName: 'UpgradeFinalized',
-      paramMap: { upgradedFrom: oldContract,
-                  sender: oldContract } },
+      paramMap: { upgradedFrom: oldContract} },
     { eventName: 'Upgraded',
       paramMap: { to: contract } }] });

test/token/ETokenExplicitSender.events.js

@@ -1,7 +1,6 @@
 const utils = require('./../utils.js');
 
 module.exports = utils.makeEventMap({
-  finalizeUpgrade: (contract, senderAddr) => [{
+  finalizeUpgrade: (contract) => [{
     eventName: 'UpgradeFinalized',
-    paramMap: { upgradedFrom: contract,
-                sender: senderAddr } }] });
+    paramMap: { upgradedFrom: contract} }] });

test/token/ETokenExplicitSender.test.js

@@ -30,7 +30,7 @@ contract('ETokenExplicitSender', function ([owner, someAddress, ...rest]) {
       const token = await ETokenExplicitSender.new('tok', 't', 10,
                                                    accesslist.address, true, storage.address,
                                                    0, true, { from: owner });
-      await util.assertRevertsReason(token.finalizeUpgrade({ from: someAddress }),
+      await util.assertRevertsReason(token.finalizeUpgrade({ from: owner }),
                                      'Must have a contract to upgrade from');
     });
 
@@ -39,7 +39,7 @@ contract('ETokenExplicitSender', function ([owner, someAddress, ...rest]) {
                                                    accesslist.address, true, storage.address,
                                                    0xf00f, false, { from: owner });
       await util.assertRevertsReason(token.finalizeUpgrade({ from: someAddress }),
-                                     'Sender is not old contract');
+                                     'Proxy is the only allowed caller');
     });
 
     it('emits UpgradeFinalized event', async function () {
@@ -49,7 +49,7 @@ contract('ETokenExplicitSender', function ([owner, someAddress, ...rest]) {
 
       const tokenE = ETokenExplicitSenderE.wrap(token);
 
-      await tokenE.finalizeUpgrade(owner, owner, { from: owner });
+      await tokenE.finalizeUpgrade(owner, { from: owner });
     });
   });
   // Remainder of contract tested through EToken.