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

Options to change deploy function and proxy contract factory #1104

Merged
merged 16 commits into from
Dec 5, 2024

Conversation

gnarvaja
Copy link
Contributor

@gnarvaja gnarvaja commented Nov 28, 2024

Implementation for #231

Adds two new options that allow override of the deploy function and contract factory for the proxy.

Kind is still used to identify the kind of proxy used, but this
customization can manage variations of the proxy implementation and its
constructor parameters.

As a test case, and to show a concrete application of this change, I
implemented a AccessManagedProxy. This proxy works like a UUPS (ERC1967)
proxy, but before delegating the calls, checks with the access manager
if the call is allowed.

Adds two new options that allow override of the deploy and factory
functions.

Kind is still used to identify the kind of proxy used, but this
customization can manage variations of the proxy implementation and its
constructor parameters.

As a test case, and to show a concrete application of this change, I
implemented a AccessManagedProxy. This proxy works like a UUPS (ERC1967)
proxy, but before delegating the calls, checks with the access manager
if the call is allowed.
Quick fix, I'm not expert enough in Typescript to make the correct type
annotations
Copy link
Member

@ericglau ericglau left a comment

Choose a reason for hiding this comment

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

Thank you @gnarvaja for this PR! This looks like a good approach for allowing the proxy deployment to be overridden.

I made a few minor changes to the example proxy's comments and simplified the types for the new options.

The following would also be needed before we can merge this. Would you be interested in adding these? Thanks!

  • A basic test for deploying a custom beacon proxy
  • Add the new options in the documentation in docs/modules/ROOT/pages/api-hardhat-upgrades.adoc

Adds the documentation to the new options of the `deployProxy` method.
- Added a test case for the customization of the BeaconProxy.
- Fixed the documentation, moving the new options to common options
  since they are used both in deployProxy and deployBeaconProxy.
@gnarvaja
Copy link
Contributor Author

gnarvaja commented Dec 2, 2024

Thank you @gnarvaja for this PR! This looks like a good approach for allowing the proxy deployment to be overridden.

I made a few minor changes to the example proxy's comments and simplified the types for the new options.

The following would also be needed before we can merge this. Would you be interested in adding these? Thanks!

  • A basic test for deploying a custom beacon proxy
  • Add the new options in the documentation in docs/modules/ROOT/pages/api-hardhat-upgrades.adoc

Done, I added an example of a custom beacon proxy.

Also, I added the new options to the documentation. Including links to the default implementations of the factories and deploy functions.

@ericglau ericglau changed the title Options to change deploy and factory functions Options to change deploy function and proxy contract factory Dec 4, 2024
@ericglau
Copy link
Member

ericglau commented Dec 5, 2024

@gnarvaja I changed the proxyFactory option to directly take a ContractFactory instead of a function that returns the ContractFactory. I think this makes it simpler to use. Let me know if that works for you, then I can go ahead and release this.
Thanks again for your contributions here!

@gnarvaja
Copy link
Contributor Author

gnarvaja commented Dec 5, 2024

@gnarvaja I changed the proxyFactory option to directly take a ContractFactory instead of a function that returns the ContractFactory. I think this makes it simpler to use. Let me know if that works for you, then I can go ahead and release this. Thanks again for your contributions here!

Yes, I agree with the change. Thanks!

@ericglau ericglau merged commit 1824f94 into OpenZeppelin:master Dec 5, 2024
11 checks passed
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.

2 participants