diff --git a/.changeset/clever-pumas-beg.md b/.changeset/clever-pumas-beg.md new file mode 100644 index 000000000..5f1f4b13b --- /dev/null +++ b/.changeset/clever-pumas-beg.md @@ -0,0 +1,5 @@ +--- +'openzeppelin-solidity': major +--- + +`Ownable`: Add an `initialOwner` parameter to the constructor, making the ownership initialization explicit. diff --git a/contracts/access/Ownable.sol b/contracts/access/Ownable.sol index 1378ffb43..1351dad03 100644 --- a/contracts/access/Ownable.sol +++ b/contracts/access/Ownable.sol @@ -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); } /** diff --git a/contracts/mocks/ERC1271WalletMock.sol b/contracts/mocks/ERC1271WalletMock.sol index 015288998..4e6d3ce57 100644 --- a/contracts/mocks/ERC1271WalletMock.sol +++ b/contracts/mocks/ERC1271WalletMock.sol @@ -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); diff --git a/contracts/proxy/beacon/UpgradeableBeacon.sol b/contracts/proxy/beacon/UpgradeableBeacon.sol index 9eeb149a6..557c58f67 100644 --- a/contracts/proxy/beacon/UpgradeableBeacon.sol +++ b/contracts/proxy/beacon/UpgradeableBeacon.sol @@ -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_); } diff --git a/contracts/proxy/transparent/ProxyAdmin.sol b/contracts/proxy/transparent/ProxyAdmin.sol index 78a409e01..3739ad85f 100644 --- a/contracts/proxy/transparent/ProxyAdmin.sol +++ b/contracts/proxy/transparent/ProxyAdmin.sol @@ -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`. * diff --git a/test/access/Ownable.test.js b/test/access/Ownable.test.js index 109150874..07b8764a5 100644 --- a/test/access/Ownable.test.js +++ b/test/access/Ownable.test.js @@ -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 () { diff --git a/test/access/Ownable2Step.test.js b/test/access/Ownable2Step.test.js index ce043057b..dfda6b708 100644 --- a/test/access/Ownable2Step.test.js +++ b/test/access/Ownable2Step.test.js @@ -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 () { diff --git a/test/proxy/beacon/BeaconProxy.test.js b/test/proxy/beacon/BeaconProxy.test.js index 968f00be8..68db10ddc 100644 --- a/test/proxy/beacon/BeaconProxy.test.js +++ b/test/proxy/beacon/BeaconProxy.test.js @@ -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 () { @@ -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 () { @@ -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(); @@ -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'); @@ -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) @@ -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'); diff --git a/test/proxy/beacon/UpgradeableBeacon.test.js b/test/proxy/beacon/UpgradeableBeacon.test.js index d65f3e0a5..c19b250b9 100644 --- a/test/proxy/beacon/UpgradeableBeacon.test.js +++ b/test/proxy/beacon/UpgradeableBeacon.test.js @@ -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 () { diff --git a/test/proxy/transparent/ProxyAdmin.test.js b/test/proxy/transparent/ProxyAdmin.test.js index efaaf94c2..85b6695df 100644 --- a/test/proxy/transparent/ProxyAdmin.test.js +++ b/test/proxy/transparent/ProxyAdmin.test.js @@ -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); });