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

Make Ownable's initial owner explicit #4267

Merged
merged 5 commits into from
May 23, 2023
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
5 changes: 5 additions & 0 deletions .changeset/clever-pumas-beg.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
'openzeppelin-solidity': major
---

`Ownable`: Add an `initialOwner` parameter to the constructor, making the ownership initialization explicit.
4 changes: 2 additions & 2 deletions contracts/access/Ownable.sol
Original file line number Diff line number Diff line change
Expand Up @@ -25,8 +25,8 @@ abstract contract Ownable is Context {
/**
* @dev Initializes the contract setting the deployer as the initial owner.
*/
constructor() {
_transferOwnership(_msgSender());
constructor(address initialOwner) {
_transferOwnership(initialOwner);
}

/**
Expand Down
4 changes: 1 addition & 3 deletions contracts/mocks/ERC1271WalletMock.sol
Original file line number Diff line number Diff line change
Expand Up @@ -7,9 +7,7 @@ import "../interfaces/IERC1271.sol";
import "../utils/cryptography/ECDSA.sol";

contract ERC1271WalletMock is Ownable, IERC1271 {
constructor(address originalOwner) {
transferOwnership(originalOwner);
}
constructor(address originalOwner) Ownable(originalOwner) {}

function isValidSignature(bytes32 hash, bytes memory signature) public view override returns (bytes4 magicValue) {
return ECDSA.recover(hash, signature) == owner() ? this.isValidSignature.selector : bytes4(0);
Expand Down
5 changes: 2 additions & 3 deletions contracts/proxy/beacon/UpgradeableBeacon.sol
Original file line number Diff line number Diff line change
Expand Up @@ -21,10 +21,9 @@ contract UpgradeableBeacon is IBeacon, Ownable {
event Upgraded(address indexed implementation);

/**
* @dev Sets the address of the initial implementation, and the deployer account as the owner who can upgrade the
* beacon.
* @dev Sets the address of the initial implementation, and the initial owner who can upgrade the beacon.
*/
constructor(address implementation_) {
constructor(address implementation_, address initialOwner) Ownable(initialOwner) {
_setImplementation(implementation_);
}

Expand Down
5 changes: 5 additions & 0 deletions contracts/proxy/transparent/ProxyAdmin.sol
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,11 @@ import "../../access/Ownable.sol";
* explanation of why you would want to use this see the documentation for {TransparentUpgradeableProxy}.
*/
contract ProxyAdmin is Ownable {
/**
* @dev Sets the initial owner who can perform upgrades.
*/
constructor(address initialOwner) Ownable(initialOwner) {}

/**
* @dev Changes the admin of `proxy` to `newAdmin`.
*
Expand Down
2 changes: 1 addition & 1 deletion test/access/Ownable.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ contract('Ownable', function (accounts) {
const [owner, other] = accounts;

beforeEach(async function () {
this.ownable = await Ownable.new({ from: owner });
this.ownable = await Ownable.new(owner);
});

it('has an owner', async function () {
Expand Down
2 changes: 1 addition & 1 deletion test/access/Ownable2Step.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ contract('Ownable2Step', function (accounts) {
const [owner, accountA, accountB] = accounts;

beforeEach(async function () {
this.ownable2Step = await Ownable2Step.new({ from: owner });
this.ownable2Step = await Ownable2Step.new(owner);
});

describe('transfer ownership', function () {
Expand Down
12 changes: 6 additions & 6 deletions test/proxy/beacon/BeaconProxy.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ const BadBeaconNoImpl = artifacts.require('BadBeaconNoImpl');
const BadBeaconNotContract = artifacts.require('BadBeaconNotContract');

contract('BeaconProxy', function (accounts) {
const [anotherAccount] = accounts;
const [upgradeableBeaconAdmin, anotherAccount] = accounts;

describe('bad beacon is not accepted', async function () {
it('non-contract beacon', async function () {
Expand Down Expand Up @@ -49,7 +49,7 @@ contract('BeaconProxy', function (accounts) {
});

beforeEach('deploy beacon', async function () {
this.beacon = await UpgradeableBeacon.new(this.implementationV0.address);
this.beacon = await UpgradeableBeacon.new(this.implementationV0.address, upgradeableBeaconAdmin);
});

it('no initialization', async function () {
Expand Down Expand Up @@ -81,7 +81,7 @@ contract('BeaconProxy', function (accounts) {
});

it('upgrade a proxy by upgrading its beacon', async function () {
const beacon = await UpgradeableBeacon.new(this.implementationV0.address);
const beacon = await UpgradeableBeacon.new(this.implementationV0.address, upgradeableBeaconAdmin);

const value = '10';
const data = this.implementationV0.contract.methods.initializeNonPayableWithValue(value).encodeABI();
Expand All @@ -96,7 +96,7 @@ contract('BeaconProxy', function (accounts) {
expect(await dummy.version()).to.eq('V1');

// upgrade beacon
await beacon.upgradeTo(this.implementationV1.address);
await beacon.upgradeTo(this.implementationV1.address, { from: upgradeableBeaconAdmin });

// test upgraded version
expect(await dummy.version()).to.eq('V2');
Expand All @@ -106,7 +106,7 @@ contract('BeaconProxy', function (accounts) {
const value1 = '10';
const value2 = '42';

const beacon = await UpgradeableBeacon.new(this.implementationV0.address);
const beacon = await UpgradeableBeacon.new(this.implementationV0.address, upgradeableBeaconAdmin);

const proxy1InitializeData = this.implementationV0.contract.methods
.initializeNonPayableWithValue(value1)
Expand All @@ -130,7 +130,7 @@ contract('BeaconProxy', function (accounts) {
expect(await dummy2.version()).to.eq('V1');

// upgrade beacon
await beacon.upgradeTo(this.implementationV1.address);
await beacon.upgradeTo(this.implementationV1.address, { from: upgradeableBeaconAdmin });

// test upgraded version
expect(await dummy1.version()).to.eq('V2');
Expand Down
7 changes: 5 additions & 2 deletions test/proxy/beacon/UpgradeableBeacon.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -9,13 +9,16 @@ contract('UpgradeableBeacon', function (accounts) {
const [owner, other] = accounts;

it('cannot be created with non-contract implementation', async function () {
await expectRevert(UpgradeableBeacon.new(accounts[0]), 'UpgradeableBeacon: implementation is not a contract');
await expectRevert(
UpgradeableBeacon.new(accounts[0], owner),
'UpgradeableBeacon: implementation is not a contract',
);
});

context('once deployed', async function () {
beforeEach('deploying beacon', async function () {
this.v1 = await Implementation1.new();
this.beacon = await UpgradeableBeacon.new(this.v1.address, { from: owner });
this.beacon = await UpgradeableBeacon.new(this.v1.address, owner);
});

it('returns implementation', async function () {
Expand Down
3 changes: 1 addition & 2 deletions test/proxy/transparent/ProxyAdmin.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -17,12 +17,11 @@ contract('ProxyAdmin', function (accounts) {

beforeEach(async function () {
const initializeData = Buffer.from('');
this.proxyAdmin = await ProxyAdmin.new({ from: proxyAdminOwner });
this.proxyAdmin = await ProxyAdmin.new(proxyAdminOwner);
const proxy = await TransparentUpgradeableProxy.new(
this.implementationV1.address,
this.proxyAdmin.address,
initializeData,
{ from: proxyAdminOwner },
);
this.proxy = await ITransparentUpgradeableProxy.at(proxy.address);
});
Expand Down