OpenZeppelin / openzeppelin-sdk

OpenZeppelin SDK repository for CLI and upgrades.js. No longer actively developed.
MIT License
431 stars 200 forks source link

Initializable to have onlyInitialized modifier and isInitialized function #1541

Open 3sGgpQ8H opened 4 years ago

3sGgpQ8H commented 4 years ago

Currently, storage variable initialized is declared as private in Initializable smart contract. It is common situation, that some functions of an initializable smart contract (probably all the function except for initialize) may only be called when smart contract is properly initialized. However, currently there is no easy way to enforce this. I suggest to add two things to Initializable smart contract: onlyInitialized modifier that will revert unless smart contract is initialized, and internal function isInitialized that will return true in case smart contract is initialized, and false otherwise.

These simple improvements will make Initializable smart contract much more useful and should not break backward compatibility.

abcoathup commented 4 years ago

Hi @3sGgpQ8H ! Thanks for the suggestion, it is really appreciated.

The project owner should review your suggestion during the next week.

Please wait until we have discussed this idea before writing any code or submitting a Pull Request, so we can go through the design beforehand. We don’t want you to waste your time!

frangio commented 4 years ago

Thank you for the suggestion @3sGgpQ8H.

I'm not sure there exists a scenario where one would want to allow a function call before initialization. The way we currently enforce that no functions are called before initialization is that the initializer is called in the same transaction that creates the proxy. This is what we would recommend to anyone using proxies even if they don't use our tools specifically. In that scenario, manually adding the onlyInitialized modifier doesn't add any value at all, in fact it will add friction by making it more difficult to use functionality during initialization.

Can you share your specific use case so we can evaluate in context whether this is something that more people may find useful? So far I don't see us wanting to add the feature.

3sGgpQ8H commented 4 years ago

If this is enforced by Proxy, then why do we need Initialiizable at all? Basically, what Initializable does, it guarantees that initializer will not be called more than once. Also, as you said, Proxy guarantees that initializer will be called at least once at the deployment. As a result, initializer is guaranteed to be called exactly once before any other function. This is exactly what we want to guarantee. However, from architectural point of view, it would be better, if Proxy would guarantee this on its own, without any assistance from the contract is wraps.

frangio commented 4 years ago

Interesting questions! For initializer functions I don't see how it would be possible for Proxy to guarantee they're called exactly once. The reason is that an initializer function can have any function selector so the proxy has no way of telling it apart from a normal function call.

There is an alternative to initializer functions that can guarantee exactly-once initialization: initializer contracts, as I proposed them in my article Towards Frictionless Upgradeability. However, this idea was never implemented in practice (that I know of) because it's more complicated.

The gist of the idea is that the constructor of Proxy can have an extra parameter which is an initializer contract, and it will perform a delegatecall into that contract. This guarantees exactly-once initialization.

I encourage you to experiment with the approach if you're interested.

3sGgpQ8H commented 4 years ago

But Proxy anyway has to know initializer selector to be able to call it on deployment. Though, currently it is not guaranteed, that a function being called by a Proxy on deployment is actually the function marked with initializer modifier.