wighawag / hardhat-deploy

hardhat deployment plugin
MIT License
1.2k stars 296 forks source link

OpenZeppelin UUPS proxy support #146

Open jaypaik opened 3 years ago

jaypaik commented 3 years ago

I see that OpenZeppelinTransparentProxy is supported. Are there any plans to add UUPS proxy support in the near future?

wighawag commented 3 years ago

Planning to support natively but for now it is still possible to use them with hardhat-deploy. the proxy jist need to have a dummy/unused owner arg in its constructor to remain compatible.

JasoonS commented 3 years ago

the proxy jist need to have a dummy/unused owner arg in its constructor to remain compatible.

Would that not make using UUPS no longer beneficial? The reason to use UUPS is to save gas on transaction calls because the proxy doesn't need to check if it is the admin making the call.

Maybe I'm missing it, could you elaborate with some pseudo-code how to use hardhat-deploy with uups proxies (with the unused argument)?

Thanks

wighawag commented 3 years ago

It will not affect gas, it is just a dummy arg so that hardhat-deploy can deploy it without change of code. the constructor does not need to do anything with that argument. Going to see if I can make the change to make it work without any change soon. it should be a small change where you can specify the constructor arg for the proxy or maybe simply specifiy that it is a UUPS proxy

JasoonS commented 3 years ago

Thanks @wighawag, got around to this finally.

For those interested I created this contract:

pragma solidity ^0.8.0;

import "@openzeppelin/contracts/proxy/ERC1967/ERC1967Proxy.sol";

// Kept for backwards compatibility with older versions of Hardhat and Truffle plugins.
contract UUPSProxy is ERC1967Proxy {
  constructor(
    address _logic,
    address, // This is completely unused by the uups proxy, required to remain compatible with hardhat deploy: https://github.com/wighawag/hardhat-deploy/issues/146
    bytes memory _data
  ) payable ERC1967Proxy(_logic, _data) {}
}

And then my deployments looks something like this (proxyContract: "UUPSProxy" being the important part):

  await deploy(CONTRACT, {
    from: deployer,
    proxy: {
      proxyContract: "UUPSProxy",
      // ... your other config, initializers etc
    },
    log: true,
  });
aspiers commented 3 years ago

@JasoonS Please can you clarify the purpose of that UUPSProxy contract code? Is it just a dummy contract so that hardhat-deploy knows the interface of the actual proxy's constructor? Or does it actually relate somehow to the proxy contract which gets deployed?

marceljay commented 3 years ago

What exactly is the role of admin, I get an error no matter if's there, the deployer (=upgrade admin) or if I omit the field.

proxy: {
      proxyContract: "UUPSProxy",
      execute: {
        methodName: "initialize",
        args: [admin],
      },
    },
aspiers commented 3 years ago

@marceljay Please (always) paste the error(s) you get :-)

JasoonS commented 3 years ago

Hi @aspiers , I haven't dug into hardhat-deploy deeply so please correct me @wighawag. But from what I understand it knows how to deploy transparent proxies. And transparent proxies are deployed with an admin that manages those proxies. In UUPS contracts you don't have an external contract (proxy admin) managing the upgrades, it is rather in the implementation itself - so isn't passed into the proxy constructor.

So to make this work with hardhat-deploy we deploy it with the same constructor as a transparent proxy, but we just ignore the admin field and get the same effect.

Seems to be working for me @marceljay

Disclaimer my code snippet isn't audited :sweat_smile: - I take no responsibility for badly configured proxies :laughing:

JasoonS commented 3 years ago

@marceljay Ah, yes, I can guess your issue. I edited my original response.

The following code is what to do on the implementation. So I have a function on the implementation (not the proxy) called initialize that takes in 1 argument (the admin).

execute: {
  methodName: "initialize",
  args: [admin],
},
marceljay commented 3 years ago

@marceljay Ah, yes, I can guess your issue. I edited my original response.

The following code is what to do on the implementation. So I have a function on the implementation (not the proxy) called initialize that takes in 1 argument (the admin).

execute: {
  methodName: "initialize",
  args: [admin],
},

Yeah now it makes perfect sense, saw you edited the answer :)

Do you know if the ERC1967Proxy contract is the same that's used by OZ upgrades plugin?

aspiers commented 3 years ago

@JasoonS Thanks a lot for the explanations but I still don't fully get it. Above you posted a contract starting:

contract UUPSProxy is ERC1967Proxy {

Does that actually get deployed for real by the sample await deploy code you posted above, or is it just a dummy contract so that hardhat-deploy knows the interface of the actual proxy's constructor, or something else? You wrote:

we deploy it with the same constructor as a transparent proxy

but if it's really deploying that then I don't see how it could possibly work, because that contract inherits from ERC1967Proxy not UUPSUpgradeable, so where would the actual UUPS functionality come from?

JasoonS commented 3 years ago

Does that actually get deployed for real by the sample await deploy code you posted above

Yes, it actually deploys it.

because that contract inherits from ERC1967Proxy not UUPSUpgradeable

It is supposed to use the ERC1967Proxy (both UUPS and transparent proxies from openzeppelin use erc1967, https://docs.openzeppelin.com/contracts/4.x/api/proxy#transparent-vs-uups). The UUPSUpgradeable contract is used by the implementation contract (it isn't part of the proxy).

so where would the actual UUPS functionality come from?

By design the implementation contract holds the UUPS functionality, (which is good because it is more gas efficient since the logic isn't in the proxy itself, but bad because it is possible to upgrade to a non-compatible contract by mistake and break upgradeability).

Also, I'm definitely not an expert on proxies, this is just from my own research.

aspiers commented 3 years ago

@JasoonS commented on September 1, 2021 9:18 PM:

because that contract inherits from ERC1967Proxy not UUPSUpgradeable

It is supposed to use the ERC1967Proxy (both UUPS and transparent proxies from openzeppelin use erc1967, docs.openzeppelin.com/contracts/4.x/api/proxy#transparent-vs-uups).

Right. And ERC-1967 doesn't do much except specify the storage location of the implementation address, beacon address, and admin address. BTW fun fact I noticed: in conforming to ERC-1967, the OZ UUPS implementation actually violates EIP1822 which specifies a slightly different storage location for the implementation address.

The UUPSUpgradeable contract is used by the implementation contract (it isn't part of the proxy).

D'oh, of course! Thanks for helping me see what I was stupidly missing here.

so where would the actual UUPS functionality come from?

By design the implementation contract holds the UUPS functionality, (which is good because it is more gas efficient since the logic isn't in the proxy itself, but bad because it is possible to upgrade to a non-compatible contract by mistake and break upgradeability).

Yep, so the implementation has to inherit from UUPSUpgradeable.

Thanks a lot again for this great help!

swaylock commented 3 years ago

My deploy script is as follows:

const { ethers } = require("hardhat");

module.exports = async ({ deployments }) => {
  const { deploy } = deployments;
  const accountHashtagAdmin = await ethers.getNamedSigner("accountHashtagAdmin");
  const accountHashtagPublisher = await ethers.getNamedSigner("accountHashtagPublisher");

  await deploy("HashtagAccessControls", {
    // Learn more about args here: https://www.npmjs.com/package/hardhat-deploy#deploymentsdeploy
    from: accountHashtagAdmin.address,
    proxy: {
      //owner: accountHashtagAdmin,
      proxyContract: "UUPSProxy",
      execute: {
        init: {
          methodName: "initialize", // Function to call when deployed first time.
        },
        onUpgrade: {
          methodName: "postUpgrade", // method to be executed when the proxy is upgraded (not first deployment)
          args: ["hello"],
        },
      },
    },
    log: true,
  });
};
module.exports.tags = ["HashtagAccessControls", "dev"];

Things deploy & initialize fine the first time around.

To test upgrading, I'm making a minuscule change to HashtagAccessControls.sol (in this case adding a new hardhat console.log line to postUpgrade). And when I run hardhat deploy again, the change is compiled, and the deployment is attempted but I get the following error:

User@swaylock ~/Sites/hashtag-protocol/hashtag-contracts (253-upgradeable-contracts)$ hh deploy --network localhost --tags dev,Ethernal    [2.3.0]
Compiling 27 files with 0.8.3
Compilation finished successfully
deploying "HashtagAccessControls_Implementation" (tx: 0xb4668d7a4dd594a9718745b08da1ebbf67e6f5e2a44870e2076490e4a3e2b364)...: deployed at 0xCf7Ed3AccA5a467e9e704C703E8D87F634fB0Fc9 with 1181486 gas
An unexpected error occurred:

Error: ERROR processing /Users/User/Sites/hashtag-protocol/hashtag-contracts/deploy/01_access_controls.js:
Error: To change owner/admin, you need to call the proxy directly
    at _deployViaEIP173Proxy (/Users/User/Sites/hashtag-protocol/hashtag-contracts/node_modules/hardhat-deploy/src/helpers.ts:1196:17)
...

Wondering what the proper method for is for upgrading the implementation contract?

theekruger commented 3 years ago

My deploy script is as follows:

const { ethers } = require("hardhat");

module.exports = async ({ deployments }) => {
  const { deploy } = deployments;
  const accountHashtagAdmin = await ethers.getNamedSigner("accountHashtagAdmin");
  const accountHashtagPublisher = await ethers.getNamedSigner("accountHashtagPublisher");

  await deploy("HashtagAccessControls", {
    // Learn more about args here: https://www.npmjs.com/package/hardhat-deploy#deploymentsdeploy
    from: accountHashtagAdmin.address,
    proxy: {
      //owner: accountHashtagAdmin,
      proxyContract: "UUPSProxy",
      execute: {
        init: {
          methodName: "initialize", // Function to call when deployed first time.
        },
        onUpgrade: {
          methodName: "postUpgrade", // method to be executed when the proxy is upgraded (not first deployment)
          args: ["hello"],
        },
      },
    },
    log: true,
  });
};
module.exports.tags = ["HashtagAccessControls", "dev"];

Things deploy & initialize fine the first time around.

To test upgrading, I'm making a minuscule change to HashtagAccessControls.sol (in this case adding a new hardhat console.log line to postUpgrade). And when I run hardhat deploy again, the change is compiled, and the deployment is attempted but I get the following error:

User@swaylock ~/Sites/hashtag-protocol/hashtag-contracts (253-upgradeable-contracts)$ hh deploy --network localhost --tags dev,Ethernal    [2.3.0]
Compiling 27 files with 0.8.3
Compilation finished successfully
deploying "HashtagAccessControls_Implementation" (tx: 0xb4668d7a4dd594a9718745b08da1ebbf67e6f5e2a44870e2076490e4a3e2b364)...: deployed at 0xCf7Ed3AccA5a467e9e704C703E8D87F634fB0Fc9 with 1181486 gas
An unexpected error occurred:

Error: ERROR processing /Users/User/Sites/hashtag-protocol/hashtag-contracts/deploy/01_access_controls.js:
Error: To change owner/admin, you need to call the proxy directly
    at _deployViaEIP173Proxy (/Users/User/Sites/hashtag-protocol/hashtag-contracts/node_modules/hardhat-deploy/src/helpers.ts:1196:17)
...

Wondering what the proper method for is for upgrading the implementation contract?

bumping this q

wighawag commented 3 years ago

@theekruger as mentioned on discord it most likely your uups implementation contract do not set the eip-1967 admin and so hardhat-deploy assume the admin is address(0) which is why it complain that the account trying to perform the upgrade is not the admin

gigamesh commented 2 years ago

@JasoonS Have you successfully verified that proxy on etherscan? I tried your solution but I can't get it to verify with @nomiclabs/hardhat-etherscan

JasoonS commented 2 years ago

@gigamesh Yes, but the upgrade process is partially manual (which suits us), this is due to us overlooking this: https://github.com/wighawag/hardhat-deploy/issues/146#issuecomment-945277351

Here it is on polygon scan: https://polygonscan.com/address/0x168a5d1217AEcd258b03018d5bF1A1677A07b733#code (this is for https://float.capital)

gigamesh commented 2 years ago

@JasoonS Is there any chance I could see how you're verifying on etherscan? Every time I make some contract changes, it gets borked and I can't figure out how to get it to recognize the proxies consistently

JasoonS commented 2 years ago

@gigamesh try this hardhat --network <network name> etherscan-verify --api-key <your etherscan api key> --force-license --license UNLICENSED (don't do 'force-license' if you don't want it not to have a licence)

JasoonS commented 2 years ago

Hey guys, I've made a bit of a hack on hardhat deploy to make upgrades smooth (before this I was doing a manual step).

It is hacky, but here it is: https://www.npmjs.com/package/@float-capital/hardhat-deploy

And the code (diff off current master): https://github.com/Float-Capital/hardhat-deploy/compare/ccfce77..1254fd3

It adds two optional parameters to a deploy:

  isUups?: boolean;
  uupsAdmin?: string;

It seems to be working with the custom proxy I created as mentioned above.

I don't know if this is useful for you @wighawag at all, but just sharing what is working for me :)

wighawag commented 2 years ago

Hey all, you can now specify the constructor arguments used for the proxy itself, allowing you to use the ERC1967Proxy from openzeppelin without any change,

see :https://github.com/wighawag/template-ethereum-contracts/blob/f4d48728ebcabd9129669af7bb42a739594aff78/deploy/005_deploy_erc20_via_openzeppelin_uups.ts#L11-L26

available in 0.9.23

hanselke commented 2 years ago

hi @wighawag , your sample keeps failing with Error: expected 2 constructor arguments, got 3

const myContract = await deploy("MyContract", { from: deployer, args: [], proxy: { proxyContract: 'ERC1967Proxy', proxyArgs: ['{implementation}', '{data}',"test"], execute: { init: { methodName: 'initialize', args: [maxSupply,wallet], }, }, }, log: true, });

MyContract does not have a constructor, and takes an initialize function.

wighawag commented 2 years ago

What ERC1967Proxy are you using?

if you use openzeppelin one, it expect only 2 arguments

If so your call would be incorrect, it would need to be

const myContract = await deploy("MyContract", { from: deployer, args: [], proxy: { proxyContract: 'ERC1967Proxy', proxyArgs: ['{implementation}', '{data}'], execute: { init: { methodName: 'initialize', args: [maxSupply,wallet], }, }, }, log: true, });

in particular, see proxyArgs: ['{implementation}', '{data}']

arnlen commented 2 years ago

Hey all,

IMHO, the current support for UUPS Proxy could be improved. Currently, it is still a bit harsh to use:

On the other hand, when using a sample script like the one presented in this OpenZeppelin workshop - which uses methods hre.upgrades.deployProxy and hre.upgrades.upgradeProxy - is quite easy.

What I found wonderful with this hardhat-deploy plugin, is that it helps to reduce frictions during deployments. Unfortunately, in the current implementation of the UUPS Proxy is not that easy to implement.

I really like @JasoonS's proposition, and I think using this kind of flag would be magic!

isUups?: boolean;

What do you think?

wighawag commented 2 years ago

Hey @arnlen thanks for your comment.

The plan is actually to add an option, but did not find the time for it yet. the idea is to simply add another option here : https://github.com/wighawag/hardhat-deploy/blob/8be0207d1634b712e0a279d9987f344ecc7376a3/src/helpers.ts#L1156-L1172

so you would do

await deploy('MyContract', {
    from: deployer,
    proxy: {
      proxyContract: 'UUPSProxy',
    },
  });

I could reopen, but wanted to allow new comer to see that it is possible without needing to implement your own

Happy to review PR by the way, including the doc part

drop19 commented 2 years ago

I use ERC1967Proxy as proxy and during upgrade, I failed at at following line

if (currentOwner.toLowerCase() !== proxyAdmin.toLowerCase()) 

It seems to assume there's a proxyAdmin which might not be the case in UUPS proxy. I wonder what could I do to make it work?

wighawag commented 2 years ago

@drop19

It seems to assume there's a proxyAdmin which might not be the case in UUPS proxy. I wonder what could I do to make it work?

That is not the case, proxyAdmin here means owner and these do not match. mean you are now specifying a different owner.

proxyAdmin contract (Which you refers to) are handled by the proxyAdmin config. I guess hardhat-deploy source code is a bit confusing on that, but that is 2 different thing

drop19 commented 2 years ago

Is proxyAdmin the _ADMIN_SLOT of ERC1967UpgradeUpgradeable?

I guess I failed it because I didn't set the _ADMIN_SLOT. And I I use onlyOwner for authorization in the following function: function _authorizeUpgrade(address) internal override onlyOwner {}

I am a bit confused as I thought the admin slot is only used in TransparentUpgradeableProxy but not UUPS proxy. UUPSUpgradeable doesn't seem to rely on this storage slot

wighawag commented 2 years ago

the admin slot is for 1967 compliance and is what harhat-deploy read to check too. it can be used by any type of proxy and is also the recommended way to be compatible with proxy tool

drop19 commented 2 years ago

This is however what the tutorial on OZ forum shows:

contract MyTokenV1 is Initializable, ERC20Upgradeable, UUPSUpgradeable, OwnableUpgradeable {
    function initialize() initializer public {
      __ERC20_init("MyToken", "MTK");
      __Ownable_init();
      __UUPSUpgradeable_init();

      _mint(msg.sender, 1000 * 10 ** decimals());
    }

    /// @custom:oz-upgrades-unsafe-allow constructor
    constructor() initializer {}

    function _authorizeUpgrade(address) internal override onlyOwner {}
}

https://forum.openzeppelin.com/t/uups-proxies-tutorial-solidity-javascript/7786

wighawag commented 2 years ago

@drop19 I am not sure what you mean.

hardhat-deploy is not used in that tutorial

wighawag commented 2 years ago

I see, so OwnableUpgradeable are not compatible with EIP-1967, that's a shame

I could make a call to owner() as a fallback I guess

drop19 commented 2 years ago

I try to make OwnableUpgradeable to be compatible with EIP-1967 and combine it with UUPSUpgradeable. This looks silly and not yet tested. Anyone with better idea are welcomed

import '@openzeppelin/contracts-upgradeable/proxy/utils/UUPSUpgradeable.sol';
import '@openzeppelin/contracts-upgradeable/access/OwnableUpgradeable.sol';

// This contract makes OwnableUpgradeable compatible to ERC1967
contract UUPSUpgradeableOwnableCompatible is OwnableUpgradeable, UUPSUpgradeable {
    function __UUPSUpgradeableOwnableCompatible_init() internal onlyInitializing {
        __Ownable_init();
        __UUPSUpgradeableOwnableCompatible_init_unchained();
    }

    function __UUPSUpgradeableOwnableCompatible_init_unchained() internal onlyInitializing {
        __UUPSUpgradeable_init_unchained();
    }

    function _authorizeUpgrade(address newImplementation) internal override onlyOwner {}

    /**
     * @dev Transfers ownership of the contract to a new account (`newOwner`).
     * Internal function without access restriction.
     */
    function _transferOwnership(address newOwner) internal override {
        super._transferOwnership(newOwner);
        assembly {
            sstore(_ADMIN_SLOT, newOwner)
        }
    }
}
idkravitz commented 2 years ago

@wighawag Just to clarify, EIP-1967 in fact IS COMPATIBLE with OwnableUpgradable. Here is an example of the UUPS-upgradable contract I deployed using hadhat-upgrades: https://etherscan.io/address/0x695fae4a583a4d55c9665cf2194b37c5d214fda0 The implementation is here: https://etherscan.io/address/0xde3b3b4864a83d2a30d336f8b66a21155c95ab0f Openzeppelin somehow is capable of handling this usecase. But I'm unable to switch to hardhat-deploy in any straightforward way. I guess this should be opened as a separate issue.

wighawag commented 2 years ago

@idkravitz The discussion is foggy now for me, but from what I see the contract you linked have the implementation stored as EIP-1967, but not the admin, see : https://eips.ethereum.org/EIPS/eip-1967#admin-address

idkravitz commented 2 years ago

@wighawag I mean the proxy is EIP-1967 the implementation is OwnableUpgradable, hardhat-upgrades handles this case fine and the pair I showed you (proxy and implementation) was deployed with hardhat-upgrades. However I can't deploy it with hardhat-deploy, because it crashes with duplicated owner field in interface or something like that. Yes in my case it's not an admin proxy case, its a plain uups one (just proxy and implementation, implementation handles upgrades authorization with authorize_upgrade)

sbaranov commented 2 years ago

@wighawag "I could make a call to owner() as a fallback I guess"

This might help if implementation logic uses OwnableUpgradable, but not if it uses AccessControlUpgradeable. As I understand it, one of the points of UUPS is to be able to query custom access control: onlyOwner, onlyRole, or whatever.

The UUPS example that others have mentioned works with openzeppelin's hardhat-upgrades plugin. But with hardhat-deploy it gives error "To change owner/admin, you need to call the proxy directly". It'd be great if it could also work with hardhat-deploy as-is.

sbaranov commented 2 years ago

Here's where the code that triggers the error: https://github.com/wighawag/hardhat-deploy/blob/ee7e872fc8f8fa5ed63a00deaf25c6c7cd464a18/src/helpers.ts#L1545

It reads from ADMIN_SLOT, but as I understand ERC-1967 (which isn't explicit about this), only TUPS normally use this slot, and not necessarily UUPS. Perhaps hardhat-deploy could call _authorizeUpgrade() instead of (or in addition to) reading from ADMIN_SLOT, to be compliant with openzeppelin's UUPS implementation?

johnwhitton commented 2 years ago

Hi all,

I've read through all of the above and still cannot work out how to deploy an openzepplin upgradeable contract using hardhat deploy and the ERC1967 proxy. The closest example I have seen is in wighawag/template-ethereum-contracts examples/openzeppelin branch Which appears to just use the standard openzeppelin ERC1967.sol in Import.sol However when I replicate this I get that the ownerStorage slot is not set i.e. 0x

I also tried using a copy of Hardhat deploy ERC1967.sol which is slightly different from the openzeppelin version with this line added assert(_IMPLEMENTATION_SLOT == bytes32(uint256(keccak256("eip1967.proxy.implementation")) - 1)); which seems unrelated to the ADMIN slot.

Is there a way to use hardhat deploy and ERC1967Proxy.sol together?

Below are the logs and scripts I used.

Here is a log of my deployment with a couple of additional console.logs added

johnlaptop miniwallet (miniV0) $ npx hardhat deploy --tags MiniIDCombo --network ethLocal
Nothing to compile
No need to generate any newer typings.
✅ Generated documentation for 83 contracts
 ·-----------------|--------------|----------------·
 |  Contract Name  ·  Size (KiB)  ·  Change (KiB)  │
 ··················|··············|·················
 |  MiniWallet     ·       9.128  ·                │
 ·-----------------|--------------|----------------·
chainId: 1337
deploying "MiniID_Implementation" (tx: 0x1de21cca18b26fc1771c79a864a136c42cee348b87968d792d3e8b9c6e806a9a)...: deployed at 0x5FbDB2315678afecb367f032d93F642f64180aa3 with 3590609 gas
In Helper proxyName: MiniID_Proxy
!proxy: true
deploying "MiniID_Proxy" (tx: 0x7a5f8f6db00136986080d9720c7920e40974d5b5a733d95e4726242b3180c9e6)...: deployed at 0xe7f1725E7734CE288F8367e1Bb143E90bb3F0512 with 466049 gas
deploying "MiniID_Implementation" (tx: 0x88f57637a7f8731c1b840e7bc9500028432cfec699de7a8c49a43e4a9543cbdc)...: deployed at 0x9fE46736679d2D9a65F0992F2272dE9f3c7fa6e0 with 3590609 gas
In Helper proxyName: MiniID_Proxy
!proxy: false
proxy.address: 0xe7f1725E7734CE288F8367e1Bb143E90bb3F0512
ownerStorage: 0x
An unexpected error occurred:

Error: ERROR processing /Users/john/one-wallet/sms-wallet/miniwallet/deploy/998_deploy_miniID.ts:
Error: invalid address (argument="address", value="0x0x", code=INVALID_ARGUMENT, version=address/5.7.0)
    at Logger.makeError (/Users/john/one-wallet/sms-wallet/miniwallet/node_modules/@ethersproject/logger/src.ts/index.ts:269:28)
    at Logger.throwError (/Users/john/one-wallet/sms-wallet/miniwallet/node_modules/@ethersproject/logger/src.ts/index.ts:281:20)
    at Logger.throwArgumentError (/Users/john/one-wallet/sms-wallet/miniwallet/node_modules/@ethersproject/logger/src.ts/index.ts:285:21)
    at Object.getAddress (/Users/john/one-wallet/sms-wallet/miniwallet/node_modules/@ethersproject/address/src.ts/index.ts:109:16)
    at _deployViaProxy (/Users/john/one-wallet/sms-wallet/miniwallet/node_modules/hardhat-deploy/src/helpers.ts:1541:32)
    at processTicksAndRejections (node:internal/process/task_queues:95:5)
    at async Object.deployFunction [as func] (/Users/john/one-wallet/sms-wallet/miniwallet/deploy/998_deploy_miniID.ts:33:20)
    at async DeploymentsManager.executeDeployScripts (/Users/john/one-wallet/sms-wallet/miniwallet/node_modules/hardhat-deploy/src/DeploymentsManager.ts:1219:22)
    at async DeploymentsManager.runDeploy (/Users/john/one-wallet/sms-wallet/miniwallet/node_modules/hardhat-deploy/src/DeploymentsManager.ts:1052:5)
    at async SimpleTaskDefinition.action (/Users/john/one-wallet/sms-wallet/miniwallet/node_modules/hardhat-deploy/src/index.ts:438:5)
    at DeploymentsManager.executeDeployScripts (/Users/john/one-wallet/sms-wallet/miniwallet/node_modules/hardhat-deploy/src/DeploymentsManager.ts:1222:19)
    at processTicksAndRejections (node:internal/process/task_queues:95:5)
    at async DeploymentsManager.runDeploy (/Users/john/one-wallet/sms-wallet/miniwallet/node_modules/hardhat-deploy/src/DeploymentsManager.ts:1052:5)
    at async SimpleTaskDefinition.action (/Users/john/one-wallet/sms-wallet/miniwallet/node_modules/hardhat-deploy/src/index.ts:438:5)
    at async Environment._runTaskDefinition (/Users/john/one-wallet/sms-wallet/miniwallet/node_modules/hardhat/src/internal/core/runtime-environment.ts:219:14)
    at async Environment.run (/Users/john/one-wallet/sms-wallet/miniwallet/node_modules/hardhat/src/internal/core/runtime-environment.ts:131:14)
    at async SimpleTaskDefinition.action (/Users/john/one-wallet/sms-wallet/miniwallet/node_modules/hardhat-deploy/src/index.ts:584:32)
    at async Environment._runTaskDefinition (/Users/john/one-wallet/sms-wallet/miniwallet/node_modules/hardhat/src/internal/core/runtime-environment.ts:219:14)
    at async Environment.run (/Users/john/one-wallet/sms-wallet/miniwallet/node_modules/hardhat/src/internal/core/runtime-environment.ts:131:14)
    at async SimpleTaskDefinition.action (/Users/john/one-wallet/sms-wallet/miniwallet/node_modules/hardhat-deploy/src/index.ts:669:5)
johnlaptop miniwallet (miniV0) $

and here is my deployment Script

// import config from '../config'
import { HardhatRuntimeEnvironment } from 'hardhat/types'
import { DeployFunction } from 'hardhat-deploy/types'
// import { ethers } from 'hardhat'

const deployFunction: DeployFunction = async function (
  hre: HardhatRuntimeEnvironment
) {
  const { deployments, getNamedAccounts, getChainId } = hre
  const { deploy } = deployments
  const { deployer } = await getNamedAccounts()
  const chainId = await getChainId()

  console.log(`chainId: ${chainId}`)

  let deployedMiniID = await deploy('MiniID', {
    contract: 'MiniID',
    from: deployer,
    args: [],
    proxy: {
      proxyContract: 'ERC1967Proxy',
      proxyArgs: ['{implementation}', '{data}'],
      execute: {
        init: {
          methodName: 'initialize',
          args: []
        }
      }
    },
    log: true
  })

  deployedMiniID = await deploy('MiniID', {
    contract: 'MiniID_v2',
    from: deployer,
    args: [],
    proxy: {
      proxyContract: 'ERC1967Proxy',
      proxyArgs: ['{implementation}', '{data}']
    },
    log: true
  })

  const miniID = await hre.ethers.getContractAt('MiniID', deployedMiniID.address)

  console.log('MiniID deployed to  :', miniID.address)
  console.log('MiniID Name         : ', await miniID.name())
  console.log('MiniID Symbol       : ', await miniID.symbol())
}

deployFunction.dependencies = []
deployFunction.tags = ['MiniID', '004', 'MiniIDCombo']
export default deployFunction
sbaranov commented 2 years ago

I ended up deploying using openzeppelin's plugin and then manually saving hardhat-deploy's artifact:

const factory = await hre.ethers.getContractFactory(name, { signer, libraries });
const contract = await hre.upgrades.deployProxy(factory, args, { kind: "uups" });
await contract.deployed();
const artifact = await hre.deployments.getExtendedArtifact(contract);
await hre.deployments.save(name, { ...artifact, address });

You can also optionally notify openzeppelin's Defender tool (if you use it) about the new deployment:

import { AdminClient } from "defender-admin-client";
import { fromChainId } from "defender-base-client";

const defender = new AdminClient({ apiKey: hre.config.defender!.apiKey, apiSecret: hre.config.defender!.apiSecret });
const network = fromChainId(parseInt(await hre.getChainId()))!;
await defender.addContract({ name, address, network, abi: JSON.stringify(artifact.abi) });
wighawag commented 2 years ago

@sbaranov and @johnwhitton did you try that branch :https://github.com/wighawag/template-ethereum-contracts/tree/examples/openzeppelin-proxies

It use EIP1967 from openzeppelin

await deploy('SimpleERC20_via_UUPS', {
    contract: 'SimpleERC20UUPSReady',
    from: deployer,
    args: [simpleERC20Beneficiary, parseEther('1000000000')],
    proxy: {
      proxyContract: 'ERC1967Proxy',
      proxyArgs: ['{implementation}', '{data}'],
      execute: {
        init: {
          methodName: 'init',
          args: [proxy01Owner, simpleERC20Beneficiary, parseEther('1000000000')],
        },
      },
    },
    log: true,
  });

  // UPGRADE:
  await deploy('SimpleERC20_via_UUPS', {
    contract: 'SimpleERC20UUPSReady_v2',
    from: proxy01Owner,
    args: [simpleERC20Beneficiary, parseEther('1000000000')],
    proxy: {
      proxyContract: 'ERC1967Proxy',
      proxyArgs: ['{implementation}', '{data}'],
    },
    log: true,
  });
johnwhitton commented 2 years ago

@sbaranov Thanks for the advice 🙏 @wighawag Yes I used this as a baseline The closest example I have seen is in wighawag/template-ethereum-contracts examples/openzeppelin branch However I could not get it working in my codebase (see logs above). Deploy works fine, but the upgrade was failing trying to look up the _ADMIN_SLOT Complete codebase is here. Note the deployment script shown above is not in the codebase as it is failing. Next steps: I'll clone that repo and test there ( if it works I'll do more analysis on the differences with my local version) 🙏 💙 🙏

johnwhitton commented 2 years ago

Thanks @wighawag

This is now working, for others who run into this you need to populate the ADMIN_SLOT when using hardhat-deploy and openzeppelin UUPS contracts.

i.e. you must insert code like this in your contract

        // solhint-disable-next-line security/no-inline-assembly
        assembly {
            sstore(0xb53127684a568b3173ae13b9f8a6016e243e63b6e8ee1178d6a717850b5d6103, owner)
        }

🙏 💙 🙏

sbaranov commented 2 years ago

Thank you for the workaround @johnwhitton, it should help in the basic case. Longer term this should be fixed in hardhat-deploy because UUPS code isn't supposed to look in the ADMIN_SLOT at all. It's should rely only on the _authorizeUpgrade() override function in Solidity that can check any role or access condition.

wighawag commented 2 years ago

@sbaranov hardhat-deploy do not provide explicit UUPS proxy support and so there is nothing to fix in that regard

You and @johnwhitton can implements _authorizeUpgrade the way you want

the contract here choose to use slot 0xb53127684a568b3173ae13b9f8a6016e243e63b6e8ee1178d6a717850b5d6103 and the proxied helper but you can do what you wish

drop19 commented 2 years ago

@wighawag I wonder if you welcome folks here to implement & merge UUPS support into the main branch.

Honestly speaking UUPS is still poorly supported by tools and explorers. But hopefully this could be a step to embrace UUPS

wighawag commented 2 years ago

Hey @drop19 yes, happy to help any PR with that goal.

should actually be quite easy by adding a if case here: https://github.com/wighawag/hardhat-deploy/blob/e2b3696046eb827394bc8966bdfdc6d10e344e9a/src/helpers.ts#L1223-L1243

0xTimepunk commented 1 year ago

Anyone following up on this? @wighawag

wighawag commented 1 year ago

@0xTimepunk Just added the UUPS option in v0.11.21

you just do the following :

await deploy('MyContract', {
  from: deployer,
  proxy: {
    proxyContract: 'UUPS',
  }
});

or if you need some initialization:

await deploy('MyContract', {
  from: deployer,
  proxy: {
    proxyContract: 'UUPS',
    execute: {
      init: {
        methodName: 'init',
        args: ['hello'],
      },
    },
  },
});