Solidity: Stabilize and reorder parent contracts that are not related to the main base/extension contracts

[ericglau]

Overview

When features are enabled in the generated contract based on the selected options, they are added in the order in which they appear in the code generation logic.

Some features require other features, for example ERC721's Mintable requires its function to be restricted to privileged roles so enables Ownable by default. Normally we attempt to add Ownable after the other features (for example, extensions of ERC721) are added, but these kinds of dependencies affect the order that parent contracts are inherited.

The drawbacks of this behavior are: a. The inheritance order, constructor arguments, and initializer call order can change when some features are added (either by the user via selection of options, or between different Wizard releases when we add/update features), and b. Certain contracts such as Ownable are sometimes inherited before the main base contract's extensions, which looks less clean.

Proposal

When adding any inheritance, constructor/initializer parameters, and possibly functions for the parent contracts Ownable, AccessControl, AccessManager, UUPSUpgradeable, or their upgradeable variants, add those after other contract features have been added. If a contract feature requires one of these parent contracts as a dependency, the contract feature should still use those parent contracts, but the addition of the parent contracts themselves should be deferred.

Implementation note: This may require implementing an abstraction in packages/core/solidity/src/contract.ts so that the parent contracts (and their associated code) that we want to add last are deferred, such that they are actually added after the other features.

Particularly, it is NOT sufficient to only sort the inheritance order prior to printing the code, because if the contract is upgradeable, the calls to parent initializers should also match the linearization order, otherwise the Upgrades Plugins would give a warning.

Linearization

The order of the specific parent contracts listed above can be changed because none of the other base/extension contracts depend on them. Other parent contracts may not be movable in this way, because doing so in some cases with multiple inheritance could cause linearization to become impossible.

Linearization is validated for all possible imports in the yarn prepare script via packages/core/solidity/src/generate/sources.ts.

Examples of current behavior

1. ERC721 with Votes, Ownable

contract MyToken is ERC721, EIP712, ERC721Votes, Ownable {
    constructor(address initialOwner)
        ERC721("MyToken", "MTK")
        EIP712("MyToken", "1")
        Ownable(initialOwner)
    {}

2. ERC721 with Mintable, Votes, Ownable

contract MyToken is ERC721, Ownable, EIP712, ERC721Votes {
    constructor(address initialOwner)
        ERC721("MyToken", "MTK")
        Ownable(initialOwner)
        EIP712("MyToken", "1")
    {}

3. ERC721 with Votes, Ownable, UUPS

contract MyToken is Initializable, ERC721Upgradeable, EIP712Upgradeable, ERC721VotesUpgradeable, OwnableUpgradeable, UUPSUpgradeable {
    /// @custom:oz-upgrades-unsafe-allow constructor
    constructor() {
        _disableInitializers();
    }

    function initialize(address initialOwner) public initializer {
        __ERC721_init("MyToken", "MTK");
        __EIP712_init("MyToken", "1");
        __ERC721Votes_init();
        __Ownable_init(initialOwner);
    }

4. ERC721 with Mintable, Votes, Ownable, UUPS

contract MyToken is Initializable, ERC721Upgradeable, OwnableUpgradeable, EIP712Upgradeable, ERC721VotesUpgradeable, UUPSUpgradeable {
    /// @custom:oz-upgrades-unsafe-allow constructor
    constructor() {
        _disableInitializers();
    }

    function initialize(address initialOwner) public initializer {
        __ERC721_init("MyToken", "MTK");
        __Ownable_init(initialOwner);
        __EIP712_init("MyToken", "1");
        __ERC721Votes_init();
    }

Expected behavior

Example 2's inheritance order should look the same as Example 1. Example 4's inheritance order and parent initializer calls should look the same as Example 3.

Reference work

This commit attempted to change the inheritance order by itself, but that is NOT sufficient for the reasons listed in the Proposal section above. The proper implementation may be to implement some deferral logic elsewhere instead.