Two-step proxy initialization call moved to the second step #323
Conversation
We replaced implementation address in the storage with version ID. Version ID will be calculated as a keccak256 of version string and used to track implementations in a mapping.
This contract can be used to hold implementation details in a two-step upgradeable proxy to hold details of the implementation. In proxy pattern data should be stored in a ways that reduces possibility of collisions between proxy and implementation contracts. In this contract we use a mapping which is allocating storage slot for the data in a dynamic way.
Set implementation ID and implementation details in storage in the constructor.
Previously we initialized implementation contract in the first step of the upgrade that gave some possibility to interactions with the new contract before it should actually be used. Here we move the initialization call to complete upgrade function and update handling of the implementation details to use the ones from UpgradableProxyStorage.
Extracted all code related to the two-step upgradability proxy storage to a seprate contract where we define slots and variables to be stored.
nkuba
changed the title
|
|
||
| /// @title Storage for two-step upgradable proxy contract. | ||
| /// @dev This contract can be used to hold implementation details in a two-step | ||
| /// upgradeable proxy to hold details of the implementation. In proxy pattern |
There was a problem hiding this comment.
This contract can be used to hold implementation details in a two-step upgradeable proxy to hold details of the implementation.
🤔 🤔 🤔
| /// data should be stored in a ways that reduces possibility of collisions between | ||
| /// proxy and implementation contracts. In this contract we define variables at | ||
| /// fixed positions according to the Unstructured Storage pattern and a mapping | ||
| /// which is allocating storage slot for the data in a dynamic way. |
There was a problem hiding this comment.
I think it's worth mentioning what this map is used for. It is not generic-purpose storage.
| address _implementation, | ||
| bytes memory _initializationData | ||
| ) internal { | ||
| uint256 versionInt = uint256(keccak256(abi.encodePacked(_version))); |
| bytes initialization, | ||
| uint256 timestamp | ||
| ); | ||
| event UpgradeCompleted(string version, address implementation); |
There was a problem hiding this comment.
Should we emit address of the implementation again here? I think emitting just the version should be enough. We emitted it for UpgradeStarted event.
| return implementations[_version]; | ||
| } | ||
|
|
||
| function setImplementation( |
There was a problem hiding this comment.
Thoughts on naming it addImplementation? We are storing all implementations in the map, we are not overwriting anything.
EDIT: I think we shouldn't store implementations in this mapping. Everyone can track the history of upgrades from the events we are emitting. Storing just an initialization data we are committing to should be enough. And we can even delete the data from this mapping once the initialization is done.
| function _implementation() internal view returns (address) { | ||
| Implementation memory implementation = getImplementation( | ||
| currentImplementationID() | ||
| ); |
There was a problem hiding this comment.
Do we know what's the impact on the gas cost? It affects all the calls to the system. In the previous implementation we were doing a single sload, without reading the entire map key and calling other functions.
I think the most effective and simplest solution is to store the current implementation data as before with just
bytes32 slot = IMPLEMENTATION_SLOT;
/* solium-disable-next-line */
assembly {
impl := sload(slot)
}
here and have a mapping between the new version ID and initialization call data. When the upgrade starts, we commit to initialization and add the data to the mapping. Then, when the upgrade completes we execute the initialization using the data we previously committed to. If one wants to go through the history of upgrades they can use events.
|
We replaced this implementation with a much simpler solution in #327 |
This PR is a followup covering #297 (comment).
With the previous implementation of the two-step proxy upgradeability, we allowed the admin to interact with the implementation contract at the first step of the upgrade process, which gave some exploits possibilities. In this PR we moved the call to an initialize function of the implementation (logic contract) to a second step so it can only be executed after the required time delay.
To be able to pass the initialization call data between the two steps we were required to store dynamic length bytes array in the contract storage. Due to the risk of clashing data storage between proxy and implementation we previously decided to use Unstructured Storage pattern where we define fixed positions for each of the variables used in the proxy contract. Unfortunately, it didn't work for dynamic bytes array, so we decided to store this dynamic type data in a mapping which is distributing storage based on the data's hash.
To clean up the code a little bit and prepare it for reusing we extracted storage variables for the two-step upgradeability pattern to a separate contract. There are more parts that can be extracted to separate contracts but it does not make much sense now taking into consideration the timeframes and that we are willing to reorganize our repositories in the near future.