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

Add Proxies from OpenZeppelin SDK #2335

Merged
merged 13 commits into from
Aug 28, 2020
Merged

Conversation

frangio
Copy link
Contributor

@frangio frangio commented Aug 21, 2020

As mentioned in #2207, we're moving the proxies from OpenZeppelin SDK into the Contracts project so they can 1) be more readily accessible to users, and 2) benefit from the community participation in maintenance and security assessment.

This PR is a simple port of the contracts and their tests from the SDK repo, the main difference being the removal of the "base" and "initializable" distintion (see here) for simplicity.

Some potential renames:

  • UpgradeabilityProxyEIP1967Proxy (or ERC1967Proxy?)
  • AdminUpgradeabilityProxyAdminTransparentProxy

@frangio frangio marked this pull request as ready for review August 21, 2020 22:16
@frangio frangio requested a review from spalladino August 21, 2020 22:16
spalladino
spalladino previously approved these changes Aug 24, 2020
Copy link
Contributor

@spalladino spalladino left a comment

Choose a reason for hiding this comment

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

LGTM @frangio! My thoughts on naming:

Proxy
I'd rename to DelegateProxy, as opposed to a ForwardProxy

UpgradeabilityProxy
I'd consider changing to UpgradeableProxy or UpgradeProxy. EIP1967 is tempting, but it's only part of the implementation. For instance, UUPS is 1967 compatible, but uses a different approach where upgradeTo is not part of the proxy.

AdminUpgradeabilityProxy
I really like the idea of including Transparent on this one. AdminTransparentProxy sounds fine, and I'd also consider TransparentUpgradeProxy (or TransparentUpgradeableProxy).

Last: have you considered moving Initializable out of /proxy? Looks odd in there. But I'm not sure where else to place it.

@frangio
Copy link
Contributor Author

frangio commented Aug 25, 2020

@spalladino Initializable could go in utils as well. But we could also leave it in proxy, since that's really what it's designed for.

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