Merge pull request #210 from semaphore-protocol/refactor/general

General refactoring and additional documentation to contracts

Author: cedoor

packages/contracts/contracts/Semaphore.sol

@@ -6,6 +6,12 @@ import "./interfaces/ISemaphoreVerifier.sol";
 import "./base/SemaphoreGroups.sol";
 
 /// @title Semaphore
+/// @dev This contract uses the Semaphore base contracts to provide a complete service
+/// to allow admins to create and manage groups and their members to generate Semaphore proofs
+/// and verify them. Group admins can add, update or remove group members, and can be
+/// an Ethereum account or a smart contract. This contract also assigns each new Merkle tree
+/// generated with a new root a duration (or an expiry) within which the proofs generated with that root
+/// can be validated.
 contract Semaphore is ISemaphore, SemaphoreGroups {
     ISemaphoreVerifier public verifier;
 
@@ -151,12 +157,16 @@ contract Semaphore is ISemaphore, SemaphoreGroups {
         uint256 externalNullifier,
         uint256[8] calldata proof
     ) external override {
-        if (getMerkleTreeDepth(groupId) == 0) {
+        uint256 merkleTreeDepth = getMerkleTreeDepth(groupId);
+
+        if (merkleTreeDepth == 0) {
             revert Semaphore__GroupDoesNotExist();
         }
 
         uint256 currentMerkleTreeRoot = getMerkleTreeRoot(groupId);
 
+        // A proof could have used an old Merkle tree root.
+        // https://github.com/semaphore-protocol/semaphore/issues/98
         if (merkleTreeRoot != currentMerkleTreeRoot) {
             uint256 merkleRootCreationDate = groups[groupId].merkleRootCreationDates[merkleTreeRoot];
             uint256 merkleTreeDuration = groups[groupId].merkleTreeDuration;
@@ -174,8 +184,6 @@ contract Semaphore is ISemaphore, SemaphoreGroups {
             revert Semaphore__YouAreUsingTheSameNillifierTwice();
         }
 
-        uint256 merkleTreeDepth = getMerkleTreeDepth(groupId);
-
         verifier.verifyProof(merkleTreeRoot, nullifierHash, signal, externalNullifier, proof, merkleTreeDepth);
 
         groups[groupId].nullifierHashes[nullifierHash] = true;

packages/contracts/contracts/base/SemaphoreGroups.sol

@@ -6,13 +6,13 @@ import "@zk-kit/incremental-merkle-tree.sol/IncrementalBinaryTree.sol";
 import "@openzeppelin/contracts/utils/Context.sol";
 
 /// @title Semaphore groups contract.
-/// @dev The following code allows you to create groups, add and remove members.
+/// @dev This contract allows you to create groups, add, remove and update members.
 /// You can use getters to obtain informations about groups (root, depth, number of leaves).
 abstract contract SemaphoreGroups is Context, ISemaphoreGroups {
     using IncrementalBinaryTree for IncrementalTreeData;
 
     /// @dev Gets a group id and returns the tree data.
-    mapping(uint256 => IncrementalTreeData) internal merkleTree;
+    mapping(uint256 => IncrementalTreeData) internal merkleTrees;
 
     /// @dev Creates a new group by initializing the associated tree.
     /// @param groupId: Id of the group.
@@ -22,12 +22,12 @@ abstract contract SemaphoreGroups is Context, ISemaphoreGroups {
             revert Semaphore__GroupAlreadyExists();
         }
 
-        // The zeroValue is in fact an implicit member of the group.
+        // The zeroValue is an implicit member of the group, or an implicit leaf of the Merkle tree.
         // Although there is a remote possibility that the preimage of
-        // the hash may be calculated, using this value minimizes the risk.
+        // the hash may be calculated, using this value we aim to minimize the risk.
         uint256 zeroValue = uint256(keccak256(abi.encodePacked(groupId))) >> 8;
 
-        merkleTree[groupId].init(merkleTreeDepth, zeroValue);
+        merkleTrees[groupId].init(merkleTreeDepth, zeroValue);
 
         emit GroupCreated(groupId, merkleTreeDepth, zeroValue);
     }
@@ -40,7 +40,7 @@ abstract contract SemaphoreGroups is Context, ISemaphoreGroups {
             revert Semaphore__GroupDoesNotExist();
         }
 
-        merkleTree[groupId].insert(identityCommitment);
+        merkleTrees[groupId].insert(identityCommitment);
 
         uint256 merkleTreeRoot = getMerkleTreeRoot(groupId);
         uint256 index = getNumberOfMerkleTreeLeaves(groupId) - 1;
@@ -66,7 +66,7 @@ abstract contract SemaphoreGroups is Context, ISemaphoreGroups {
             revert Semaphore__GroupDoesNotExist();
         }
 
-        merkleTree[groupId].update(identityCommitment, newIdentityCommitment, proofSiblings, proofPathIndices);
+        merkleTrees[groupId].update(identityCommitment, newIdentityCommitment, proofSiblings, proofPathIndices);
 
         uint256 merkleTreeRoot = getMerkleTreeRoot(groupId);
         uint256 index = proofPathIndicesToMemberIndex(proofPathIndices);
@@ -90,7 +90,7 @@ abstract contract SemaphoreGroups is Context, ISemaphoreGroups {
             revert Semaphore__GroupDoesNotExist();
         }
 
-        merkleTree[groupId].remove(identityCommitment, proofSiblings, proofPathIndices);
+        merkleTrees[groupId].remove(identityCommitment, proofSiblings, proofPathIndices);
 
         uint256 merkleTreeRoot = getMerkleTreeRoot(groupId);
         uint256 index = proofPathIndicesToMemberIndex(proofPathIndices);
@@ -100,17 +100,17 @@ abstract contract SemaphoreGroups is Context, ISemaphoreGroups {
 
     /// @dev See {ISemaphoreGroups-getMerkleTreeRoot}.
     function getMerkleTreeRoot(uint256 groupId) public view virtual override returns (uint256) {
-        return merkleTree[groupId].root;
+        return merkleTrees[groupId].root;
     }
 
     /// @dev See {ISemaphoreGroups-getMerkleTreeDepth}.
     function getMerkleTreeDepth(uint256 groupId) public view virtual override returns (uint256) {
-        return merkleTree[groupId].depth;
+        return merkleTrees[groupId].depth;
     }
 
     /// @dev See {ISemaphoreGroups-getNumberOfMerkleTreeLeaves}.
     function getNumberOfMerkleTreeLeaves(uint256 groupId) public view virtual override returns (uint256) {
-        return merkleTree[groupId].numberOfLeaves;
+        return merkleTrees[groupId].numberOfLeaves;
     }
 
     /// @dev Converts the path indices of a Merkle proof to the identity commitment index in the tree.

packages/contracts/contracts/base/SemaphoreVerifier.sol

@@ -5,10 +5,10 @@ import "../interfaces/ISemaphoreVerifier.sol";
 
 /// @title Semaphore verifier contract.
 /// @notice Minimal code to allow users to verify their Semaphore proofs.
-/// @dev The following code verifies that the proof is correct and it is a
-/// modified version of the Verifier Groth16 template of SnarkJS
+/// @dev This contract allows you to verify whether a Semaphore proof is correct.
+/// It is a modified version of the Groth16 verifier template of SnarkJS
 /// (https://github.com/iden3/snarkjs) adapted to Semaphore. The Pairing library
-/// is extarnal.
+/// is external.
 contract SemaphoreVerifier is ISemaphoreVerifier {
     using Pairing for *;
 

packages/contracts/contracts/extensions/SemaphoreVoting.sol

@@ -6,6 +6,7 @@ import "../interfaces/ISemaphoreVerifier.sol";
 import "../base/SemaphoreGroups.sol";
 
 /// @title Semaphore voting contract.
+/// @notice It allows users to vote anonymously in a poll.
 /// @dev The following code allows you to create polls, add voters and allow them to vote anonymously.
 contract SemaphoreVoting is ISemaphoreVoting, SemaphoreGroups {
     ISemaphoreVerifier public verifier;

packages/contracts/contracts/extensions/SemaphoreWhistleblowing.sol

@@ -6,9 +6,10 @@ import "../interfaces/ISemaphoreVerifier.sol";
 import "../base/SemaphoreGroups.sol";
 
 /// @title Semaphore whistleblowing contract.
+/// @notice It allows users to leak information anonymously .
 /// @dev The following code allows you to create entities for whistleblowers (e.g. non-profit
-/// organization, newspaper) and to allow them to publish news leaks anonymously.
-/// Leaks can be IPFS hashes, permanent links or other kinds of reference.
+/// organization, newspaper) and allow them to leak anonymously.
+/// Leaks can be IPFS hashes, permanent links or other kinds of references.
 contract SemaphoreWhistleblowing is ISemaphoreWhistleblowing, SemaphoreGroups {
     ISemaphoreVerifier public verifier;
 

packages/contracts/contracts/interfaces/ISemaphore.sol

@@ -1,8 +1,7 @@
 //SPDX-License-Identifier: MIT
 pragma solidity 0.8.4;
 
-/// @title Semaphore interface.
-/// @dev Interface of a Semaphore contract.
+/// @title Semaphore contract interface.
 interface ISemaphore {
     error Semaphore__CallerIsNotTheGroupAdmin();
     error Semaphore__MerkleTreeDepthIsNotSupported();
@@ -42,8 +41,8 @@ interface ISemaphore {
     /// @param signal: Semaphore signal.
     event ProofVerified(
         uint256 indexed groupId,
-        uint256 merkleTreeRoot,
-        uint256 externalNullifier,
+        uint256 indexed merkleTreeRoot,
+        uint256 indexed externalNullifier,
         uint256 nullifierHash,
         uint256 signal
     );

packages/contracts/contracts/interfaces/ISemaphoreGroups.sol

@@ -1,12 +1,10 @@
 //SPDX-License-Identifier: MIT
 pragma solidity 0.8.4;
 
-/// @title SemaphoreGroups interface.
-/// @dev Interface of a SemaphoreGroups contract.
+/// @title SemaphoreGroups contract interface.
 interface ISemaphoreGroups {
     error Semaphore__GroupDoesNotExist();
     error Semaphore__GroupAlreadyExists();
-    error Semaphore__GroupIdIsNotLessThanSnarkScalarField();
 
     /// @dev Emitted when a new group is created.
     /// @param groupId: Id of the group.

packages/contracts/contracts/interfaces/ISemaphoreVerifier.sol

@@ -3,8 +3,7 @@ pragma solidity 0.8.4;
 
 import "../base/Pairing.sol";
 
-/// @title SemaphoreVerifier interface.
-/// @dev Interface of SemaphoreVerifier contract.
+/// @title SemaphoreVerifier contract interface.
 interface ISemaphoreVerifier {
     struct VerificationKey {
         Pairing.G1Point alfa1;
@@ -20,7 +19,7 @@ interface ISemaphoreVerifier {
         Pairing.G1Point C;
     }
 
-    /// @dev Verifies that the zero-knowledge proof is valid.
+    /// @dev Verifies whether a Semaphore proof is valid.
     /// @param merkleTreeRoot: Root of the Merkle tree.
     /// @param nullifierHash: Nullifier hash.
     /// @param signal: Semaphore signal.

packages/contracts/contracts/interfaces/ISemaphoreVoting.sol

@@ -1,8 +1,7 @@
 //SPDX-License-Identifier: MIT
 pragma solidity 0.8.4;
 
-/// @title SemaphoreVoting interface.
-/// @dev Interface of SemaphoreVoting contract.
+/// @title SemaphoreVoting contract interface.
 interface ISemaphoreVoting {
     error Semaphore__CallerIsNotThePollCoordinator();
     error Semaphore__MerkleTreeDepthIsNotSupported();

packages/contracts/contracts/interfaces/ISemaphoreWhistleblowing.sol

@@ -1,8 +1,7 @@
 //SPDX-License-Identifier: MIT
 pragma solidity 0.8.4;
 
-/// @title SemaphoreWhistleblowing interface.
-/// @dev Interface of SemaphoreWhistleblowing contract.
+/// @title SemaphoreWhistleblowing contract interface.
 interface ISemaphoreWhistleblowing {
     error Semaphore__CallerIsNotTheEditor();
     error Semaphore__MerkleTreeDepthIsNotSupported();