From bca8fcf6af6a78d6dd21f23a3fa05af123988ca7 Mon Sep 17 00:00:00 2001 From: mllwchrry Date: Fri, 31 May 2024 12:02:33 +0300 Subject: [PATCH 1/4] Added primary owner to the MultiOwnable contract --- contracts/access/MultiOwnable.sol | 21 ++++++ contracts/interfaces/access/IMultiOwnable.sol | 1 + test/access/MultiOwnable.test.ts | 66 +++++++++++++++++++ 3 files changed, 88 insertions(+) diff --git a/contracts/access/MultiOwnable.sol b/contracts/access/MultiOwnable.sol index 4e432807..60bd0c36 100644 --- a/contracts/access/MultiOwnable.sol +++ b/contracts/access/MultiOwnable.sol @@ -74,6 +74,15 @@ abstract contract MultiOwnable is IMultiOwnable, Initializable { return _owners.values(); } + /** + * @notice The function to get the address of the primary owner. + * @dev The function ensures compatibility with protocols like OpenSea where a single owner is required + * @return the address of the primary owner + */ + function owner() public view virtual returns (address) { + return _owners.at(0); + } + /** * @notice The function to check the ownership of a user * @param address_ the user to check @@ -89,10 +98,15 @@ abstract contract MultiOwnable is IMultiOwnable, Initializable { * @param newOwners_ the array of addresses to add to _owners */ function _addOwners(address[] memory newOwners_) private { + bool isSetEmpty_ = _owners.length() == 0; _owners.add(newOwners_); require(!_owners.contains(address(0)), "MultiOwnable: zero address can not be added"); + if (isSetEmpty_) { + emit OwnershipTransferred(address(0), _owners.at(0)); + } + emit OwnersAdded(newOwners_); } @@ -106,8 +120,15 @@ abstract contract MultiOwnable is IMultiOwnable, Initializable { * @param oldOwners_ the array of addresses to remove from _owners */ function _removeOwners(address[] memory oldOwners_) private { + address previousOwner_ = _owners.at(0); + _owners.remove(oldOwners_); + address newOwner_ = _owners.length() > 0 ? _owners.at(0) : address(0); + if (newOwner_ != previousOwner_) { + emit OwnershipTransferred(previousOwner_, newOwner_); + } + emit OwnersRemoved(oldOwners_); } diff --git a/contracts/interfaces/access/IMultiOwnable.sol b/contracts/interfaces/access/IMultiOwnable.sol index c59bb5b0..19cc79f4 100644 --- a/contracts/interfaces/access/IMultiOwnable.sol +++ b/contracts/interfaces/access/IMultiOwnable.sol @@ -7,6 +7,7 @@ pragma solidity ^0.8.4; interface IMultiOwnable { event OwnersAdded(address[] newOwners); event OwnersRemoved(address[] removedOwners); + event OwnershipTransferred(address indexed previousOwner, address indexed newOwner); /** * @notice The function to add equally rightful owners to the contract diff --git a/test/access/MultiOwnable.test.ts b/test/access/MultiOwnable.test.ts index d10d5a66..4489c66d 100644 --- a/test/access/MultiOwnable.test.ts +++ b/test/access/MultiOwnable.test.ts @@ -5,6 +5,7 @@ import { Reverter } from "@/test/helpers/reverter"; import { ZERO_ADDR } from "@/scripts/utils/constants"; import { MultiOwnableMock } from "@ethers-v6"; +import { EventLog } from "ethers"; describe("MultiOwnable", () => { const reverter = new Reverter(); @@ -60,6 +61,30 @@ describe("MultiOwnable", () => { expect(await multiOwnable.isOwner(THIRD.address)).to.be.true; }); + it("should emit OwnershipTransferred event when init function is called", async () => { + const MultiOwnableMock = await ethers.getContractFactory("MultiOwnableMock"); + const multiOwnable = await MultiOwnableMock.deploy(); + + const transactionInit = await multiOwnable.connect(SECOND).__MultiOwnableMock_init(); + const receiptInit = await transactionInit.wait(); + const eventInit = receiptInit?.logs.find((log) => { + const eventLog = log as EventLog; + return eventLog?.fragment?.name === "OwnershipTransferred"; + }); + + expect((eventInit as EventLog).args[0]).to.equal(ZERO_ADDR); + expect((eventInit as EventLog).args[1]).to.equal(SECOND); + + const transactionAdd = await multiOwnable.connect(SECOND).addOwners([SECOND.address, THIRD.address]); + const receiptAdd = await transactionAdd.wait(); + const eventAdd = receiptAdd?.logs.find((log) => { + const eventLog = log as EventLog; + return eventLog?.fragment?.name === "OwnershipTransferred"; + }); + + expect(eventAdd).to.be.undefined; + }); + it("should not add null address", async () => { await expect(multiOwnable.addOwners([ZERO_ADDR])).to.be.revertedWith( "MultiOwnable: zero address can not be added", @@ -75,6 +100,41 @@ describe("MultiOwnable", () => { expect(await multiOwnable.isOwner(SECOND.address)).to.be.false; expect(await multiOwnable.isOwner(FIRST.address)).to.be.true; }); + + it("should emit OwnershipTransferred event only when primary owner is changed", async () => { + await multiOwnable.addOwners([SECOND.address]); + const transactionChanged = await multiOwnable.connect(SECOND).removeOwners([FIRST.address]); + + const receiptChanged = await transactionChanged.wait(); + const eventChanged = receiptChanged?.logs.find((log) => { + const eventLog = log as EventLog; + return eventLog?.fragment?.name === "OwnershipTransferred"; + }); + + expect((eventChanged as EventLog).args[0]).to.equal(FIRST.address); + expect((eventChanged as EventLog).args[1]).to.equal(SECOND.address); + + await multiOwnable.connect(SECOND).addOwners([FIRST.address]); + + const transactionNoChange = await multiOwnable.removeOwners([FIRST.address]); + const receiptNoChange = await transactionNoChange.wait(); + const eventNoChange = receiptNoChange?.logs.find((log) => { + const eventLog = log as EventLog; + return eventLog?.fragment?.name === "OwnershipTransferred"; + }); + + expect(eventNoChange).to.be.undefined; + + const transactionEmpty = await multiOwnable.connect(SECOND).removeOwners([SECOND.address]); + const receiptEmpty = await transactionEmpty.wait(); + const eventEmpty = receiptEmpty?.logs.find((log) => { + const eventLog = log as EventLog; + return eventLog?.fragment?.name === "OwnershipTransferred"; + }); + + expect((eventEmpty as EventLog).args[0]).to.equal(SECOND.address); + expect((eventEmpty as EventLog).args[1]).to.equal(ZERO_ADDR); + }); }); describe("renounceOwnership()", () => { @@ -93,6 +153,12 @@ describe("MultiOwnable", () => { }); }); + describe("owner()", () => { + it("should correctly get the primary owner", async () => { + expect(await multiOwnable.owner()).to.be.equal(FIRST.address); + }); + }); + describe("isOwner()", () => { it("should correctly check the initial owner", async () => { expect(await multiOwnable.isOwner(FIRST.address)).to.be.true; From 7ea477c9c9252784aef6c15d68d867065f40a680 Mon Sep 17 00:00:00 2001 From: mllwchrry Date: Fri, 31 May 2024 12:55:56 +0300 Subject: [PATCH 2/4] refactored event tests --- test/access/MultiOwnable.test.ts | 59 ++++++++------------------------ 1 file changed, 14 insertions(+), 45 deletions(-) diff --git a/test/access/MultiOwnable.test.ts b/test/access/MultiOwnable.test.ts index 4489c66d..48cdc7cf 100644 --- a/test/access/MultiOwnable.test.ts +++ b/test/access/MultiOwnable.test.ts @@ -5,7 +5,6 @@ import { Reverter } from "@/test/helpers/reverter"; import { ZERO_ADDR } from "@/scripts/utils/constants"; import { MultiOwnableMock } from "@ethers-v6"; -import { EventLog } from "ethers"; describe("MultiOwnable", () => { const reverter = new Reverter(); @@ -65,24 +64,14 @@ describe("MultiOwnable", () => { const MultiOwnableMock = await ethers.getContractFactory("MultiOwnableMock"); const multiOwnable = await MultiOwnableMock.deploy(); - const transactionInit = await multiOwnable.connect(SECOND).__MultiOwnableMock_init(); - const receiptInit = await transactionInit.wait(); - const eventInit = receiptInit?.logs.find((log) => { - const eventLog = log as EventLog; - return eventLog?.fragment?.name === "OwnershipTransferred"; - }); + await expect(multiOwnable.connect(SECOND).__MultiOwnableMock_init()) + .to.emit(multiOwnable, "OwnershipTransferred") + .withArgs(ZERO_ADDR, SECOND.address); - expect((eventInit as EventLog).args[0]).to.equal(ZERO_ADDR); - expect((eventInit as EventLog).args[1]).to.equal(SECOND); - - const transactionAdd = await multiOwnable.connect(SECOND).addOwners([SECOND.address, THIRD.address]); - const receiptAdd = await transactionAdd.wait(); - const eventAdd = receiptAdd?.logs.find((log) => { - const eventLog = log as EventLog; - return eventLog?.fragment?.name === "OwnershipTransferred"; - }); - - expect(eventAdd).to.be.undefined; + await expect(multiOwnable.connect(SECOND).addOwners([SECOND.address, THIRD.address])).not.to.emit( + multiOwnable, + "OwnershipTransferred", + ); }); it("should not add null address", async () => { @@ -103,37 +92,17 @@ describe("MultiOwnable", () => { it("should emit OwnershipTransferred event only when primary owner is changed", async () => { await multiOwnable.addOwners([SECOND.address]); - const transactionChanged = await multiOwnable.connect(SECOND).removeOwners([FIRST.address]); - - const receiptChanged = await transactionChanged.wait(); - const eventChanged = receiptChanged?.logs.find((log) => { - const eventLog = log as EventLog; - return eventLog?.fragment?.name === "OwnershipTransferred"; - }); - expect((eventChanged as EventLog).args[0]).to.equal(FIRST.address); - expect((eventChanged as EventLog).args[1]).to.equal(SECOND.address); + await expect(multiOwnable.connect(SECOND).removeOwners([FIRST.address])) + .to.emit(multiOwnable, "OwnershipTransferred") + .withArgs(FIRST.address, SECOND.address); await multiOwnable.connect(SECOND).addOwners([FIRST.address]); + await expect(multiOwnable.removeOwners([FIRST.address])).not.to.emit(multiOwnable, "OwnershipTransferred"); - const transactionNoChange = await multiOwnable.removeOwners([FIRST.address]); - const receiptNoChange = await transactionNoChange.wait(); - const eventNoChange = receiptNoChange?.logs.find((log) => { - const eventLog = log as EventLog; - return eventLog?.fragment?.name === "OwnershipTransferred"; - }); - - expect(eventNoChange).to.be.undefined; - - const transactionEmpty = await multiOwnable.connect(SECOND).removeOwners([SECOND.address]); - const receiptEmpty = await transactionEmpty.wait(); - const eventEmpty = receiptEmpty?.logs.find((log) => { - const eventLog = log as EventLog; - return eventLog?.fragment?.name === "OwnershipTransferred"; - }); - - expect((eventEmpty as EventLog).args[0]).to.equal(SECOND.address); - expect((eventEmpty as EventLog).args[1]).to.equal(ZERO_ADDR); + await expect(multiOwnable.connect(SECOND).removeOwners([SECOND.address])) + .to.emit(multiOwnable, "OwnershipTransferred") + .withArgs(SECOND.address, ZERO_ADDR); }); }); From 3c671d99bce606404366cec169dd00ec5d202094 Mon Sep 17 00:00:00 2001 From: mllwchrry Date: Mon, 3 Jun 2024 12:25:24 +0300 Subject: [PATCH 3/4] fixed style issues --- contracts/access/MultiOwnable.sol | 2 ++ 1 file changed, 2 insertions(+) diff --git a/contracts/access/MultiOwnable.sol b/contracts/access/MultiOwnable.sol index 60bd0c36..0f9078d6 100644 --- a/contracts/access/MultiOwnable.sol +++ b/contracts/access/MultiOwnable.sol @@ -99,6 +99,7 @@ abstract contract MultiOwnable is IMultiOwnable, Initializable { */ function _addOwners(address[] memory newOwners_) private { bool isSetEmpty_ = _owners.length() == 0; + _owners.add(newOwners_); require(!_owners.contains(address(0)), "MultiOwnable: zero address can not be added"); @@ -125,6 +126,7 @@ abstract contract MultiOwnable is IMultiOwnable, Initializable { _owners.remove(oldOwners_); address newOwner_ = _owners.length() > 0 ? _owners.at(0) : address(0); + if (newOwner_ != previousOwner_) { emit OwnershipTransferred(previousOwner_, newOwner_); } From d20ce82260916720dcc2e4549d86433ed1272534 Mon Sep 17 00:00:00 2001 From: mllwchrry Date: Mon, 3 Jun 2024 12:37:30 +0300 Subject: [PATCH 4/4] patch version update --- package-lock.json | 4 ++-- package.json | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/package-lock.json b/package-lock.json index 88cf618c..5f8cef30 100644 --- a/package-lock.json +++ b/package-lock.json @@ -1,12 +1,12 @@ { "name": "@solarity/solidity-lib", - "version": "2.7.6", + "version": "2.7.7", "lockfileVersion": 3, "requires": true, "packages": { "": { "name": "@solarity/solidity-lib", - "version": "2.7.6", + "version": "2.7.7", "license": "MIT", "dependencies": { "@openzeppelin/contracts": "4.9.5", diff --git a/package.json b/package.json index f9d25cdb..b0b52671 100644 --- a/package.json +++ b/package.json @@ -1,6 +1,6 @@ { "name": "@solarity/solidity-lib", - "version": "2.7.6", + "version": "2.7.7", "license": "MIT", "author": "Distributed Lab", "readme": "README.md",