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

Make TransparentUpgradeableProxy deploy its ProxyAdmin and optimize proxy interfaces #4382

Merged
merged 48 commits into from
Jul 13, 2023

Conversation

ernestognw
Copy link
Member

@ernestognw ernestognw commented Jun 23, 2023

  • Makes TransparentUpgradeableProxy deploy its own ProxyAdmin
  • Removes upgrade/upgradeTo in favor of upgradeAndCall/upgradeToAndCall from ProxyAdmin and UUPSUpgradeable, now the data argument is ignored if empty.

Fixes LIB-947
Fixes LIB-1008
Fixes LIB-999

PR Checklist

  • Tests
  • Documentation
  • Changeset entry (run npx changeset add)

@changeset-bot
Copy link

changeset-bot bot commented Jun 23, 2023

🦋 Changeset detected

Latest commit: 703659e

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
openzeppelin-solidity Major

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@ernestognw ernestognw changed the title Make TransparentUpgradeableProxy deploy its own ProxyAdmin Make TransparentUpgradeableProxy deploy its own ProxyAdmin Jun 23, 2023
@frangio frangio marked this pull request as draft June 26, 2023 16:29
@frangio
Copy link
Contributor

frangio commented Jul 1, 2023

We are able to optimize TransparentUpgradeableProxy by removing upgradeToAndCall, because it will always be attached to a ProxyAdmin which can just use upgradeTo and separately call a function atomically. This is the "reduced" interface measured in the table below.

We make TransparentUpgradeableProxy deploy a ProxyAdmin in its constructor. This is the "embedded" version below. I tried a few variations to see whether making ProxyAdmin attached to a single proxy improves cost, these are the "1:1" variations.

These are the measured deployment costs. Note that the total for non-embedded is the sum of admin + proxy. But when embedded, the total is just the proxy.

ProxyAdmin TransparentUpgradeableProxy total
baseline (4.9) 443289 630553 1073842
baseline (master) 321406 445272 766678
reduced 366449 298173 664622
reduced + embedded + 1:1 (storage) 376956 655177 655177
reduced + embedded 366449 644874 644874
reduced + embedded + 1:1 (immutable) 364405 642642 642642

The cheapest version we get is 1:1 using an immutable variable. In this case, the upgrade functions don't have a proxy argument, because the proxy is kept in a contract variable. Note that this changes the interface and we have a similar problem in Upgrades Plugins than what we've discussed before: how do the plugins know which function to call? Unless we add some way to inspect the ProxyAdmin version, we don't know. At least in the case of changing the function signatures it would fail.

There is one option that is cheaper which is removing upgradeTo from ProxyAdmin and making upgradeToAndCall skip the call if data is empty. In this case we can get the cost down to 618300 (or 629536 for 1:1 variation). This one is problematic because it makes it hard for tooling like Upgrades Plugins to know which function to call for an upgrade. Depending on the version one function may not be available, and the other behaves different in each version.

I slightly lean towards reduced + embedded in the table above, because I feel there is some risk that a ProxyAdmin could be reused for multiple proxies and break with the 1:1 variation. This is the version in the PR currently.

Copy link
Collaborator

@Amxx Amxx left a comment

Choose a reason for hiding this comment

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

Minor comments. We should merge that before the audit

contracts/proxy/transparent/ProxyAdmin.sol Outdated Show resolved Hide resolved
contracts/proxy/transparent/ProxyAdmin.sol Outdated Show resolved Hide resolved
This was referenced Sep 10, 2024
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.

6 participants