Best practices for multiple inheritance

Support Upgrades
1

I have my contract that uses multiple inheritance from several upgradeable contracts:

contract PolicyPool is
  IPolicyPool,
  ERC721Upgradeable,
  ERC721EnumerableUpgradeable,
  PausableUpgradeable,
  AccessControlUpgradeable
{

My question is what is the best practice for doing my initialize function.

This alternative (calling parents _init):

  function initialize(
    string memory name_,
    string memory symbol_,
    IERC20 curreny_,
    address treasury_,
    address assetManager_
  ) public initializer  {
    __AccessControl_init();
    __Pausable_init();
    __ERC721_init(name_, symbol_);
    __ERC721Enumerable_init();
    __PolicyPool_init_unchained(currency_, treasury_, assetManager_);
  }

If I call this way, some common ancestors like __Context_init_unchained and __ERC165_init_unchained will be called several times.

Or should I use a fully unchained alternative:

  function initialize(
    string memory name_,
    string memory symbol_,
    IERC20 curreny_,
    address treasury_,
    address assetManager_
  ) public initializer  {
    __Context_init_unchained();
    __ERC165_init_unchained();
    __ERC721_init_unchained(name_, symbol_);
    __ERC721Enumerable_init_unchained();
    __Pausable_init_unchained();
    __AccessControl_init_unchained()
    __PolicyPool_init_unchained(currency_, treasury_, assetManager_);
  }

The problem I see with this alternative is I need to analyze the hierarchy of my contract’s parents.

The first one is more clean and perhaps I can live with some duplicate call or not?

1 Like
2

Great question, and I agree with the problem you point out. Ideally you wouldn't need to analyze the hierarchy of your contract's parents.

The duplicate calls that you have if you call _init are actually not important, because they're empty functions as those contracts don't have any initialization logic.

The recommendation is to use _init unless you have some non-empty initializer that is invoked twice, so in your case you should use _init.

I know this is not much better because you still need to look at the source code of your parents to figure out if there is a problem. There's an upcoming feature in OpenZeppelin Upgrades Plugins where this will be checked automatically and the error message will guide you if you need to make any changes. You can subscribe to issue https://github.com/OpenZeppelin/openzeppelin-upgrades/issues/160 for updates.

1 Like
3

That means all the _init functions are empty unless they are overwritten by the developer. Is that correct ?

4

No, why would that be correct? The init functions contain initialization logic. In some cases it might be empty.

5

I'm using AccessControlEnumerableUpgradeable and I see __AccessControlEnumerable_init is empty, so I was wondering if that's a pattern you follow in most of the upgradeable contracts.

6

It's only empty if there is nothing to run to initialize that particular contract. We include the function even if it's empty so you can always expect it to be there, it potentially simplifies use.

7

I understand, thank you :slight_smile: