Update in response to review comments

Author: kronosapiens

contracts/colony/Colony.sol

@@ -370,39 +370,18 @@ contract Colony is ColonyStorage, PatriciaTreeProofs, MultiChain {
     return IColonyNetwork(colonyNetworkAddress).installExtension(_extensionId, _version);
   }
 
-  // Deprecated
-  function upgradeExtension(bytes32 _extensionId, uint256 _newVersion)
-  public stoppable auth
-  {
-    IColonyNetwork(colonyNetworkAddress).upgradeExtension(_extensionId, _newVersion);
-  }
-
   function upgradeExtension(address _extension, uint256 _newVersion)
   public stoppable auth
   {
     IColonyNetwork(colonyNetworkAddress).upgradeExtension(_extension, _newVersion);
   }
 
-  // Deprecated
-  function deprecateExtension(bytes32 _extensionId, bool _deprecated)
-  public stoppable auth
-  {
-    IColonyNetwork(colonyNetworkAddress).deprecateExtension(_extensionId, _deprecated);
-  }
-
   function deprecateExtension(address _extension, bool _deprecated)
   public stoppable auth
   {
     IColonyNetwork(colonyNetworkAddress).deprecateExtension(_extension, _deprecated);
   }
 
-  // Deprecated
-  function uninstallExtension(bytes32 _extensionId)
-  public stoppable auth
-  {
-    IColonyNetwork(colonyNetworkAddress).uninstallExtension(_extensionId);
-  }
-
   function uninstallExtension(address _extension)
   public stoppable auth
   {

contracts/colonyNetwork/ColonyNetworkExtensions.sol

@@ -70,7 +70,7 @@ contract ColonyNetworkExtensions is ColonyNetworkStorage {
     public
     stoppable
   {
-    address extension = migrateToMultiExtension(_extensionId);
+    address extension = migrateToMultiExtension(_extensionId, msg.sender);
     upgradeExtension(extension, _newVersion);
 
     emit ExtensionUpgraded(_extensionId, msg.sender, _newVersion);
@@ -101,7 +101,7 @@ contract ColonyNetworkExtensions is ColonyNetworkStorage {
     public
     stoppable
   {
-    address extension = migrateToMultiExtension(_extensionId);
+    address extension = migrateToMultiExtension(_extensionId, msg.sender);
     deprecateExtension(extension, _deprecated);
 
     emit ExtensionDeprecated(_extensionId, msg.sender, _deprecated);
@@ -122,7 +122,7 @@ contract ColonyNetworkExtensions is ColonyNetworkStorage {
     public
     stoppable
   {
-    address extension = migrateToMultiExtension(_extensionId);
+    address extension = migrateToMultiExtension(_extensionId, msg.sender);
     uninstallExtension(extension);
 
     emit ExtensionUninstalled(_extensionId, msg.sender);
@@ -142,6 +142,18 @@ contract ColonyNetworkExtensions is ColonyNetworkStorage {
     emit ExtensionUninstalled(_extension, msg.sender);
   }
 
+  function migrateToMultiExtension(bytes32 _extensionId, address _colony)
+    public
+    stoppable
+    returns (address)
+  {
+    address extension = installations[_extensionId][_colony];
+    require(extension != address(0x0), "colony-network-extension-not-installed");
+
+    multiInstallations[extension] = payable(_colony);
+    return extension;
+  }
+
   // Public view functions
 
   function getExtensionResolver(bytes32 _extensionId, uint256 _version)
@@ -183,12 +195,4 @@ contract ColonyNetworkExtensions is ColonyNetworkStorage {
     address extension = Resolver(_resolver).lookup(VERSION_SIG);
     return ColonyExtension(extension).version();
   }
-
-  function migrateToMultiExtension(bytes32 _extensionId) internal returns (address) {
-    address extension = installations[_extensionId][msg.sender];
-    require(extension != address(0x0), "colony-network-extension-not-installed");
-
-    multiInstallations[extension] = msg.sender;
-    return extension;
-  }
 }

contracts/colonyNetwork/IColonyNetwork.sol

@@ -350,6 +350,12 @@ interface IColonyNetwork is ColonyNetworkDataTypes, IRecovery {
   /// @param extension Address of the extension installation
   function uninstallExtension(address extension) external;
 
+  /// @notice Migrate extension bookkeeping to multiExtension
+  /// @param extensionId keccak256 hash of the extension name, used as an indentifier
+  /// @param colony Address of the colony the extension is installed in
+  /// @return extension The address of the extension
+  function migrateToMultiExtension(bytes32 extensionId, address colony) external returns (address extension);
+
   /// @notice Get an extension's resolver.
   /// @param extensionId keccak256 hash of the extension name, used as an indentifier
   /// @param version Version of the extension

docs/_Interface_IColonyNetwork.md

@@ -733,6 +733,24 @@ Reverse lookup a username from an address.
 |---|---|---|
 |domain|string|A string containing the colony-based ENS name corresponding to addr
 
+### `migrateToMultiExtension`
+
+Migrate extension bookkeeping to multiExtension
+
+
+**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
+
+**Return Parameters**
+
+|Name|Type|Description|
+|---|---|---|
+|extension|address|The address of the extension
+
 ### `punishStakers`
 
 Function called to punish people who staked against a new reputation root hash that turned out to be incorrect.

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

@@ -29,7 +29,6 @@ const ContractEditing = artifacts.require("ContractEditing");
 
 contract("Colony Network Extensions", (accounts) => {
   let colonyNetwork;
-  let editableColonyNetwork;
   let metaColony;
   let colony;
   let token;
@@ -73,13 +72,6 @@ contract("Colony Network Extensions", (accounts) => {
     colonyNetwork = await setupColonyNetwork();
     ({ metaColony } = await setupMetaColonyWithLockedCLNYToken(colonyNetwork));
 
-    const colonyNetworkAsER = await EtherRouter.at(colonyNetwork.address);
-    const colonyNetworkResolverAddress = await colonyNetworkAsER.resolver();
-    const colonyNetworkResolver = await Resolver.at(colonyNetworkResolverAddress);
-    const contractEditing = await ContractEditing.new();
-    await colonyNetworkResolver.register("setStorageSlot(uint256,bytes32)", contractEditing.address);
-    editableColonyNetwork = await ContractEditing.at(colonyNetwork.address);
-
     ({ colony, token } = await setupRandomColony(colonyNetwork));
     await colony.addDomain(1, UINT256_MAX, 1); // Domain 2
 
@@ -154,28 +146,67 @@ contract("Colony Network Extensions", (accounts) => {
       await metaColony.addExtensionToNetwork(TEST_EXTENSION, testExtension2Resolver.address);
     });
 
-    it("allows a root user to install an extension with any version", async () => {
-      const tx = await colony.installExtension(TEST_EXTENSION, 2, { from: ROOT });
+    it("allows a root user to install an extension", async () => {
+      const tx = await colony.installExtension(TEST_EXTENSION, 1, { from: ROOT });
 
       const extensionAddress = getExtensionAddressFromTx(tx);
-      const extension = await TestExtension2.at(extensionAddress);
+      const extension = await TestExtension1.at(extensionAddress);
       const owner = await extension.owner();
       expect(owner).to.equal(colonyNetwork.address);
 
       const identifier = await extension.identifier();
       const version = await extension.version();
       const colonyAddress = await extension.getColony();
       expect(identifier).to.equal(TEST_EXTENSION);
-      expect(version).to.eq.BN(2);
+      expect(version).to.eq.BN(1);
       expect(colonyAddress).to.equal(colony.address);
 
       // Only colonyNetwork can install the extension
       await checkErrorRevert(extension.install(colony.address), "ds-auth-unauthorized");
     });
 
+    it("allows a root user to install an extension with any version", async () => {
+      const tx = await colony.installExtension(TEST_EXTENSION, 2, { from: ROOT });
+
+      const extensionAddress = getExtensionAddressFromTx(tx);
+      const extension = await TestExtension2.at(extensionAddress);
+
+      const identifier = await extension.identifier();
+      const version = await extension.version();
+      expect(identifier).to.equal(TEST_EXTENSION);
+      expect(version).to.eq.BN(2);
+    });
+
     it("does not allow an extension to be installed with a nonexistent resolver", async () => {
       await checkErrorRevert(colony.installExtension(TEST_EXTENSION, 0, { from: ROOT }), "colony-network-extension-bad-version");
     });
+
+    it("allows colonies to migrate to multiExtension bookkeeping", async () => {
+      const colonyNetworkAsER = await EtherRouter.at(colonyNetwork.address);
+      const colonyNetworkResolverAddress = await colonyNetworkAsER.resolver();
+      const colonyNetworkResolver = await Resolver.at(colonyNetworkResolverAddress);
+      const contractEditing = await ContractEditing.new();
+      await colonyNetworkResolver.register("setStorageSlot(uint256,bytes32)", contractEditing.address);
+      const editableColonyNetwork = await ContractEditing.at(colonyNetwork.address);
+
+      const extension = await TestExtension1.new();
+      await extension.install(colony.address);
+
+      let colonyAddress;
+
+      colonyAddress = await colonyNetwork.getExtensionMultiInstallation(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)}`;
+      await editableColonyNetwork.setStorageSlot(slot, value);
+
+      await colonyNetwork.migrateToMultiExtension(TEST_EXTENSION, colony.address);
+
+      colonyAddress = await colonyNetwork.getExtensionMultiInstallation(extension.address);
+      expect(colonyAddress).to.equal(colony.address);
+    });
   });
 
   describe("upgrading extensions", () => {
@@ -200,26 +231,6 @@ contract("Colony Network Extensions", (accounts) => {
       expect(version).to.eq.BN(2);
     });
 
-    it("allows root users to upgrade an extension, with the deprecated interface", async () => {
-      const tx = await colony.installExtension(TEST_EXTENSION, 1, { from: ROOT });
-
-      const extensionAddress = getExtensionAddressFromTx(tx);
-      let extension = await ColonyExtension.at(extensionAddress);
-      let version = await extension.version();
-      expect(version).to.eq.BN(1);
-
-      // Set up `installations` mapping in the old style
-      const slot = soliditySha3(`0x000000000000000000000000${colony.address.slice(2)}`, soliditySha3(TEST_EXTENSION, 39));
-      const value = `0x000000000000000000000000${extensionAddress.slice(2)}`;
-      await editableColonyNetwork.setStorageSlot(slot, value);
-
-      await colony.methods["upgradeExtension(bytes32,uint256)"](TEST_EXTENSION, 2, { from: ROOT });
-
-      extension = await ColonyExtension.at(extensionAddress);
-      version = await extension.version();
-      expect(version).to.eq.BN(2);
-    });
-
     it("does not allow non-root users to upgrade an extension", async () => {
       const tx = await colony.installExtension(TEST_EXTENSION, 1, { from: ROOT });
 
@@ -279,28 +290,6 @@ contract("Colony Network Extensions", (accounts) => {
       await extension.foo();
     });
 
-    it("allows root users to deprecate and undeprecate an extension, with the deprecated interface", async () => {
-      const tx = await colony.installExtension(TEST_EXTENSION, 1, { from: ROOT });
-
-      const extensionAddress = getExtensionAddressFromTx(tx);
-      const extension = await TestExtension1.at(extensionAddress);
-
-      // Set up `installations` mapping in the old style
-      const slot = soliditySha3(`0x000000000000000000000000${colony.address.slice(2)}`, soliditySha3(TEST_EXTENSION, 39));
-      const value = `0x000000000000000000000000${extensionAddress.slice(2)}`;
-      await editableColonyNetwork.setStorageSlot(slot, value);
-
-      await extension.foo();
-
-      await colony.methods["deprecateExtension(bytes32,bool)"](TEST_EXTENSION, true, { from: ROOT });
-
-      await checkErrorRevert(extension.foo(), "colony-extension-deprecated");
-
-      await colony.methods["deprecateExtension(bytes32,bool)"](TEST_EXTENSION, false, { from: ROOT });
-
-      await extension.foo();
-    });
-
     it("does not allow non-root users to deprecate an extension", async () => {
       const tx = await colony.installExtension(TEST_EXTENSION, 1, { from: ROOT });
 
@@ -330,27 +319,6 @@ contract("Colony Network Extensions", (accounts) => {
       expect(new BN(colonyBalance)).to.eq.BN(100);
     });
 
-    it("allows root users to uninstall an extension and send ether to the beneficiary, with the deprecated interface", async () => {
-      const tx = await colony.installExtension(TEST_EXTENSION, 1, { from: ROOT });
-
-      const extensionAddress = getExtensionAddressFromTx(tx);
-      const extension = await TestExtension1.at(extensionAddress);
-      await extension.send(100);
-
-      // Set up `installations` mapping in the old style
-      const slot = soliditySha3(`0x000000000000000000000000${colony.address.slice(2)}`, soliditySha3(TEST_EXTENSION, 39));
-      const value = `0x000000000000000000000000${extensionAddress.slice(2)}`;
-      await editableColonyNetwork.setStorageSlot(slot, value);
-
-      // Only colonyNetwork can uninstall
-      await checkErrorRevert(extension.uninstall(), "ds-auth-unauthorized");
-
-      await colony.methods["uninstallExtension(bytes32)"](TEST_EXTENSION, { from: ROOT });
-
-      const colonyBalance = await web3GetBalance(colony.address);
-      expect(new BN(colonyBalance)).to.eq.BN(100);
-    });
-
     it("does not allow non-root users to uninstall an extension", async () => {
       await checkErrorRevert(colony.methods["uninstallExtension(address)"](ethers.constants.AddressZero, { from: USER }), "ds-auth-unauthorized");
     });