-
Notifications
You must be signed in to change notification settings - Fork 112
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
base: develop
Are you sure you want to change the base?
Conversation
d96e4b8
to
f96a680
Compare
f96a680
to
f4ebba5
Compare
There was a problem hiding this 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
.
@area Ah good observation, I had not thought about the network/colony upgrade discrepancy. Perhaps we can add a version check in |
b7e6ec1
to
adb422e
Compare
9f52e61
to
952344e
Compare
56d1f4a
to
89f9921
Compare
89f9921
to
ca0dd96
Compare
There was a problem hiding this 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"); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
34df2d0
to
a76b8b8
Compare
a76b8b8
to
30c0a32
Compare
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 thebytes32
identifier, so all extension-management functions (upgradeExtension
, etc) now call for theaddress
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.