feat: complete all tests and refactor test code

src/SemaphoreMSAValidator.sol

@@ -35,7 +35,7 @@
     error InvalidCommitment();
     error InvalidThreshold();
     error MaxMemberReached();
-    error NotSortedAndUnique();
+    error CommitmentsNotUnique();
     error MemberNotExists(address account, uint256 cmt);
     error IsMemberAlready(address acount, uint256 cmt);
     error TxHasBeenInitiated(address account, bytes32 txHash);
@@ -119,12 +119,12 @@
         if (cmts.length > MAX_MEMBERS) revert MaxMemberReached();
         if (cmts.length < threshold) revert InvalidThreshold();
 
-        // Check if all commitments are valid
+        // Check no duplicate commitment and no `0`
+        cmts.insertionSort();
+        if (!cmts.isSortedAndUniquified()) revert CommitmentsNotUnique();
         (bool found,) = cmts.searchSorted(uint256(0));
         if (found) revert InvalidCommitment();
 
-        if (!cmts.isSortedAndUniquified()) revert NotSortedAndUnique();
-
         // Completed all checks by this point. Write to the storage.
         thresholds[account] = threshold;
 
@@ -183,19 +183,19 @@
         emit AddedMember(account, cmt);
     }
 
-    function removeMember(uint256 rmOwner) external moduleInstalled {
+    function removeMember(uint256 cmt) external moduleInstalled {
         address account = msg.sender;
 
         if (memberCount(account) == thresholds[account]) revert CannotRemoveOwner();
 
         uint256 groupId = groupMapping[account];
-        if (!groups.hasMember(groupId, rmOwner)) revert MemberNotExists(account, rmOwner);
+        if (!groups.hasMember(groupId, cmt)) revert MemberNotExists(account, cmt);
 
         //TODO: add the 3rd param: merkleProofSiblings. Now I set it to 0 to make it passes the
         // compiler
-        semaphore.removeMember(groupId, rmOwner, new uint256[](0));
+        semaphore.removeMember(groupId, cmt, new uint256[](0));
 
-        emit RemovedMember(account, rmOwner);
+        emit RemovedMember(account, cmt);
     }
 
     function getNextSeqNum(address account) external view returns (uint256) {
@@ -271,16 +271,20 @@
         uint256 groupId = groupMapping[account];
 
         // Check if the txHash exist
-        ExtCallCount storage cdc = acctTxCount[account][txHash];
-        if (cdc.count == 0) revert TxHashNotFound(account, txHash);
-
-        semaphore.validateProof(groupId, proof);
+        ExtCallCount storage ecc = acctTxCount[account][txHash];
+        if (ecc.count == 0) revert TxHashNotFound(account, txHash);
 
-        cdc.count += 1;
-        emit SignedTx(account, txHash);
+        try semaphore.validateProof(groupId, proof) {
+            ecc.count += 1;
+            emit SignedTx(account, txHash);
 
-        // execute the transaction if condition allows
-        if (execute && cdc.count >= thresholds[account]) executeTx(txHash);
+            // execute the transaction if condition allows
+            if (execute && ecc.count >= thresholds[account]) executeTx(txHash);
+        } catch Error(string memory reason) {
+            revert InvalidSemaphoreProof(bytes(reason));
+        } catch (bytes memory reason) {
+            revert InvalidSemaphoreProof(reason);
+        }
     }
 
     function executeTx(bytes32 txHash) public moduleInstalled returns (bytes memory) {
@@ -289,13 +293,11 @@
         uint8 threshold = thresholds[account];
         ExtCallCount storage ecc = acctTxCount[account][txHash];
 
-        // console.log("executeTx");
         if (ecc.count == 0) revert TxHashNotFound(account, txHash);
         if (ecc.count < threshold) revert ThresholdNotReach(account, threshold, ecc.count);
 
-        // console.log("executeTx - check pass");
-        // REVIEW: Is there a better way to make external contract call given the target address,
-        // value, and call data.
+        // TODO: Review if there a better way to make external contract call given the
+        // target address, value, and call data.
         address payable targetAddr = payable(ecc.targetAddr);
         (bool success, bytes memory returnData) = targetAddr.call{ value: ecc.value }(ecc.callData);
         if (!success) revert ExecuteTxFailure(account, targetAddr, ecc.value, ecc.callData);
@@ -321,12 +323,6 @@
         override
         returns (ValidationData)
     {
-        // console.log("userOp sender: %s", userOp.sender);
-        // console.log("userOp callData:");
-        // console.logBytes(userOp.callData);
-        // console.log("userOpHash:");
-        // console.logBytes32(userOpHash);
-
         // you want to exclude initiateTx, signTx, executeTx from needing tx count.
         // you just need to ensure they are a valid proof from the semaphore group members
         address account = userOp.sender;
@@ -346,7 +342,6 @@
         // Verify if the identity commitment is one of the semaphore group members
         bytes memory pubKey = LibBytes.slice(userOp.signature, 0, 66);
         uint256 cmt = Identity.getCommitment(pubKey);
-
         if (!groups.hasMember(groupId, cmt)) revert MemberNotExists(account, cmt);
 
         // We don't allow call to other contract.
@@ -356,13 +351,8 @@
         // For callData, the first 120 bytes are reserved by ERC-7579 use. Then 32 bytes of value,
         //   then the remaining as the callData passed in getExecOps
         bytes memory valAndCallData = userOp.callData[120:];
-        // (uint256 val) = abi.decode(LibBytes.slice(valAndCallData, 0, 32), (uint256));
         bytes4 funcSel = bytes4(LibBytes.slice(valAndCallData, 32, 36));
 
-        // console.log("val: %s", val);
-        // console.log("funcSel:");
-        // console.logBytes4(funcSel);
-
         // Allow only these few types on function calls to pass, and reject all other on-chain
         //   calls. They must be executed via `executeTx()` function.
         if (_isAllowedSelector(funcSel)) return VALIDATION_SUCCESS;
@@ -371,9 +361,9 @@
     }
 
     function isValidSignatureWithSender(
         address sender,
         bytes32 hash,
         bytes calldata signature
     )
         external
         view

src/utils/ValidatorLibBytes.sol

@@ -1,8 +1,6 @@
 // SPDX-License-Identifier: MIT
 pragma solidity >=0.8.23 <=0.8.29;
 
-import { LibBytes } from "solady/utils/LibBytes.sol";
-
 library ValidatorLibBytes {
     uint8 internal constant CMT_BYTELEN = 32;
 
@@ -11,9 +9,7 @@ library ValidatorLibBytes {
 
         cmts = new uint256[](cmtNum);
         for (uint256 i = 0; i < cmtNum; i++) {
-            bytes memory oneCmtByte =
-                LibBytes.slice(cmtBytes, i * CMT_BYTELEN, (i + 1) * CMT_BYTELEN);
-            cmts[i] = abi.decode(oneCmtByte, (uint256));
+            cmts[i] = uint256(bytes32(cmtBytes[i * CMT_BYTELEN:(i + 1) * CMT_BYTELEN]));
         }
     }
 }

test/Semaphore.t.sol

@@ -19,7 +19,7 @@
 }
 
 contract SemaphoreUnitTest is Test {
-    Semaphore semaphore;
+    Semaphore internal semaphore;
     User[] internal $users;
 
     function setUp() public virtual {
@@ -101,8 +101,8 @@
         ISemaphore.SemaphoreProof memory goodProof = ISemaphore.SemaphoreProof({
             merkleTreeDepth: merkleTreeDepth,
             merkleTreeRoot: merkleTreeRoot,
             nullifier: 9_258_620_728_367_181_689_082_100_997_241_864_348_984_639_649_085_246_237_074_656_141_003_522_567_612,
             message: 32_745_724_963_520_459_128_167_607_516_703_083_632_076_522_816_298_193_357_160_756_506_792_738_947_072,
             scope: groupId,
             points: points
         });