From 25eba62fb827e1bc2c59c18c5806011440049a24 Mon Sep 17 00:00:00 2001 From: Gabriel Fournier Date: Wed, 29 May 2024 15:08:01 +0200 Subject: [PATCH] remove OwnableUpgradeable and create local owner storage --- contracts/beacon/Voucher.sol | 41 ++++++++++++++++++++++--------- contracts/mocks/VoucherV2Mock.sol | 18 +++++++------- test/VoucherHub.test.ts | 2 -- test/beacon/Voucher.test.ts | 12 ++++----- 4 files changed, 44 insertions(+), 29 deletions(-) diff --git a/contracts/beacon/Voucher.sol b/contracts/beacon/Voucher.sol index 089e879..0919d11 100644 --- a/contracts/beacon/Voucher.sol +++ b/contracts/beacon/Voucher.sol @@ -6,8 +6,8 @@ pragma solidity ^0.8.20; import {IexecLibOrders_v5} from "@iexec/poco/contracts/libs/IexecLibOrders_v5.sol"; import {IexecPoco1} from "@iexec/poco/contracts/modules/interfaces/IexecPoco1.v8.sol"; import {IexecPocoBoost} from "@iexec/poco/contracts/modules/interfaces/IexecPocoBoost.sol"; -import {OwnableUpgradeable} from "@openzeppelin/contracts-upgradeable/access/OwnableUpgradeable.sol"; import {IERC20} from "@openzeppelin/contracts/token/ERC20/IERC20.sol"; +import {Initializable} from "@openzeppelin/contracts-upgradeable/proxy/utils/Initializable.sol"; import {IVoucherHub} from "../IVoucherHub.sol"; import {IVoucher} from "./IVoucher.sol"; @@ -15,7 +15,7 @@ import {IVoucher} from "./IVoucher.sol"; * @title Implementation of the voucher contract. * Deployed along the Beacon contract using "Upgrades" plugin of OZ. */ -contract Voucher is OwnableUpgradeable, IVoucher { +contract Voucher is Initializable, IVoucher { // keccak256(abi.encode(uint256(keccak256("iexec.voucher.storage.Voucher")) - 1)) // & ~bytes32(uint256(0xff)); bytes32 private constant VOUCHER_STORAGE_LOCATION = @@ -28,6 +28,15 @@ contract Voucher is OwnableUpgradeable, IVoucher { uint256 _type; mapping(address => bool) _authorizedAccounts; mapping(bytes32 dealId => uint256) _sponsoredAmounts; + address _owner; + } + + /** + * @dev Throws if called by any account other than the owner. + */ + modifier onlyOwner() { + require(msg.sender == owner(), "Voucher: sender is not owner"); + _; } modifier onlyAuthorized() { @@ -63,8 +72,8 @@ contract Voucher is OwnableUpgradeable, IVoucher { uint256 expiration, uint256 voucherTypeId ) external initializer { - __Ownable_init(owner); VoucherStorage storage $ = _getVoucherStorage(); + $._owner = owner; $._voucherHub = voucherHub; $._expiration = expiration; $._type = voucherTypeId; @@ -167,15 +176,6 @@ contract Voucher is OwnableUpgradeable, IVoucher { return dealId; } - /** - * Retrieve the address of the voucher hub associated with the voucher. - * @return voucherHubAddress The address of the voucher hub. - */ - function getVoucherHub() public view returns (address) { - VoucherStorage storage $ = _getVoucherStorage(); - return $._voucherHub; - } - /** * Retrieve the type of the voucher. * @return voucherType The type of the voucher. @@ -220,6 +220,23 @@ contract Voucher is OwnableUpgradeable, IVoucher { return $._sponsoredAmounts[dealId]; } + /** + * Retrieve the address of the voucher hub associated with the voucher. + * @return voucherHubAddress The address of the voucher hub. + */ + function getVoucherHub() public view returns (address) { + VoucherStorage storage $ = _getVoucherStorage(); + return $._voucherHub; + } + + /** + * @dev Returns the address of the current owner. + */ + function owner() public view returns (address) { + VoucherStorage storage $ = _getVoucherStorage(); + return $._owner; + } + /** * Internal function to set authorization for an account. * @param account The account to set authorization for. diff --git a/contracts/mocks/VoucherV2Mock.sol b/contracts/mocks/VoucherV2Mock.sol index 05c8d03..69bf660 100644 --- a/contracts/mocks/VoucherV2Mock.sol +++ b/contracts/mocks/VoucherV2Mock.sol @@ -1,11 +1,11 @@ // SPDX-FileCopyrightText: 2024 IEXEC BLOCKCHAIN TECH // SPDX-License-Identifier: Apache-2.0 -pragma solidity ^0.8.20; +import {Initializable} from "@openzeppelin/contracts-upgradeable/proxy/utils/Initializable.sol"; -import {OwnableUpgradeable} from "@openzeppelin/contracts-upgradeable/access/OwnableUpgradeable.sol"; +pragma solidity ^0.8.20; -contract VoucherV2Mock is OwnableUpgradeable { +contract VoucherV2Mock is Initializable { /// @custom:storage-location erc7201:iexec.voucher.storage.Voucher struct VoucherStorage { address _voucherHub; @@ -13,6 +13,7 @@ contract VoucherV2Mock is OwnableUpgradeable { uint256 _type; mapping(address => bool) _authorizedAccounts; mapping(bytes32 dealId => uint256) _sponsoredAmounts; + address _owner; uint256 _newStateVariable; } @@ -21,16 +22,11 @@ contract VoucherV2Mock is OwnableUpgradeable { bytes32 private constant VOUCHER_STORAGE_LOCATION = 0xc2e244293dc04d6c7fa946e063317ff8e6770fd48cbaff411a60f1efc8a7e800; - /// @custom:oz-upgrades-unsafe-allow constructor - constructor() { - _disableInitializers(); - } - /** * Initialize new implementation contract. * @param newStateVariable test variable. */ - function initializeV2(uint256 newStateVariable) external reinitializer(2) { + function initializeV2(uint256 newStateVariable) external { VoucherStorage storage $ = _getVoucherStorage(); $._newStateVariable = newStateVariable; } @@ -44,6 +40,10 @@ contract VoucherV2Mock is OwnableUpgradeable { VoucherStorage storage $ = _getVoucherStorage(); return $._newStateVariable; } + function owner() public view returns (address) { + VoucherStorage storage $ = _getVoucherStorage(); + return $._owner; + } function _getVoucherStorage() private pure returns (VoucherStorage storage $) { assembly { diff --git a/test/VoucherHub.test.ts b/test/VoucherHub.test.ts index 07cb2db..ee1b609 100644 --- a/test/VoucherHub.test.ts +++ b/test/VoucherHub.test.ts @@ -375,8 +375,6 @@ describe('VoucherHub', function () { await expect(createVoucherTx) .to.emit(voucherAsProxy, 'BeaconUpgraded') .withArgs(await beacon.getAddress()) - .to.emit(voucher, 'OwnershipTransferred') - .withArgs(ethers.ZeroAddress, voucherOwner1.address) .to.emit(voucherHub, 'VoucherCreated') .withArgs( voucherAddress, diff --git a/test/beacon/Voucher.test.ts b/test/beacon/Voucher.test.ts index 3960d26..56e98ad 100644 --- a/test/beacon/Voucher.test.ts +++ b/test/beacon/Voucher.test.ts @@ -234,18 +234,18 @@ describe('Voucher', function () { }); it('Should not authorize account if sender is not the owner', async function () { - await expect( - voucherAsAnyone.authorizeAccount(anyone.address), - ).to.be.revertedWithCustomError(voucherAsOwner, 'OwnableUnauthorizedAccount'); + await expect(voucherAsAnyone.authorizeAccount(anyone.address)).to.be.revertedWith( + 'Voucher: sender is not owner', + ); }); it('Should not unauthorize account if sender is not the owner', async function () { await voucherAsOwner.authorizeAccount(anyone.address).then((tx) => tx.wait()); expect(await voucherAsOwner.isAccountAuthorized(anyone.address)).to.be.true; // unauthorize the account - await expect( - voucherAsAnyone.unauthorizeAccount(anyone.address), - ).to.be.revertedWithCustomError(voucherAsOwner, 'OwnableUnauthorizedAccount'); + await expect(voucherAsAnyone.unauthorizeAccount(anyone.address)).to.be.revertedWith( + 'Voucher: sender is not owner', + ); // Check that the state of mapping is not modified from. expect(await voucherAsOwner.isAccountAuthorized(anyone.address)).to.be.true; });