OpenZeppelin / openzeppelin-upgrades

Plugins for Hardhat and Foundry to deploy and manage upgradeable contracts on Ethereum.
MIT License
630 stars 271 forks source link

Hardhat compile error with `@openzeppelin/hardhat-upgrades` #917

Closed vladikopl01 closed 1 year ago

vladikopl01 commented 1 year ago

I am developing a project on hardhat with foundry together, which uses the ERC6551 standard, which relies on account-abstraction. I have moved all ERC6551 contracts to my contracts/lib/erc6551 directory and installed @account-abstraction/contracts as a dependency of ERC6551 contracts. It was done because ERC6551 contracts did not use the new 5th version of OpenZeppelin contracts and had some broken imports while initiating it via foundry libs. But when I ran compile on hardhat I received such an error:

An unexpected error occurred:

Error: Failed to compile modified contracts for namespaced storage:

DeclarationError: Identifier already declared.
  --> @account-abstraction/contracts/core/Helpers.sol:32:5:
   |
32 |     uint256 constant _packValidationData = 0;
   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
Note: The previous declaration is here:
  --> @account-abstraction/contracts/core/Helpers.sol:29:5:
   |
29 |     uint256 constant _packValidationData = 0;
   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

Please report this at https://zpl.in/upgrades/report. If possible, include the source code for the contracts mentioned in the errors above.

The solidity code which provides such an error is located inside @account-abstraction/contracts lib:

// I remove natspec comments from the code to be cleaner
// Also, need to mention that these functions are defined as 'free functions', which are not located inside the contracts

function _packValidationData(
    ValidationData memory data
) pure returns (uint256) {
    return
        uint160(data.aggregator) |
        (uint256(data.validUntil) << 160) |
        (uint256(data.validAfter) << (160 + 48));
}

function _packValidationData(
    bool sigFailed,
    uint48 validUntil,
    uint48 validAfter
) pure returns (uint256) {
    return
        (sigFailed ? 1 : 0) |
        (uint256(validUntil) << 160) |
        (uint256(validAfter) << (160 + 48));
}

I have tried such things to define what the problem is with compilation:

  1. Reinstalled @account-abstraction/contracts lib, the error still was there.
  2. Cloned @account-abstraction/contracts lib to check if it is not broken and could compile, also tried to compile it with my EVM compiler settings, lib compiled successfully.
  3. Check if the installed version of @account-abstraction/contracts lib is the same as the ERC6551 dependency, it was the same and was 0.6.0.
  4. Find a fork of @account-abstraction/contracts which uses version 5 of OpenZeppelin contracts and tried to compile with it, got the same error.
  5. Change env compiler versions: paris, london etc, did not help at all.

The last thing that I tried was to check hardhat's config properties one by one. I set up a new empty project with hardhat alone and installed some contract dependencies such as OpneZeppelin default and upgradeable contracts and @account-abstraction/contracts lib. Then tried to compile and the error was gone. Next was checking the hardhat configs, and I tried to remove @openzeppelin/hardhat-upgrades lib because a compile error left this message Please report this at https://zpl.in/upgrades/report. and it worked. The compilation was successful.

Is there an issue with @openzeppelin/hardhat-upgrades lib and does it override compilation settings somehow?

How I can use @openzeppelin/hardhat-upgrades lib in my situation as I need your lib to deploy and update a bunch of proxy contracts which I have in my project?

Repo with reproduction: https://github.com/vladikopl01/op-upgrades-compile-error

Amxx commented 1 year ago

Hello @vladikopl01

It looks like the error comes from the existance of two versions (signatures) of the free function _packValidationData.

I'm not sure what the error is. I can tell that free functions are not something we use a lot, so support for them might be limited. We'll have to reproduce the issue to trace which part of the upgrades plugin is to blame!

vladikopl01 commented 1 year ago

Hello @vladikopl01

It looks like the error comes from the existance of two versions (signatures) of the free function _packValidationData.

I'm not sure what the error is. I can tell that free functions are not something we use a lot, so support for them might be limited. We'll have to reproduce the issue to trace which part of the upgrades plugin is to blame!

I already have created a repo for reproduction, you could check it out https://github.com/vladikopl01/op-upgrades-compile-error

Amxx commented 1 year ago

The bug is part of the "new" NamespaceStorage check that is added to TASK_COMPILE_SOLIDITY_COMPILE by the upgrades plugin. Unfortunatelly, there is no easy way to disable that check.

What happens is that the plugin takes the input source code and partly rewrites it. That happens in the makeNamespacedInput which is part of @openzeppelin/upgrades-core. I'm trying to see how to fix that.

chopachom commented 1 year ago

Not sure if the PR mentioned above fixes it, but just to note there is also a DocstringParsingError

An unexpected error occurred:

Error: Failed to compile modified contracts for namespaced storage:

DeclarationError: Identifier already declared.
  --> prb-math/ud60x18/Conversions.sol:10:1:
   |
10 | uint256 constant convert = 0;
   | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^
Note: The previous declaration is here:
 --> prb-math/ud60x18/Conversions.sol:8:1:
  |
8 | uint256 constant convert = 0;
  | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^

DocstringParsingError: Documentation tag @notice not valid for file-level variables.
  --> prb-math/ud60x18/Math.sol:26:1:
   |
26 | /// @notice Calculates the arithmetic average of x and y using the following formula:
   | ^ (Relevant source part starts here and spans across multiple lines).

Repo with reproduction: https://github.com/chopachom/oz-upgrades-DocstringParsingError

ericglau commented 1 year ago

@chopachom Thanks for bringing that up, we will update the PR to address that as well.