Content directory smart contracts: Upgradability

[Lezek123]

In current design (#1520 (comment)) we need to migrate data (like channels) if we want to change its representation directly in existing storage contract (like ChannelStorage).

It is possible to avoid migration if we introduce a new storage contract that will only store, for example, part of the channel data that we wish to include in the new logic, ie.:

contract ChannelRewardsStorage {
    mapping (uint => address) rewardAddressByChannelId
    setChannelRewardAddress(_channelId, _address)
}

We can then redeploy the logic contract to which we would transfer ownership of all existing storage contracts (channels, videos, groups) and make it the owner of the new stroage contract (we'd may need to add a small bit of code related to transferring the new channelRewards storage to allow migrating it in the future too).

This process would be similar to introducing completely new concept like Music as described in #1520 (comment)

Alternatives

We could use delegatecall proxies (one of the patters described in https://blog.openzeppelin.com/the-state-of-smart-contract-upgrades/), which may have a few benefits, like keeping the same contract address and possibly reducing the amout of steps needed to make an upgrage (ie. transferring storage contracts ownerships), but most of the delagetecall proxy patters suffer from major drawbacks in terms of security (ie. possible storage clashes) or complexity (ie. the "eternal storage" pattern).

[Lezek123]

I think there are 2 possible strategies to evaluate:

• Strategy 1: Current strategy where we have multiple storage contracts that cannot be upgraded without migration (but we can "transfer" them to a new logic contract and we can also add new storage contracts)
• Strategy 2: A single storage contract (separate from logic) behind a delegatecall proxy contract. In that case we can upgrade the storage contract in some ways without the need for migration, but it may be risky if not done carefully.

Strategy 1

Cons:

• We cannot in any way update existing storage contract without migration and changing its address
• Logic contract becomes increasingly complex as we keep adding new storage contracts
• Frontend that interacts with storage contracts (ie. tests, possibly CLI) becomes increasingly more complex as we keep adding storage contracts
• The upgrade process is quite complex. We need to pass ownership of all existing storage contracts to new logic, so we need to make sure the upgrade/migrate logic (ie. part of old logic contract) executed by the council is always up to date

Pros:

• Completely avoiding storage clashes
• "Forced" modularity can possibly help sidestep any max contract size / gas limit problems (though this is currently not much of a concern in case of storage contracts)

Example upgrade flow:

This is a simple case where we just add a single storage contract, but this will get increasingly more complex if we add more storage contracts or change and migrate data in some existing ones (which may often be the case if we want to introduce some non-trivial changes):

• override the logic contract migrate / upgrade method to make sure it includes logic for passing ownership of the new storage contract
• deploy new logic with current storage contracts addresses that also instantializes new storage contract (either in constructor or separate method that will require additional call to avoid reaching the gas limit)
• call a function of old logic contract (migrate / upgrade) that will pass ownerships of all existing storage contracts to new logic

Strategy 2

Cons:

• Possible storage clashes and security risk if the upgrade is not done carefully, but:
  • There are multiple, well-documented strategies related to delegatecall proxy upgradability patterns (https://blog.openzeppelin.com/the-state-of-smart-contract-upgrades/)
  • There are tools that help validating and testing the upgrades (https://github.com/OpenZeppelin/openzeppelin-upgrades)
  • Having good test coverage and code documentation can reduce the risk to minimum
• Storage contract may become too big (this was my initial concern, but all current storage contracts combined currently cost no more than ~2 mln gas, which is over 3x less than the current limit per block, so this shouldn't be an issue anytime soon)
• In case migration is required - we have to migrate all the data (we cannot do it partially as in Strategy 1)

Pros:

• We can do quite a lot of upgrades that are considered safe and won't require migration:
  • Adding new storage variables/mappings is safe as long as we follow a the append-only pattern
  • Adding properties to structs that we only use inside mappings (like Channel, Video) should be safe as long as we follow append-only principle (see: https://forum.openzeppelin.com/t/are-there-restrictions-on-modifying-enums-and-structs-in-upgradeable-contracts/2088/2)
  • Adding new enum variants should be safe as long as it doesn't change the enum representation in storage (It seems like by default it's uint8, so it should be safe unless we have more than 256 variants)
  • Changing storage contract "setter"/"getter" methods without changing the storage layout itself should be completely safe (some of those methods may include some storage-specific logic like incrementing nextChannelId when adding a new channel etc.)
• As menitioned before - we can use some existing tooling that can help validate and test the upgrades (https://github.com/OpenZeppelin/openzeppelin-upgrades)
• For upgrades that don't require migration we keep a single storage address that remains unchanged, which minimalizes the effect of migration of the frontend
• Most of the upgrades will follow a relatively simple 3-step process described below
• With custom gas/size limits we can combine logic and storage into one contract (we can sitll separate the code using abstract contracts) to make the upgrades even simpler (though I'm not sure if changing the default limits just for this purpose will be a good idea)

Example upgrade flow:
We can upgrade both logic and storage (assuming no-migration case) just by:

• deploying new logic contract (it can extend some existing base contract that will handle transferring storage proxy ownership)
• deploying new storage implementation (owned by new logic)
• calling old logic's method that will set a new implementation for storage proxy and pass the ownership of the proxy to the new logic contract

[bedeho]

It seems to me that the two strategies basically differ in their costs in that one is more fool-proof and simple for smaller changes, but with lots of cumulative upgrades it can get out of hand, while the other is harder to pull off, but does not become more complex to maintain if pulled off correctly.

If this is correct, then the first version is more aligned with doing something that a small community is able to pull off on their own without lots of investment in audits and access to highly experienced and fully occupied developers. We have solid amount of time to just iterate internally before mainnet to get a robust feature set, so many frequent substantial changes should not be urgent in near term once live. After a few upgrades using this approach, the community is more likely to both have built the capability to do something more complex, and also know what their long term requirements look like, at which point they could change the upgradeability pattern.

What do you think?