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

Support Upgrades
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.

21

Thank you @frangio . As I can see, even in diamond storage, you would create a struct for each contract and store it in specific hash slot. If I understand correctly, even in this pattern, you wont be able to add the new variable in the middle of the struct or remove the variable from it. This pattern just helps only with the case of inheritance which is still a big step forward. Does OZ plan on that even with diamond, you would solve it so that it will be possible to even remove the variable from struct ? I would at least design my contracts with this pattern but have a feeling that removing variables and adding them in the middle is still not possible. Thoughts ?

22

Yes, it is not possible to remove or add from the middle of the struct, except if the variable is added in a place where there didn't use to be another variable for reasons of padding and data type sizes.

For example, if a struct contains uint8 x; uint256 y; there is likely empty space between x and y and it is safe to add a new variable uint8 x; uint8 Z; uint256 y;.

Recent versions of OpenZeppelin Upgrades Plugins accept this kind of change to variables and structs.

23

Amazing !!

https://github.com/GeniusVentures/openzeppelin-contracts-diamond is this how you are gonna implement as well ? I would love to follow the same principle as you(OZ)

24

We haven't decided the details yet. But based on what I remember from that fork I think it's not what we'll do, we will probably do a bit simpler and not have a separate contract for Storage.

1 Like