docs(contracts): update code comments

Author: 0xjei

packages/contracts/contracts/extensions/SemaphoreChecker.sol

@@ -1,61 +1,59 @@
 // SPDX-License-Identifier: MIT
-pragma solidity >=0.8.23 <=0.8.28;
+pragma solidity ^0.8.20;
 
 import {ISemaphore} from "@semaphore-protocol/contracts/interfaces/ISemaphore.sol";
 import {BaseChecker} from "../checker/BaseChecker.sol";
 
 /// @title SemaphoreChecker
-/// @notice Semaphore proof of membership validator.
-/// @dev Extends BaseChecker to implement proof of membership validation logic using Semaphore.
-/// Please note that once a identity is used to register, it cannot be used again, thanks to the nullifier.
+/// @notice Implements proof of membership validation using Semaphore.
+/// @dev Inherits from BaseChecker to extend the validation logic.
+/// Ensures unique identity usage through nullifier tracking.
 contract SemaphoreChecker is BaseChecker {
-    /// @notice Address of the Semaphore contract used for proof of membership validation.
+    /// @notice Address of the Semaphore contract used for proof verification.
     ISemaphore public semaphore;
 
-    /// @notice The unique identifier of the Semaphore group.
-    /// @dev The subject can prove membership only for the group having the following identifier.
+    /// @notice Unique identifier for the Semaphore group.
+    /// @dev Proofs are validated against this specific group ID.
     uint256 public groupId;
 
-    /// @notice Error thrown when the subject sends a proof with an invalid or mismatching prover in the scope.
+    /// @notice Error thrown when the provided proof does not match the subject's address.
     error IncorrectProver();
 
-    /// @notice Error thrown when the subject sends a proof with an invalid or mismatching group id in the scope.
+    /// @notice Error thrown when the provided proof does not match the expected group ID.
     error IncorrectGroupId();
 
-    /// @notice Error thrown when the subject sends an invalid proof.
+    /// @notice Error thrown when the proof is invalid or fails verification.
     error InvalidProof();
 
-    /// @notice Initializes the contract with necessary parameters values.
-    /// @dev Decodes the appended bytes from the clone to set the parameters values.
+    /// @notice Initializes the SemaphoreChecker with the provided Semaphore contract address and group ID.
+    /// @dev Decodes initialization parameters from appended bytes for clone deployments.
     function _initialize() internal override {
         super._initialize();
 
         bytes memory data = _getAppendedBytes();
-
         (address _semaphore, uint256 _groupId) = abi.decode(data, (address, uint256));
 
         semaphore = ISemaphore(_semaphore);
         groupId = _groupId;
     }
 
-    /// @notice Validates whether the subject is a member of the group.
-    /// @dev Decodes the proof from evidence and checks group membership based on proof validity.
-    /// @param subject Address to validate ownership for.
-    /// @param evidence Encoded proof used for validation.
-    /// @return Boolean indicating whether the subject is a member of the group or not.
+    /// @notice Verifies if the given subject is a valid member of the Semaphore group.
+    /// @dev Decodes the evidence to extract the Semaphore proof and validates the subject's membership.
+    /// @param subject The address of the user whose membership is being verified.
+    /// @param evidence Encoded Semaphore proof containing membership verification details.
+    /// @return Boolean indicating the success or failure of the membership verification.
     function _check(address subject, bytes calldata evidence) internal view override returns (bool) {
         super._check(subject, evidence);
 
         ISemaphore.SemaphoreProof memory proof = abi.decode(evidence, (ISemaphore.SemaphoreProof));
 
-        // the scope is (uint256(uint160(_addr)) << 96) | uint256(_num).
-        // this can avoid frontrunning (ie. subject encoded in the scope of the proof).
+        // The proof scope encodes both the subject address and group ID to prevent front-running attacks.
         uint256 _scope = proof.scope;
 
-        // first 20 byte (160bits) are the address.
+        // Extract the subject's address (first 20 bytes, 160 bits) from the scope.
         address _prover = address(uint160(_scope >> 96));
 
-        // remaining 12 bytes (96bits) are for the group identifier.
+        // Extract the group ID (remaining 12 bytes, 96 bits) from the scope.
         uint96 _groupId = uint96(_scope & ((1 << 96) - 1));
 
         if (_prover != subject) revert IncorrectProver();

packages/contracts/contracts/extensions/SemaphoreCheckerFactory.sol

@@ -6,20 +6,19 @@ import {Factory} from "../proxy/Factory.sol";
 
 /// @title SemaphoreCheckerFactory
 /// @notice Factory contract for deploying minimal proxy instances of SemaphoreChecker.
-/// @dev Simplifies deployment of Semaphore checker clones with appended configuration data.
+/// @dev Utilizes the Factory pattern to streamline deployment of SemaphoreChecker clones with configuration data.
 contract SemaphoreCheckerFactory is Factory {
     /// @notice Initializes the factory with the SemaphoreChecker implementation.
+    /// @dev The constructor sets the SemaphoreChecker contract as the implementation for cloning.
     constructor() Factory(address(new SemaphoreChecker())) {}
 
-    /// @notice Deploys a new SemaphoreChecker clone with the specified target Semaphore contract and group identifier.
-    /// @dev Encodes the Semaphore contract and group identifier as configuration data for the clone.
+    /// @notice Deploys a new SemaphoreChecker clone with the specified Semaphore contract and group ID.
+    /// @dev Encodes the Semaphore contract address and group ID as initialization data for the clone.
     /// @param _semaphore Address of the Semaphore contract.
     /// @param _groupId Unique identifier of the Semaphore group.
     function deploy(address _semaphore, uint256 _groupId) public {
         bytes memory data = abi.encode(_semaphore, _groupId);
-
         address clone = super._deploy(data);
-
         SemaphoreChecker(clone).initialize();
     }
 }

packages/contracts/contracts/extensions/SemaphorePolicy.sol

@@ -5,25 +5,26 @@ import {ISemaphore} from "@semaphore-protocol/contracts/interfaces/ISemaphore.so
 import {BasePolicy} from "../policy/BasePolicy.sol";
 
 /// @title SemaphorePolicy
-/// @notice Policy contract enforcing proof of membership of Semaphore group validation.
-/// @dev Extends BasePolicy to add specific behavior for Semaphore proof of membership validation.
+/// @notice Policy contract enforcing Semaphore group membership validation.
+/// @dev Extends BasePolicy to add logic for tracking and preventing nullifier reuse, ensuring one-time proof validity.
 contract SemaphorePolicy is BasePolicy {
-    /// @notice Tracks nullifier spent for each valid proof / check.
+    /// @notice Mapping to track spent nullifiers for each validated proof.
     mapping(uint256 => bool) public spentNullifiers;
 
-    /// @notice Error thrown when the subject sends a proof with an already spent nullifier.
+    /// @notice Error thrown when a proof is submitted with an already spent nullifier.
     error AlreadySpentNullifier();
 
-    /// @notice Returns a trait identifier for the policy.
-    /// @dev Used to identify the policy type.
-    /// @return The trait string "Semaphore".
+    /// @notice Returns the policy trait identifier.
+    /// @dev Identifies the policy as a Semaphore-based validation mechanism.
+    /// @return The trait identifier string "Semaphore".
     function trait() external pure returns (string memory) {
         return "Semaphore";
     }
 
-    /// @notice Internal logic for enforcing checks.
-    /// @param subject Address to enforce the policy on.
-    /// @param evidence Custom validation data.
+    /// @notice Internal enforcement logic to validate proofs and track nullifier usage.
+    /// @dev Decodes the Semaphore proof from evidence and ensures the nullifier hasn't been previously used.
+    /// @param subject Address of the entity being validated.
+    /// @param evidence Encoded Semaphore proof data.
     function _enforce(address subject, bytes calldata evidence) internal override {
         ISemaphore.SemaphoreProof memory proof = abi.decode(evidence, (ISemaphore.SemaphoreProof));
         uint256 _nullifier = proof.nullifier;

packages/contracts/contracts/extensions/SemaphorePolicyFactory.sol

@@ -6,20 +6,18 @@ import {Factory} from "../proxy/Factory.sol";
 
 /// @title SemaphorePolicyFactory
 /// @notice Factory contract for deploying minimal proxy instances of SemaphorePolicy.
-/// @dev Simplifies deployment of Semaphore policy clones with appended configuration data.
+/// @dev Utilizes the Factory pattern to deploy SemaphorePolicy clones with custom configuration data.
 contract SemaphorePolicyFactory is Factory {
     /// @notice Initializes the factory with the SemaphorePolicy implementation.
+    /// @dev The constructor sets the SemaphorePolicy contract as the implementation for cloning.
     constructor() Factory(address(new SemaphorePolicy())) {}
 
     /// @notice Deploys a new SemaphorePolicy clone with the specified checker address.
-    /// @dev Encodes the checker address and caller as configuration data for the clone.
-    /// @param _checker Address of the Semaphore checker to use for validation.
+    /// @dev Encodes the deployer (as owner) and checker address as initialization data for the clone.
+    /// @param _checker Address of the Semaphore checker used for proof validation.
     function deploy(address _checker) public {
-        // Encode the caller (owner) and checker address for appended data.
         bytes memory data = abi.encode(msg.sender, _checker);
-
         address clone = super._deploy(data);
-
         SemaphorePolicy(clone).initialize();
     }
 }

packages/contracts/contracts/test/extensions/Semaphore.t.sol

@@ -466,191 +466,3 @@ contract SemaphoreMockTest is Test {
         semaphoreMock.validateProof(0, validProof);
     }
 }
-
-// contract Voting is Test {
-//     event Registered(address voter);
-//     event Voted(address voter, uint8 option);
-
-//     NFT internal nft;
-//     BaseERC721Checker internal checker;
-//     BaseERC721CheckerFactory internal checkerFactory;
-//     BaseERC721Policy internal policy;
-//     BaseERC721PolicyFactory internal policyFactory;
-//     BaseVoting internal voting;
-
-//     address public deployer = vm.addr(0x1);
-//     address public subject = vm.addr(0x2);
-//     address public notOwner = vm.addr(0x3);
-
-//     function setUp() public virtual {
-//         vm.startPrank(deployer);
-
-//         nft = new NFT();
-
-//         checkerFactory = new BaseERC721CheckerFactory();
-//         policyFactory = new BaseERC721PolicyFactory();
-
-//         vm.recordLogs();
-//         checkerFactory.deploy(address(nft));
-//         Vm.Log[] memory entries = vm.getRecordedLogs();
-//         address checkerClone = address(uint160(uint256(entries[0].topics[1])));
-//         checker = BaseERC721Checker(checkerClone);
-
-//         vm.recordLogs();
-//         policyFactory.deploy(address(checker));
-//         entries = vm.getRecordedLogs();
-//         address policyClone = address(uint160(uint256(entries[0].topics[1])));
-//         policy = BaseERC721Policy(policyClone);
-
-//         voting = new BaseVoting(policy);
-
-//         vm.stopPrank();
-//     }
-
-//     function test_voting_deployed() public view {
-//         assertEq(address(voting.POLICY()), address(policy));
-//         assertEq(voting.hasVoted(subject), false);
-//         assertEq(voting.registered(subject), false);
-//     }
-
-//     function test_register_whenCallerNotTarget_reverts() public {
-//         vm.startPrank(deployer);
-
-//         policy.setTarget(deployer);
-//         nft.mint(subject);
-
-//         vm.stopPrank();
-
-//         vm.startPrank(notOwner);
-
-//         vm.expectRevert(abi.encodeWithSelector(IPolicy.TargetOnly.selector));
-//         voting.register(0);
-
-//         vm.stopPrank();
-//     }
-
-//     function test_register_whenTokenDoesNotExist_reverts() public {
-//         vm.startPrank(deployer);
-
-//         policy.setTarget(address(voting));
-//         nft.mint(subject);
-
-//         vm.stopPrank();
-
-//         vm.startPrank(subject);
-
-//         vm.expectRevert(abi.encodeWithSelector(IERC721Errors.ERC721NonexistentToken.selector, uint256(1)));
-//         voting.register(1);
-
-//         vm.stopPrank();
-//     }
-
-//     function test_register_whenCheckFails_reverts() public {
-//         vm.startPrank(deployer);
-
-//         policy.setTarget(address(voting));
-//         nft.mint(subject);
-
-//         vm.stopPrank();
-
-//         vm.startPrank(notOwner);
-
-//         vm.expectRevert(abi.encodeWithSelector(IPolicy.UnsuccessfulCheck.selector));
-//         voting.register(0);
-
-//         vm.stopPrank();
-//     }
-
-//     function test_register_whenValid_succeeds() public {
-//         vm.startPrank(deployer);
-
-//         policy.setTarget(address(voting));
-//         nft.mint(subject);
-
-//         vm.stopPrank();
-
-//         vm.startPrank(subject);
-
-//         vm.expectEmit(true, true, true, true);
-//         emit Registered(subject);
-
-//         voting.register(0);
-
-//         vm.stopPrank();
-//     }
-
-//     function test_vote_whenNotRegistered_reverts() public {
-//         vm.startPrank(deployer);
-
-//         policy.setTarget(address(voting));
-//         nft.mint(subject);
-
-//         vm.stopPrank();
-
-//         vm.startPrank(subject);
-
-//         vm.expectRevert(abi.encodeWithSelector(BaseVoting.NotRegistered.selector));
-//         voting.vote(0);
-
-//         vm.stopPrank();
-//     }
-
-//     function test_vote_whenInvalidOption_reverts() public {
-//         vm.startPrank(deployer);
-
-//         policy.setTarget(address(voting));
-//         nft.mint(subject);
-
-//         vm.stopPrank();
-
-//         vm.startPrank(subject);
-//         voting.register(0);
-
-//         vm.expectRevert(abi.encodeWithSelector(BaseVoting.InvalidOption.selector));
-//         voting.vote(3);
-
-//         vm.stopPrank();
-//     }
-
-//     function test_vote_whenValid_succeeds() public {
-//         vm.startPrank(deployer);
-
-//         policy.setTarget(address(voting));
-//         nft.mint(subject);
-
-//         vm.stopPrank();
-
-//         vm.startPrank(subject);
-//         voting.register(0);
-
-//         assertEq(voting.registered(subject), true);
-
-//         vm.expectEmit(true, true, true, true);
-//         emit Voted(subject, 0);
-
-//         voting.vote(0);
-
-//         assertEq(voting.hasVoted(subject), true);
-
-//         vm.stopPrank();
-//     }
-
-//     function test_vote_whenAlreadyVoted_reverts() public {
-//         vm.startPrank(deployer);
-
-//         policy.setTarget(address(voting));
-//         nft.mint(subject);
-
-//         vm.stopPrank();
-
-//         vm.startPrank(subject);
-
-//         voting.register(0);
-//         voting.vote(0);
-
-//         vm.expectRevert(abi.encodeWithSelector(BaseVoting.AlreadyVoted.selector));
-//         voting.vote(0);
-
-//         vm.stopPrank();
-//     }
-// }

packages/contracts/contracts/test/extensions/mocks/BaseCheckerMock.sol

@@ -4,10 +4,13 @@ pragma solidity ^0.8.20;
 import {IBaseChecker} from "../../../interfaces/IBaseChecker.sol";
 
 /// @title BaseCheckerMock
-/// @notice This contract is a mock implementation of the IBaseChecker interface for testing purposes.
+/// @notice Mock implementation of the IBaseChecker interface for testing purposes.
+/// @dev Provides a dummy check function that always returns false.
 contract BaseCheckerMock is IBaseChecker {
-    ///@dev mock check() method that returns always false.
-    function check(address /*subject*/, bytes calldata /*evidence*/) external pure override returns (bool) {
+    /// @notice Mock check function that always returns false.
+    /// @dev This function simulates a failed check for testing.
+    /// @return Always returns false.
+    function check(address, bytes calldata) external pure override returns (bool) {
         return false;
     }
 }