Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Multiple extension installations #971

Open
wants to merge 11 commits into
base: develop
Choose a base branch
from
41 changes: 24 additions & 17 deletions contracts/colony/Colony.sol
Original file line number Diff line number Diff line change
Expand Up @@ -79,10 +79,12 @@ contract Colony is ColonyStorage, PatriciaTreeProofs, MultiChain {
external stoppable self
returns (bool)
{
// Prevent transactions to network contracts
// Prevent transactions to network contracts or network-managed extensions installed in this colony
require(_to != address(this), "colony-cannot-target-self");
require(_to != colonyNetworkAddress, "colony-cannot-target-network");
require(_to != tokenLockingAddress, "colony-cannot-target-token-locking");
require(isContract(_to), "colony-must-target-contract");
require(!isOwnExtension(_to), "colony-cannot-target-own-extensions");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You've ended up doubling up on the isContract check in the refactoring. I'd leave it in isOwnExtension, and delete the standalone one here.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought about that, but the problem is that the contract check might be useful feedback (i.e. if someone tries to make an arbitrary tx to a user address, the error shouldn't be colony-cannot-target-own-extensions. On the flip side, if we remove it from isOwnExtension, that function could throw in a confusing way. So I sort of think it's best this way, even if a bit redundant. The other idea is to have the isContract check inside isOwnExtension throw an error, which is a little bit of a mixup for a boolean-valued function but I could roll with it.


// Prevent transactions to transfer held tokens
bytes4 sig;
Expand All @@ -93,16 +95,6 @@ contract Colony is ColonyStorage, PatriciaTreeProofs, MultiChain {
else if (sig == TRANSFER_SIG) { transferTransactionPreparation(_to, _action); }
else if (sig == BURN_GUY_SIG || sig == TRANSFER_FROM_SIG) { burnGuyOrTransferFromTransactionPreparation(_action); }

// Prevent transactions to network-managed extensions installed in this colony
require(isContract(_to), "colony-to-must-be-contract");
// slither-disable-next-line unused-return
try ColonyExtension(_to).identifier() returns (bytes32 extensionId) {
require(
IColonyNetwork(colonyNetworkAddress).getExtensionInstallation(extensionId, address(this)) != _to,
"colony-cannot-target-extensions"
);
} catch {}

bool res = executeCall(_to, 0, _action);

if (sig == APPROVE_SIG) { approveTransactionCleanup(_to, _action); }
Expand Down Expand Up @@ -378,22 +370,28 @@ contract Colony is ColonyStorage, PatriciaTreeProofs, MultiChain {
IColonyNetwork(colonyNetworkAddress).installExtension(_extensionId, _version);
}

function upgradeExtension(bytes32 _extensionId, uint256 _newVersion)
function upgradeExtension(address _extension, uint256 _newVersion)
public stoppable auth
{
IColonyNetwork(colonyNetworkAddress).upgradeExtension(_extension, _newVersion);
}

function deprecateExtension(address _extension, bool _deprecated)
public stoppable auth
{
IColonyNetwork(colonyNetworkAddress).upgradeExtension(_extensionId, _newVersion);
IColonyNetwork(colonyNetworkAddress).deprecateExtension(_extension, _deprecated);
}

function deprecateExtension(bytes32 _extensionId, bool _deprecated)
function uninstallExtension(address _extension)
public stoppable auth
{
IColonyNetwork(colonyNetworkAddress).deprecateExtension(_extensionId, _deprecated);
IColonyNetwork(colonyNetworkAddress).uninstallExtension(_extension);
}

function uninstallExtension(bytes32 _extensionId)
function migrateToMultiExtension(bytes32 _extensionId)
public stoppable auth
{
IColonyNetwork(colonyNetworkAddress).uninstallExtension(_extensionId);
IColonyNetwork(colonyNetworkAddress).migrateToMultiExtension(_extensionId);
}

function addDomain(uint256 _permissionDomainId, uint256 _childSkillIndex, uint256 _parentDomainId) public
Expand Down Expand Up @@ -501,6 +499,15 @@ contract Colony is ColonyStorage, PatriciaTreeProofs, MultiChain {
sig = bytes4(keccak256("setDefaultGlobalClaimDelay(uint256)"));
colonyAuthority.setRoleCapability(uint8(ColonyRole.Root), address(this), sig, true);

sig = bytes4(keccak256("upgradeExtension(address,uint256)"));
colonyAuthority.setRoleCapability(uint8(ColonyRole.Root), address(this), sig, true);

sig = bytes4(keccak256("deprecateExtension(address,bool)"));
colonyAuthority.setRoleCapability(uint8(ColonyRole.Root), address(this), sig, true);

sig = bytes4(keccak256("uninstallExtension(address)"));
colonyAuthority.setRoleCapability(uint8(ColonyRole.Root), address(this), sig, true);

sig = bytes4(keccak256("setExpenditureMetadata(uint256,uint256,uint256,string)"));
colonyAuthority.setRoleCapability(uint8(ColonyRole.Arbitration), address(this), sig, true);
}
Expand Down
4 changes: 4 additions & 0 deletions contracts/colony/ColonyAuthority.sol
Original file line number Diff line number Diff line change
Expand Up @@ -117,6 +117,10 @@ contract ColonyAuthority is CommonAuthority {
// Added in colony v8 (ebony-lwss)
addRoleCapability(ROOT_ROLE, "makeArbitraryTransactions(address[],bytes[],bool)");
addRoleCapability(ROOT_ROLE, "setDefaultGlobalClaimDelay(uint256)");
addRoleCapability(ROOT_ROLE, "upgradeExtension(address,uint256)");
addRoleCapability(ROOT_ROLE, "deprecateExtension(address,bool)");
addRoleCapability(ROOT_ROLE, "uninstallExtension(address)");
kronosapiens marked this conversation as resolved.
Show resolved Hide resolved
addRoleCapability(ROOT_ROLE, "migrateToMultiExtension(bytes32)");
addRoleCapability(ARBITRATION_ROLE, "setExpenditureMetadata(uint256,uint256,uint256,string)");
}

Expand Down
31 changes: 18 additions & 13 deletions contracts/colony/ColonyStorage.sol
Original file line number Diff line number Diff line change
Expand Up @@ -239,19 +239,7 @@ contract ColonyStorage is CommonStorage, ColonyDataTypes, ColonyNetworkDataTypes
}

modifier onlyExtension() {
// Ensure msg.sender is a contract
require(isContract(msg.sender), "colony-sender-must-be-contract");

// Ensure msg.sender is an extension
// slither-disable-next-line unused-return
try ColonyExtension(msg.sender).identifier() returns (bytes32 extensionId) {
require(
IColonyNetwork(colonyNetworkAddress).getExtensionInstallation(extensionId, address(this)) == msg.sender,
"colony-must-be-extension"
);
} catch {
require(false, "colony-must-be-extension");
}
require(isOwnExtension(msg.sender), "colony-must-be-extension");
_;
}

Expand Down Expand Up @@ -300,6 +288,23 @@ contract ColonyStorage is CommonStorage, ColonyDataTypes, ColonyNetworkDataTypes
return size > 0;
}

// slither-disable-next-line unused-return
function isOwnExtension(address addr) internal returns (bool) {
// Ensure addr is a contract first, otherwise `try` block will revert
if (!isContract(addr)) { return false; }

// Ensure addr is an extension installed in the colony, must check old & new formats
try ColonyExtension(addr).identifier() returns (bytes32 extensionId) {
return (
IColonyNetwork(colonyNetworkAddress).getExtensionColony(addr) == address(this) ||
IColonyNetwork(colonyNetworkAddress).getExtensionInstallation(extensionId, address(this)) == addr
);
} catch {
return false;
}

}

function domainExists(uint256 domainId) internal view returns (bool) {
return domainId > 0 && domainId <= domainCount;
}
Expand Down
14 changes: 9 additions & 5 deletions contracts/colony/IColony.sol
Original file line number Diff line number Diff line change
Expand Up @@ -266,20 +266,24 @@ interface IColony is ColonyDataTypes, IRecovery {
function installExtension(bytes32 extensionId, uint256 version) external;

/// @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 extension The address of the extension installation
/// @param newVersion The version to upgrade to (must be one larger than the current version)
function upgradeExtension(bytes32 extensionId, uint256 newVersion) external;
function upgradeExtension(address extension, uint256 newVersion) external;

/// @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 extension The address of the extension installation
/// @param deprecated Whether to deprecate the extension or not
function deprecateExtension(bytes32 extensionId, bool deprecated) external;
function deprecateExtension(address extension, bool deprecated) 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
/// @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 uninstallExtension(bytes32 extensionId) external;
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.
Expand Down
35 changes: 34 additions & 1 deletion contracts/colonyNetwork/ColonyNetworkDataTypes.sol
Original file line number Diff line number Diff line change
Expand Up @@ -114,29 +114,62 @@ 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
/// @param version The version of the extension
event ExtensionInstalled(bytes32 indexed extensionId, address indexed extension, address indexed colony, uint256 version);
kronosapiens marked this conversation as resolved.
Show resolved Hide resolved

/// @dev DEPRECATED
/// @notice Event logged when an extension is upgraded in a colony
/// @param extensionId The identifier for the extension
/// @param colony The address of the colony
/// @param version The new version of the extension
event ExtensionUpgraded(bytes32 indexed extensionId, address indexed colony, uint256 version);

/// @notice Event logged when an extension is upgraded in a colony
/// @param extension Address of the extension installation
/// @param colony The address of the colony
/// @param version The new version of the extension
event ExtensionUpgraded(address indexed extension, address indexed colony, uint256 version);

/// @dev DEPRECATED
/// @notice Event logged when an extension is (un)deprecated in a colony
/// @param extensionId The identifier for the extension
/// @param colony The address of the colony
/// @param deprecated Whether the extension is deprecated or not
event ExtensionDeprecated(bytes32 indexed extensionId, address indexed colony, bool deprecated);

/// @notice Event logged when an extension is (un)deprecated in a colony
/// @param extension Address of the extension installation
/// @param colony The address of the colony
/// @param deprecated Whether the extension is deprecated or not
event ExtensionDeprecated(address indexed extension, address indexed colony, bool deprecated);

/// @dev DEPRECATED
/// @notice Event logged when an extension is uninstalled from a colony
/// @param extensionId The identifier for the extension
/// @param colony The address of the colony
event ExtensionUninstalled(bytes32 indexed extensionId, address indexed colony);

/// @notice Event logged when an extension is uninstalled from a colony
/// @param extension Address of the extension installation
/// @param colony The address of the colony
event ExtensionUninstalled(address indexed extension, address indexed colony);

/// @notice Event logged when an extension is migrated from old to new storage schemes
/// @param extensionId The identifier for the extension
/// @param colony The address of the colony
/// @param extension Address of the extension installation
event ExtensionMigrated(bytes32 indexed extensionId, address indexed colony, address extension);

struct Skill {
// total number of parent skills
uint128 nParents;
Expand Down
90 changes: 86 additions & 4 deletions contracts/colonyNetwork/ColonyNetworkExtensions.sol
Original file line number Diff line number Diff line change
Expand Up @@ -50,19 +50,32 @@ contract ColonyNetworkExtensions is ColonyNetworkStorage {
public
stoppable
calledByColony
returns (address)
{
require(resolvers[_extensionId][_version] != address(0x0), "colony-network-extension-bad-version");
require(installations[_extensionId][msg.sender] == address(0x0), "colony-network-extension-already-installed");

EtherRouter extension = new EtherRouter();
installations[_extensionId][msg.sender] = address(extension);

// 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);

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, msg.sender, _version);

return address(extension);
}

// Deprecated
function upgradeExtension(bytes32 _extensionId, uint256 _newVersion)
public
stoppable
Expand All @@ -81,6 +94,27 @@ contract ColonyNetworkExtensions is ColonyNetworkStorage {
emit ExtensionUpgraded(_extensionId, msg.sender, _newVersion);
}

function upgradeExtension(address _extension, uint256 _newVersion)
public
stoppable
calledByColony
{
require(multiInstallations[_extension] == msg.sender, "colony-network-extension-not-installed");

bytes32 extensionId = ColonyExtension(_extension).identifier();

require(_newVersion == ColonyExtension(_extension).version() + 1, "colony-network-extension-bad-increment");
require(resolvers[extensionId][_newVersion] != address(0x0), "colony-network-extension-bad-version");

EtherRouter(payable(_extension)).setResolver(resolvers[extensionId][_newVersion]);
ColonyExtension(_extension).finishUpgrade();

assert(ColonyExtension(_extension).version() == _newVersion);

emit ExtensionUpgraded(_extension, msg.sender, _newVersion);
}

// Deprecated
function deprecateExtension(bytes32 _extensionId, bool _deprecated)
public
stoppable
Expand All @@ -91,6 +125,17 @@ contract ColonyNetworkExtensions is ColonyNetworkStorage {
emit ExtensionDeprecated(_extensionId, msg.sender, _deprecated);
}

function deprecateExtension(address _extension, bool _deprecated)
public
stoppable
calledByColony
{
ColonyExtension(_extension).deprecate(_deprecated);

emit ExtensionDeprecated(_extension, msg.sender, _deprecated);
}

// Deprecated
function uninstallExtension(bytes32 _extensionId)
public
stoppable
Expand All @@ -99,12 +144,41 @@ contract ColonyNetworkExtensions is ColonyNetworkStorage {
require(installations[_extensionId][msg.sender] != address(0x0), "colony-network-extension-not-installed");

ColonyExtension extension = ColonyExtension(installations[_extensionId][msg.sender]);
installations[_extensionId][msg.sender] = address(0x0);
delete installations[_extensionId][msg.sender];
kronosapiens marked this conversation as resolved.
Show resolved Hide resolved
extension.uninstall();

emit ExtensionUninstalled(_extensionId, msg.sender);
}

function uninstallExtension(address _extension)
public
stoppable
calledByColony
{
require(multiInstallations[_extension] == msg.sender, "colony-network-extension-not-installed");

delete multiInstallations[_extension];
ColonyExtension(_extension).uninstall();

emit ExtensionUninstalled(_extension, msg.sender);
}

function migrateToMultiExtension(bytes32 _extensionId)
public
stoppable
calledByColony
{
address extension = installations[_extensionId][msg.sender];

require(extension != address(0x0), "colony-network-extension-not-installed");

multiInstallations[extension] = payable(msg.sender);

delete installations[_extensionId][msg.sender];

emit ExtensionMigrated(_extensionId, msg.sender, extension);
}

// Public view functions

function getExtensionResolver(bytes32 _extensionId, uint256 _version)
Expand All @@ -115,6 +189,14 @@ contract ColonyNetworkExtensions is ColonyNetworkStorage {
return resolvers[_extensionId][_version];
}

function getExtensionColony(address _extension)
public
view
returns (address)
{
return multiInstallations[_extension];
}

function getExtensionInstallation(bytes32 _extensionId, address _colony)
public
view
Expand Down
7 changes: 5 additions & 2 deletions contracts/colonyNetwork/ColonyNetworkStorage.sol
Original file line number Diff line number Diff line change
Expand Up @@ -93,13 +93,16 @@ contract ColonyNetworkStorage is CommonStorage, ColonyNetworkDataTypes, DSMath {
uint256 DEPRECATED_lastMetaColonyStipendIssued; // Storage slot 37

// [_extensionId][version] => resolver
mapping(bytes32 => mapping(uint256 => address)) resolvers; // Storage slot 38
mapping (bytes32 => mapping(uint256 => address)) resolvers; // Storage slot 38
// [_extensionId][colony] => address
mapping(bytes32 => mapping(address => address payable)) installations; // Storage slot 39
mapping (bytes32 => mapping(address => address payable)) installations; // Storage slot 39

// Used for whitelisting payout tokens
mapping (address => bool) payoutWhitelist; // Storage slot 40

// [_extension] => colony
mapping (address => address payable) multiInstallations; // Storage slot 41

modifier calledByColony() {
require(_isColony[msg.sender], "colony-caller-must-be-colony");
_;
Expand Down
Loading