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

Remove admin and implementation getters from TransparentUpgradeableProxy #3820

Merged
merged 15 commits into from
Nov 25, 2022
Merged

Remove admin and implementation getters from TransparentUpgradeableProxy #3820

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
15 commits
Select commit Hold shift + click to select a range
5bc5eab
Remove admin and implementation getters
JulissaDantes Nov 16, 2022
07920a3
create changelog entry
JulissaDantes Nov 17, 2022
0f238ea
Include PR number
JulissaDantes Nov 17, 2022
9032c18
Update test
JulissaDantes Nov 17, 2022
7b08af9
Update test
JulissaDantes Nov 17, 2022
3974b81
Update CHANGELOG.md
JulissaDantes Nov 25, 2022
1da2602
Update ProxyAdmin tests
JulissaDantes Nov 25, 2022
6f773c7
Use helper function
JulissaDantes Nov 25, 2022
55527a0
Update documentation
JulissaDantes Nov 25, 2022
69b9662
remove extra space
frangio Nov 25, 2022
a1a636b
remove extra space
frangio Nov 25, 2022
76291fe
Add helper
JulissaDantes Nov 25, 2022
ae2e0e8
Merge branch 'remove-ifAdmin' of https://github.com/JulissaDantes/ope…
JulissaDantes Nov 25, 2022
46f866a
rename variable
frangio Nov 25, 2022
c2751e7
Merge branch 'next-v5.0' into remove-ifAdmin
frangio Nov 25, 2022
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
1 change: 1 addition & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
## Unreleased (Breaking)

* `TimelockController`: Changed the role architecture to use `DEFAULT_ADMIN_ROLE` as the admin for all roles, instead of the bespoke `TIMELOCK_ADMIN_ROLE` that was used previously. This aligns with the general recommendation for `AccessControl` and makes the addition of new roles easier. Accordingly, the `admin` parameter and timelock will now be granted `DEFAULT_ADMIN_ROLE` instead of `TIMELOCK_ADMIN_ROLE`. ([#3799](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/3799))
* `TransparentUpgradeableProxy`: The `admin()` and `implementation()` getters were removed, this impacts the `ProxyAdmin` contract as well, removing the `getProxyAdmin` and `getProxyImplementation` getters. ([#3820](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/3820))
JulissaDantes marked this conversation as resolved.
Show resolved Hide resolved

## Unreleased

Expand Down
30 changes: 0 additions & 30 deletions contracts/proxy/transparent/ProxyAdmin.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,36 +11,6 @@ import "../../access/Ownable.sol";
* 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`.
*
Expand Down
26 changes: 0 additions & 26 deletions contracts/proxy/transparent/TransparentUpgradeableProxy.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,32 +50,6 @@ contract TransparentUpgradeableProxy is ERC1967Proxy {
}
}

/**
* @dev Returns the current admin.
*
* NOTE: Only the admin can call this function. See {ProxyAdmin-getProxyAdmin}.
*
* TIP: To get this value clients can read directly from the storage slot shown below (specified by EIP1967) using the
* https://eth.wiki/json-rpc/API#eth_getstorageat[`eth_getStorageAt`] RPC call.
* `0xb53127684a568b3173ae13b9f8a6016e243e63b6e8ee1178d6a717850b5d6103`
JulissaDantes marked this conversation as resolved.
Show resolved Hide resolved
*/
function admin() external ifAdmin returns (address admin_) {
admin_ = _getAdmin();
}

/**
* @dev Returns the current implementation.
*
* NOTE: Only the admin can call this function. See {ProxyAdmin-getProxyImplementation}.
*
* TIP: To get this value clients can read directly from the storage slot shown below (specified by EIP1967) using the
* https://eth.wiki/json-rpc/API#eth_getstorageat[`eth_getStorageAt`] RPC call.
* `0x360894a13ba1a3210667c828492db98dca3e2076cc3735a920a3ca505d382bbc`
JulissaDantes marked this conversation as resolved.
Show resolved Hide resolved
*/
function implementation() external ifAdmin returns (address implementation_) {
implementation_ = _implementation();
}

/**
* @dev Changes the admin of the proxy.
*
Expand Down
33 changes: 6 additions & 27 deletions test/proxy/transparent/ProxyAdmin.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,17 +30,6 @@ contract('ProxyAdmin', function (accounts) {
expect(await this.proxyAdmin.owner()).to.equal(proxyAdminOwner);
});

describe('#getProxyAdmin', function () {
it('returns proxyAdmin as admin of the proxy', async function () {
const admin = await this.proxyAdmin.getProxyAdmin(this.proxy.address);
expect(admin).to.be.equal(this.proxyAdmin.address);
});

it('call to invalid proxy', async function () {
await expectRevert.unspecified(this.proxyAdmin.getProxyAdmin(this.implementationV1.address));
});
});

describe('#changeProxyAdmin', function () {
it('fails to change proxy admin if its not the proxy owner', async function () {
await expectRevert(
Expand All @@ -51,18 +40,8 @@ contract('ProxyAdmin', function (accounts) {

it('changes proxy admin', async function () {
await this.proxyAdmin.changeProxyAdmin(this.proxy.address, newAdmin, { from: proxyAdminOwner });
expect(await this.proxy.admin.call({ from: newAdmin })).to.eq(newAdmin);
});
});

describe('#getProxyImplementation', function () {
it('returns proxy implementation address', async function () {
const implementationAddress = await this.proxyAdmin.getProxyImplementation(this.proxy.address);
expect(implementationAddress).to.be.equal(this.implementationV1.address);
});

it('call to invalid proxy', async function () {
await expectRevert.unspecified(this.proxyAdmin.getProxyImplementation(this.implementationV1.address));
// Allows new admin to call ifAdmin functions
await this.proxy.changeAdmin(newAdmin, { from: newAdmin });// expect to not revert
JulissaDantes marked this conversation as resolved.
Show resolved Hide resolved
});
});

Expand All @@ -79,8 +58,8 @@ contract('ProxyAdmin', function (accounts) {
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 this.proxyAdmin.getProxyImplementation(this.proxy.address);
expect(implementationAddress).to.be.equal(this.implementationV2.address);
const dummy = await new ImplV2(this.proxy.address);
expect(await dummy.version()).to.be.equals('V2');
JulissaDantes marked this conversation as resolved.
Show resolved Hide resolved
});
});
});
Expand Down Expand Up @@ -116,8 +95,8 @@ contract('ProxyAdmin', function (accounts) {
await this.proxyAdmin.upgradeAndCall(this.proxy.address, this.implementationV2.address, callData,
{ from: proxyAdminOwner },
);
const implementationAddress = await this.proxyAdmin.getProxyImplementation(this.proxy.address);
expect(implementationAddress).to.be.equal(this.implementationV2.address);
const dummy = await new ImplV2(this.proxy.address);
expect(await dummy.version()).to.be.equals('V2');
JulissaDantes marked this conversation as resolved.
Show resolved Hide resolved
});
});
});
Expand Down
53 changes: 0 additions & 53 deletions test/proxy/transparent/TransparentUpgradeableProxy.behaviour.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,12 +33,6 @@ module.exports = function shouldBehaveLikeTransparentUpgradeableProxy (createPro
});

describe('implementation', function () {
it('returns the current implementation address', async function () {
const implementation = await this.proxy.implementation.call({ from: proxyAdminAddress });

expect(implementation).to.be.equal(this.implementationV0);
});

it('delegates to the implementation', async function () {
const dummy = new DummyImplementation(this.proxyAddress);
const value = await dummy.get();
Expand All @@ -52,13 +46,6 @@ module.exports = function shouldBehaveLikeTransparentUpgradeableProxy (createPro
const from = proxyAdminAddress;

describe('when the given implementation is different from the current one', function () {
it('upgrades to the requested implementation', async function () {
await this.proxy.upgradeTo(this.implementationV1, { from });

const implementation = await this.proxy.implementation.call({ from: proxyAdminAddress });
expect(implementation).to.be.equal(this.implementationV1);
});

it('emits an event', async function () {
expectEvent(
await this.proxy.upgradeTo(this.implementationV1, { from }),
Expand Down Expand Up @@ -107,11 +94,6 @@ module.exports = function shouldBehaveLikeTransparentUpgradeableProxy (createPro
this.receipt = await this.proxy.upgradeToAndCall(this.behavior.address, initializeData, { from, value });
});

it('upgrades to the requested implementation', async function () {
JulissaDantes marked this conversation as resolved.
Show resolved Hide resolved
const implementation = await this.proxy.implementation.call({ from: proxyAdminAddress });
expect(implementation).to.be.equal(this.behavior.address);
});

it('emits an event', function () {
expectEvent(this.receipt, 'Upgraded', { implementation: this.behavior.address });
});
Expand Down Expand Up @@ -172,12 +154,6 @@ module.exports = function shouldBehaveLikeTransparentUpgradeableProxy (createPro
this.receipt = await this.proxy.upgradeToAndCall(this.behaviorV1.address, v1MigrationData, { from, value });
});

it('upgrades to the requested version and emits an event', async function () {
const implementation = await this.proxy.implementation.call({ from: proxyAdminAddress });
expect(implementation).to.be.equal(this.behaviorV1.address);
expectEvent(this.receipt, 'Upgraded', { implementation: this.behaviorV1.address });
});

it('calls the \'initialize\' function and sends given value to the proxy', async function () {
const migratable = new MigratableMockV1(this.proxyAddress);

Expand All @@ -198,12 +174,6 @@ module.exports = function shouldBehaveLikeTransparentUpgradeableProxy (createPro
await this.proxy.upgradeToAndCall(this.behaviorV2.address, v2MigrationData, { from, value });
});

it('upgrades to the requested version and emits an event', async function () {
const implementation = await this.proxy.implementation.call({ from: proxyAdminAddress });
expect(implementation).to.be.equal(this.behaviorV2.address);
expectEvent(this.receipt, 'Upgraded', { implementation: this.behaviorV2.address });
});

it('calls the \'migrate\' function and sends given value to the proxy', async function () {
const migratable = new MigratableMockV2(this.proxyAddress);

Expand All @@ -227,12 +197,6 @@ module.exports = function shouldBehaveLikeTransparentUpgradeableProxy (createPro
await this.proxy.upgradeToAndCall(this.behaviorV3.address, v3MigrationData, { from, value });
});

it('upgrades to the requested version and emits an event', async function () {
const implementation = await this.proxy.implementation.call({ from: proxyAdminAddress });
expect(implementation).to.be.equal(this.behaviorV3.address);
expectEvent(this.receipt, 'Upgraded', { implementation: this.behaviorV3.address });
});

it('calls the \'migrate\' function and sends given value to the proxy', async function () {
const migratable = new MigratableMockV3(this.proxyAddress);

Expand Down Expand Up @@ -273,11 +237,6 @@ module.exports = function shouldBehaveLikeTransparentUpgradeableProxy (createPro
this.receipt = await this.proxy.changeAdmin(newAdmin, { from: proxyAdminAddress });
});

it('assigns new proxy admin', async function () {
const newProxyAdmin = await this.proxy.admin.call({ from: newAdmin });
expect(newProxyAdmin).to.be.equal(anotherAccount);
});

it('emits an event', function () {
expectEvent(this.receipt, 'AdminChanged', {
previousAdmin: proxyAdminAddress,
Expand Down Expand Up @@ -332,18 +291,6 @@ module.exports = function shouldBehaveLikeTransparentUpgradeableProxy (createPro
'TransparentUpgradeableProxy: admin cannot fallback to proxy target',
);
});

context('when function names clash', function () {
it('when sender is proxy admin should run the proxy function', async function () {
const value = await this.proxy.admin.call({ from: proxyAdminAddress });
expect(value).to.be.equal(proxyAdminAddress);
});

it('when sender is other should delegate to implementation', async function () {
const value = await this.proxy.admin.call({ from: anotherAccount });
expect(value).to.be.equal('0x0000000000000000000000000000000011111142');
});
});
});

describe('regression', () => {
Expand Down