Define a Standard for Proxy Storage Variables

[HCastano]

In the Ethereum world there is EIP-1967 which defines a well-known set of storage
slots used to represent key variables in the Proxy pattern, such as the admin and
implementation.

It would be nice to have something similar for ink!.

The EIP-1967 approach uses storage slots that are known to not be used by the Solidity
compiler. I don't think we can make the same guarantees with pallet-contracts or
Substrate, so we'll probably need to find a different approach to this.

[SkymanOne]

What's the status of this issue? Is it still planned to implement?

[xermicus]

I have a very strong opinion on this. Please use the exact storage slots as defined in EIP-1967. From a technical standpoint, it doesn't really matter. However, if this will be different with ink!, it creates a perfect footgun regarding compatibility with Solidity contracts. If we use different values, we must yell at everyone using Solang (or ask!) that by the way, this is different on Polkadot (for no apparent reason?). People will inevitably just use existing Solidity code, getting caught off-guard by this. Unless we first hand to decide to stick to those storage slots and call it a day.

[SkymanOne]

AFAIK we are using a different hash function across ink! (blake2x256). Secondly, using storage slots implies using Lazy with ManualKey in a proxy contract which adds some overhead, and since we don't have a problem with storage collision as in solidity, developers can just define those in a packed layoutful storage.

If we have a strong requirement to have matching storage slots with solidity, we can surely propose a PSP
with a fields like implementation: Lazy<AccountId, ManualKey< 0x360894a13ba1a3210667c828492db98dca3e2076cc3735a920a3ca505d382bbc>>

[xermicus]

AFAIK we are using a different hash function across ink! (blake2x256).

How does the this matter (also, both are 32 bytes)? The pre-image for the storage slots in EIP-1967 isn't known anyways (on purpose).

Secondly, using storage slots implies using Lazy with ManualKey in a proxy contract which adds some overhead, and since we don't have a problem with storage collision as in solidity, developers can just define those in a packed layoutful storage.

ink! contracts do potentially have storage collisions when using delegatecall? EIP-1967 just defines address slots and asks the compiler to not allocate them (at least knowingly, as per convention). Reading them is fine (the ref. impl. in the EIP does it).

If we have a strong requirement to have matching storage slots with solidity

Technically, I don't see a strong requirement for picking any particular slot. And given this is true, picking different ones would just set us up for bad interop with Solidity.

[SkymanOne]

How does the this matter (also, both are 32 bytes)? The pre-image for the storage slots in EIP-1967 isn't known anyways (on purpose).

The hash is derived by running the hash function on the known preimage and then subtracting 1. We can either take hash from EIP directly or derive our own using blake2x256.

EIP-1967 just defines address slots and asks the compiler to not allocate them

We don't have a compiler. We can achieve this only using lints.

If we get to hard-code those slots into ink! storage somehow, that would imply that every new contract would be deployed with these 3 empty proxy slots that are also wrapped into Lazy (since it is the only way to specify a key). Lazy automatically adds some processing overhead. So we are ultimately forcing devs to use Lazy for proxy fields. Do we want this?

[xermicus]

So we are ultimately forcing devs to use Lazy for proxy fields. Do we want this?

Ultimately, you can't force anyone to store anything specific in any storage slot. At least not on the contract source level. But if you define a standard, yes you are asking contract devs to adhere to that. And for upgradable contracts and proxy implementations, not using all manual keys is a bad idea anyways isn't it?

[SkymanOne]

The only way I see we can ensure that proxy storage slots are not used by anything else is to "silently" define those storage fields in the contract storage definition, and hide them until the user uses them.

Ultimately, this is what I mentioned above is that every future contract deployed will have these well-known storage keys for proxy fields that may and may not have a value.

This is essentially a breaking change and some thoughts need to be put into the API.

If we just introduce the PSP defining without safety guarantees, then it looks kind of pointless.

[ascjones]

I assume we are discussing the following section of the OZ report https://blog.openzeppelin.com/security-review-ink-cargo-contract

My initial thought is that this (predefined storage slots for proxy storage) is not something that should be baked into the language itself, and that such standards are defined and adopted "organically", e.g. with the existing PSP standards from openbrush.

Further, the Rationale for https://eips.ethereum.org/EIPS/eip-1967 states that:

The rationale for this is that proxies should never expose functions to end users that could potentially clash with those of the logic contract.

We now have built in protection against selector clashing attacks: #1708 so this is not an issue, the public fallback function can be called directly to force the upgrade.

They are chosen in such a way so they are guaranteed to not clash with state variables allocated by the compiler, since they depend on the hash of a string that does not start with a storage index

A less opinionated way to do it, to allow standards to be developed, would be to similarly define a range of storage keys we guarantee will never be written to by the derived Storable implementations. I don't know how feasible/practical this is.

set_code_hash

Ideally we would promote the usage of set_code_hash as the preferred mechanism for implementing the basic "proxy" pattern. It is something that already exists and would allow the implementation of cargo contract upgrade with a pattern such as #1909. The upgrade command could look for common message names such as migrate or set_code_hash on the contracts and execute those.

[cmichi]

The upgrade command could look for common message names such as migrate or set_code_hash on the contracts and execute those.

I think, best would be to have the cargo contract upgrade command ask which message to execute. Otherwise it'll always be a heuristic. Could be used to suggest possible message names though, in case none is provided.

[xgreenx]

My initial thought is that this (predefined storage slots for proxy storage) is not something that should be baked into the language itself, and that such standards are defined and adopted "organically", e.g. with the existing PSP standards from openbrush.

I agree. The implementer of the Proxy contract is responsible for using a separate storage cell to avoid overlapping. As an example, OpenBrush already uses a separate storage cell.

A less opinionated way to do it, to allow standards to be developed, would be to similarly define a range of storage keys we guarantee will never be written to by the derived Storable implementations. I don't know how feasible/practical this is.

I don't think it is possible to implement because any public API provided by ink! can invalidate/corrupt the storage. ink! generates code that works with the storage on the user's side, so it uses a public API. Anyone can easily reuse these public functions in Storable implementation.

Plus, the safest way is to control it during runtime -> it affects the performance.

The upgrade command could look for common message names such as migrate or set_code_hash on the contracts and execute those.

Usually, this kind of functionality is hidden or uses other names, but yeah, we can ask users to use pre-defined names. But it already sounds like some standardization=)

Maybe we can have cargo contract upgrade or cargo contract migrate commands that receive old_contract.metadata and new_contract.metadata compare them and check for storage layout conflicts?

The problem with this solution is that it will not work in the case of the Diamond standard. But we can have one command for storage migration(between two contracts) and another like cargo contract check-conflicts that works with multiple files and scans for overlapping.

[ascjones]

Usually, this kind of functionality is hidden or uses other names, but yeah, we can ask users to use pre-defined names. But it already sounds like some standardization=)

Agreed, then we can simply require some arguments e.g. --init-migration-message and --perform-migration-message

[SkymanOne]

Overall, I agree that set_code_hash should be our "standardized" way of upgradeability. However, before we move on to cargo contract upgrade I think we need to develop necessary API for the migration in case there are amendments to the storage layout - #1388

A less opinionated way to do it, to allow standards to be developed, would be to similarly define a range of storage keys we guarantee will never be written to by the derived Storable implementations. I don't know how feasible/practical this is.

I still believe we need to address this point for future purposes. Perhaps reserve some chunk of keys for "system" purposes, have some range of keys which are never automatically allocated.

[ascjones]

pallet-contracts already has a built-in mechanism for contract upgradeability via set_code_hash. This already satisfies many of the requirements of EIP-1967:

• protection from being overwritten by contract upgrades
• standard "interface" for block explorers to keep track of contract upgrades, see the ContractCodeUpdated event.

It is possible that such a standard could still be proposed/developed and adopted, As @xgreenx mentioned there is something like this in OpenBrush.

But this should not be baked into the language itself, given that we have the built-in upgradeability mechanism already which achieves most of what we want.