OpenZeppelin / openzeppelin-contracts-upgradeable

Upgradeable variant of OpenZeppelin Contracts, meant for use in upgradeable contracts.
https://openzeppelin.com/contracts
MIT License
1k stars 436 forks source link

Making 2771 use init instead of constructor #155

Closed KfishNFT closed 2 years ago

KfishNFT commented 2 years ago

This broke in this commit https://github.com/OpenZeppelin/openzeppelin-contracts-upgradeable/commit/2924ea3bd7536ba39fed61374c5bb2bf67207452

Fixes #????

PR Checklist

Amxx commented 2 years ago

Hello @KfishNFT

Please don't submit PRs to modify the contracts in this repository. The contracts here are the result of the transpilation of the code in OpenZeppelin/openzeppelin-contracts.

Also note that it is on purpose that we use an immutable slot, set at construction for all proxy instances, as it is way more gas efficient.

KfishNFT commented 2 years ago

Got it, thank you. Does using the constructor make this unusable by upgradeable contracts? I'm having issues figuring out how to set it up properly this way. This is my first time setting up a contract that makes use of ERC2771.

Thank you!

Amxx commented 2 years ago

No, having a constructor does NOT make it unusable with upgradeable contracts.

You can set the forwarder (and other immutable variables), through the implementation's constructor ... and then deploy you proxy (and call the initializer)

KfishNFT commented 2 years ago

Thank you for the assistance. I hope to contribute one day :)

eliashezron commented 2 years ago

No, having a constructor does NOT make it unusable with upgradeable contracts.

You can set the forwarder (and other immutable variables), through the implementation's constructor ... and then deploy you proxy (and call the initializer)

how will this work for "uups" type contracts?

Amxx commented 2 years ago

You can set the forwarder (and other immutable variables), through the implementation's constructor ... and then deploy you proxy (and call the initializer)

how will this work for "uups" type contracts?

Just like explained above:

  1. You set the forwarder (and other immutable variables), through the implementation's constructor
  2. You deploy your UUPS proxy using the address of the implementation.
aenhsaihan commented 2 years ago

Just like explained above:

You set the forwarder (and other immutable variables), through the implementation's constructor You deploy your UUPS proxy using the address of the implementation.

Is the below code more or less what you're suggesting, or am I way off the mark?

contract MyContract is
    Initializable,
    ERC2771ContextUpgradeable
{

    /// @custom:oz-upgrades-unsafe-allow constructor
    constructor(address forwarder)
        ERC2771ContextUpgradeable(forwarder)
    {}

    function initialize(string memory _tokenURI) public initializer {
        _setTokenUri(0, _tokenURI);
    }
}
    const MyContract = await hre.ethers.getContractFactory("MyContract");
    myContract = await MyContract.deploy(forwarderAddress);
    await myContract.deployed();
    console.log("MyContract deployed to:", myContract.address);

    await myContract.initialize(_tokenURI);
    console.log("MyContract has been initialized");

The above deployment script works for me but I'm wondering how I would be able to use the upgrade plugin like below?

myContract = await hre.upgrades.deployProxy(MyContract, [_tokenURI]);

As far as I know, using the upgrade plugin to deploy the proxy as such would cause it to complain about the constructor.

Please forgive me if I misunderstood you, this page is literally the only resource online dealing with this edge case when it comes to deploying upgradeable contracts.

ericglau commented 2 years ago

@aenhsaihan For the upgrades plugin, to pass in constructor arguments, use the constructorArgs option with deployProxy or upgradeProxy. See https://docs.openzeppelin.com/upgrades-plugins/1.x/api-hardhat-upgrades#common-options

For example in the below, [_tokenURI] is for the initializer args, while { constructorArgs: [forwarderAddress] } is for the constructor args:

myContract = await hre.upgrades.deployProxy(MyContract, [_tokenURI], { constructorArgs: [forwarderAddress] });