address comments from PR

contracts/proxy/ERC1822/IProxiable.sol → contracts/interfaces/draft-IERC1822.sol

@@ -3,6 +3,6 @@
 
 pragma solidity ^0.8.0;
 
-interface IProxiable {
+interface IERC1822Proxiable {
     function proxiableUUID() external view returns (bytes32);
 }

contracts/proxy/ERC1967/ERC1967Upgrade.sol

@@ -4,7 +4,7 @@
 pragma solidity ^0.8.2;
 
 import "../beacon/IBeacon.sol";
-import "../ERC1822/IProxiable.sol";
+import "../../interfaces/draft-IERC1822.sol";
 import "../../utils/Address.sol";
 import "../../utils/StorageSlot.sol";
 
@@ -83,11 +83,14 @@ abstract contract ERC1967Upgrade {
         bytes memory data,
         bool forceCall
     ) internal {
+        // Upgrades from old implementations will perform a rollback test. This test requires this
+        // new implementation to upgrade to an old, non-ERC1822 compliant, implementation. Removing
+        // this special case will break upgrade paths from old UUPS implementation to new ones.
         if (StorageSlot.getBooleanSlot(_ROLLBACK_SLOT).value) {
             _setImplementation(newImplementation);
         } else {
             require(
-                IProxiable(newImplementation).proxiableUUID() == _IMPLEMENTATION_SLOT,
+                IERC1822Proxiable(newImplementation).proxiableUUID() == _IMPLEMENTATION_SLOT,
                 "Invalid proxiableUUID value"
             );
             _upgradeToAndCall(newImplementation, data, forceCall);

contracts/proxy/utils/UUPSUpgradeable.sol

@@ -3,7 +3,7 @@
 
 pragma solidity ^0.8.0;
 
-import "../ERC1822/IProxiable.sol";
+import "../../interfaces/draft-IERC1822.sol";
 import "../ERC1967/ERC1967Upgrade.sol";
 
 /**
@@ -18,7 +18,7 @@ import "../ERC1967/ERC1967Upgrade.sol";
  *
  * _Available since v4.1._
  */
-abstract contract UUPSUpgradeable is IProxiable, ERC1967Upgrade {
+abstract contract UUPSUpgradeable is IERC1822Proxiable, ERC1967Upgrade {
     /// @custom:oz-upgrades-unsafe-allow state-variable-immutable state-variable-assignment
     address private immutable __self = address(this);
 

test/helpers/erc1967.js

@@ -0,0 +1,24 @@
+const { BN } = require('@openzeppelin/test-helpers');
+const ethereumjsUtil = require('ethereumjs-util');
+
+const ImplementationLabel = 'eip1967.proxy.implementation';
+const AdminLabel = 'eip1967.proxy.admin';
+const BeaconLabel = 'eip1967.proxy.beacon';
+
+function labelToSlot (label) {
+  return '0x' + new BN(ethereumjsUtil.keccak256(Buffer.from(label))).subn(1).toString(16);
+}
+
+function toChecksumAddress (address) {
+  return ethereumjsUtil.toChecksumAddress('0x' + address.replace(/^0x/, '').padStart(40, '0'));
+}
+
+module.exports = {
+  ImplementationLabel,
+  AdminLabel,
+  BeaconLabel,
+  ImplementationSlot: labelToSlot(ImplementationLabel),
+  AdminSlot: labelToSlot(AdminLabel),
+  BeaconSlot: labelToSlot(BeaconLabel),
+  toChecksumAddress,
+};

test/proxy/Proxy.behaviour.js

@@ -1,16 +1,10 @@
-const { BN, expectRevert } = require('@openzeppelin/test-helpers');
-const ethereumjsUtil = require('ethereumjs-util');
+const { expectRevert } = require('@openzeppelin/test-helpers');
+const { ImplementationSlot, toChecksumAddress } = require('../helpers/erc1967');
 
 const { expect } = require('chai');
 
 const DummyImplementation = artifacts.require('DummyImplementation');
 
-const IMPLEMENTATION_LABEL = 'eip1967.proxy.implementation';
-
-function toChecksumAddress (address) {
-  return ethereumjsUtil.toChecksumAddress('0x' + address.replace(/^0x/, '').padStart(40, '0'));
-}
-
 module.exports = function shouldBehaveLikeProxy (createProxy, proxyAdminAddress, proxyCreator) {
   it('cannot be initialized with a non-contract address', async function () {
     const nonContractAddress = proxyCreator;
@@ -23,14 +17,14 @@ module.exports = function shouldBehaveLikeProxy (createProxy, proxyAdminAddress,
   });
 
   before('deploy implementation', async function () {
-    this.implementation = web3.utils.toChecksumAddress((await DummyImplementation.new()).address);
+    this.implementation = toChecksumAddress((await DummyImplementation.new()).address);
   });
 
   const assertProxyInitialization = function ({ value, balance }) {
     it('sets the implementation address', async function () {
-      const slot = '0x' + new BN(ethereumjsUtil.keccak256(Buffer.from(IMPLEMENTATION_LABEL))).subn(1).toString(16);
-      const implementation = toChecksumAddress((await web3.eth.getStorageAt(this.proxy, slot)).substr(-40));
-      expect(implementation).to.be.equal(this.implementation);
+      const implementationSlot = await web3.eth.getStorageAt(this.proxy, ImplementationSlot);
+      const implementationAddress = toChecksumAddress(implementationSlot.substr(-40));
+      expect(implementationAddress).to.be.equal(this.implementation);
     });
 
     it('initializes the proxy', async function () {

test/proxy/beacon/BeaconProxy.test.js

@@ -1,6 +1,6 @@
-const { BN, expectRevert } = require('@openzeppelin/test-helpers');
-const ethereumjsUtil = require('ethereumjs-util');
-const { keccak256 } = ethereumjsUtil;
+const { expectRevert } = require('@openzeppelin/test-helpers');
+const { keccak256 } = require('ethereumjs-util');
+const { BeaconSlot, toChecksumAddress } = require('../../helpers/erc1967');
 
 const { expect } = require('chai');
 
@@ -11,13 +11,6 @@ const DummyImplementationV2 = artifacts.require('DummyImplementationV2');
 const BadBeaconNoImpl = artifacts.require('BadBeaconNoImpl');
 const BadBeaconNotContract = artifacts.require('BadBeaconNotContract');
 
-function toChecksumAddress (address) {
-  return ethereumjsUtil.toChecksumAddress('0x' + address.replace(/^0x/, '').padStart(40, '0').substr(-40));
-}
-
-const BEACON_LABEL = 'eip1967.proxy.beacon';
-const BEACON_SLOT = '0x' + new BN(keccak256(Buffer.from(BEACON_LABEL))).subn(1).toString(16);
-
 contract('BeaconProxy', function (accounts) {
   const [anotherAccount] = accounts;
 
@@ -53,7 +46,8 @@ contract('BeaconProxy', function (accounts) {
   describe('initialization', function () {
     before(function () {
       this.assertInitialized = async ({ value, balance }) => {
-        const beaconAddress = toChecksumAddress(await web3.eth.getStorageAt(this.proxy.address, BEACON_SLOT));
+        const beaconSlot = await web3.eth.getStorageAt(this.proxy.address, BeaconSlot);
+        const beaconAddress = toChecksumAddress(beaconSlot.substr(-40));
         expect(beaconAddress).to.equal(this.beacon.address);
 
         const dummy = new DummyImplementation(this.proxy.address);

test/proxy/transparent/TransparentUpgradeableProxy.behaviour.js

@@ -1,6 +1,6 @@
 const { BN, expectRevert, expectEvent, constants } = require('@openzeppelin/test-helpers');
 const { ZERO_ADDRESS } = constants;
-const ethereumjsUtil = require('ethereumjs-util');
+const { ImplementationSlot, AdminSlot, toChecksumAddress } = require('../../helpers/erc1967');
 
 const { expect } = require('chai');
 
@@ -16,13 +16,6 @@ const InitializableMock = artifacts.require('InitializableMock');
 const DummyImplementation = artifacts.require('DummyImplementation');
 const ClashingImplementation = artifacts.require('ClashingImplementation');
 
-const IMPLEMENTATION_LABEL = 'eip1967.proxy.implementation';
-const ADMIN_LABEL = 'eip1967.proxy.admin';
-
-function toChecksumAddress (address) {
-  return ethereumjsUtil.toChecksumAddress('0x' + address.replace(/^0x/, '').padStart(40, '0').substr(-40));
-}
-
 module.exports = function shouldBehaveLikeTransparentUpgradeableProxy (createProxy, accounts) {
   const [proxyAdminAddress, proxyAdminOwner, anotherAccount] = accounts;
 
@@ -312,15 +305,15 @@ module.exports = function shouldBehaveLikeTransparentUpgradeableProxy (createPro
 
   describe('storage', function () {
     it('should store the implementation address in specified location', async function () {
-      const slot = '0x' + new BN(ethereumjsUtil.keccak256(Buffer.from(IMPLEMENTATION_LABEL))).subn(1).toString(16);
-      const implementation = toChecksumAddress(await web3.eth.getStorageAt(this.proxyAddress, slot));
-      expect(implementation).to.be.equal(this.implementationV0);
+      const implementationSlot = await web3.eth.getStorageAt(this.proxyAddress, ImplementationSlot);
+      const implementationAddress = toChecksumAddress(implementationSlot.substr(-40));
+      expect(implementationAddress).to.be.equal(this.implementationV0);
     });
 
     it('should store the admin proxy in specified location', async function () {
-      const slot = '0x' + new BN(ethereumjsUtil.keccak256(Buffer.from(ADMIN_LABEL))).subn(1).toString(16);
-      const proxyAdmin = toChecksumAddress(await web3.eth.getStorageAt(this.proxyAddress, slot));
-      expect(proxyAdmin).to.be.equal(proxyAdminAddress);
+      const proxyAdminSlot = await web3.eth.getStorageAt(this.proxyAddress, AdminSlot);
+      const proxyAdminAddress = toChecksumAddress(proxyAdminSlot.substr(-40));
+      expect(proxyAdminAddress).to.be.equal(proxyAdminAddress);
     });
   });
 

test/proxy/utils/UUPSUpgradeable.test.js

@@ -1,4 +1,6 @@
 const { expectEvent, expectRevert } = require('@openzeppelin/test-helpers');
+const { web3 } = require('@openzeppelin/test-helpers/src/setup');
+const { ImplementationSlot, toChecksumAddress } = require('../../helpers/erc1967');
 
 const ERC1967Proxy = artifacts.require('ERC1967Proxy');
 const UUPSUpgradeableMock = artifacts.require('UUPSUpgradeableMock');
@@ -55,9 +57,8 @@ contract('UUPSUpgradeable', function (accounts) {
 
   // delegate to a non existing upgradeTo function causes a low level revert
   it('reject upgrade to non uups implementation', async function () {
-    await expectRevert(
+    await expectRevert.unspecified(
       this.instance.upgradeTo(this.implUpgradeNonUUPS.address),
-      'Transaction reverted: function selector was not recognized and there\'s no fallback function',
     );
   });
 
@@ -88,6 +89,10 @@ contract('UUPSUpgradeable', function (accounts) {
         expect(UpgradedEvents.length).to.be.equal(1);
 
         expectEvent(receipt, 'Upgraded', { implementation: this.implInitial.address });
+
+        const implementationSlot = await web3.eth.getStorageAt(legacyInstance.address, ImplementationSlot);
+        const implementationAddress = toChecksumAddress(implementationSlot.substr(-40));
+        expect(implementationAddress).to.be.equal(this.implInitial.address);
       });
     };
   });