feat: [#13] allow setting the owner during upgrade

contracts/ParcelNFT.sol

@@ -281,6 +281,16 @@ contract ParcelNFT is
     _unpause();
   }
 
+  function transferOwnership(address newOwner) public virtual override whenNotPaused {
+    address sender = _msgSender();
+    require(
+      owner() == sender || hasRole(Roles.OWNERSHIP_MANAGER, sender),
+      'Ownable: caller is not the owner nor ownership manager.'
+    );
+    require(newOwner != address(0), 'Ownable: new owner is the zero address');
+    _transferOwnership(newOwner);
+  }
+
   function _burn(uint256 tokenId)
     internal
     virtual

contracts/Roles.sol

@@ -4,6 +4,7 @@ pragma solidity ^0.8.9;
 
 library Roles {
   bytes32 public constant SUPER_ADMIN = 0x00;
+  bytes32 public constant OWNERSHIP_MANAGER = keccak256('citydao.OwnershipManager');
   bytes32 public constant PARCEL_MANAGER = keccak256('citydao.ParcelManager');
   bytes32 public constant PAUSER = keccak256('citydao.Pauser');
   bytes32 public constant UPGRADER = keccak256('citydao.Upgrader');

src/constants/roles.ts

@@ -10,3 +10,4 @@ export const SUPER_ADMIN_ROLE = toAccessRole(0);
 export const PARCEL_MANAGER_ROLE = keccak256(toUtf8Bytes('citydao.ParcelManager'));
 export const PAUSER_ROLE = keccak256(toUtf8Bytes('citydao.Pauser'));
 export const UPGRADER_ROLE = keccak256(toUtf8Bytes('citydao.Upgrader'));
+export const OWNERSHIP_MANAGER_ROLE = keccak256(toUtf8Bytes('citydao.OwnershipManager'));

src/contracts/ownable.ts

@@ -0,0 +1,5 @@
+import { OwnableUpgradeable__factory } from '../../types/contracts';
+import { EthereumAddress } from '../constants/accounts';
+
+export const encodeTransferOwnershipFunction = (newOwner: EthereumAddress) =>
+  OwnableUpgradeable__factory.createInterface().encodeFunctionData('transferOwnership', [newOwner]);

src/contracts/upgradeableProxy.ts

@@ -1,8 +1,9 @@
 import { TransactionRequest } from '@ethersproject/providers';
-import { Signer } from 'ethers';
+import { BigNumber, providers, Signer } from 'ethers';
 import { BytesLike } from 'ethers/lib/utils';
-import { UpgradeableProxy__factory } from '../../types/contracts';
-import { ContractAddress } from '../constants/accounts';
+import { ERC1967Proxy, UpgradeableProxy__factory } from '../../types/contracts';
+import { ContractAddress, toEthereumAddress } from '../constants/accounts';
+import { getStorageLocationFromText, readStorageValue } from '../utils/storage';
 
 /**
  * Deploy an upgradeable proxy contract with the given contract address to be used
@@ -29,3 +30,20 @@ export const buildDeployUpgradeableProxyTransactionRequest = (
   logicAddress: ContractAddress,
   initFunction: BytesLike = '0x',
 ): TransactionRequest => new UpgradeableProxy__factory().getDeployTransaction(logicAddress, initFunction);
+
+/**
+ * Retrieves the proxy implementation address from a proxy contract's storage.
+ *
+ * @param proxy the contract to retrieve the proxy implementation from
+ * @param provider (optional) the provider to use. Defaults to contract provider
+ */
+export const getProxyImplementationAddress = async (proxy: ERC1967Proxy, provider?: providers.Provider) =>
+  toEthereumAddress(
+    BigNumber.from(
+      await readStorageValue(
+        provider || proxy.provider,
+        proxy.address,
+        getStorageLocationFromText('eip1967.proxy.implementation').sub(1),
+      ),
+    ),
+  );

test/helpers/storage.ts → src/utils/storage.ts

@@ -1,6 +1,6 @@
 import { BigNumber, providers } from 'ethers';
 import { keccak256, toUtf8Bytes } from 'ethers/lib/utils';
-import { ContractAddress } from '../../src/constants/accounts';
+import { ContractAddress } from '../constants/accounts';
 
 export const readStorageValue = async (provider: providers.Provider, address: ContractAddress, location: BigNumber) =>
   (await readStorageValues(provider, address, location, 1))[0];

test/contracts/access/ownable.spec.ts

@@ -1,7 +1,16 @@
 import { expect } from 'chai';
 import { ZERO_ADDRESS } from '../../../src/constants/accounts';
+import {
+  OWNERSHIP_MANAGER_ROLE,
+  PARCEL_MANAGER_ROLE,
+  PAUSER_ROLE,
+  SUPER_ADMIN_ROLE,
+  UPGRADER_ROLE,
+} from '../../../src/constants/roles';
+import { encodeTransferOwnershipFunction } from '../../../src/contracts/ownable';
+import { createParcelNFT as createParcelNFTProxy } from '../../../src/contracts/parcelNFT';
 import { INITIALIZER, USER1 } from '../../helpers/Accounts';
-import { createParcelNFT } from '../../helpers/contracts/ParcelNFTHelper';
+import { createParcelNFT, deployParcelNFT } from '../../helpers/contracts/ParcelNFTHelper';
 
 describe('Owner initialized with zero address', () => {
   it('should set caller as super admin', async () => {
@@ -18,19 +27,61 @@ describe('Owner initialized with another address', () => {
 });
 
 describe('transferOwnership', () => {
-  it('should transfer ownership', async () => {
+  it('should transfer ownership if owner', async () => {
     const parcelNFT = await createParcelNFT({ superAdmin: ZERO_ADDRESS });
     await parcelNFT.transferOwnership(USER1.address);
 
     expect(await parcelNFT.owner()).to.eq(USER1.address);
   });
 
-  it('should fail to transfer ownership when not owner', async () => {
+  it('should transfer ownership if ownership manager', async () => {
     const parcelNFT = await createParcelNFT({ superAdmin: ZERO_ADDRESS });
+    await parcelNFT.grantRole(OWNERSHIP_MANAGER_ROLE, USER1.address);
+
+    await parcelNFT.connect(USER1).transferOwnership(USER1.address);
+
+    expect(await parcelNFT.owner()).to.eq(USER1.address);
+  });
+
+  it('should fail to transfer ownership when not owner or ownership manager', async () => {
+    const parcelNFT = await createParcelNFT({ superAdmin: ZERO_ADDRESS });
+    await parcelNFT.grantRole(SUPER_ADMIN_ROLE, USER1.address);
+    await parcelNFT.grantRole(PARCEL_MANAGER_ROLE, USER1.address);
+    await parcelNFT.grantRole(PAUSER_ROLE, USER1.address);
+    await parcelNFT.grantRole(UPGRADER_ROLE, USER1.address);
+
     await expect(parcelNFT.connect(USER1).transferOwnership(USER1.address)).to.be.revertedWith(
-      'caller is not the owner',
+      'caller is not the owner nor ownership manager',
     );
 
     expect(await parcelNFT.owner()).to.eq(INITIALIZER.address);
   });
+
+  it('should fail to transfer ownership when paused', async () => {
+    const parcelNFT = await createParcelNFT({ superAdmin: ZERO_ADDRESS });
+    await parcelNFT.grantRole(PAUSER_ROLE, INITIALIZER.address);
+
+    await parcelNFT.pause();
+
+    await expect(parcelNFT.transferOwnership(USER1.address)).to.be.revertedWith('paused');
+
+    expect(await parcelNFT.owner()).to.eq(INITIALIZER.address);
+  });
+});
+
+describe('setOwner', () => {
+  it('should set owner during upgrade', async () => {
+    const parcelNFTLogic = await deployParcelNFT();
+    const parcelNFT = await createParcelNFTProxy(INITIALIZER, parcelNFTLogic.address);
+    await parcelNFT.grantRole(UPGRADER_ROLE, INITIALIZER.address);
+    await parcelNFT.grantRole(OWNERSHIP_MANAGER_ROLE, INITIALIZER.address);
+
+    // reset it back to as if it were uninitialized
+    await parcelNFT.renounceOwnership();
+
+    const newParcelNFTLogic = await deployParcelNFT();
+    await parcelNFT.upgradeToAndCall(newParcelNFTLogic.address, encodeTransferOwnershipFunction(USER1.address));
+
+    expect(await parcelNFT.owner()).to.eq(USER1.address);
+  });
 });

test/contracts/pausable/pausable.spec.ts

@@ -82,8 +82,11 @@ describe('unpause', () => {
 
     expect(await parcelNFT.paused()).to.be.true;
 
-    await parcelNFT.grantRole(PAUSER_ROLE, USER1.address);
+    await parcelNFT.unpause();
     await parcelNFT.connect(USER1).transferOwnership(USER2.address);
+    await parcelNFT.pause();
+
+    await parcelNFT.grantRole(PAUSER_ROLE, USER1.address);
 
     expect(parcelNFT.connect(USER2).unpause()).to.be.revertedWith('missing role');
 

test/contracts/proxy/proxy.spec.ts

@@ -1,16 +1,17 @@
 import { expect } from 'chai';
 import { SUPER_ADMIN_ROLE, UPGRADER_ROLE } from '../../../src/constants/roles';
 import { createParcelNFT } from '../../../src/contracts/parcelNFT';
+import { getProxyImplementationAddress } from '../../../src/contracts/upgradeableProxy';
 import { INITIALIZER, USER1 } from '../../helpers/Accounts';
 import { deployParcelNFT } from '../../helpers/contracts/ParcelNFTHelper';
-import { getProxyImplementationAddress } from '../../helpers/contracts/ProxyHelper';
+import { asProxy } from '../../helpers/contracts/ProxyHelper';
 
 describe('createParcelNFT', () => {
   it('should create and initialize the contract', async () => {
     const parcelNFTLogic = await deployParcelNFT();
     const parcelNFT = await createParcelNFT(INITIALIZER, parcelNFTLogic.address, { superAdmin: USER1.address });
 
-    expect(await getProxyImplementationAddress(parcelNFT)).to.eq(parcelNFTLogic.address);
+    expect(await getProxyImplementationAddress(asProxy(parcelNFT))).to.eq(parcelNFTLogic.address);
 
     expect(await parcelNFT.hasRole(SUPER_ADMIN_ROLE, USER1.address)).to.be.true;
     expect(await parcelNFT.hasRole(SUPER_ADMIN_ROLE, INITIALIZER.address)).to.be.false;
@@ -24,7 +25,7 @@ describe('createParcelNFT', () => {
     const newParcelNFTLogic = await deployParcelNFT();
     await parcelNFT.upgradeTo(newParcelNFTLogic.address);
 
-    expect(await getProxyImplementationAddress(parcelNFT)).to.eq(newParcelNFTLogic.address);
+    expect(await getProxyImplementationAddress(asProxy(parcelNFT))).to.eq(newParcelNFTLogic.address);
   });
 
   it('should fail to upgrade when not an upgrader', async () => {
@@ -36,6 +37,31 @@ describe('createParcelNFT', () => {
     const newParcelNFTLogic = await deployParcelNFT();
     await expect(parcelNFT.connect(USER1).upgradeTo(newParcelNFTLogic.address)).to.be.revertedWith('missing role');
 
-    expect(await getProxyImplementationAddress(parcelNFT)).to.eq(parcelNFTLogic.address);
+    expect(await getProxyImplementationAddress(asProxy(parcelNFT))).to.eq(parcelNFTLogic.address);
+  });
+});
+
+describe('upgradeTo', () => {
+  it('should upgrade contract', async () => {
+    const parcelNFTLogic = await deployParcelNFT();
+    const parcelNFT = await createParcelNFT(INITIALIZER, parcelNFTLogic.address);
+    await parcelNFT.grantRole(UPGRADER_ROLE, INITIALIZER.address);
+
+    const newParcelNFTLogic = await deployParcelNFT();
+    await parcelNFT.upgradeTo(newParcelNFTLogic.address);
+
+    expect(await getProxyImplementationAddress(asProxy(parcelNFT))).to.eq(newParcelNFTLogic.address);
+  });
+
+  it('should fail to upgrade when not an upgrader', async () => {
+    const parcelNFTLogic = await deployParcelNFT();
+    const parcelNFT = await createParcelNFT(INITIALIZER, parcelNFTLogic.address);
+    await parcelNFT.grantRole(UPGRADER_ROLE, INITIALIZER.address);
+    await parcelNFT.transferOwnership(USER1.address);
+
+    const newParcelNFTLogic = await deployParcelNFT();
+    await expect(parcelNFT.connect(USER1).upgradeTo(newParcelNFTLogic.address)).to.be.revertedWith('missing role');
+
+    expect(await getProxyImplementationAddress(asProxy(parcelNFT))).to.eq(parcelNFTLogic.address);
   });
 });

test/helpers/contracts/ProxyHelper.ts

@@ -1,20 +1,7 @@
 import { SignerWithAddress } from '@nomiclabs/hardhat-ethers/signers';
-import { BigNumber, Contract } from 'ethers';
-import { toEthereumAddress } from '../../../src/constants/accounts';
+import { Contract } from 'ethers';
 import { ERC1967Proxy__factory } from '../../../types/contracts';
 import { INITIALIZER } from '../Accounts';
-import { getStorageLocationFromText, readStorageValue } from '../storage';
 
 export const asProxy = (contract: Contract, signer: SignerWithAddress = INITIALIZER) =>
   ERC1967Proxy__factory.connect(contract.address, signer);
-
-export const getProxyImplementationAddress = async (contract: Contract) =>
-  toEthereumAddress(
-    BigNumber.from(
-      await readStorageValue(
-        INITIALIZER.provider!!,
-        contract.address,
-        getStorageLocationFromText('eip1967.proxy.implementation').sub(1),
-      ),
-    ),
-  );