Respond to review comments II

Author: kronosapiens

contracts/colony/Colony.sol

@@ -365,9 +365,9 @@ contract Colony is ColonyStorage, PatriciaTreeProofs, MultiChain {
   }
 
   function installExtension(bytes32 _extensionId, uint256 _version)
-  public stoppable auth returns (address)
+  public stoppable auth
   {
-    return IColonyNetwork(colonyNetworkAddress).installExtension(_extensionId, _version);
+    IColonyNetwork(colonyNetworkAddress).installExtension(_extensionId, _version);
   }
 
   function upgradeExtension(address _extension, uint256 _newVersion)
@@ -388,6 +388,12 @@ contract Colony is ColonyStorage, PatriciaTreeProofs, MultiChain {
     IColonyNetwork(colonyNetworkAddress).uninstallExtension(_extension);
   }
 
+  function migrateToMultiExtension(bytes32 _extensionId)
+  public stoppable auth
+  {
+    IColonyNetwork(colonyNetworkAddress).migrateToMultiExtension(_extensionId);
+  }
+
   function addDomain(uint256 _permissionDomainId, uint256 _childSkillIndex, uint256 _parentDomainId) public
   stoppable
   authDomain(_permissionDomainId, _childSkillIndex, _parentDomainId)

contracts/colony/ColonyAuthority.sol

@@ -120,6 +120,7 @@ contract ColonyAuthority is CommonAuthority {
     addRoleCapability(ROOT_ROLE, "upgradeExtension(address,uint256)");
     addRoleCapability(ROOT_ROLE, "deprecateExtension(address,bool)");
     addRoleCapability(ROOT_ROLE, "uninstallExtension(address)");
+    addRoleCapability(ROOT_ROLE, "migrateToMultiExtension(bytes32)");
     addRoleCapability(ARBITRATION_ROLE, "setExpenditureMetadata(uint256,uint256,uint256,string)");
   }
 

contracts/colony/ColonyStorage.sol

@@ -296,7 +296,7 @@ contract ColonyStorage is CommonStorage, ColonyDataTypes, ColonyNetworkDataTypes
     // Ensure addr is an extension installed in the colony, must check old & new formats
     try ColonyExtension(addr).identifier() returns (bytes32 extensionId) {
       return (
-        IColonyNetwork(colonyNetworkAddress).getExtensionMultiInstallation(addr) == address(this) ||
+        IColonyNetwork(colonyNetworkAddress).getExtensionColony(addr) == address(this) ||
         IColonyNetwork(colonyNetworkAddress).getExtensionInstallation(extensionId, address(this)) == addr
       );
     } catch {

contracts/colony/IColony.sol

@@ -263,8 +263,7 @@ interface IColony is ColonyDataTypes, IRecovery {
   /// @notice Install an extension to the colony. Secured function to authorised members.
   /// @param extensionId keccak256 hash of the extension name, used as an indentifier
   /// @param version The new extension version to install
-  /// @return extension The address of the extension installation
-  function installExtension(bytes32 extensionId, uint256 version) external returns (address extension);
+  function installExtension(bytes32 extensionId, uint256 version) external;
 
   /// @notice Upgrade an extension in a colony. Secured function to authorised members.
   /// @param extension The address of the extension installation
@@ -282,6 +281,10 @@ interface IColony is ColonyDataTypes, IRecovery {
   /// @param extension The address of the extension installation
   function uninstallExtension(address extension) external;
 
+  /// @notice Migrate extension bookkeeping to multiExtension. Secured function to authorised members.
+  /// @param extensionId keccak256 hash of the extension name, used as an indentifier
+  function migrateToMultiExtension(bytes32 extensionId) external;
+
   /// @notice Add a colony domain, and its respective local skill under skill with id `_parentSkillId`.
   /// New funding pot is created and associated with the domain here.
   /// @param _permissionDomainId The domainId in which I have the permission to take this action

contracts/colonyNetwork/ColonyNetworkDataTypes.sol

@@ -114,7 +114,13 @@ interface ColonyNetworkDataTypes {
   /// @param version The version of the extension
   event ExtensionAddedToNetwork(bytes32 indexed extensionId, uint256 version);
 
-  /// @notice Event logged when an extension is installed in a colony
+  /// @notice Event logged when an extension is installed in a colony (for v7 and below)
+  /// @param extensionId The identifier for the extension
+  /// @param colony The address of the colony
+  /// @param version The version of the extension
+  event ExtensionInstalled(bytes32 indexed extensionId, address indexed colony, uint256 version);
+
+  /// @notice Event logged when an extension is installed in a colony (for v8 and above)
   /// @param extensionId The identifier for the extension
   /// @param extension Address of the extension installation
   /// @param colony The address of the colony

contracts/colonyNetwork/ColonyNetworkExtensions.sol

@@ -60,14 +60,17 @@ contract ColonyNetworkExtensions is ColonyNetworkStorage {
     if (IColony(msg.sender).version() <= 7) {
       require(installations[_extensionId][msg.sender] == address(0x0), "colony-network-extension-already-installed");
       installations[_extensionId][msg.sender] = address(extension);
+
+      emit ExtensionInstalled(_extensionId, address(extension), _version);
     } else {
       multiInstallations[address(extension)] = msg.sender;
+
+      emit ExtensionInstalled(_extensionId, address(extension), msg.sender, _version);
     }
 
     extension.setResolver(resolvers[_extensionId][_version]);
     ColonyExtension(address(extension)).install(msg.sender);
 
-    emit ExtensionInstalled(_extensionId, address(extension), msg.sender, _version);
 
     return address(extension);
   }
@@ -160,15 +163,16 @@ contract ColonyNetworkExtensions is ColonyNetworkStorage {
     emit ExtensionUninstalled(_extension, msg.sender);
   }
 
-  function migrateToMultiExtension(bytes32 _extensionId, address _colony)
+  function migrateToMultiExtension(bytes32 _extensionId)
     public
     stoppable
+    calledByColony
   {
-    require(installations[_extensionId][_colony] != address(0x0), "colony-network-extension-not-installed");
+    require(installations[_extensionId][msg.sender] != address(0x0), "colony-network-extension-not-installed");
 
-    multiInstallations[installations[_extensionId][_colony]] = payable(_colony);
+    multiInstallations[installations[_extensionId][msg.sender]] = payable(msg.sender);
 
-    delete installations[_extensionId][_colony];
+    delete installations[_extensionId][msg.sender];
   }
 
   // Public view functions
@@ -181,7 +185,7 @@ contract ColonyNetworkExtensions is ColonyNetworkStorage {
     return resolvers[_extensionId][_version];
   }
 
-  function getExtensionMultiInstallation(address _extension)
+  function getExtensionColony(address _extension)
     public
     view
     returns (address)

contracts/colonyNetwork/IColonyNetwork.sol

@@ -316,8 +316,7 @@ interface IColonyNetwork is ColonyNetworkDataTypes, IRecovery {
   /// @notice Install an extension in a colony. Can only be called by a Colony.
   /// @param extensionId keccak256 hash of the extension name, used as an indentifier
   /// @param version Version of the extension to install
-  /// @return extension The address of the extension installation
-  function installExtension(bytes32 extensionId, uint256 version) external returns (address extension);
+  function installExtension(bytes32 extensionId, uint256 version) external;
 
   /// @dev DEPRECATED
   /// @notice Upgrade an extension in a colony. Can only be called by a Colony.
@@ -350,10 +349,9 @@ interface IColonyNetwork is ColonyNetworkDataTypes, IRecovery {
   /// @param extension Address of the extension installation
   function uninstallExtension(address extension) external;
 
-  /// @notice Migrate extension bookkeeping to multiExtension
+  /// @notice Migrate extension bookkeeping to multiExtension. Can only be called by a Colony.
   /// @param extensionId keccak256 hash of the extension name, used as an indentifier
-  /// @param colony Address of the colony the extension is installed in
-  function migrateToMultiExtension(bytes32 extensionId, address colony) external;
+  function migrateToMultiExtension(bytes32 extensionId) external;
 
   /// @notice Get an extension's resolver.
   /// @param extensionId keccak256 hash of the extension name, used as an indentifier
@@ -370,7 +368,7 @@ interface IColonyNetwork is ColonyNetworkDataTypes, IRecovery {
   /// @notice Get an extension's installed colony.
   /// @param extension Address of the extension installation
   /// @return colony Address of the colony the extension is installed in
-  function getExtensionMultiInstallation(address extension) external view returns (address colony);
+  function getExtensionColony(address extension) external view returns (address colony);
 
   /// @notice Return 1 / the fee to pay to the network. e.g. if the fee is 1% (or 0.01), return 100.
   /// @return _feeInverse The inverse of the network fee

docs/_Interface_IColony.md

@@ -1038,11 +1038,6 @@ Install an extension to the colony. Secured function to authorised members.
 |extensionId|bytes32|keccak256 hash of the extension name, used as an indentifier
 |version|uint256|The new extension version to install
 
-**Return Parameters**
-
-|Name|Type|Description|
-|---|---|---|
-|extension|address|The address of the extension installation
 
 ### `lockExpenditure`
 
@@ -1160,6 +1155,18 @@ Make a new task in the colony. Secured function to authorised members.
 |_dueDate|uint256|The due date of the task, can set to `0` for no-op
 
 
+### `migrateToMultiExtension`
+
+Migrate extension bookkeeping to multiExtension. Secured function to authorised members.
+
+
+**Parameters**
+
+|Name|Type|Description|
+|---|---|---|
+|extensionId|bytes32|keccak256 hash of the extension name, used as an indentifier
+
+
 ### `mintTokens`
 
 Mint `_wad` amount of colony tokens. Secured function to authorised members.

docs/_Interface_IColonyNetwork.md

@@ -347,40 +347,40 @@ Returns the address of the ENSRegistrar for the Network.
 |---|---|---|
 |address|address|The address the ENSRegistrar resolves to
 
-### `getExtensionInstallation`
+### `getExtensionColony`
 
-Get an extension's installation.
+Get an extension's installed colony.
 
 
 **Parameters**
 
 |Name|Type|Description|
 |---|---|---|
-|extensionId|bytes32|keccak256 hash of the extension name, used as an indentifier
-|colony|address|Address of the colony the extension is installed in
+|extension|address|Address of the extension installation
 
 **Return Parameters**
 
 |Name|Type|Description|
 |---|---|---|
-|installation|address|The address of the installed extension
+|colony|address|Address of the colony the extension is installed in
 
-### `getExtensionMultiInstallation`
+### `getExtensionInstallation`
 
-Get an extension's installed colony.
+Get an extension's installation.
 
 
 **Parameters**
 
 |Name|Type|Description|
 |---|---|---|
-|extension|address|Address of the extension installation
+|extensionId|bytes32|keccak256 hash of the extension name, used as an indentifier
+|colony|address|Address of the colony the extension is installed in
 
 **Return Parameters**
 
 |Name|Type|Description|
 |---|---|---|
-|colony|address|Address of the colony the extension is installed in
+|installation|address|The address of the installed extension
 
 ### `getExtensionResolver`
 
@@ -693,11 +693,6 @@ Install an extension in a colony. Can only be called by a Colony.
 |extensionId|bytes32|keccak256 hash of the extension name, used as an indentifier
 |version|uint256|Version of the extension to install
 
-**Return Parameters**
-
-|Name|Type|Description|
-|---|---|---|
-|extension|address|The address of the extension installation
 
 ### `isColony`
 
@@ -735,15 +730,14 @@ Reverse lookup a username from an address.
 
 ### `migrateToMultiExtension`
 
-Migrate extension bookkeeping to multiExtension
+Migrate extension bookkeeping to multiExtension. Can only be called by a Colony.
 
 
 **Parameters**
 
 |Name|Type|Description|
 |---|---|---|
 |extensionId|bytes32|keccak256 hash of the extension name, used as an indentifier
-|colony|address|Address of the colony the extension is installed in
 
 
 ### `punishStakers`

test/contracts-network/colony-network-extensions.js

@@ -196,21 +196,21 @@ contract("Colony Network Extensions", (accounts) => {
 
       let colonyAddress;
 
-      colonyAddress = await colonyNetwork.getExtensionMultiInstallation(extension.address);
+      colonyAddress = await colonyNetwork.getExtensionColony(extension.address);
       expect(colonyAddress).to.equal(ethers.constants.AddressZero);
 
       // Set up `installations` mapping in the old style
-      const slot = soliditySha3(`0x000000000000000000000000${colony.address.slice(2)}`, soliditySha3(TEST_EXTENSION, 39));
-      const value = `0x000000000000000000000000${extension.address.slice(2)}`;
+      const slot = soliditySha3(ethers.utils.hexZeroPad(colony.address, 32), soliditySha3(TEST_EXTENSION, 39));
+      const value = ethers.utils.hexZeroPad(extension.address, 32);
       await editableColonyNetwork.setStorageSlot(slot, value);
 
       let extensionAddress;
       extensionAddress = await colonyNetwork.getExtensionInstallation(TEST_EXTENSION, colony.address);
       expect(extensionAddress).to.not.equal(ethers.constants.AddressZero);
 
-      await colonyNetwork.migrateToMultiExtension(TEST_EXTENSION, colony.address);
+      await colony.migrateToMultiExtension(TEST_EXTENSION);
 
-      colonyAddress = await colonyNetwork.getExtensionMultiInstallation(extension.address);
+      colonyAddress = await colonyNetwork.getExtensionColony(extension.address);
       expect(colonyAddress).to.equal(colony.address);
 
       extensionAddress = await colonyNetwork.getExtensionInstallation(TEST_EXTENSION, colony.address);
@@ -221,8 +221,8 @@ contract("Colony Network Extensions", (accounts) => {
       const version7Colony = await Version7.new(colonyNetwork.address);
 
       // Add version7Colony to _isColony mapping
-      const slot = soliditySha3(`0x000000000000000000000000${version7Colony.address.slice(2)}`, 19);
-      const value = `0x0000000000000000000000000000000000000000000000000000000000000001`;
+      const slot = soliditySha3(ethers.utils.hexZeroPad(version7Colony.address, 32), 19);
+      const value = ethers.utils.zeroPad(1, 32);
       await editableColonyNetwork.setStorageSlot(slot, value);
 
       await version7Colony.installExtension(TEST_EXTENSION, 1);
@@ -297,8 +297,8 @@ contract("Colony Network Extensions", (accounts) => {
       const version7Colony = await Version7.new(colonyNetwork.address);
 
       // Add version7Colony to _isColony mapping
-      const slot = soliditySha3(`0x000000000000000000000000${version7Colony.address.slice(2)}`, 19);
-      const value = `0x0000000000000000000000000000000000000000000000000000000000000001`;
+      const slot = soliditySha3(ethers.utils.hexZeroPad(version7Colony.address, 32), 19);
+      const value = ethers.utils.zeroPad(1, 32);
       await editableColonyNetwork.setStorageSlot(slot, value);
 
       await version7Colony.installExtension(TEST_EXTENSION, 1);
@@ -346,8 +346,8 @@ contract("Colony Network Extensions", (accounts) => {
       const version7Colony = await Version7.new(colonyNetwork.address);
 
       // Add version7Colony to _isColony mapping
-      const slot = soliditySha3(`0x000000000000000000000000${version7Colony.address.slice(2)}`, 19);
-      const value = `0x0000000000000000000000000000000000000000000000000000000000000001`;
+      const slot = soliditySha3(ethers.utils.hexZeroPad(version7Colony.address, 32), 19);
+      const value = ethers.utils.zeroPad(1, 32);
       await editableColonyNetwork.setStorageSlot(slot, value);
 
       await version7Colony.installExtension(TEST_EXTENSION, 1);
@@ -397,8 +397,8 @@ contract("Colony Network Extensions", (accounts) => {
       const version7Colony = await Version7.new(colonyNetwork.address);
 
       // Add version7Colony to _isColony mapping
-      const slot = soliditySha3(`0x000000000000000000000000${version7Colony.address.slice(2)}`, 19);
-      const value = `0x0000000000000000000000000000000000000000000000000000000000000001`;
+      const slot = soliditySha3(ethers.utils.hexZeroPad(version7Colony.address, 32), 19);
+      const value = ethers.utils.zeroPad(1, 32);
       await editableColonyNetwork.setStorageSlot(slot, value);
 
       await version7Colony.installExtension(TEST_EXTENSION, 1);