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.

Sign up for GitHub

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

Jump to bottom

Make TransparentUpgradeableProxy deploy its ProxyAdmin and optimize proxy interfaces #4382

Merged
merged 48 commits into from
Jul 13, 2023
Merged

Make TransparentUpgradeableProxy deploy its ProxyAdmin and optimize proxy interfaces #4382

Show file tree
Hide file tree
Changes from 12 commits
Commits
Show all changes
48 commits
Select commit Hold shift + click to select a range
a4215ca
Make TransparentUpgradeableProxy deploy its own ProxyAdmin
ernestognw Jun 23, 2023
9d39cf0
Merge branch 'master' into proxy-admin-optimizations
frangio Jun 30, 2023
2acbf84
revert changes to other proxies
frangio Jun 30, 2023
e48fa5a
change transparent proxy to have upgradeTo only
frangio Jun 30, 2023
d999991
embed ProxyAdmin creation in proxy constructor
frangio Jun 30, 2023
03c2cf5
remove gas snapshot
frangio Jun 30, 2023
f570cdc
revert test changes
frangio Jun 30, 2023
0303cc7
add back upgrade
frangio Jul 1, 2023
347fac0
remove extra space
frangio Jul 6, 2023
9de9ce8
inline behavior into test file
frangio Jul 6, 2023
34381f6
remove unnecessary argument
frangio Jul 6, 2023
bff3bc7
switch proxy to upgradeToAndCall
frangio Jul 7, 2023
d9e192e
Update contracts/proxy/transparent/TransparentUpgradeableProxy.sol
ernestognw Jul 7, 2023
2dd7c05
Lint
ernestognw Jul 7, 2023
62bbd7b
Remove unused import
ernestognw Jul 7, 2023
b71fc98
Fix tests
ernestognw Jul 8, 2023
2e0c396
add else branch
frangio Jul 8, 2023
ca99577
use forceCall = false
frangio Jul 8, 2023
909fe1d
use rlp from declared dependency
frangio Jul 8, 2023
d01100a
use hardhat-network-helpers
frangio Jul 8, 2023
b425a38
hardcode nonce for address calculation
frangio Jul 8, 2023
d4f5535
remove owner argument to createProxy function
frangio Jul 8, 2023
6e48769
update docs
frangio Jul 8, 2023
0f479c6
Update create.js
Amxx Jul 8, 2023
853dc83
Update account.js
Amxx Jul 8, 2023
864be51
Update create.js
Amxx Jul 8, 2023
9349f30
Update create.js
Amxx Jul 8, 2023
cd1ac92
Update GovernorTimelockCompound.test.js
Amxx Jul 8, 2023
c4d49a4
fix lint
Amxx Jul 8, 2023
6e95dac
change proxyadmin test descriptions
frangio Jul 8, 2023
4d3bb2f
use constant string
frangio Jul 8, 2023
c37cab0
remove forceCall and upgradeTo/AndCall distinction
frangio Jul 8, 2023
5c77c03
remove unused return value from transparent proxy dispatch
frangio Jul 8, 2023
ff6a70e
lint
frangio Jul 8, 2023
223d840
remove unused import
frangio Jul 8, 2023
66a3ebf
remove unused error
frangio Jul 8, 2023
be31da2
Merge branch 'master' into proxy-admin-optimizations
frangio Jul 9, 2023
802def2
Moaaar review
ernestognw Jul 9, 2023
a22e182
don't pass empty opts to constructor removes the need for {from:...} …
Amxx Jul 9, 2023
dc040f1
Improve docs and apply suggestions
ernestognw Jul 12, 2023
4b9ec42
Improve changeset
ernestognw Jul 12, 2023
22698a6
Remove unnecessary slohint annotations
ernestognw Jul 13, 2023
60123c7
Merge branch 'master' into proxy-admin-optimizations
ernestognw Jul 13, 2023
9f18301
Add ERC1967Utils tests
ernestognw Jul 13, 2023
75ea1de
Add UpgradeableBeaconMock
ernestognw Jul 13, 2023
08ad9b5
remove unused import
frangio Jul 13, 2023
7e5c33b
Use events for checking delegatecalls
ernestognw Jul 13, 2023
703659e
Use events for checking delegatecalls
ernestognw Jul 13, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 7 additions & 15 deletions contracts/proxy/transparent/ProxyAdmin.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,33 +3,25 @@

pragma solidity ^0.8.19;

import {ITransparentUpgradeableProxy, TransparentUpgradeableProxy} from "./TransparentUpgradeableProxy.sol";
import {ITransparentUpgradeableProxy} from "./TransparentUpgradeableProxy.sol";
import {Ownable} from "../../access/Ownable.sol";
import {Address} from "../../utils/Address.sol";

/**
* @dev This is an auxiliary contract meant to be assigned as the admin of a {TransparentUpgradeableProxy}. For an
* explanation of why you would want to use this see the documentation for {TransparentUpgradeableProxy}.
*/
contract ProxyAdmin is Ownable {
uint256 public immutable PROXY_ADMIN_VERSION = 2;
frangio marked this conversation as resolved.
Show resolved Hide resolved

/**
* @dev Sets the initial owner who can perform upgrades.
*/
constructor(address initialOwner) Ownable(initialOwner) {}

/**
* @dev Upgrades `proxy` to `implementation`. See {TransparentUpgradeableProxy-upgradeTo}.
*
* Requirements:
*
* - This contract must be the admin of `proxy`.
*/
function upgrade(ITransparentUpgradeableProxy proxy, address implementation) public virtual onlyOwner {
proxy.upgradeTo(implementation);
}

/**
* @dev Upgrades `proxy` to `implementation` and calls a function on the new implementation. See
* {TransparentUpgradeableProxy-upgradeToAndCall}.
* @dev Upgrades `proxy` to `implementation` and calls a function on the new implementation.
* See {TransparentUpgradeableProxy-_dispatchUpgradeToAndCall}.
*
* Requirements:
*
Expand All @@ -40,6 +32,6 @@ contract ProxyAdmin is Ownable {
address implementation,
bytes memory data
) public payable virtual onlyOwner {
proxy.upgradeToAndCall{value: msg.value}(implementation, data);
proxy.upgradeToAndCall{ value: msg.value }(implementation, data);
}
}
47 changes: 16 additions & 31 deletions contracts/proxy/transparent/TransparentUpgradeableProxy.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,17 +6,16 @@ pragma solidity ^0.8.19;
import {ERC1967Utils} from "../ERC1967/ERC1967Utils.sol";
import {ERC1967Proxy} from "../ERC1967/ERC1967Proxy.sol";
import {IERC1967} from "../../interfaces/IERC1967.sol";
import {ProxyAdmin} from "./ProxyAdmin.sol";

/**
* @dev Interface for {TransparentUpgradeableProxy}. In order to implement transparency, {TransparentUpgradeableProxy}
* does not implement this interface directly, and some of its functions are implemented by an internal dispatch
* does not implement this interface directly, and its upgradeability mechanism is implemented by an internal dispatch
* mechanism. The compiler is unaware that these functions are implemented by {TransparentUpgradeableProxy} and will not
* include them in the ABI so this interface must be used to interact with it.
*/
interface ITransparentUpgradeableProxy is IERC1967 {
function upgradeTo(address) external;

function upgradeToAndCall(address, bytes memory) external payable;
function upgradeToAndCall(address, bytes calldata) external payable;
}

/**
Expand All @@ -28,8 +27,8 @@ interface ITransparentUpgradeableProxy is IERC1967 {
* things that go hand in hand:
*
* 1. If any account other than the admin calls the proxy, the call will be forwarded to the implementation, even if
* that call matches one of the admin functions exposed by the proxy itself.
* 2. If the admin calls the proxy, it can access the admin functions, but its calls will never be forwarded to the
* that call matches the {ITransparentUpgradeableProxy-upgradeTo} function exposed by the proxy itself.
* 2. If the admin calls the proxy, it can call the `upgradeTo` function but any other call won't be forwarded to the
* implementation. If the admin tries to call a function on the implementation it will fail with an error indicating the
* proxy admin cannot fallback to the target implementation.
*
Expand All @@ -42,9 +41,9 @@ interface ITransparentUpgradeableProxy is IERC1967 {
* {Ownable} contract to allow for changing the proxy's admin owner.
*
* NOTE: The real interface of this proxy is that defined in `ITransparentUpgradeableProxy`. This contract does not
* inherit from that interface, and instead the admin functions are implicitly implemented using a custom dispatch
* mechanism in `_fallback`. Consequently, the compiler will not produce an ABI for this contract. This is necessary to
* fully implement transparency without decoding reverts caused by selector clashes between the proxy and the
* inherit from that interface, and instead the `upgradeTo` is implicitly implemented using a custom dispatch mechanism
* in `_fallback`. Consequently, the compiler will not produce an ABI for this contract. This is necessary to fully
* implement transparency without decoding reverts caused by selector clashes between the proxy and the
* implementation.
*
* IMPORTANT: This contract avoids unnecessary storage reads by setting the admin only during construction as an immutable variable,
Expand All @@ -55,7 +54,7 @@ interface ITransparentUpgradeableProxy is IERC1967 {
* WARNING: It is not recommended to extend this contract to add additional external functions. If you do so, the compiler
* will not check that there are no selector conflicts, due to the note above. A selector clash between any new function
* and the functions declared in {ITransparentUpgradeableProxy} will be resolved in favor of the new one. This could
* render the admin operations inaccessible, which could prevent upgradeability. Transparency may also be compromised.
* render the `upgradeTo` function inaccessible, preventing upgradeability and compromising transparency.
*/
contract TransparentUpgradeableProxy is ERC1967Proxy {
// An immutable address for the admin avoid unnecessary SLOADs before each call
ernestognw marked this conversation as resolved.
Show resolved Hide resolved
Expand All @@ -78,10 +77,10 @@ contract TransparentUpgradeableProxy is ERC1967Proxy {
* @dev Initializes an upgradeable proxy managed by `_admin`, backed by the implementation at `_logic`, and
* optionally initialized with `_data` as explained in {ERC1967Proxy-constructor}.
ernestognw marked this conversation as resolved.
Show resolved Hide resolved
*/
constructor(address _logic, address admin_, bytes memory _data) payable ERC1967Proxy(_logic, _data) {
_admin = admin_;
constructor(address _logic, address initialOwner, bytes memory _data) payable ERC1967Proxy(_logic, _data) {
Copy link

Choose a reason for hiding this comment

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

Hey i just stumbled over this change when reviewing v5 and was wondering if it wouldn't make sense to have an overload with ProxyAdmin _admin instead of address initialOwner.

This way one could easily reuse a single ProxyAdmin for multiple contracts.

Copy link
Collaborator

Choose a reason for hiding this comment

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

This way one could easily reuse a single ProxyAdmin for multiple contracts.

This used ti be the case in v4. We decided to change that.

In v4, the admin was stored in storage, which means that any call going trough the proxy would have to load that value, costing 2100 gas.

function _fallback() internal virtual override {
        if (msg.sender == _admin) {
            ...

We wanted to reduce that gas cost, and so we decided to make the admin immutable. That means that it is not possible for a proxy to change which admin it uses. You could change the value stored in the ERC-1967 slot, but that would have no effect.

That means that if multiple proxy use the same ProxyAdmin, then you can separate the ownership. You may be able to change who owns the ProxyAdmin, but every proxy that point to it will share the same owner, forever.

That is why we decided to make the TransparentUpgradeableProxy <> ProxyAdmin a 1-to-1 relationship.

Copy link
Member Author

Choose a reason for hiding this comment

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

Agree with @Amxx.

We agreed that it's better to reduce the SLOAD cost in the proxy than allowing to reuse ProxyAdmins. The tradeoff is fine because this not only increases deployment costs but reduces every proxy interaction cost. Consider regular proxy interactions are more frequent (potentially millions of times) than admin interactions.

Copy link

Choose a reason for hiding this comment

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

I just stumbled upon this while deploying a proxy using v5. Being used to the v4 behavior, I wrongly passed in a proxy admin making the deployment unusable. As far as I could tell, this change is not documented in any changelog. I think that it would be good to add this to the v5 changelog.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks for sharing @danhper! The change is documented on both OZ upgrades plugins and OZ Contracts (see Proxy section). Though I know it's counterintuitive, we strongly advice users to be careful about major releases.

We'd be happy to hear suggestions on how to improve the way we communicate these changes.

_admin = address(new ProxyAdmin(initialOwner));
// Set the storage value and emit an event for ERC-1967 compatibility
ERC1967Utils.changeAdmin(admin_);
ERC1967Utils.changeAdmin(_admin);
}

/**
Expand All @@ -91,9 +90,7 @@ contract TransparentUpgradeableProxy is ERC1967Proxy {
if (msg.sender == _admin) {
bytes memory ret;
bytes4 selector = msg.sig;
if (selector == ITransparentUpgradeableProxy.upgradeTo.selector) {
ret = _dispatchUpgradeTo();
} else if (selector == ITransparentUpgradeableProxy.upgradeToAndCall.selector) {
if (selector == ITransparentUpgradeableProxy.upgradeToAndCall.selector) {
ret = _dispatchUpgradeToAndCall();
} else {
revert ProxyDeniedAdminAccess();
Expand All @@ -109,23 +106,11 @@ contract TransparentUpgradeableProxy is ERC1967Proxy {
/**
* @dev Upgrade the implementation of the proxy.
*/
function _dispatchUpgradeTo() private returns (bytes memory) {
_requireZeroValue();

address newImplementation = abi.decode(msg.data[4:], (address));
ERC1967Utils.upgradeToAndCall(newImplementation, bytes(""), false);

return "";
}

/**
* @dev Upgrade the implementation of the proxy, and then call a function from the new implementation as specified
* by `data`, which should be an encoded function call. This is useful to initialize new storage variables in the
* proxied contract.
*/
function _dispatchUpgradeToAndCall() private returns (bytes memory) {
bool forceCall = msg.value > 0;
frangio marked this conversation as resolved.
Show resolved Hide resolved

(address newImplementation, bytes memory data) = abi.decode(msg.data[4:], (address, bytes));
ERC1967Utils.upgradeToAndCall(newImplementation, data, true);
ERC1967Utils.upgradeToAndCall(newImplementation, data, forceCall);

return "";
}
Expand Down
2 changes: 1 addition & 1 deletion contracts/proxy/utils/UUPSUpgradeable.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -109,7 +109,7 @@ abstract contract UUPSUpgradeable is IERC1822Proxiable {
* Normally, this function will use an xref:access.adoc[access control] modifier such as {Ownable-onlyOwner}.
*
* ```solidity
* function _authorizeUpgrade(address) internal onlyOwner {}
* function _authorizeUpgrade(address) internal onlyOwner {}
* ```
*/
function _authorizeUpgrade(address newImplementation) internal virtual;
Expand Down
2 changes: 1 addition & 1 deletion test/proxy/ERC1967/ERC1967Proxy.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,5 +9,5 @@ contract('ERC1967Proxy', function (accounts) {
return ERC1967Proxy.new(implementation, initData, opts);
frangio marked this conversation as resolved.
Show resolved Hide resolved
};

shouldBehaveLikeProxy(createProxy, undefined, proxyAdminOwner);
shouldBehaveLikeProxy(createProxy, proxyAdminOwner);
});
26 changes: 13 additions & 13 deletions test/proxy/Proxy.behaviour.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,12 +5,12 @@ const { expect } = require('chai');

const DummyImplementation = artifacts.require('DummyImplementation');

module.exports = function shouldBehaveLikeProxy(createProxy, proxyAdminAddress, proxyCreator) {
module.exports = function shouldBehaveLikeProxy(createProxy, proxyCreator) {
it('cannot be initialized with a non-contract address', async function () {
const nonContractAddress = proxyCreator;
const initializeData = Buffer.from('');
await expectRevert.unspecified(
createProxy(nonContractAddress, proxyAdminAddress, initializeData, {
createProxy(nonContractAddress, initializeData, {
from: proxyCreator,
}),
);
Expand Down Expand Up @@ -43,7 +43,7 @@ module.exports = function shouldBehaveLikeProxy(createProxy, proxyAdminAddress,
describe('when not sending balance', function () {
beforeEach('creating proxy', async function () {
this.proxy = (
await createProxy(this.implementation, proxyAdminAddress, initializeData, {
await createProxy(this.implementation, initializeData, {
from: proxyCreator,
})
).address;
Expand All @@ -57,7 +57,7 @@ module.exports = function shouldBehaveLikeProxy(createProxy, proxyAdminAddress,

beforeEach('creating proxy', async function () {
this.proxy = (
await createProxy(this.implementation, proxyAdminAddress, initializeData, {
await createProxy(this.implementation, initializeData, {
from: proxyCreator,
value,
})
Expand All @@ -76,7 +76,7 @@ module.exports = function shouldBehaveLikeProxy(createProxy, proxyAdminAddress,
describe('when not sending balance', function () {
beforeEach('creating proxy', async function () {
this.proxy = (
await createProxy(this.implementation, proxyAdminAddress, initializeData, {
await createProxy(this.implementation, initializeData, {
from: proxyCreator,
})
).address;
Expand All @@ -93,7 +93,7 @@ module.exports = function shouldBehaveLikeProxy(createProxy, proxyAdminAddress,

it('reverts', async function () {
await expectRevert.unspecified(
createProxy(this.implementation, proxyAdminAddress, initializeData, { from: proxyCreator, value }),
createProxy(this.implementation, initializeData, { from: proxyCreator, value }),
);
});
});
Expand All @@ -106,7 +106,7 @@ module.exports = function shouldBehaveLikeProxy(createProxy, proxyAdminAddress,
describe('when not sending balance', function () {
beforeEach('creating proxy', async function () {
this.proxy = (
await createProxy(this.implementation, proxyAdminAddress, initializeData, {
await createProxy(this.implementation, initializeData, {
from: proxyCreator,
})
).address;
Expand All @@ -123,7 +123,7 @@ module.exports = function shouldBehaveLikeProxy(createProxy, proxyAdminAddress,

beforeEach('creating proxy', async function () {
this.proxy = (
await createProxy(this.implementation, proxyAdminAddress, initializeData, {
await createProxy(this.implementation, initializeData, {
from: proxyCreator,
value,
})
Expand All @@ -148,7 +148,7 @@ module.exports = function shouldBehaveLikeProxy(createProxy, proxyAdminAddress,
describe('when not sending balance', function () {
beforeEach('creating proxy', async function () {
this.proxy = (
await createProxy(this.implementation, proxyAdminAddress, initializeData, {
await createProxy(this.implementation, initializeData, {
from: proxyCreator,
})
).address;
Expand All @@ -165,7 +165,7 @@ module.exports = function shouldBehaveLikeProxy(createProxy, proxyAdminAddress,

it('reverts', async function () {
await expectRevert.unspecified(
createProxy(this.implementation, proxyAdminAddress, initializeData, { from: proxyCreator, value }),
createProxy(this.implementation, initializeData, { from: proxyCreator, value }),
);
});
});
Expand All @@ -180,7 +180,7 @@ module.exports = function shouldBehaveLikeProxy(createProxy, proxyAdminAddress,
describe('when not sending balance', function () {
beforeEach('creating proxy', async function () {
this.proxy = (
await createProxy(this.implementation, proxyAdminAddress, initializeData, {
await createProxy(this.implementation, initializeData, {
from: proxyCreator,
})
).address;
Expand All @@ -197,7 +197,7 @@ module.exports = function shouldBehaveLikeProxy(createProxy, proxyAdminAddress,

beforeEach('creating proxy', async function () {
this.proxy = (
await createProxy(this.implementation, proxyAdminAddress, initializeData, {
await createProxy(this.implementation, initializeData, {
from: proxyCreator,
value,
})
Expand All @@ -216,7 +216,7 @@ module.exports = function shouldBehaveLikeProxy(createProxy, proxyAdminAddress,

it('reverts', async function () {
await expectRevert(
createProxy(this.implementation, proxyAdminAddress, initializeData, { from: proxyCreator }),
createProxy(this.implementation, initializeData, { from: proxyCreator }),
'DummyImplementation reverted',
);
});
Expand Down
21 changes: 0 additions & 21 deletions test/proxy/transparent/ProxyAdmin.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,27 +32,6 @@ contract('ProxyAdmin', function (accounts) {
expect(await this.proxyAdmin.owner()).to.equal(proxyAdminOwner);
});

describe('#upgrade', function () {
context('with unauthorized account', function () {
it('fails to upgrade', async function () {
await expectRevertCustomError(
this.proxyAdmin.upgrade(this.proxy.address, this.implementationV2.address, { from: anotherAccount }),
'OwnableUnauthorizedAccount',
[anotherAccount],
);
});
});

context('with authorized account', function () {
it('upgrades implementation', async function () {
await this.proxyAdmin.upgrade(this.proxy.address, this.implementationV2.address, { from: proxyAdminOwner });

const implementationAddress = await getAddressInSlot(this.proxy, ImplementationSlot);
expect(implementationAddress).to.be.equal(this.implementationV2.address);
});
});
});

describe('#upgradeAndCall', function () {
context('with unauthorized account', function () {
it('fails to upgrade', async function () {
Expand Down
Loading
Loading