Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

#

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Jump to bottom

Make ValidatorManager contracts compatible with OpenZeppelin upgrades plugins #648

Open
Nuttymoon opened this issue Nov 18, 2024 · 3 comments · May be fixed by #684
Open

Make ValidatorManager contracts compatible with OpenZeppelin upgrades plugins #648

Nuttymoon opened this issue Nov 18, 2024 · 3 comments · May be fixed by #684
Labels
enhancement New feature or request L1 Staking Contract

Comments

@Nuttymoon
Copy link
Contributor

Nuttymoon commented Nov 18, 2024 •
edited
Loading

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?
@Nuttymoon Nuttymoon added the enhancement New feature or request label Nov 18, 2024
@cam-schultz
Copy link
Contributor

cam-schultz commented Nov 18, 2024 •
edited
Loading

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
Copy link

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
Copy link
Contributor Author

Nuttymoon commented Nov 19, 2024 •
edited
Loading

@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.

@cam-schultz cam-schultz linked a pull request Dec 12, 2024 that will close this issue
Draft
# for free to join this conversation on GitHub. Already have an account? # to comment
Assignees
No one assigned
Labels
enhancement New feature or request L1 Staking Contract
Projects
Platform Engineering Group
Status: Backlog 🗄️
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Relax OpenZeppelin upgrades plugin validation checks ava-labs/icm-contracts
4 participants
@geoff-vball @Nuttymoon @minghinmatthewlam @cam-schultz