From ca0dd96c13b1d83bf14353caf65e9d293f0f01c9 Mon Sep 17 00:00:00 2001 From: Daniel Kronovet Date: Tue, 3 Aug 2021 10:37:56 -0700 Subject: [PATCH] Add support for old and new extension management schemes simultaneously --- contracts/colony/IColony.sol | 19 ---- .../colonyNetwork/ColonyNetworkExtensions.sol | 43 +++++--- contracts/colonyNetwork/IColonyNetwork.sol | 3 +- contracts/testHelpers/PreviousVersion.sol | 32 +++++- docs/_Interface_IColony.md | 39 -------- docs/_Interface_IColonyNetwork.md | 5 - .../colony-network-extensions.js | 97 +++++++++++++++++-- 7 files changed, 152 insertions(+), 86 deletions(-) diff --git a/contracts/colony/IColony.sol b/contracts/colony/IColony.sol index 16eecfbb76..85c1eb6a21 100644 --- a/contracts/colony/IColony.sol +++ b/contracts/colony/IColony.sol @@ -266,35 +266,16 @@ interface IColony is ColonyDataTypes, IRecovery { /// @return extension The address of the extension installation function installExtension(bytes32 extensionId, uint256 version) external returns (address extension); - /// @dev DEPRECATED - /// @notice Upgrade an extension in a colony. Secured function to authorised members. - /// @param extensionId keccak256 hash of the extension name, used as an indentifier - /// @param newVersion The version to upgrade to (must be one larger than the current version) - function upgradeExtension(bytes32 extensionId, uint256 newVersion) external; - /// @notice Upgrade an extension in a colony. Secured function to authorised members. /// @param extension The address of the extension installation /// @param newVersion The version to upgrade to (must be one larger than the current version) function upgradeExtension(address extension, uint256 newVersion) external; - /// @dev DEPRECATED - /// @notice Set the deprecation of an extension in a colony. Secured function to authorised members. - /// @param extensionId keccak256 hash of the extension name, used as an indentifier - /// @param deprecated Whether to deprecate the extension or not - function deprecateExtension(bytes32 extensionId, bool deprecated) external; - /// @notice Set the deprecation of an extension in a colony. Secured function to authorised members. /// @param extension The address of the extension installation /// @param deprecated Whether to deprecate the extension or not function deprecateExtension(address extension, bool deprecated) external; - /// @dev DEPRECATED - /// @notice Uninstall an extension from a colony. Secured function to authorised members. - /// @dev This is a permanent action -- re-installing the extension will deploy a new contract - /// @dev It is recommended to deprecate an extension before uninstalling to allow active objects to be resolved - /// @param extensionId keccak256 hash of the extension name, used as an indentifier - function uninstallExtension(bytes32 extensionId) external; - /// @notice Uninstall an extension from a colony. Secured function to authorised members. /// @dev This is a permanent action -- re-installing the extension will deploy a new contract /// @dev It is recommended to deprecate an extension before uninstalling to allow active objects to be resolved diff --git a/contracts/colonyNetwork/ColonyNetworkExtensions.sol b/contracts/colonyNetwork/ColonyNetworkExtensions.sol index c43592838f..9e4f0c3202 100644 --- a/contracts/colonyNetwork/ColonyNetworkExtensions.sol +++ b/contracts/colonyNetwork/ColonyNetworkExtensions.sol @@ -55,7 +55,14 @@ contract ColonyNetworkExtensions is ColonyNetworkStorage { require(resolvers[_extensionId][_version] != address(0x0), "colony-network-extension-bad-version"); EtherRouter extension = new EtherRouter(); - multiInstallations[address(extension)] = msg.sender; + + // Install in old installations mapping if version 7 or below + if (IColony(msg.sender).version() <= 7) { + require(installations[_extensionId][msg.sender] == address(0x0), "colony-network-extension-already-installed"); + installations[_extensionId][msg.sender] = address(extension); + } else { + multiInstallations[address(extension)] = msg.sender; + } extension.setResolver(resolvers[_extensionId][_version]); ColonyExtension(address(extension)).install(msg.sender); @@ -69,9 +76,17 @@ contract ColonyNetworkExtensions is ColonyNetworkStorage { function upgradeExtension(bytes32 _extensionId, uint256 _newVersion) public stoppable + calledByColony { - address extension = migrateToMultiExtension(_extensionId, msg.sender); - upgradeExtension(extension, _newVersion); + require(installations[_extensionId][msg.sender] != address(0x0), "colony-network-extension-not-installed"); + + address payable extension = installations[_extensionId][msg.sender]; + require(_newVersion == ColonyExtension(extension).version() + 1, "colony-network-extension-bad-increment"); + require(resolvers[_extensionId][_newVersion] != address(0x0), "colony-network-extension-bad-version"); + + EtherRouter(extension).setResolver(resolvers[_extensionId][_newVersion]); + ColonyExtension(extension).finishUpgrade(); + assert(ColonyExtension(extension).version() == _newVersion); emit ExtensionUpgraded(_extensionId, msg.sender, _newVersion); } @@ -100,9 +115,9 @@ contract ColonyNetworkExtensions is ColonyNetworkStorage { function deprecateExtension(bytes32 _extensionId, bool _deprecated) public stoppable + calledByColony { - address extension = migrateToMultiExtension(_extensionId, msg.sender); - deprecateExtension(extension, _deprecated); + ColonyExtension(installations[_extensionId][msg.sender]).deprecate(_deprecated); emit ExtensionDeprecated(_extensionId, msg.sender, _deprecated); } @@ -121,9 +136,13 @@ contract ColonyNetworkExtensions is ColonyNetworkStorage { function uninstallExtension(bytes32 _extensionId) public stoppable + calledByColony { - address extension = migrateToMultiExtension(_extensionId, msg.sender); - uninstallExtension(extension); + require(installations[_extensionId][msg.sender] != address(0x0), "colony-network-extension-not-installed"); + + ColonyExtension extension = ColonyExtension(installations[_extensionId][msg.sender]); + delete installations[_extensionId][msg.sender]; + extension.uninstall(); emit ExtensionUninstalled(_extensionId, msg.sender); } @@ -136,7 +155,6 @@ contract ColonyNetworkExtensions is ColonyNetworkStorage { require(multiInstallations[_extension] == msg.sender, "colony-network-extension-not-installed"); delete multiInstallations[_extension]; - ColonyExtension(_extension).uninstall(); emit ExtensionUninstalled(_extension, msg.sender); @@ -145,13 +163,12 @@ contract ColonyNetworkExtensions is ColonyNetworkStorage { function migrateToMultiExtension(bytes32 _extensionId, address _colony) public stoppable - returns (address) { - address extension = installations[_extensionId][_colony]; - require(extension != address(0x0), "colony-network-extension-not-installed"); + require(installations[_extensionId][_colony] != address(0x0), "colony-network-extension-not-installed"); + + multiInstallations[installations[_extensionId][_colony]] = payable(_colony); - multiInstallations[extension] = payable(_colony); - return extension; + delete installations[_extensionId][_colony]; } // Public view functions diff --git a/contracts/colonyNetwork/IColonyNetwork.sol b/contracts/colonyNetwork/IColonyNetwork.sol index 2e3df8e107..e8f94728b7 100644 --- a/contracts/colonyNetwork/IColonyNetwork.sol +++ b/contracts/colonyNetwork/IColonyNetwork.sol @@ -353,8 +353,7 @@ interface IColonyNetwork is ColonyNetworkDataTypes, IRecovery { /// @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); + function migrateToMultiExtension(bytes32 extensionId, address colony) external; /// @notice Get an extension's resolver. /// @param extensionId keccak256 hash of the extension name, used as an indentifier diff --git a/contracts/testHelpers/PreviousVersion.sol b/contracts/testHelpers/PreviousVersion.sol index bcb10479ab..7fec757173 100644 --- a/contracts/testHelpers/PreviousVersion.sol +++ b/contracts/testHelpers/PreviousVersion.sol @@ -1,5 +1,7 @@ pragma solidity 0.7.3; +import "./../colonyNetwork/IColonyNetwork.sol"; + contract Version3 { function version() pure external returns (uint256) { return 3; @@ -10,4 +12,32 @@ contract Version4 { function version() pure external returns (uint256) { return 4; } -} \ No newline at end of file +} + +contract Version7 { + function version() public pure returns (uint256) { + return 7; + } + + address colonyNetworkAddress; + + constructor(address _colonyNetworkAddress) public { + colonyNetworkAddress = _colonyNetworkAddress; + } + + function installExtension(bytes32 _extensionId, uint256 _version) public { + IColonyNetwork(colonyNetworkAddress).installExtension(_extensionId, _version); + } + + function upgradeExtension(bytes32 _extensionId, uint256 _newVersion) public { + IColonyNetwork(colonyNetworkAddress).upgradeExtension(_extensionId, _newVersion); + } + + function deprecateExtension(bytes32 _extensionId, bool _deprecated) public { + IColonyNetwork(colonyNetworkAddress).deprecateExtension(_extensionId, _deprecated); + } + + function uninstallExtension(bytes32 _extensionId) public { + IColonyNetwork(colonyNetworkAddress).uninstallExtension(_extensionId); + } +} diff --git a/docs/_Interface_IColony.md b/docs/_Interface_IColony.md index ca24f8499c..09d7834339 100644 --- a/docs/_Interface_IColony.md +++ b/docs/_Interface_IColony.md @@ -261,19 +261,6 @@ Set the deprecation of an extension in a colony. Secured function to authorised |deprecated|bool|Whether to deprecate the extension or not -### `deprecateExtension` - -Set the deprecation of an extension in a colony. Secured function to authorised members. - - -**Parameters** - -|Name|Type|Description| -|---|---|---| -|extensionId|bytes32|keccak256 hash of the extension name, used as an indentifier -|deprecated|bool|Whether to deprecate the extension or not - - ### `editColony` Called to change the metadata associated with a colony. Expected to be a IPFS hash of a JSON blob, but not enforced to any degree by the contracts @@ -1915,19 +1902,6 @@ Uninstall an extension from a colony. Secured function to authorised members. |extension|address|The address of the extension installation -### `uninstallExtension` - -Uninstall an extension from a colony. Secured function to authorised members. - -*Note: This is a permanent action -- re-installing the extension will deploy a new contract* - -**Parameters** - -|Name|Type|Description| -|---|---|---| -|extensionId|bytes32|keccak256 hash of the extension name, used as an indentifier - - ### `unlockToken` unlock the native colony token, if possible @@ -1999,19 +1973,6 @@ Upgrade an extension in a colony. Secured function to authorised members. |newVersion|uint256|The version to upgrade to (must be one larger than the current version) -### `upgradeExtension` - -Upgrade an extension in a colony. Secured function to authorised members. - - -**Parameters** - -|Name|Type|Description| -|---|---|---| -|extensionId|bytes32|keccak256 hash of the extension name, used as an indentifier -|newVersion|uint256|The version to upgrade to (must be one larger than the current version) - - ### `userCanSetRoles` Check whether a given user can modify roles in the target domain `_childDomainId`. Mostly a convenience function to provide a uniform interface for extension contracts validating permissions diff --git a/docs/_Interface_IColonyNetwork.md b/docs/_Interface_IColonyNetwork.md index 2de0e5711a..1f359eb5a6 100644 --- a/docs/_Interface_IColonyNetwork.md +++ b/docs/_Interface_IColonyNetwork.md @@ -745,11 +745,6 @@ Migrate extension bookkeeping to multiExtension |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` diff --git a/test/contracts-network/colony-network-extensions.js b/test/contracts-network/colony-network-extensions.js index c8fb4a9df5..e3312eba5b 100644 --- a/test/contracts-network/colony-network-extensions.js +++ b/test/contracts-network/colony-network-extensions.js @@ -26,9 +26,11 @@ const TestVotingToken = artifacts.require("TestVotingToken"); const Resolver = artifacts.require("Resolver"); const RequireExecuteCall = artifacts.require("RequireExecuteCall"); const ContractEditing = artifacts.require("ContractEditing"); +const Version7 = artifacts.require("Version7"); contract("Colony Network Extensions", (accounts) => { let colonyNetwork; + let editableColonyNetwork; let metaColony; let colony; let token; @@ -72,6 +74,13 @@ 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 @@ -182,13 +191,6 @@ contract("Colony Network Extensions", (accounts) => { }); 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); @@ -202,10 +204,34 @@ contract("Colony Network Extensions", (accounts) => { const value = `0x000000000000000000000000${extension.address.slice(2)}`; 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); colonyAddress = await colonyNetwork.getExtensionMultiInstallation(extension.address); expect(colonyAddress).to.equal(colony.address); + + extensionAddress = await colonyNetwork.getExtensionInstallation(TEST_EXTENSION, colony.address); + expect(extensionAddress).to.equal(ethers.constants.AddressZero); + }); + + it("allows old colonies to install extensions correctly", async () => { + const version7Colony = await Version7.new(colonyNetwork.address); + + // Add version7Colony to _isColony mapping + const slot = soliditySha3(`0x000000000000000000000000${version7Colony.address.slice(2)}`, 19); + const value = `0x0000000000000000000000000000000000000000000000000000000000000001`; + await editableColonyNetwork.setStorageSlot(slot, value); + + await version7Colony.installExtension(TEST_EXTENSION, 1); + + const extensionAddress = await colonyNetwork.getExtensionInstallation(TEST_EXTENSION, version7Colony.address); + expect(extensionAddress).to.not.equal(ethers.constants.AddressZero); + + // But not twice + await checkErrorRevert(version7Colony.installExtension(TEST_EXTENSION, 1), "colony-network-extension-already-installed"); }); }); @@ -266,6 +292,25 @@ contract("Colony Network Extensions", (accounts) => { "colony-network-extension-bad-increment" ); }); + + it("allows old colonies to upgrade extensions correctly", async () => { + const version7Colony = await Version7.new(colonyNetwork.address); + + // Add version7Colony to _isColony mapping + const slot = soliditySha3(`0x000000000000000000000000${version7Colony.address.slice(2)}`, 19); + const value = `0x0000000000000000000000000000000000000000000000000000000000000001`; + await editableColonyNetwork.setStorageSlot(slot, value); + + await version7Colony.installExtension(TEST_EXTENSION, 1); + + const extensionAddress = await colonyNetwork.getExtensionInstallation(TEST_EXTENSION, version7Colony.address); + + await version7Colony.upgradeExtension(TEST_EXTENSION, 2); + + const extension = await ColonyExtension.at(extensionAddress); + const version = await extension.version(); + expect(version).to.eq.BN(2); + }); }); describe("deprecating extensions", () => { @@ -296,6 +341,24 @@ contract("Colony Network Extensions", (accounts) => { const extensionAddress = getExtensionAddressFromTx(tx); await checkErrorRevert(colony.methods["deprecateExtension(address,bool)"](extensionAddress, true, { from: ARCHITECT }), "ds-auth-unauthorized"); }); + + it("allows old colonies to deprecate extensions correctly", async () => { + const version7Colony = await Version7.new(colonyNetwork.address); + + // Add version7Colony to _isColony mapping + const slot = soliditySha3(`0x000000000000000000000000${version7Colony.address.slice(2)}`, 19); + const value = `0x0000000000000000000000000000000000000000000000000000000000000001`; + await editableColonyNetwork.setStorageSlot(slot, value); + + await version7Colony.installExtension(TEST_EXTENSION, 1); + + const extensionAddress = await colonyNetwork.getExtensionInstallation(TEST_EXTENSION, version7Colony.address); + const extension = await TestExtension1.at(extensionAddress); + + await version7Colony.deprecateExtension(TEST_EXTENSION, true); + + await checkErrorRevert(extension.foo(), "colony-extension-deprecated"); + }); }); describe("uninstalling extensions", () => { @@ -329,6 +392,26 @@ contract("Colony Network Extensions", (accounts) => { "colony-network-extension-not-installed" ); }); + + it("allows old colonies to uninstall extensions correctly", async () => { + const version7Colony = await Version7.new(colonyNetwork.address); + + // Add version7Colony to _isColony mapping + const slot = soliditySha3(`0x000000000000000000000000${version7Colony.address.slice(2)}`, 19); + const value = `0x0000000000000000000000000000000000000000000000000000000000000001`; + await editableColonyNetwork.setStorageSlot(slot, value); + + await version7Colony.installExtension(TEST_EXTENSION, 1); + + let extensionAddress; + extensionAddress = await colonyNetwork.getExtensionInstallation(TEST_EXTENSION, version7Colony.address); + expect(extensionAddress).to.not.equal(ethers.constants.AddressZero); + + await version7Colony.uninstallExtension(TEST_EXTENSION); + + extensionAddress = await colonyNetwork.getExtensionInstallation(TEST_EXTENSION, version7Colony.address); + expect(extensionAddress).to.equal(ethers.constants.AddressZero); + }); }); describe("using extensions", () => {