Move proxy initialization to constructors #106
Conversation
facuspagnuolo
force-pushed
the
move_proxy_initialization_to_constructors
branch
from
b242825
to
7d174a4
Compare
There was a problem hiding this comment.
It's always nice when we get to remove code like this :-)
I like the PR, and I think it can be merged right away (though it will conflict heavily with #99 I'm afraid). But there is one pending discussion around supporting initializer functions vs contracts that I think we need to settle.
| @@ -75,8 +73,9 @@ contract BaseApp is Ownable { | |||
| * @return Address of the new proxy. | |||
| */ | |||
| function create(string packageName, string contractName) public returns (AdminUpgradeabilityProxy) { | |||
| address implementation = getImplementation(packageName, contractName); | |||
| return factory.createProxy(this, implementation); | |||
| // TODO: should we merge this function with the createAndCall? | |||
| assert(IMPLEMENTATION_SLOT == keccak256("org.zeppelinos.proxy.implementation")); | ||
|
|
||
| if(_data.length > 0) { | ||
| require(_implementation.delegatecall(_data)); |
There was a problem hiding this comment.
This means we are settling with initializer functions and not initializer contracts. Should we consider instead accepting address implementation, address initializer, bytes data, and run initializer.delegatecall(data) if initializer is not zero? The App could initially use the same address for both, but leaves the door open for initializer contracts in the future. @frangio care to chime in here?
There was a problem hiding this comment.
I'd like to leave the door open but I'm worried that it might be overkill for now. Would it be too disruptive to change that in a future version?
Note that if we do add the address initializer parameter, we should allow _data.length == 0 because initializer contracts can run with no data.
There was a problem hiding this comment.
I'd like the idea of initializer contracts but in that case, I'd expect the CLI to solve the initializers functions to initializer contracts conversions and it might be an overkill for now. I think we can provide a new implementation of the proxy using initializer contracts with the required CLI improvements to deal with it in the future.
There was a problem hiding this comment.
So the CLI change would be transparent and backwards compatible. But the Proxy and App changes would not be backwards compatible and would need a major bump.
Are we comfortable with that?
There was a problem hiding this comment.
The App change could be backwards compatible, as we could just add a new createAndCall method that receives the initializer address parameter as well as the others. The only breaking change would be around proxies, but since it only affects the ctor, I don't think it'd be breaking at all.
There was a problem hiding this comment.
What do we mean by "breaking" exactly? If we do change the constructor it could break deployment scripts that use the Proxy directly.
There was a problem hiding this comment.
True, I was counting on that not many people use them directly
| require(address(_factory) != address(0), "Cannot set the proxy factory of an app to the zero address"); | ||
| factory = _factory; | ||
| } | ||
| constructor() public {} |
There was a problem hiding this comment.
I hesitated about entirely removing the ProxyFactory, since the original idea was that we could have a single ProxyFactory shared by all Apps, so each user would only need to deploy the App. However, since we are now aiming at making these contracts upgradeable, gas cost for the end user is exactly the same, since they only need to deploy a proxy to App, regardless of where the factory code for proxies is. So +1 to this change.
|
@facuspagnuolo I've just noted that there are some tests on SimpleProject failing. I started working on #88 on top of this PR without noticing that, and I'm already fixing them on my branch. We can either:
|
facuspagnuolo
force-pushed
the
move_proxy_initialization_to_constructors
branch
from
414d32d
to
60d078a
Compare
facuspagnuolo
force-pushed
the
move_proxy_initialization_to_constructors
branch
from
60d078a
to
836824e
Compare
|
@spalladino I committed some new changes, please take another look. I also cherry-picked the commits you mentioned about the |
* Move proxy initialization to proxy constructor * Drop UpgradeabilityProxyFactory contract * Update status checker in order to use App to fetch proxies * Fix AppProject test * Fix proxy to set implementation before initializing * Clarify data parameter in proxy constructor * Merge create and createAndCall functions of BaseApp * Update SimpleProject and Proxy models * Update lib-simple example to new SimpleProject interface
* Move proxy initialization to proxy constructor * Drop UpgradeabilityProxyFactory contract * Update status checker in order to use App to fetch proxies * Fix AppProject test * Fix proxy to set implementation before initializing * Clarify data parameter in proxy constructor * Merge create and createAndCall functions of BaseApp * Update SimpleProject and Proxy models * Update lib-simple example to new SimpleProject interface
Fixes #100