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

feat: upgrade transparent proxy #48

Merged
merged 7 commits into from
Nov 18, 2024
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
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
38 changes: 0 additions & 38 deletions src/contracts/transparent-proxy/ERC1967Proxy.sol

This file was deleted.

119 changes: 0 additions & 119 deletions src/contracts/transparent-proxy/ERC1967Upgrade.sol

This file was deleted.

85 changes: 17 additions & 68 deletions src/contracts/transparent-proxy/ProxyAdmin.sol
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe we should just import from oz directly? As we have 0 diff atm

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This:
import {ITransparentUpgradeableProxy} from './TransparentUpgradeableProxy.sol'; is the diff.

I don't see a way without copying.

Original file line number Diff line number Diff line change
@@ -1,93 +1,42 @@
// SPDX-License-Identifier: MIT
// OpenZeppelin Contracts (last updated v5.0.0) (proxy/transparent/ProxyAdmin.sol)

/**
* @dev OpenZeppelin Contracts v4.4.1 (proxy/transparent/ProxyAdmin.sol)
* From https://github.com/OpenZeppelin/openzeppelin-contracts/tree/8b778fa20d6d76340c5fac1ed66c80273f05b95a
*
* BGD Labs adaptations:
* - Linting
*/
pragma solidity ^0.8.20;

pragma solidity ^0.8.0;

import './TransparentUpgradeableProxy.sol';
import '../oz-common/Ownable.sol';
import {ITransparentUpgradeableProxy} from './TransparentUpgradeableProxy.sol';
import {Ownable} from 'openzeppelin-contracts/contracts/access/Ownable.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 {
/**
* @dev Returns the current implementation of `proxy`.
*
* Requirements:
*
* - This contract must be the admin of `proxy`.
*/
function getProxyImplementation(
TransparentUpgradeableProxy proxy
) public view virtual returns (address) {
// We need to manually run the static call since the getter cannot be flagged as view
// bytes4(keccak256("implementation()")) == 0x5c60da1b
(bool success, bytes memory returndata) = address(proxy).staticcall(hex'5c60da1b');
require(success);
return abi.decode(returndata, (address));
}

/**
* @dev Returns the current admin of `proxy`.
*
* Requirements:
*
* - This contract must be the admin of `proxy`.
*/
function getProxyAdmin(TransparentUpgradeableProxy proxy) public view virtual returns (address) {
// We need to manually run the static call since the getter cannot be flagged as view
// bytes4(keccak256("admin()")) == 0xf851a440
(bool success, bytes memory returndata) = address(proxy).staticcall(hex'f851a440');
require(success);
return abi.decode(returndata, (address));
}

/**
* @dev Changes the admin of `proxy` to `newAdmin`.
*
* Requirements:
*
* - This contract must be the current admin of `proxy`.
* @dev The version of the upgrade interface of the contract. If this getter is missing, both `upgrade(address)`
* and `upgradeAndCall(address,bytes)` are present, and `upgradeTo` must be used if no function should be called,
* while `upgradeAndCall` will invoke the `receive` function if the second argument is the empty byte string.
* If the getter returns `"5.0.0"`, only `upgradeAndCall(address,bytes)` is present, and the second argument must
* be the empty byte string if no function should be called, making it impossible to invoke the `receive` function
* during an upgrade.
*/
function changeProxyAdmin(
TransparentUpgradeableProxy proxy,
address newAdmin
) public virtual onlyOwner {
proxy.changeAdmin(newAdmin);
}
string public constant UPGRADE_INTERFACE_VERSION = '5.0.0';

/**
* @dev Upgrades `proxy` to `implementation`. See {TransparentUpgradeableProxy-upgradeTo}.
*
* Requirements:
*
* - This contract must be the admin of `proxy`.
* @dev Sets the initial owner who can perform upgrades.
*/
function upgrade(
TransparentUpgradeableProxy proxy,
address implementation
) public virtual onlyOwner {
proxy.upgradeTo(implementation);
}
constructor(address initialOwner) Ownable(initialOwner) {}

/**
* @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:
*
* - This contract must be the admin of `proxy`.
* - If `data` is empty, `msg.value` must be zero.
*/
function upgradeAndCall(
TransparentUpgradeableProxy proxy,
ITransparentUpgradeableProxy proxy,
address implementation,
bytes memory data
) public payable virtual onlyOwner {
Expand Down
43 changes: 23 additions & 20 deletions src/contracts/transparent-proxy/TransparentProxyFactoryBase.sol
Copy link
Contributor

Choose a reason for hiding this comment

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

Following the pattern, similiar to ProxyAdmin, maybe we can ditch IOwnable, it's not used anywhere outside I think, and use full Ownable

Copy link
Contributor Author

Choose a reason for hiding this comment

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

kk removed. As we have oz as a dependency, we should probably also remove most things from oz-common, but will defer to another pr.

Original file line number Diff line number Diff line change
Expand Up @@ -16,21 +16,16 @@ import {ProxyAdmin} from './ProxyAdmin.sol';
**/
abstract contract TransparentProxyFactoryBase is ITransparentProxyFactory {
/// @inheritdoc ITransparentProxyFactory
function create(
address logic,
address admin,
bytes calldata data
) external returns (address) {
function create(address logic, ProxyAdmin admin, bytes calldata data) external returns (address) {
Copy link
Contributor

Choose a reason for hiding this comment

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

why do we use ProxyAdmin here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it's a bit debatable, that's why i asked for careful review 😅

My thinking was that if passing a non ProxyAdmin here would potentiall break things. It's simply assumed to pass a proxy admin. While an interface ofc is no perfect enforcement, the intention is to at least highlight what should be passed.

address proxy = address(new TransparentUpgradeableProxy(logic, admin, data));

emit ProxyCreated(proxy, logic, admin);
emit ProxyCreated(proxy, logic, address(admin));
return proxy;
}

/// @inheritdoc ITransparentProxyFactory
function createProxyAdmin(address adminOwner) external returns (address) {
address proxyAdmin = address(new ProxyAdmin());
IOwnable(proxyAdmin).transferOwnership(adminOwner);
address proxyAdmin = address(new ProxyAdmin(adminOwner));
Copy link
Contributor

Choose a reason for hiding this comment

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

and here return it as address, not ProxyAdmin

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Here i simply did not change anything as nothing really changed.
From a user perspective, i usually find it easier to receive an address and cast to the interface i need.


emit ProxyAdminCreated(proxyAdmin, adminOwner);
return proxyAdmin;
Expand All @@ -39,23 +34,22 @@ abstract contract TransparentProxyFactoryBase is ITransparentProxyFactory {
/// @inheritdoc ITransparentProxyFactory
function createDeterministic(
address logic,
address admin,
ProxyAdmin admin,
bytes calldata data,
bytes32 salt
) external returns (address) {
address proxy = address(new TransparentUpgradeableProxy{salt: salt}(logic, admin, data));

emit ProxyDeterministicCreated(proxy, logic, admin, salt);
emit ProxyDeterministicCreated(proxy, logic, address(admin), salt);
return proxy;
}

/// @inheritdoc ITransparentProxyFactory
function createDeterministicProxyAdmin(address adminOwner, bytes32 salt)
external
returns (address)
{
address proxyAdmin = address(new ProxyAdmin{salt: salt}());
IOwnable(proxyAdmin).transferOwnership(adminOwner);
function createDeterministicProxyAdmin(
address adminOwner,
bytes32 salt
) external returns (address) {
address proxyAdmin = address(new ProxyAdmin{salt: salt}(adminOwner));
Copy link
Contributor

Choose a reason for hiding this comment

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

and here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

same here, did not change as imo doesn't help in this case


emit ProxyAdminDeterministicCreated(proxyAdmin, adminOwner, salt);
return proxyAdmin;
Expand All @@ -64,7 +58,7 @@ abstract contract TransparentProxyFactoryBase is ITransparentProxyFactory {
/// @inheritdoc ITransparentProxyFactory
function predictCreateDeterministic(
address logic,
address admin,
ProxyAdmin admin,
bytes calldata data,
bytes32 salt
) public view returns (address) {
Expand All @@ -73,13 +67,22 @@ abstract contract TransparentProxyFactoryBase is ITransparentProxyFactory {
address(this),
salt,
type(TransparentUpgradeableProxy).creationCode,
abi.encode(logic, admin, data)
abi.encode(logic, address(admin), data)
);
}

/// @inheritdoc ITransparentProxyFactory
function predictCreateDeterministicProxyAdmin(bytes32 salt) public view returns (address) {
return _predictCreate2Address(address(this), salt, type(ProxyAdmin).creationCode, abi.encode());
function predictCreateDeterministicProxyAdmin(
bytes32 salt,
address initialOwner
) public view returns (address) {
return
_predictCreate2Address(
address(this),
salt,
type(ProxyAdmin).creationCode,
abi.encode(initialOwner)
);
}

function _predictCreate2Address(
Expand Down
Loading