refactor and add test

hackdays-io · Feb 7, 2024 · 532e9fc · 532e9fc
1 parent 4d612f7
commit 532e9fc
Show file tree

Hide file tree

Showing 3 changed files with 76 additions and 41 deletions.
diff --git a/circom b/circom
diff --git a/hardhat/contracts/Event.sol b/hardhat/contracts/Event.sol
@@ -61,6 +61,11 @@ contract EventManager is OwnableUpgradeable {
         private memberRolesByGroupId;
     mapping(uint256 => address[]) private memberAddressesByGroupId;
 
+    modifier onlyGroupOwner(uint256 _groupId) {
+        require(_isGroupOwner(_groupId, msg.sender), "You have no permission");
+        _;
+    }
+
     modifier onlyAdminAccess(uint256 _groupId) {
         require(
             _hasAdminAccess(_groupId, msg.sender),
@@ -116,6 +121,11 @@ contract EventManager is OwnableUpgradeable {
 
     event CreateGroup(address indexed owner, uint256 groupId);
     event CreateEvent(address indexed owner, uint256 eventId);
+    event TransferGroupOwner(
+        address indexed prevOwner,
+        address indexed newOwner,
+        uint256 groupId
+    );
 
     // Currently, reinitializer(3) was executed as constructor.
     function initialize(
@@ -197,20 +207,8 @@ contract EventManager is OwnableUpgradeable {
     function transferGroupOwner(
         uint256 _groupId,
         address _newOwnerAddress
-    ) external whenNotPaused {
-        bool isOwner = false;
-        require(
-            _newOwnerAddress != address(0),
-            "New owner address is blank"
-        );
-
-        for (uint256 i = 0; i < ownGroupIds[msg.sender].length; i++) {
-            if (ownGroupIds[msg.sender][i] == _groupId) {
-                isOwner = true;
-                break;
-            }
-        }
-        require(isOwner, "Caller is not the owner of the group");
+    ) external whenNotPaused onlyGroupOwner(_groupId) {
+        require(_newOwnerAddress != address(0), "New owner address is blank");
 
         for (uint256 i = 0; i < groups.length; i++) {
             if (groups[i].groupId == _groupId) {
@@ -219,17 +217,26 @@ contract EventManager is OwnableUpgradeable {
             }
         }
 
-        // Update the mapping of ownGroupIds for the new owner
         ownGroupIds[_newOwnerAddress].push(_groupId);
 
-        // Remove the groupId from the list for the current owner
         for (uint256 i = 0; i < ownGroupIds[msg.sender].length; i++) {
             if (ownGroupIds[msg.sender][i] == _groupId) {
-                ownGroupIds[msg.sender][i] = ownGroupIds[msg.sender][ownGroupIds[msg.sender].length - 1];
+                ownGroupIds[msg.sender][i] = ownGroupIds[msg.sender][
+                    ownGroupIds[msg.sender].length - 1
+                ];
                 ownGroupIds[msg.sender].pop();
                 break;
             }
         }
+
+        emit TransferGroupOwner(msg.sender, _newOwnerAddress, _groupId);
+    }
+
+    function _isGroupOwner(
+        uint256 _groupId,
+        address _address
+    ) private view returns (bool) {
+        return groups[_groupId - 1].ownerAddress == _address;
     }
 
     function createEventRecord(
@@ -435,7 +442,7 @@ contract EventManager is OwnableUpgradeable {
         require(_groupId > 0 && _groupId <= groups.length, "Invalid groupId");
 
         return
-            groups[_groupId - 1].ownerAddress == _address ||
+            _isGroupOwner(_groupId, _address) ||
             _hasRole(_groupId, _address, ADMIN_ROLE);
     }
 

diff --git a/hardhat/test/EventManager.ts b/hardhat/test/EventManager.ts
@@ -88,7 +88,8 @@ describe("EventManager", function () {
           relayer.address,
           250000,
           1000000,
-          operationController.address],
+          operationController.address,
+        ],
         {
           initializer: "initialize",
         }
@@ -488,7 +489,7 @@ describe("EventManager", function () {
           relayer.address,
           500000,
           1000000,
-          operationController.address
+          operationController.address,
         ],
         {
           initializer: "initialize",
@@ -543,7 +544,7 @@ describe("EventManager", function () {
           relayer.address,
           500000,
           1000000,
-          operationController.address
+          operationController.address,
         ],
         {
           initializer: "initialize",
@@ -618,7 +619,7 @@ describe("EventManager", function () {
           relayer.address,
           250000,
           1000000,
-          operationController.address
+          operationController.address,
         ],
         {
           initializer: "initialize",
@@ -699,49 +700,77 @@ describe("EventManager", function () {
     });
   });
 
-  describe("GetOwnTransfer", function () {
+  describe("Transfer Owner", function () {
     let eventManager: EventManager;
     before(async () => {
-      const eventManagerContractFactory = await ethers.getContractFactory("EventManager");
+      const eventManagerContractFactory = await ethers.getContractFactory(
+        "EventManager"
+      );
       const deployedEventManagerContract = await upgrades.deployProxy(
         eventManagerContractFactory,
         [
           organizer.address,
           relayer.address,
           250000,
           1000000,
-          operationController.address
+          operationController.address,
         ],
         { initializer: "initialize" }
       );
       eventManager = deployedEventManagerContract as EventManager;
       await eventManager.deployed();
       await eventManager.setMintNFTAddr(mintNFT.address);
       await mintNFT.setEventManagerAddr(eventManager.address);
-
-      const createGroupTx = await eventManager.connect(organizer).createGroup("transferGroup");
+
+      const createGroupTx = await eventManager
+        .connect(organizer)
+        .createGroup("transferGroup");
       await createGroupTx.wait();
-  
+
       const createdGroups = await eventManager.getGroups();
-      expect(createdGroups.some(group => group.name === "transferGroup")).to.be.true;
+      expect(createdGroups.some((group) => group.name === "transferGroup")).to
+        .be.true;
     });
-  
+
     it("Should transfer group ownership", async function () {
       const groups = await eventManager.getGroups();
-      const group = groups.find(group => group.name === "transferGroup");
-      expect(group, "Group not found").to.exist;
-
+      const group = groups.find((group) => group.name === "transferGroup");
       const groupId = group!.groupId;
-
-      const transferTx = await eventManager.connect(organizer).transferGroupOwner(groupId, newOwner.address);
+
+      const transferTx = await eventManager
+        .connect(organizer)
+        .transferGroupOwner(groupId, newOwner.address);
       await transferTx.wait();
-
+      expect(transferTx)
+        .to.emit(eventManager, "TransferOwner")
+        .withArgs(groupId, organizer.address, newOwner.address);
+
       const updatedGroups = await eventManager.getGroups();
-      const updatedGroup = updatedGroups.find(group => group.groupId.eq(groupId));
-
+      const updatedGroup = updatedGroups.find((group) =>
+        group.groupId.eq(groupId)
+      );
+
       expect(updatedGroup, "Updated group not found").to.exist;
       expect(updatedGroup!.ownerAddress).to.equal(newOwner.address);
-    });    
+
+      const ownGroups = await eventManager.getOwnGroups(newOwner.address);
+      expect(ownGroups.some((group) => group.groupId.eq(groupId))).to.be.true;
+      const ownGroups2 = await eventManager.getOwnGroups(organizer.address);
+      expect(ownGroups2.every((group) => !group.groupId.eq(groupId))).to.be
+        .true;
+    });
+
+    it("Should revert if not group owner", async function () {
+      const groups = await eventManager.getGroups();
+      const group = groups.find((group) => group.name === "transferGroup");
+      const groupId = group!.groupId;
+
+      await expect(
+        eventManager
+          .connect(participant1)
+          .transferGroupOwner(groupId, newOwner.address)
+      ).to.be.revertedWith("You have no permission");
+    });
   });
 
   describe("Role", async () => {
@@ -758,7 +787,7 @@ describe("EventManager", function () {
           relayer.address,
           250000,
           1000000,
-          operationController.address
+          operationController.address,
         ],
         {
           initializer: "initialize",