OpenZeppelin / openzeppelin-labs

A space for the community to interact and exchange ideas on the OpenZeppelin platform. Do not use in production!
https://openzeppelin.com/
MIT License
374 stars 115 forks source link

Allow upgradable contracts to inherit directly from EternalStorage #49

Closed cwhinfrey closed 6 years ago

cwhinfrey commented 6 years ago

Switching EternalStorageProxy to inherit from EternalStorage before OwnedUpgradeabilityProxy makes the EternalStorage state come first. This allows upgradable smart contracts (like Token_V0 in the example) to inherit directly from EternalStorage instead of OwnedUpgradeabilityStorage and completely eliminates the need for OwnedUpgradeabilityStorage. OwnedUpgradeabilityStorage was necessary to make sure the EternalStorage state was in the right memory slots but if no state comes before the EternalStorage state, it is not necessary.

AugustoL commented 6 years ago

:clap: :clap: @cwhinfrey . I think you will need to change the diagram of the implementation in the docs too and remove the OwnedUpgradeabilityStorage too, right?

cwhinfrey commented 6 years ago

👍 @AugustoL I'll get that updated

cwhinfrey commented 6 years ago

@AugustoL I update the docs. Let me know if it makes sense. (Also, didn't mean to close the PR a little bit ago)

facuspagnuolo commented 6 years ago

In this case we are breaking the idea of having a Token_V0 that follows exactly the same storage structure needed by the proxy, am I right? I mean, it won't be aware of the upgradeabilityProxyOwner, right?

cwhinfrey commented 6 years ago

@facuspagnuolo Right, the storage structure would be the same for the EternalStorage state variables but Token_V0 functionality would not be aware of _upgradeabilityOwner, _version, and _implementation unless it also inherited from UpgradeabilityOwnerStorage and UpgradeabilityStorage. My thinking was that typically contracts like Token_V0 are only interacting EternalStorage state variables and that the state and functionality from UpgradeabilityOwnerStorage and UpgradeabilityStorage was unnecessary for contracts like Token_V0.

rstormsf commented 6 years ago

I think it's the right way of doing it

facuspagnuolo commented 6 years ago

Oh, I see.. Looking at the readme diagram it wasn't clear that you were changing the storage structure of the EternalStorageProxy too, nice catch! Given that left-to-right order in the diagram was intended, we should flip the readme diagram to reflect how EternalStorageProxy's inheritance structure works now.

cwhinfrey commented 6 years ago

@facuspagnuolo Good point. Just updated the readme diagram to reflect the inheritance structure.

facuspagnuolo commented 6 years ago

@cwhinfrey please indent the readme diagram two more spaces to the right, otherwise it won't be interpreted as a block code and the result will be the following:

image

Thanks!

cwhinfrey commented 6 years ago

@facuspagnuolo updated 😃