-
Notifications
You must be signed in to change notification settings - Fork 114
Multiple extension installations #971
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
base: develop
Are you sure you want to change the base?
Conversation
d96e4b8 to
f96a680
Compare
f96a680 to
f4ebba5
Compare
area
left a comment
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
area
left a comment
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 != 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
addressof the extension, rather than thebytes32identifier, so all extension-management functions (upgradeExtension, etc) now call for theaddressof 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.