To Inherit Version1 to Version2, Or to Copy Code + Inheritance Order from Version1 to Version2?

Support Upgrades
1

I'm game planning for upgrades of implementation contracts using UUPSUpgradable contracts, and had some questions about how to carry the logic and inherited base contracts over SAFELY over from Version 1 to Version 2.

I've come across two patterns:

1: Inheritance Version 1 inherits Version2 like Version1 is Version0, CoolNewBaseContract, and expands from there with new base contracts, methods, etc.

This would look like:

Version0 is OwnableUpgradable {
    function myMethod() public view returns (uint256) {
        return 1;
    }
}

`Version1 is Version0, CoolNewBaseContract {
    // Gets OwnableUpgradable via inheritance from Version0
    // Inherits CoolNewBaseContract AFTER Version0 to preserve storage layout

    // Cannot override myMethod, so has to declare a new method
    function newMethod() public view returns (uint256) {
        return 2;
    }
}

2: Copying Code Across Versions, Not Inheriting Old Implementation Copying code from V1 into V2, inheriting the same contracts in the same order on the new V1 to preserve storage layout, and then updating/adding/deleting any methods as needed.

This would look like:

Version0 is OwnableUpgradable {
    function myMethod() public view returns (uint256) {
        return 1;
    }
}

`Version1 is OwnableUpgradable, CoolNewBaseContract {
    // Inherits in correct order, AND adds a cool new base contract with storage variables and methods

    // Same method signature, new logic
    function myMethod() public view returns (uint256) {
        return 2;
    }
}

ACTUAL EXAMPLE USING INHERITANCE

The original version 0 inherits all kinds of contracts-upgradable contracts from Open Zeppelin: https://github.com/OpenQDev/OpenQ-Contracts/blob/production/contracts/OpenQ/Implementations/OpenQV0.sol#L19

I was planning on upgrading like this in order to inherit the former version: https://github.com/OpenQDev/OpenQ-Contracts/blob/development/contracts/OpenQ/Implementations/OpenQVersionUpgrades.sol#L48

COMPOUND: ACTUAL EXAMPLE USING CODE COPY

I noticed Compound has deployed 7 implementation contracts, all of which inherit from one changing storage base contract. Besides updating their inherited Storage, they basically copy in the old contract and modify as needed: https://github.com/compound-finance/compound-protocol/tree/9e93e858dbad48ae4e0c59b13bb4c70856c596d9/contracts

BUT COMPOUND UPDATES NEVER INHERIT ANY NEW BASE CONTRACTS! And therein lies a secondary question about C3 linearized resolution order of storage layout...

SUGGESTIONS + DANGERS OF EACH APPROACH?

So which would you suggest?

  1. Inheriting the older versions in a chain
  • One downside is you can't override functions from the old version :grimacing:. Would end up with many methodName1, methodName2
  1. Making a copy of V1 in V2, with the same inheritance order to preserve storage layout?
  • What about the fact that storage layout results from the C3 linearization of the inherited base contracts? Could inheriting from a new 3rd party contract mess this up, EVEN IF we preserve the order of inheritance?

From the Solidity docs:

For contracts that use inheritance, the ordering of state variables is determined by the C3-linearized order of contracts starting with the most base-ward contract. If allowed by the above rules, state variables from different contracts do share the same storage slot.

I am leaning towards the Compound approach of not inheriting older versions since it allows for preserving method signatures.

I just want to make sure it will be safe to inherit from new base contracts from 3rd parties and such without knocking storage layouts due to funky C3 linearization resolution.

2

Hi @flaco_jones, that looks like a good summary of the tradeoffs, since both approaches have their pros and cons.

For the code copy approach, you are right that inheriting new parent contracts could cause conflicts with storage slots.

  • If your implementation contract itself does not have any state variables, then this is fine since you can add new parent contracts to the end of the inheritance list e.g. contract V1 is A, B -> contract V2 is A, B, C.
  • If your implementation contract itself does have state variables, then you cannot do the above unless you have storage gaps in your implementation, and then reduce those gaps as you use more storage slots in later versions (either in your contract itself or due to inheriting more parent contracts).
3

That's correct, my plan was to not have any storage variables in the implementation itself. ALL storage would live in the StorageV1 style contracts. That way there we don't have to think about __gaps. Leave that to the geniuses at OpenZeppelin :-).

I think I found a possible solution to this which leans mainly on the Compound code-copy approach and abstract contracts to keep inheritance ordered.

// Inherit all things onto an abstract contract, and add whatever storage vars you need after that
abstract contract OpenQStorageV0 is
    OwnableUpgradeable,
    UUPSUpgradeable,
    ReentrancyGuardUpgradeable,
    Oraclize
{
    BountyFactory public bountyFactory;
    OpenQTokenWhitelist public openQTokenWhitelist;
}

// Inherit the abstract contract onto your main implementation
// This works, as OpenQV0 overrides any necessary methods from the base contracts
contract OpenQV0 is OpenQStorageV0 {...}

// ----- MANY YEARS AND HACKS LATER -----

// Later, there's a New Base Contract you want to inherit
contract NewBaseContract {
    uint256 public foo;

    function setFoo(uint256 _foo) public {
        foo = _foo;
    }
}

// We Inherit the old storage contract, THEN we inherit from NewBaseContract, THEN we add any new custom storage variables
// I believe this maintains the storage order from before, and adds new ones after them.
// UNLESS there are dependency interactions between NewBaseContract and the old ones resulting from C3 linearization?
abstract contract OpenQStorageV1 is OpenQStorageV0, NewBaseContract {
    uint256 public newStorageVar = 456;
}

// Inherit the newer abstract contract onto your main implementation
contract OpenQV1 is OpenQStorageV1 {...}

I just tried this upgrade approach using code copy + abstract contracts for storage, and it worked without messing up storage layout :cowboy_hat_face:. I could update foo and newStorageVar.

So after this sequence of inheritance (NEVER putting storage vars directly in the implementation contract), I believe the storage layout of OpenQV1 would look something like this after upgrade:

[OpenZeppelin upgradable inherited contracts]
[2 OpenQStorageV0 storage variables]

after upgrade append....

[1 foo from NewBaseContract]
[1 newStorageVar from OpenQStorageV1]

Then I'd continue this chaining into the bright and storied future of the dApp...

Thoughts? Is this awful? What gotchas am I missing, or will this let me safely add both NEW CUSTOM STORAGE VARIABLES and NEW 3rd PARTY BASE CONTRACTS?

4

That looks like a reasonable approach! An alternative could be to define the inheritance in the implementation itself rather than the abstract storage contract, but that may be just a matter of preference. For example:

abstract contract V1Storage {
  (V1 state variables only)
}
contract V1 is ParentA, V1Storage {
  (functions only)
}

abstract contract V2Storage {
  (V2 state variables only)
}
contract V2 is ParentA, V1Storage, ParentB, V2Storage {
  (functions only)
}

This overall pattern that you described is indeed used, for example The Graph uses storage contracts as well.

1 Like
5

That's great!

I arrived at the abstract contract approach by first considering the contract V2 is ParentA, V1Storage, ParentB, V2Storage approach you mentioned.

I opted for the abstract contract approach so that the inheritance can be the single responsibility of the storage contracts.

So instead of contract V2 is ParentA, V1Storage, ParentB, V2Storage I can just do:

contract V2 is V2Storage

which resolves the same inheritance graph since

V2Storage is ParentA, V1Storage, ParentB

Thank you for the Graph link! We love and use them a lot.

1 Like
6

I do wonder if perhaps there are some 3rd party contracts that need to be inherited by contract rather than abstract contract. E.g., some contract which requires its direct parent to do implement some function.

If so that would seriously hamstring my approach.

Do such things exist?

If so, I suppose I could just implement any necessary methods in the abstract storage contract.

7

If you mean a contract that requires an override, then you should be able to override it in the implementation rather than the abstract contract (either place would work, although doing it in the abstract contract may prevent you from upgrading its logic with your example).

1 Like
8

@ericglau Hi Eric, It turns out that what you say might be wrong for the cody-copy approach even if implementation has storage gaps.

The author mentions:

Version0 is OwnableUpgradable {
    function myMethod() public view returns (uint256) {
        return 1;
    }
}

`Version1 is OwnableUpgradable, CoolNewBaseContract {
    // Inherits in correct order, AND adds a cool new base contract with storage variables and methods

    // Same method signature, new logic
    function myMethod() public view returns (uint256) {
        return 2;
    }
}

Let's imagine gap is in every contract and it's 50 total.

Inheritance chain of Version0 was Version0 => OwnableUpgradable which means:
0-50 slots belong to OwnableUpgradable
51-100 slots belong to Version0.

In the cody-copy updated code, we got the chain:

Version1 => CoolNewBaseContract => OwnableUpgradable
0-50 slots belong to OwnableUpgradable
51-100 slots belong to CoolNewBaseContract.
101-151 slots belong to Version1

It seems like the slots of CoolNewBaseContract overrides the slots of Version1((which includes the old state vars as well) Even including the gap in the implementation is nowhere enough , but you mention that if implementation includes gaps, it's all good.

Am I missing something ? or how is it possible with code copy approach ? remember that your answer was when you guys didn't talk about storage contracts separately !

9

I see. Yeah I noticed that on earlier attempts, which is how I ended up going with a "chain of storage contract versioning" which is then the SOLE inherited contract of the implementation.

Like Compound

You mention Version1((which includes the old state vars as well but in the code sample you provide Version1 does not inherit Version0, so how does it container the old state vars?

10

Hi @novaknole, what I meant was if you have storage gaps and want to add new parent contracts (which also have variables and/or gaps), you could do this by reducing the gaps in the child contract. This only works if the gap in Version0 was declared before any of its other variables.

In your example:
0-50 slots belong to OwnableUpgradable
51-100 slots belong to gap in Version0 .
(101 and above slots belong to other variables in Version 0)

In the code-copy updated code, reduce the gap by the proper amount (in this case removing it since the gap was only 50 slots to begin with):
Version1 => CoolNewBaseContract => OwnableUpgradable
0-50 slots belong to OwnableUpgradable
51-100 slots belong to CoolNewBaseContract.
no more gap in Version1
(101 and above slots belong to other variables in Version 0)

But if you didn't have a gap at the beginning of Version0, then this doesn't work so this approach is quite limited!

11

Thanks @ericglau so much for the great answer.

I will take a chance and would appreciate if you could look into this as well.

  1. It turns out uint[50] in the beginning before the variaables is good, but 50 won't be enough. what if there might be multiple parent needed to inherit from ? if 2, then we reserve 100.

  2. It turns out that the [50] _gaps is only useful in the beginning of the contract before the variables if that contract is the implementation one (last one in the inheritance chain). The ones it inherits from don't need it in the beginning, unless they can also act as the implementation contracts in some other cases.

  3. is contracts-upgrades package solid ? does levenshtein work well ? can it detect every layout and wrong slot change even in complicated inheritance correctly ? and we can be safe ?

  4. I heard that you're thinking of moving to diamonds pattern, is it true ? Not a big fan to be honest, but this gaps also make me crazy if inheritance update is required. if yes, when do you plan on making it published ?

Thank you in advance.

12

Hi @novaknole,

1 & 2 - I agree. I'm not aware of whether this pattern is used in practice (gaps at the beginning of implementation), but it is technically possible as described. Different patterns have their own advantages as discussed above.

  1. The upgradeable contracts at https://github.com/OpenZeppelin/openzeppelin-contracts-upgradeable/ use gaps at the end of contracts. This is also the case for presets e.g. here (although those presets are deprecated).

  2. Assuming you are asking about Upgrades Plugins - it validates storage layout and gives an error if it determines an upgrade is unsafe (or may be unsafe).
    For storage gaps, it only supports limited use cases described in this doc (and would give errors otherwise) -- if you have a specific use case that is not supported (including some described in this thread), feel free to open a GitHub issue with your scenario.

  3. Refer to https://github.com/OpenZeppelin/openzeppelin-contracts/issues/2964

13

@ericglau

  1. Note that in this solution, you mention, problem is inheritting contract should be reducing slot gap size depending on its parent(if new variable gets added in parent, you have to go to inheritting contract and reduce the gap). Correct ?

If so, This increases complexity x times. Now, not only you care about the gap for the current contract that you're updating, you should go, find all contracts that inherits from it and reduce their gap sizes. This is madness.

  1. How does OZ handle this ? which way do you choose ? i understand you don't have the implementation contracts. all your contracts turn out that it shouldn't be deployed directly(i mean, they can, but you don't and since customers inherit from your contracts, it's customers' responsibility to handle gaps in their contracts)., but I still wanted to ask maybe you needed it somewhere. Because let's look at this:

contract OZ_contract1 {
    uint k;
    uint[49] _gap;
}

contracts OZ_contract1 is OZ_contract2 {
    uint z;
    uint[49] _gap;
}

contract MyContract is OZ_contract1 {
   // no variable....
}

//                        50-100         0-50(slots)
// Chain MyContract => OZ_contract1 => OZ_contract2

// Lets say OZ updated `OZ_contract1` such as 

contracts OZ_contract1 is OZ_contract2, OZ_contract3 {
    uint z;
    uint[49] _gap;
}

//                         100-150      50-100         0-50(slots)
// Chain MyContract => OZ_contract1 => OZ_contract3 => OZ_contract2

Looks like OZ framework got the same problem, bigger problem. Just inheritting from new contract broke my chain slots. Note that I didn't even have storage for myContract. So there was nothing I could do. It's just OZ that messed it up. Why don't you use the slots then in the beginning ? how did you solve this ? @flaco_jones @ericglau maybe OZ thinks and is aware of this and ensures that they will never add new contract in the inheritance chain ever. If they still need to do it, they still won't and live with the consequences. or what am I missing . + @flaco_jones note that even in your way, if you want to update it in a way that OpenQStorageV0 now needs to inherit from one more contract, you still can't and only thing you could do is to add inherit in the most, derived, implementation contract which is huge limitation.

  1. you don't fully go to Diamond pattern right ? but only go to diamond storage pattern ? there's huge difference as diamond pattern also suggests to split functions in multiple contracts.
14
  1. Note that in this solution, you mention, problem is inheritting contract should be reducing slot gap size depending on its parent(if new variable gets added in parent, you have to go to inheritting contract and reduce the gap). Correct ?

Not really. If a new variable is added in the parent, the parent should reduce its own gap. So it does not affect child contracts.

  1. How does OZ handle this ?

This is answered in point 3 above. The libraries have gaps at the end, and we don't add gaps at the beginning since it would be a breaking change at this point. When changes are needed, they are designed while keeping this restriction in mind (regarding inheritance and storage layout).

  1. you don't fully go to Diamond pattern right ? but only go to diamond storage pattern ?

Correct, the plan is only for diamond storage.

15

Hm, I think for 1 & 2, you didn't get me.. @ericglau

  1. For 1, I meant if user chooses to add gap in the beginning of the contract before the variables. In that case, I repeat my question.

Note that in this solution, you mention, problem is inheritting contract should be reducing slot gap size depending on its parent(if new variable gets added in parent, you have to go to inheritting contract and reduce the gap). Correct ? If so, This increases complexity x times. Now, not only you care about the gap for the current contract that you're updating, you should go, find all contracts that inherits from it and reduce their gap sizes. This is madness.

Imagine the following:

contract Voting_V1 is OZ_contract1 {
    uint num1;
}

// Let's say  I directly added variables in `Voting_V1` Now, If I need to update and follow code copy approach and inherit from another contract:

contract Voting_V2 is OZ_contract1, 3rdContract {
    uint num1;

    uint[49] _gap;
}

// inheritance: Voting_V2, 3rdContract, OZ_contract1

As shown, inheritance chain is messed up because 3rdContract overwrote num1's slot. So unless I had had gaps in the beginning of the Voting_V1, I am stuck(Let's just I only want to use gaps and not split storage in its own contract - if tha'ts so, please look at my question again just a couple of lines above)

  1. This one which I'm gonna again repeat here is something OZ's gaps in the end of each contract really doesn't solve it. Note that example only uses OZ's contracts + my implementation contract that doesn't have state variables to show you exacty what I mean.
contract OZ_contract1 {
    uint k;
    uint[49] _gap;
}

contracts OZ_contract1 is OZ_contract2 {
    uint z;
    uint[49] _gap;
}

contract MyContract is OZ_contract1 {
   // no variable....
}

//                        50-100         0-50(slots)
// Chain MyContract => OZ_contract1 => OZ_contract2

// Lets say OZ updated `OZ_contract1` such as 

contracts OZ_contract1 is OZ_contract2, OZ_contract3 {
    uint z;
    uint[49] _gap;
}

//                         100-150      50-100         0-50(slots)
// Chain MyContract => OZ_contract1 => OZ_contract3 => OZ_contract2

Look, OZ updated their own contract by which I mean, they updated OZ_contract1 to inherit from another OZ contract. (I need to compile my code again and update MyContract again for this - not always, but this could be needed/required). Look at the new inheritance chain in the comments. It messed it up the slots. So, OZ having gaps in the end of their contract(which I showed you by example) didn't help in that case. what's wrong with my example ?

I hope you realize what I mean ! Thanks again.

16

You're correct that the scenarios in your examples won't work if your contracts were set up that way. This is why having gaps (at the beginning and end of your contract) could help. OZ contracts do not have gaps at the beginning so we avoid getting into those situations by designing around it.

17

How do you avoid then the situation in my second example ? Thats what I am curious about.

18

@novaknole Just to clarify, have you actually ran into this issue with an ugprade to your OpenZeppelin dependencies?

There is nothing particularly smart that we do to avoid the situation in your second example. We just avoid making those changes to inheritance, because we know that it would cause problems to users downstream. (We may have missed this at some point which is why I asked the previous question.)

For your first point, the solution to the problem is to create a new contract Voting_V1_Storage to define Voting_V2.

contract Voting_V1_Storage {
    uint num1;

    function foo() external { ... }
}

contract Voting_V2 is OZ_Contract1, Voting_V1_Storage, ThirdContract {
    function foo() external { ... }
}

This will keep num1 in the same place.

It is pretty clear at this point that using gaps to keep storage align doesn't scale and still has many problems. This is why the next major version of OpenZeppelin Upgradeable Contracts will use a different way to organize storage.

19

Thanks @frangio.

I just want to be sure of one thing .

So if my contract inherits from OZ’s contract and then later Oz’s contract updates and now it inherits from another OZ contract, as I showed in my example, then it is broken Already. You cant avoid this in my opinion. So it seems this is limitation from OZ and you just assume that your contracts never get updates in a way that it starts inheritting from another OZ contract(otherwise it will be broken). Is this correct ? Could you confirm ?:pray:

We will be one of the hugest users of OZ so I want to be sure we are not missing anything. I love the diamond storage but solidity wont publish 0.9 until Q1 2023 so i am in a hurry :ok_hand:

20

Yes, this is correct. We don't just assume this, we accept it as a limitation we need to work with. It has happened before that there are changes (usually refactors) that we can't implement until 5.0 because they would break storage compatibility in exactly the way you're describing.