Skip to content
New issue

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

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

Already on GitHub? Sign in to your account

Deploy proxy contracts from OpenZeppelin Contracts 5.0 #919

Merged
merged 31 commits into from
Nov 30, 2023

Conversation

ericglau
Copy link
Member

@ericglau ericglau commented Nov 9, 2023

Targets release branch hardhat-upgrades-v3 to allow for alpha version.


Deploys proxy contracts from OpenZeppelin Contracts 5.0, but continues to support importing and working with existing deployments from Contracts 4.x and below.

Updates Etherscan verification to verify proxy admins which are deployed by a transparent proxy rather than directly by the plugin.

upgrades-core will have proxy bytecodes for both 5.0 and 4.x proxies for backwards compatibility with older version of hardhat-upgrades.

Fixes #907

  • Tested with local npm packages
  • Tested with Defender
  • Tested with Hardhat Verify

Copy link

socket-security bot commented Nov 9, 2023

Updated and removed dependencies detected. Learn more about Socket for GitHub ↗︎

Packages Version New capabilities Transitives Size Publisher
ethers 6.6.7...6.8.1 None +6/-4 17.2 MB ricmoo
@openzeppelin/contracts-upgradeable 4.8.3...5.0.0 None +0/-0 1.21 MB frangio
solidity-ast 0.4.55...0.4.49 None +0/-82 231 kB frangio

🚮 Removed packages: @openzeppelin/[email protected]

Copy link

socket-security bot commented Nov 10, 2023

👍 Dependency issues cleared. Learn more about Socket for GitHub ↗︎

This PR previously contained dependency changes with security issues that have been resolved, removed, or ignored.

@ericglau ericglau requested a review from a team November 10, 2023 16:45
@ericglau ericglau marked this pull request as ready for review November 10, 2023 22:29
@rsimonton
Copy link

@ericglau hello ser, is this blocked? Anxiously awaiting this functionality, thanks.

packages/core/contracts/test/Proxiable.sol Outdated Show resolved Hide resolved
packages/core/scripts/copy-build-info-v5.js Outdated Show resolved Hide resolved
packages/core/scripts/copy-artifacts.sh Outdated Show resolved Hide resolved
Comment on lines 255 to 256
const constructorArgs = ethers.AbiCoder.defaultAbiCoder().encode(['address'], [owner]).replace(/^0x/, '');
await verifyContractWithConstructorArgs(etherscan, adminAddress, artifact, constructorArgs, errorReport);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The current owner may not be the one used for deployment. Not sure if that would be easy to support, but if that is not the case, we probably want to have a verbose error.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point. Changed it to look for the OwnershipTransferred event that should have been logged during construction and get the original owner from that.

packages/plugin-hardhat/test/import-with-deploy-v5.js Outdated Show resolved Hide resolved
@ericglau
Copy link
Member Author

@rsimonton We're planning to have this available soon. We'll likely do an alpha release once this is merged so that you can start using it (it will be alpha for now since we still need to evaluate some breaking changes, particularly #912)

@ericglau ericglau requested a review from Amxx November 23, 2023 16:32
@ericglau ericglau changed the base branch from master to hardhat-upgrades-v3 November 28, 2023 19:56
Copy link
Member

@ernestognw ernestognw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On a side note, I think we should change all of the @openzeppelin/contracts-v5 imports and use @openzeppelin/contracts-v4 for v4 instead.

Should be added before the final 3.0 release, right?

README.md Outdated Show resolved Hide resolved
docs/modules/ROOT/pages/faq.adoc Outdated Show resolved Hide resolved
packages/plugin-hardhat/test/uups-initial-owner.js Outdated Show resolved Hide resolved
packages/plugin-hardhat/test/import-with-deploy-v5.js Outdated Show resolved Hide resolved
@ericglau
Copy link
Member Author

@ernestognw

On a side note, I think we should change all of the @openzeppelin/contracts-v5 imports and use @openzeppelin/contracts-v4 for v4 instead.

Should be added before the final 3.0 release, right?

I'd prefer that, however these paths are in upgrades-core and we want to keep it backwards compatible. We aren't doing a major release for that package.

Since upgrades-core already packages /artifacts/@openzeppelin/contracts/proxy for v4 proxies, we need to define a different naming scheme for v5 proxies.

@ericglau ericglau requested a review from ernestognw November 30, 2023 16:35
@ernestognw
Copy link
Member

Since upgrades-core already packages /artifacts/@openzeppelin/contracts/proxy for v4 proxies, we need to define a different naming scheme for v5 proxies.

Right, thanks for clarifying. I'd suggest creating an issue to track this if we ever bump upgrades/core so we can consider a fix, but you decide.

Copy link
Member

@ernestognw ernestognw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Alright, this LGTM 🚀

@ericglau
Copy link
Member Author

Since upgrades-core already packages /artifacts/@openzeppelin/contracts/proxy for v4 proxies, we need to define a different naming scheme for v5 proxies.

Right, thanks for clarifying. I'd suggest creating an issue to track this if we ever bump upgrades/core so we can consider a fix, but you decide.

Opened issue #931 to track.

@ericglau ericglau merged commit d6be275 into OpenZeppelin:hardhat-upgrades-v3 Nov 30, 2023
5 of 6 checks passed
@ericglau ericglau deleted the 5.0proxies branch November 30, 2023 19:08
@rsimonton
Copy link

You all are legends!  🫡

@rsimonton
Copy link

I don't have time to open an issue atm but it looks like the deployProxy function doesn't support/respect explicitly typed data.

In my case I have contracts A and B, where B extends A. Both are upgradeable. B's initializer adds an additional argument. When calling B's initializer (prior to my incorporating the upgrades plugin) in order for ethers to know which version of initialize to call, I had to explicitly type B's additional argument as described, for example, here.

Trying to do this when calling deployProxy, like so:

     const proxy = await upgrades.deployProxy(
            contractFactory,
            [
                // .. other initializer args here
                ethers.Typed.uint16(foo)
            ],
            {
                kind: "uups",
            }
        )

...the deployment fails with TypeError: ambiguous function description (i.e. matches "initialize(...

Is it possible for the upgrades plugin to support the ethers v6 typed data API? Or is there some other way to work around? Thanks!

@ericglau
Copy link
Member Author

ericglau commented Dec 7, 2023

@rsimonton You can try specifying a fragment using the initializer option. See Specifying Fragments (that's from ethers v5 doc but it should be the same for v6).

For example, you can provide the initializer function's signature like:

     const proxy = await upgrades.deployProxy(
            contractFactory,
            [
                // .. other initializer args here
                foo
            ],
            {
                kind: "uups",
                initializer: 'initialize(uint256)',
            }
        )

If this is still a problem for you, please open a separate issue, thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Deploy proxy contracts from OpenZeppelin Contracts 5.0
4 participants