Multiple inheritance with upgradeable contracts

[R-Santev]

🧐 Motivation
Hello OpenZeppelin team,

Navigating the world of multiple inheritance with upgradeable contracts can be a tad complex, especially for those who are newer to the ecosystem. I've noticed there isn't a clear guideline on this in the documentation, which led me to develop a potential approach. My intention is to foster a discussion here to refine it and maybe consider incorporating some variant of it into the OpenZeppelin documentation, if deemed useful.

📝 Details
Here is a question I found on StackOverflow:

I'm trying to convert an existing non-upgradeable contract that has multiple inheritance into an upgradeable one. I'm following the tutorial at https://docs.openzeppelin.com/contracts/4.x/upgradeable and the only thing I've found in docs is the following:

Initializer functions are not linearized by the compiler like constructors. Because of this, each __{ContractName}_init function embeds the linearized calls to all parent initializers. As a consequence, calling two of these init functions can potentially initialize the same contract twice.

The function __{ContractName}_init_unchained found in every contract is the initializer function minus the calls to parent initializers, and can be used to avoid the double initialization problem, but doing this manually is not recommended. We hope to be able to implement safety checks for this in future versions of the Upgrades Plugins.

I don't know what to do from here. It talks about a problem, tells a workaround, but also telling that manually isn't recommended, and also telling that it will have the safety checks in the future upgrades plugins.

So what should I do? It says what I shouldn't do but doesn't mention what I should do. Am I missing something?

How can I have multiple inheritance and upgradeability at the same time with OpenZeppelin contracts? (I'm extending ERC20BurnableUpgradeable and [draft-]ERC20PermitUpgradeable, and using Solidity 0.8.9, Hardhat, OpenZeppelin 4.7.3 if it helps)

Here is a link to the question.

And here is my recommended approach:

1. If a parent contract is inherited by only one child, use the child's init() to invoke the parent's init().
2. If a parent contract is inherited by multiple child contracts, use the init() of the closest child that inherits from (wraps) the other child contracts inheriting the parent.

Consider it as bundling sections of contracts into modules. These modules are then grouped into larger modules, and this hierarchical grouping continues until you reach the most derived contract.

It sounds a bit complicated, but the following example makes it clear.

// SPDX-License-Identifier: MIT
pragma solidity ^0.8.0;

import "@openzeppelin/contracts-upgradeable/proxy/utils/Initializable.sol";

contract Animal is Initializable {
    function __Animal_init() internal onlyInitializing {
        // Most base contract
        __Animal_init_unchained();
    }

    function __Animal_init_unchained() internal onlyInitializing {
        // Initialization logic for Animal
    }
}

contract ThinkingExtension is Initializable {
    function __ThinkingExtension_init() internal onlyInitializing {
        __ThinkingExtension_init_unchained();
    }

    function __ThinkingExtension_init_unchained() internal onlyInitializing {
        // Initialization logic for ThinkingExtension
    }
}

contract Human is Animal, ThinkingExtension {
    function __Human_init() internal onlyInitializing {
        // Human is the only child that inherits ThinkingExtension
        __ThinkingExtension_init();
        __Human_init_unchained();
    }

    function __Human_init_unchained() internal onlyInitializing {
        // Initialization logic for Human
    }
}

contract HorseExtension is Initializable {
    function __HorseExtension_init() internal onlyInitializing {
        // Most base contract
        __HorseExtension_init_unchained();
    }

    function __HorseExtension_init_unchained() internal onlyInitializing {
        // Initialization logic for HorseExtension
    }
}

contract FastRunnerExtension is HorseExtension {
    function __FastRunnerExtension_init() internal onlyInitializing {
        // We don't initialize HorseExtension here, because there is another child that inherits from it on the same level
        __FastRunnerExtension_init_unchained();
    }

    function __FastRunnerExtension_init_unchained() internal onlyInitializing {
        // Initialization logic for FastRunnerExtension
    }
}

contract SaddleExtension is HorseExtension {
    function __SaddleExtension_init() internal onlyInitializing {
        // We don't initialize HorseExtension here, because there is another child that inherits from it on the same level
        __SaddleExtension_init_unchained();
    }

    function __SaddleExtension_init_unchained() internal onlyInitializing {
        // Initialization logic for SaddleExtension
    }
}

contract Horse is Animal, HorseExtension, FastRunnerExtension, SaddleExtension {
    function __Horse_init() internal onlyInitializing {
        // The Horse contract wraps all contracts that inherit from HorseExtension.
        // Therefore, we initialize HorseExtension here.
        __HorseExtension_init();
        __FastRunnerExtension_init();
        __SaddleExtension_init();
        __Horse_init_unchained();
    }

    function __Horse_init_unchained() internal onlyInitializing {
        // Initialization logic for Horse
    }
}

contract Centaur is Animal, Human, Horse {
    function initialize() public initializer {
        // The Centaur contract wraps all contracts that inherit from Animal.
        // Therefore, we initialize Animal here.
        __Animal_init();
        __Human_init();
        __Horse_init();
        // Initialization logic for Centaur
    }
}

Here is a visual representation of the Centaur contract's architecture that can help you understand how the approach works.

I'm keen to hear your thoughts on this and collaborate to refine the approach.
Thanks in advance!

[frangio]

Unfortunately I don't have a good answer to give you. We are interested in having a check for this in our upgrades tooling (OpenZeppelin/openzeppelin-upgrades#160) but we haven't made progress on that front.

I'm not sure I understood this part:

2. use the init() of the closest child that inherits from (wraps) the other child contracts inheriting the parent.

[R-Santev]

Unfortunately I don't have a good answer to give you. We are interested in having a check for this in our upgrades tooling (OpenZeppelin/openzeppelin-upgrades#160) but we haven't made progress on that front.

I'm not sure I understood this part:

2. use the init() of the closest child that inherits from (wraps) the other child contracts inheriting the parent.

@frangio Please, check out the example. Basically, I am trying to say that if two child contracts on the same level in the inheritance hierarchy inherit from the same parent that must be initialized, don't initialize the parent in one of these children, but use the higher level child which is a parent of both child contracts (it can be the top most contract). It is easier to understand by the example.

The good part is in case we have a standard for the naming of init functions and a standard for the place where a parent contract must be initialized, a check tool sounds like an easier mission.

[ernestognw]

If I'm understanding correctly, the strategy would be initializing the contracts in the highest common parent, is that correct?

It's a good heuristic and it'd solve the duplicate initialization. However, we also need to check there are no missing initializers and also they were executed in the correct order.

Concretely, the initializers should follow the same C3 linearization order Solidity does. Using the highest common parent may result in a different execution orders of initializers