Missing calls to parent __{parent}_init fns when using multiple inheritance

[vminkov]

According to the documentation here, __{contract}_init calls should make calls to their parent __{parent}_init fns, but in many contracts they don't.

e.g.

     function __ERC4626_init(IERC20MetadataUpgradeable asset_) internal onlyInitializing {
         __ERC4626_init_unchained(asset_);
   >>>     __ERC20_init(asset_.name(), asset_.symbol()); >>> this here is missing
     }

💻 Environment

Any env

Here is the example source: https://github.com/OpenZeppelin/openzeppelin-contracts-upgradeable/blob/master/contracts/token/ERC20/extensions/ERC4626Upgradeable.sol#L36

Here is a PR that fixes it https://github.com/OpenZeppelin/openzeppelin-contracts-upgradeable/pull/171

[Amxx]

Hello @vminkov

First of all, the __XXX__init function are automatically generated during the transpilation workflow. This is an automated process that is triggered every time there is a new commit on the master branch of the non upgradable contracts. As a consequence this kind of change should not be done with PR targeting the master branch, but by modifying the transpiler. This is why I closed your PR.

Now about the logic behind this situation.

In the non upgradable library, some contracts have constructors, and some contract inherit from parents without specifying the parent constructor. This is, for example the case of ERC20 extensions (they usually don't include the ERC20 parameters). This is also the case of some other contract such as EIP712.

When you build a contract with these extensions, you'll have to call the different constructors. Just go to the wizard and try to generate a simple ERC20Votes to see that it depends on ERC20Permit and ERC20, which both need constructor arguments.

We don't want the extensions to pass argument to initialize the core, because that would be an instant mess multiple extensions are used together.

The __XXX__init follow the same logic, because they are "just" a translation of the constructor. when building an upgradable contract, you need to make sure you call all the one needed. This can indeed be a pain, as the compiler won't help you, but again, the wizard can help. From out previous example, just select "Upgradeability" and you'll the the initialize() function do all that.

Last but not least, the fix your propose is actually wrong. What you are doing is using the name and symbol of the underlying asset for the vault itself. That means that if an upgradeable vault is based on Dai, then its name and symbol would match Dai? That would be really confusing for users.