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

refactor: use oz ownable opposed to custom copy #60

Merged
merged 2 commits into from
Jan 27, 2025
Merged
Show file tree
Hide file tree
Changes from all 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
9 changes: 5 additions & 4 deletions src/contracts/access-control/OwnableWithGuardian.sol
Original file line number Diff line number Diff line change
@@ -1,13 +1,13 @@
// SPDX-License-Identifier: MIT
pragma solidity >=0.7.0;

import {Ownable} from 'openzeppelin-contracts/contracts/access/Ownable.sol';
import {IWithGuardian} from './interfaces/IWithGuardian.sol';
import {Ownable} from '../oz-common/Ownable.sol';

abstract contract OwnableWithGuardian is Ownable, IWithGuardian {
address private _guardian;

constructor() {
constructor(address initialOwner) Ownable(initialOwner) {
_updateGuardian(_msgSender());
}

Expand Down Expand Up @@ -41,10 +41,11 @@ abstract contract OwnableWithGuardian is Ownable, IWithGuardian {
}

function _checkGuardian() internal view {
require(guardian() == _msgSender(), 'ONLY_BY_GUARDIAN');
if (guardian() != _msgSender()) revert OnlyGuardianInvalidCaller(_msgSender());
}

function _checkOwnerOrGuardian() internal view {
require(_msgSender() == owner() || _msgSender() == guardian(), 'ONLY_BY_OWNER_OR_GUARDIAN');
if (_msgSender() != owner() && _msgSender() != guardian())
revert OnlyGuardianOrOwnerInvalidCaller(_msgSender());
}
}
10 changes: 0 additions & 10 deletions src/contracts/access-control/UpgradeableOwnableWithGuardian.sol
Original file line number Diff line number Diff line change
Expand Up @@ -24,16 +24,6 @@ abstract contract UpgradeableOwnableWithGuardian is OwnableUpgradeable, IWithGua
}
}

/**
* @dev The caller account is not authorized to perform an operation.
*/
error OnlyGuardianInvalidCaller(address account);

/**
* @dev The caller account is not authorized to perform an operation.
*/
error OnlyGuardianOrOwnerInvalidCaller(address account);

/**
* @dev Initializes the contract setting the address provided by the deployer as the initial owner.
*/
Expand Down
10 changes: 10 additions & 0 deletions src/contracts/access-control/interfaces/IWithGuardian.sol
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,16 @@ interface IWithGuardian {
*/
event GuardianUpdated(address oldGuardian, address newGuardian);

/**
* @dev The caller account is not authorized to perform an operation.
*/
error OnlyGuardianInvalidCaller(address account);

/**
* @dev The caller account is not authorized to perform an operation.
*/
error OnlyGuardianOrOwnerInvalidCaller(address account);

/**
* @dev get guardian address;
*/
Expand Down
84 changes: 0 additions & 84 deletions src/contracts/oz-common/Ownable.sol

This file was deleted.

14 changes: 10 additions & 4 deletions test/OwnableWithGuardian.t.sol
Original file line number Diff line number Diff line change
Expand Up @@ -2,9 +2,11 @@
pragma solidity ^0.8.0;

import 'forge-std/Test.sol';
import {OwnableWithGuardian} from '../src/contracts/access-control/OwnableWithGuardian.sol';
import {OwnableWithGuardian, IWithGuardian} from '../src/contracts/access-control/OwnableWithGuardian.sol';

contract ImplOwnableWithGuardian is OwnableWithGuardian {
constructor(address initialOwner) OwnableWithGuardian(initialOwner) {}

function mock_onlyGuardian() external onlyGuardian {}

function mock_onlyOwnerOrGuardian() external onlyOwnerOrGuardian {}
Expand All @@ -17,7 +19,7 @@ contract TestOfOwnableWithGuardian is Test {
address guardian = makeAddr('guardian');

function setUp() public {
withGuardian = new ImplOwnableWithGuardian();
withGuardian = new ImplOwnableWithGuardian(address(this));
assertEq(withGuardian.owner(), address(this));
assertEq(withGuardian.guardian(), address(this));
withGuardian.transferOwnership(owner);
Expand All @@ -40,7 +42,9 @@ contract TestOfOwnableWithGuardian is Test {
}

function testGuardianUpdateNoAccess() external {
vm.expectRevert('ONLY_BY_OWNER_OR_GUARDIAN');
vm.expectRevert(
abi.encodeWithSelector(IWithGuardian.OnlyGuardianOrOwnerInvalidCaller.selector, address(this))
);
withGuardian.updateGuardian(guardian);
}

Expand All @@ -50,7 +54,9 @@ contract TestOfOwnableWithGuardian is Test {
}

function test_onlyGuardianGuard_shouldRevert() external {
vm.expectRevert('ONLY_BY_GUARDIAN');
vm.expectRevert(
abi.encodeWithSelector(IWithGuardian.OnlyGuardianInvalidCaller.selector, address(this))
);
withGuardian.mock_onlyGuardian();
}
}
17 changes: 4 additions & 13 deletions test/UpgradeableOwnableWithGuardian.t.sol
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@
pragma solidity ^0.8.20;

import 'forge-std/Test.sol';
import {UpgradeableOwnableWithGuardian} from '../src/contracts/access-control/UpgradeableOwnableWithGuardian.sol';
import {UpgradeableOwnableWithGuardian, IWithGuardian} from '../src/contracts/access-control/UpgradeableOwnableWithGuardian.sol';

contract ImplOwnableWithGuardian is UpgradeableOwnableWithGuardian {
function initialize(address owner, address guardian) public initializer {
Expand Down Expand Up @@ -33,20 +33,14 @@ contract TestOfUpgradableOwnableWithGuardian is Test {

function test_onlyGuardian() external {
vm.expectRevert(
abi.encodeWithSelector(
UpgradeableOwnableWithGuardian.OnlyGuardianInvalidCaller.selector,
address(this)
)
abi.encodeWithSelector(IWithGuardian.OnlyGuardianInvalidCaller.selector, address(this))
);
ImplOwnableWithGuardian(address(withGuardian)).mock_onlyGuardian();
}

function test_onlyOwnerOrGuardian() external {
vm.expectRevert(
abi.encodeWithSelector(
UpgradeableOwnableWithGuardian.OnlyGuardianOrOwnerInvalidCaller.selector,
address(this)
)
abi.encodeWithSelector(IWithGuardian.OnlyGuardianOrOwnerInvalidCaller.selector, address(this))
);
ImplOwnableWithGuardian(address(withGuardian)).mock_onlyOwnerOrGuardian();
}
Expand All @@ -66,10 +60,7 @@ contract TestOfUpgradableOwnableWithGuardian is Test {

vm.prank(eoa);
vm.expectRevert(
abi.encodeWithSelector(
UpgradeableOwnableWithGuardian.OnlyGuardianOrOwnerInvalidCaller.selector,
eoa
)
abi.encodeWithSelector(IWithGuardian.OnlyGuardianOrOwnerInvalidCaller.selector, eoa)
);
withGuardian.updateGuardian(newGuardian);
}
Expand Down
5 changes: 2 additions & 3 deletions test/create3Test.t.sol
Original file line number Diff line number Diff line change
Expand Up @@ -2,16 +2,15 @@
pragma solidity ^0.8.0;

import 'forge-std/Test.sol';
import {Ownable} from 'openzeppelin-contracts/contracts/access/Ownable.sol';
import {Create3Factory, ICreate3Factory, Create3} from '../src/contracts/create3/Create3Factory.sol';
import {Ownable} from '../src/contracts/oz-common/Ownable.sol';

contract MockContract is Ownable {
address public immutable SOME_ADDRESS;
address internal _someOtherAddress;

constructor(address someAddress, address owner) {
constructor(address someAddress, address owner) Ownable(owner) {
SOME_ADDRESS = someAddress;
_transferOwnership(owner);
}

function getOtherAddress() external view returns (address) {
Expand Down
Loading