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

Conversation

kronosapiens
Copy link
Member

@kronosapiens kronosapiens commented Jun 23, 2021

Closes #968

Change internal network-level extension bookkeeping to support an arbitrary number of simultaneous extensions with the same identifier. Backwards-compatible with existing extensions.

New functionality is oriented around the address of the extension, rather than the bytes32 identifier, so all extension-management functions (upgradeExtension, etc) now call for the address of the extension.

To check if an extension is installed, use getExtensionColony, passing in the address of the extension. The function will return the address of the installed colony, if installed.

@kronosapiens kronosapiens self-assigned this Jun 23, 2021
@kronosapiens kronosapiens force-pushed the feat/multiple-extensions branch 3 times, most recently from d96e4b8 to f96a680 Compare June 30, 2021 21:19
@kronosapiens kronosapiens marked this pull request as ready for review June 30, 2021 21:26
Copy link
Member

@area area left a comment

Choose a reason for hiding this comment

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

The main problem I'm seeing here is that this is currently not backwards compatible. If I am an old colony version, and network has been upgraded, I call installExtension, which calls installExtension on the upgraded network, which records the installation in multiInstallations. If I then call deprecateExtension, or one of the other extension management functions, I call a deprecated one (because I am not an upgraded colony), which fails because of the require in migrateToMultiExtension - even though I am an old colony, the installation is in multiInstallations.

contracts/colony/ColonyStorage.sol Outdated Show resolved Hide resolved
@kronosapiens
Copy link
Member Author

@area Ah good observation, I had not thought about the network/colony upgrade discrepancy. Perhaps we can add a version check in installExtension which will use installations for v6 colonies and multiInstallations for v7.

@kronosapiens kronosapiens force-pushed the feat/multiple-extensions branch 5 times, most recently from b7e6ec1 to adb422e Compare July 8, 2021 22:54
contracts/colony/ColonyAuthority.sol Outdated Show resolved Hide resolved
contracts/colony/ColonyAuthority.sol Show resolved Hide resolved
contracts/colony/IColony.sol Outdated Show resolved Hide resolved
@kronosapiens kronosapiens force-pushed the feat/multiple-extensions branch 2 times, most recently from 9f52e61 to 952344e Compare July 13, 2021 02:29
@kronosapiens kronosapiens force-pushed the feat/multiple-extensions branch 3 times, most recently from 56d1f4a to 89f9921 Compare July 27, 2021 23:45
Copy link
Member

@area area left a comment

Choose a reason for hiding this comment

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

Broadly speaking, I think a decisions need to be made based on what the client currently does and / or how much bandwidth the frontend team have to accommodate changes. At the very least, we need designs for and they'll need to implement the mechanism for calling the migrate function, and possibly more beyond that depending on what events they use for finding extensions.

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.

contracts/colony/Colony.sol Outdated Show resolved Hide resolved
contracts/colonyNetwork/ColonyNetworkExtensions.sol Outdated Show resolved Hide resolved
contracts/colonyNetwork/ColonyNetworkExtensions.sol Outdated Show resolved Hide resolved
contracts/colonyNetwork/ColonyNetworkExtensions.sol Outdated Show resolved Hide resolved
test/contracts-network/colony-network-extensions.js Outdated Show resolved Hide resolved
test/extensions/coin-machine.js Show resolved Hide resolved
test/extensions/token-supplier.js Outdated Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Allow more than one instance of an extension to be installed
2 participants