Potential false positive `Missing initializer calls for one or more parent contracts` for `ERC20PermitUpgradeable`

[heueristik]

Context

Previously, I've filed an issue that got resolved by following the suggestion by @ericglau (see the test passing).

Adding ERC20BurnableUpgradeable worked similarly (see the test passing).

Problem

However, after adding ERC20PermitUpgradeable

contract Token is ERC20Upgradeable, ERC20PermitUpgradeable, ERC20BurnableUpgradeable, UUPSUpgradeable {
    /// @custom:oz-upgrades-unsafe-allow constructor
    constructor() {
        _disableInitializers();
    }

    function initialize(address initialOwner) external initializer {
        __Token_init(initialOwner);
    }

    function __Token_init(address initialOwner) internal onlyInitializing {
        __Context_init_unchained();
        __ERC20_init_unchained("Token", "TOK");
        __ERC20Permit_init_unchained("Token");
        __EIP712_init_unchained({name: "Token", version: "1"});
        __Nonces_init_unchained();
        __ERC20Burnable_init_unchained();
        __UUPSUpgradeable_init_unchained();

        __Token_init_unchained(initialOwner);
    }

    function __Token_init_unchained(address initialOwner) internal onlyInitializing {
        _mint(initialOwner, 100);
    }

    function _authorizeUpgrade(address newImplementation) internal pure override {
        {
            newImplementation;
        }
        // NOTE: In this MWE, anyone can upgrade. DO NOT USE IN PRODUCTION.
    }
}

the test

contract TokenTest is Test {
    address internal _defaultSender;
    Token internal _tokenProxy;

    function setUp() public {
        (, _defaultSender,) = vm.readCallers();

        // Deploy proxy and mint tokens for the `_defaultSender`.
        vm.prank(_defaultSender);
        _tokenProxy = Token(
            Upgrades.deployUUPSProxy({
                contractName: "Token.sol:Token",
                initializerData: abi.encodeCall(Token.initialize, _defaultSender)
            })
        );
    }

    function test_balance() public view {
        assertEq(_tokenProxy.balanceOf(_defaultSender), 100);
    }
}

results again in an error (see the test failing)

[FAIL: Upgrade safety validation failed:
✘  src/Token.sol:Token

      src/Token.sol:23: Missing initializer calls for one or more parent contracts: `ERC20PermitUpgradeable`
          Call the parent initializers in your initializer function
          https://zpl.in/upgrades/error-001
      
      src/Token.sol:27: Missing initializer calls for one or more parent contracts: `ERC20PermitUpgradeable`
          Call the parent initializers in your initializer function
          https://zpl.in/upgrades/error-001

FAILED] setUp() (gas: 0)

I've provided a minimal example to reproduce the error in this repo https://github.com/heueristik/token.
To reproduce the problem, run

git clone --recursive https://github.com/heueristik/token
cd token
npm install @openzeppelin/upgrades-core@latest
forge clean && forge build && forge test

What parent initializer am I missing?

[heueristik]

Update:

    function __Token_init(address initialOwner) internal onlyInitializing {
        __Context_init_unchained();
        __ERC20_init_unchained("Token", "TOK");
+++     __ERC20Permit_init("Token");
---     __ERC20Permit_init_unchained("Token");
---     __EIP712_init_unchained({name: "Token", version: "1"});
---     __Nonces_init_unchained();
        __ERC20Burnable_init_unchained();
        __UUPSUpgradeable_init_unchained();

        __Token_init_unchained(initialOwner);
    }

removes the error, but seems wrong.

[ericglau]

This occurs because the validations are looking for invocations of each parent initializer, in this case __ERC20Permit_init. In this case, you are already calling the underlying code (__EIP712_init_unchained), so it looks like it can be ignored.

In general though, we recommend the pattern of calling *_init for all direct parents, rather than calling *_init_unchained for all parents in the inheritance tree. See https://forum.openzeppelin.com/t/why-keep-ownable-init-beside-ownable-init-unchained/25579

Using *_init would simplify your usage, since you already know the direct parents, and does not require you to track all transitive parents.

[heueristik]

This occurs because the validations are looking for invocations of each parent initializer, in this case __ERC20Permit_init. In this case, you are already calling the underlying code (__EIP712_init_unchained), so it looks like it can be ignored.

Are you saying that the __ERC20Permit_init _unchained(name) call can be ignored because I am calling the __EIP712_init_unchained(name, version) function? I tried removing __ERC20Permit_init _unchained, but the error remained.

In general though, we recommend the pattern of calling *_init for all direct parents, rather than calling *_init_unchained

The reason I am using *_init_unchained is that I want to inherit from Token(V1) in my TokenV2 contract draft rather than modifying the Token(V1) contract (because I want to ensure future upgradeability and compatibility).

contract TokenV2 is Token {
    event ExampleEvent();

    function initialize(address initialOwner) external override reinitializer(2) {
        __Token_init(initialOwner);
        __TokenV2_init();
    }

    function initializeFromV1() external reinitializer(2) {
        __TokenV2_init();
    }

    function __TokenV2_init() internal onlyInitializing {
        __TokenV2_init_unchained();
    }

    function __TokenV2_init_unchained() internal onlyInitializing {
        emit ExampleEvent();
    }
}

---

Would the below be the correct way?

contract Token is ERC20Upgradeable, ERC20PermitUpgradeable, ERC20BurnableUpgradeable, UUPSUpgradeable {
    /// @custom:oz-upgrades-unsafe-allow constructor
    constructor() {
        _disableInitializers();
    }

    function initialize(address initialOwner) external virtual initializer {
        __ERC20_init("Token", "TOK");
        __ERC20Permit_init("Token");
        __ERC20Burnable_init();
        __UUPSUpgradeable_init();

        __Token_init_unchained(initialOwner);
    }

    /// @custom:oz-upgrades-unsafe-allow missing-initializer-call
    function __Token_init(address initialOwner) internal onlyInitializing {
        __Context_init_unchained();
        __ERC20_init_unchained("Token", "TOK");
        __ERC20Permit_init_unchained("Token");
        __EIP712_init_unchained({name: "Token", version: "1"});
        __Nonces_init_unchained();
        __ERC20Burnable_init_unchained();
        __UUPSUpgradeable_init_unchained();

        __Token_init_unchained(initialOwner);
    }

    function __Token_init_unchained(address initialOwner) internal onlyInitializing {
        _mint(initialOwner, 100);
    }

    function _authorizeUpgrade(address newImplementation) internal pure override {
        {
            newImplementation;
        }
        // NOTE: In this MWE, anyone can upgrade. DO NOT USE IN PRODUCTION.
    }
}

[heueristik]

Nevermind, I've figured it out. The problem with the false positive (the parent initializer of__ERC20Permit_init_unchained is called) remains though.