Make ValidatorManager contracts compatible with OpenZeppelin upgrades plugins

[Nuttymoon]

Context and scope
The current implementations of ValidatorManager (PoAValidatorManager, ERC20TokenStakingManager, NativeTokenStakingManger) have multiple incompatibilities with the OpenZeppelin upgrades plugin, making the safety validation fail.

This limits the reusability of the contracts outside of the teleporter / Avalanche CLI context (ofc developers can still disable the safety validation, but this is far from ideal).

The 2 incompatibilities are:

• The constructor defined in all contracts
• The external functions present in the ValidatorMessages library

as we can see with these errors raised when trying to deploy a PoAValidatorManager with Upgrades.deployTransparentProxy:

[FAIL: setup failed: revert: Upgrade safety validation failed:
✘  lib/teleporter/contracts/validator-manager/PoAValidatorManager.sol:PoAValidatorManager

      lib/teleporter/contracts/validator-manager/PoAValidatorManager.sol:24: Contract `PoAValidatorManager` has a constructor
          Define an initializer instead
          https://zpl.in/upgrades/error-001
      
      lib/teleporter/contracts/validator-manager/ValidatorMessages.sol: Linking external libraries like `ValidatorMessages` is not yet supported
          Use libraries with internal functions only, or skip this check with the `unsafeAllowLinkedLibraries` flag 
          if you have manually checked that the libraries are upgrade safe
          https://zpl.in/upgrades/error-006

FAILED]

Solving those doesn't seem too complicated:

1. Remove the constructor
2. Make the ValidatorMessages functions internal

Discussion and alternatives
This might be a non-issue if you want ALL deployments of such contracts to be done through the Avalanche CLI (and not via Forge).

A workaround for the external libraries check would be to add @custom:oz-upgrades-unsafe-allow external-library-linking to all contracts.

Another alternative is to deploy the contracts more manually (deploy the implementation, then the proxy, etc.) but this is more error-prone.

Open questions

• What is the impact of removing the constructor that uses ICMInitializable?
• Is there any downside to making the ValidatorMessages functions internal?

[cam-schultz]

Hi @Nuttymoon ,

What is the impact of removing the constructor that uses ICMInitializable?

This was a pattern that was introduced in ICTT to enable non-upgradeable versions of the upgradeable contracts through a thin shim, rather than repeating contract logic across separate contracts (such as Openzeppelin's openzeppelin-contracts and openzeppelin-contracts-upgradeable repos. This was a major boon to supporting both the upgradeable and non-upgradeable use cases while the contracts were under heavy active development.

We don't currently support the non-upgradeable case for the ValidatorManager contracts, so removing the constructors here is certainly on the table, especially if it improves the dev-x.

Is there any downside to making the ValidatorMessages functions internal?

External library support is imperative to reducing the contract size to stay under the EIP-170 contract size limit that's enforced by Subnet EVM chains as well as the C-Chain. We could replace external libraries with standalone contracts, but that would only complicate the deployment process for the same end result. I think skipping this check as you described is reasonable.

[minghinmatthewlam]

Another add on to @cam-schultz 's point above, OZ in their upgradeable contracts recommend calling _disableInitializers in the constructor https://docs.openzeppelin.com/upgrades-plugins/1.x/writing-upgradeable#initializing_the_implementation_contract.

[Nuttymoon]

@minghinmatthewlam
Another add on to @cam-schultz 's point above, OZ in their upgradeable contracts recommends calling _disableInitializers in the constructor https://docs.openzeppelin.com/upgrades-plugins/1.x/writing-upgradeable#initializing_the_implementation_contract.

Indeed. Adding @custom:oz-upgrades-unsafe-allow constructor on top of the constructor should work. But I guess it would make sense to always call _disableInitializers instead of using the ICMInitializable logic.