pcaversaccio / xdeployer

Hardhat plugin to deploy your smart contracts across multiple EVM chains with the same deterministic address.
https://www.npmjs.com/package/xdeployer
MIT License
440 stars 42 forks source link

💥 Support `@openzeppelin/hardhat-upgrades` Plugin #246

Closed charlie-kim closed 8 months ago

charlie-kim commented 8 months ago

Describe the desired feature:

My contract uses Openzeppelin's upgradeable libs. I used to use @openzeppelin/hardhat-upgrades lib to deploy but I cannot really use it with xdeployer. The Openzeppelin lib does a lot of stuff especially for transparent proxy. It will save a lot of hassle if xdeployer and @openzeppelin/hardhat-upgrades can work together.

Thanks in advance!

Code example that solves the feature:

No response

pcaversaccio commented 8 months ago

Hey @charlie-kim, thanks for sharing this feature request. Deploying and managing upgradeable contracts on Ethereum is something very complex and that's why OpenZeppelin has built their plugin. It's out-of-scope for xdeployer to offer this tooling and thus the best way to integrate the xdeployer functionalities into the @openzeppelin/hardhat-upgrades plugin is to enhance their CREATE2 support in the existing API:

image

So, to start with they could add the option to supply a CREATE2 factory like the one used in xdeployer: CreateX with address 0xba5Ed099633D3B313e4D5F7bdc1305d3c28ba5Ed. Furthermore, they could have the attributes networks and rpcUrls similar to xdeployer. What I'm saying is that it would be best to have a deterministic, cross-chain deployment solution for upgradeable contracts natively built into @openzeppelin/hardhat-upgrades. Hence, what I would recommend doing is to open in their repo an issue with this feature request.

cc: @ericglau

charlie-kim commented 8 months ago

Thanks for your response, @pcaversaccio.

Unfortunately, Openzeppelin only supports CREATE2 for defender users which is $$$$. And it comes with way more features that I need. I guess it's too late to complain when I am already using their library...!

cairoeth commented 8 months ago

Thanks for your response, @pcaversaccio.

Unfortunately, Openzeppelin only supports CREATE2 for defender users which is $$$$. And it comes with way more features that I need. I guess it's too late to complain when I am already using their library...!

hey @charlie-kim! you should be able to use CREATE2 with the Deploy module using Defender free tier. are you getting any specific errors?

charlie-kim commented 8 months ago

Hey @cairoeth, thanks for jumping in! It's great to hear defender free tier can work.

I tried defender option with hardhat-upgrades 2.5.1, then I got this error.

Error: Deployment response with id 21ce1ca1-6dca-4583-9771-d15518ebce7f does not include a contract address

The Hardhat Upgrades plugin is not currently compatible with this type of deployment. Use a relayer for your default deploy approval process in Defender.

Here's the beginning of the contract file.

// SPDX-License-Identifier: MIT
pragma solidity ^0.8.24;

import {OwnableUpgradeable} from "@openzeppelin/contracts-upgradeable/access/OwnableUpgradeable.sol";
import {BeaconProxy} from "@openzeppelin/contracts/proxy/beacon/BeaconProxy.sol";

Also, is there a way to deploy regular contract with defender CREATE2?

ericglau commented 8 months ago

Hi @charlie-kim, the error that you see is resolved a newer version of hardhat-upgrades. The latest version is currently 3.0.5.

Note that hardhat-upgrades versions 3.x is a new major version so there are some breaking changes compared to 2.x. See https://github.com/OpenZeppelin/openzeppelin-upgrades/releases/tag/%40openzeppelin%2Fhardhat-upgrades%403.0.0 for a summary -- mainly the proxy contracts are from OpenZeppelin Contracts 5.0.

You can also deploy non-upgradeable contracts using CREATE2, by using defender.deployContract and passing in a salt to the options of that function.

charlie-kim commented 8 months ago

Hi @ericglau, the latest version is using an hardhat version that has a bug but I can still try deployment with it.

It seems like defender.deployContract has a more strict rule or bug with beacon proxy contract from @openzeppelin/contracts 4.9.5. I get this error.

Error: The contract contracts/Collateral.sol:Collateral looks like an upgradeable contract.

When I try to use defender.deployProxy, I get this error.

Error: Contract `contracts/Collateral.sol:Collateral` is not upgrade safe

contracts/Collateral.sol:109: Contract `Collateral` has a constructor
    Define an initializer instead
    https://zpl.in/upgrades/error-001

@openzeppelin/contracts/access/Ownable.sol:28: Contract `Ownable` has a constructor
    Define an initializer instead
    https://zpl.in/upgrades/error-001

I got rid of constructor of

constructor() {
        _disableInitializers();
    }

But I still get

Error: Contract `contracts/Collateral.sol:Collateral` is not upgrade safe

@openzeppelin/contracts/access/Ownable.sol:28: Contract `Ownable` has a constructor
    Define an initializer instead
    https://zpl.in/upgrades/error-00

The contract already has a initialize function.

function initialize(
        string memory _name,
        address _owner
    ) public validAddress(_owner) initializer {
        _transferOwnership(_owner);

        name = _name;
        factory = msg.sender;
        admins.push(_owner);
    }

Beacon contract uses these libs

// SPDX-License-Identifier: MIT
pragma solidity ^0.8.24;

import {UpgradeableBeacon} from "@openzeppelin/contracts/proxy/beacon/UpgradeableBeacon.sol";
import {IBeacon} from "@openzeppelin/contracts/proxy/beacon/IBeacon.sol";
import {Ownable} from "@openzeppelin/contracts/access/Ownable.sol";

Implementation contract uses these libs

// SPDX-License-Identifier: MIT
pragma solidity ^0.8.24;

import {ECDSA} from "@openzeppelin/contracts/utils/cryptography/ECDSA.sol";
import {Initializable} from "@openzeppelin/contracts-upgradeable/proxy/utils/Initializable.sol";
import {Ownable} from "@openzeppelin/contracts/access/Ownable.sol";
import {IERC20} from "@openzeppelin/contracts/token/ERC20/IERC20.sol";
import {SafeERC20} from "@openzeppelin/contracts/token/ERC20/utils/SafeERC20.sol";

Using openzeppelin libs as below

"@openzeppelin/contracts": "^4.9.5",
"@openzeppelin/contracts-upgradeable": "^4.9.5"
ericglau commented 8 months ago

@charlie-kim It seems there might be a misunderstanding of how hardhat-upgrades should be used, so I'll clarify:

charlie-kim commented 8 months ago

@ericglau The contracts look like this.

// SPDX-License-Identifier: MIT
pragma solidity ^0.8.24;

import {UpgradeableBeacon} from "@openzeppelin/contracts/proxy/beacon/UpgradeableBeacon.sol";
import {IBeacon} from "@openzeppelin/contracts/proxy/beacon/IBeacon.sol";
import {Ownable} from "@openzeppelin/contracts/access/Ownable.sol";

/**
 *  @title Beacon contract
 *  @notice Wrapper of beacon contract to Collateral implementation contract
 *  @dev Update the implementation of all Collateral contracts
 *       generated by CollateralFactory in a single transaction.
 *       Using beacon proxy pattern that Collateral contracts(proxies) points to this contract,
 *       while this contract points to the current implementation of Collateral.
 *       https://docs.openzeppelin.com/contracts/4.x/api/proxy#BeaconProxy
 */
contract Beacon is IBeacon, Ownable {
    /// @notice Holds reference to beacon contract
    UpgradeableBeacon public immutable BEACON;

    /**
     * @notice Initialize
     * @param _initialImplementation initial Collateral implementation contract address
     * @param _owner owner of this contract
     */
    constructor(address _initialImplementation, address _owner) {
        BEACON = new UpgradeableBeacon(_initialImplementation);
        transferOwnership(_owner);
    }

    /**
     * @notice update Collateral implementation
     * @dev will update implementation of multiple Collateral contracts
     */
    function updateImplementation(address _newImplementation) public onlyOwner {
        BEACON.upgradeTo(_newImplementation);
    }

    /**
     * @notice get RainCollateral implementation contract address
     * @return address of implementation contract saved in beacon
     */
    function implementation() public view returns (address) {
        return BEACON.implementation();
    }
}
// SPDX-License-Identifier: MIT
pragma solidity ^0.8.24;

import {ECDSA} from "@openzeppelin/contracts/utils/cryptography/ECDSA.sol";
import {Initializable} from "@openzeppelin/contracts-upgradeable/proxy/utils/Initializable.sol";
import {Ownable} from "@openzeppelin/contracts/access/Ownable.sol";
import {IERC20} from "@openzeppelin/contracts/token/ERC20/IERC20.sol";
import {SafeERC20} from "@openzeppelin/contracts/token/ERC20/utils/SafeERC20.sol";
import {IFactory} from "./interfaces/IFactory.sol";

using SafeERC20 for IERC20;

contract Collateral is Initializable, Ownable {
    /// @notice name of collateral
    string public name;

    /// @notice Factory Address
    ///         It is used to get controller address held in factory
    address public factory;

    /// @notice Prevent the implementation contract from being used
    constructor() {
        _disableInitializers();
    }

    /**
     * @notice Initialize this contract
     * @param _name name of collateral
     * @param _owner address of collateral owner
     *               also becomes the first admin
     */
    function initialize(
        string memory _name,
        address _owner
    ) public initializer {
        _transferOwnership(_owner);

        name = _name;
        factory = msg.sender;
    }

    /**
     * @notice Block renounceOwnership
     * @dev Prevent assets to be stuck in the contract without an owner
     */
    function renounceOwnership() public virtual override onlyOwner {
        revert DisabledOwnershipRenouncement();
    }
}
charlie-kim commented 8 months ago

I made a small progress by using OwnableUpgradeable and putting annotation above constructor.

Now I am getting this error.

Error: missing arguemnt: types/values length mismatch (count=0, expectedCount=2, code=MISSING_ARGUMENT, version=6.8.1)

When deploying with

const collateralFactory = await ethers.getContractFactory("Collateral", owner);
    const collateral = await defender.deployProxy(
      collateralFactory,
      [],
      {
        initializer: "initialize",
        salt: SALT
      }
    );

I am not sure what parameter I am supposed to use for initialize function because I am deploying an implementation contract.

Should I use defender.deployImplementation instead?

pcaversaccio commented 8 months ago

Hey guys, I personally feel this issue convo is better suited in the @openzeppelin/hardhat-upgrades repository. @charlie-kim may I ask you to open an issue in their repo and linking to this issue for the record. I will close this issue as "not planned" accordingly. Furthermore, as stated above, it's out-of-scope for xdeployer to offer tooling around upgradeable contracts, but I would like to nudge @ericglau & @cairoeth to consider incorporating CreateX into the Defender.

charlie-kim commented 8 months ago

Using defender.deployImplementation worked fine. Thanks for jumping in, all! @pcaversaccio @ericglau @cairoeth