Refined coding standards and cascaded changes to the code base

CODING_STANDARDS.md

@@ -15,7 +15,18 @@ We adhere to the `pep8` standard, using `flake8`, `pylint` and `mypy` to catch f
 
 ## Solidity
 
-As of now, the solidity code does not follow consistent style conventions. Nevertheless, new solidity code must adhere to the [solidity coding standards](https://docs.soliditylang.org/en/develop/style-guide.html). Old code will progressively be updated to converge towards the recommended style.
+Solidity code must adhere to the [solidity coding standards](https://docs.soliditylang.org/en/develop/style-guide.html), with the following amendments:
+- Test functions must end with the suffix `Test` (e.g. `reverseBytesTest`)
+- Private/internal state variables and functions must have an underscore prefix (e.g. `_myInternalFunction`)
+- Virtual functions in abstract contracts must be written last and located at the end of the contract source
+- Function parameters must not be prefixed with an underscore
+- Interface names must have a capital I prefix (e.g. `IERC20`)
+- Library names must have a capital L prefix (e.g. `LPairing`)
+- Test contract names must have a `Test` prefix (e.g. `TestMyContract`)
+- Contract names may not be PascalCase if using PascalCase is introducing confusions in the name (e.g. `BLS12377.sol` vs `BLS12_377.sol`). PascalCase should be used whenever possible
+- Event names must be prefixed by `Log` (e.g. `LogDeposit`)
+
+**Note:** Some of the files of the current code base may not fully comply with the coding standards above. Old code will progressively be updated to converge towards the recommended style.
 
 ## C++
 

client/test_contracts/test_groth16_altbn128_mixer_base.py → client/test_contracts/test_base_mixer_altbn128_contract.py

@@ -170,21 +170,21 @@
 MIXER_CLIENT: Optional[MixerClient] = None
 
 
-class TestGroth16AltBN128MixerBaseContract(TestCase):
+class TestBaseMixerAltBN128Contract(TestCase):
 
     @staticmethod
     def setUpClass() -> None:
-        print("Deploying AltBN128MixerBase_test.sol")
+        print("Deploying TestBaseMixerAltBN128.sol")
         web3, eth = mock.open_test_web3()
         deployer_eth_address = eth.accounts[0]
         _mixer_interface, mixer_instance = mock.deploy_contract(
             eth,
             deployer_eth_address,
-            "Groth16AltBN128MixerBase_test",
+            "TestBaseMixerAltBN128",
             {
-                'mk_depth': ZETH_MERKLE_TREE_DEPTH,
-                'permitted_dispatcher': deployer_eth_address,
-                'vk_hash': VK_HASH,
+                'mkDepth': ZETH_MERKLE_TREE_DEPTH,
+                'permittedDispatcher': deployer_eth_address,
+                'vkHash': VK_HASH,
             })
 
         global WEB3  # pylint: disable=global-statement

client/test_contracts/test_bls12_377_contract.py

@@ -82,12 +82,12 @@ class TestBLS12_377Contract(TestCase):
 
     @staticmethod
     def setUpClass() -> None:
-        print("Deploying BLS12_377_test.sol")
+        print("Deploying TestBLS12_377.sol")
         _web3, eth = mock.open_test_web3()
         _bls12_interface, bls12_instance = mock.deploy_contract(
             eth,
             eth.accounts[0],
-            "BLS12_377_test",
+            "TestBLS12_377",
             {})
         global BLS12_INSTANCE  # pylint: disable=global-statement
         BLS12_INSTANCE = bls12_instance
@@ -96,14 +96,14 @@ def test_bls12_ecadd(self) -> None:
         """
         Check that [6] == [2] + [4]
         """
-        result = BLS12_INSTANCE.functions.testECAdd(G1_2 + G1_4).call()
+        result = BLS12_INSTANCE.functions.ecAddTest(G1_2 + G1_4).call()
         self.assertEqual(G1_6, result)
 
     def test_bls12_ecmul(self) -> None:
         """
         Check that [-8] == -2 * [4]
         """
-        result = BLS12_INSTANCE.functions.testECMul(G1_4 + FR_MINUS_2).call()
+        result = BLS12_INSTANCE.functions.ecMulTest(G1_4 + FR_MINUS_2).call()
         self.assertEqual(G1_MINUS_8, result)
 
     def test_bls12_ecpairing(self) -> None:
@@ -113,9 +113,9 @@ def test_bls12_ecpairing(self) -> None:
         # Note, return result here is uint256(1) or uint256(0) depending on the
         # pairing check result.
         points = G1_6 + G2_4 + G1_3 + G2_8 + G1_4 + G2_4 + G1_MINUS_8 + G2_8
-        result = BLS12_INSTANCE.functions.testECPairing(points).call()
+        result = BLS12_INSTANCE.functions.ecPairingTest(points).call()
         self.assertEqual(1, result)
 
         points = G1_6 + G2_4 + G1_3 + G2_8 + G1_4 + G2_4 + G1_MINUS_8 + G2_4
-        result = BLS12_INSTANCE.functions.testECPairing(points).call()
+        result = BLS12_INSTANCE.functions.ecPairingTest(points).call()
         self.assertEqual(0, result)

client/test_contracts/test_bw6_761_contract.py

@@ -88,12 +88,12 @@ class TestBW6_761Contract(TestCase):
 
     @staticmethod
     def setUpClass() -> None:
-        print("Deploying BW6_761_test.sol")
+        print("Deploying TestBW6_761.sol")
         _web3, eth = mock.open_test_web3()
         _bw6_interface, bw6_instance = mock.deploy_contract(
             eth,
             eth.accounts[0],
-            "BW6_761_test",
+            "TestBW6_761",
             {})
         global BW6_INSTANCE  # pylint: disable=global-statement
         BW6_INSTANCE = bw6_instance
@@ -102,14 +102,14 @@ def test_bw6_ecadd(self) -> None:
         """
         Check that [6] == [2] + [4]
         """
-        result = BW6_INSTANCE.functions.testECAdd(G1_2 + G1_4).call()
+        result = BW6_INSTANCE.functions.ecAddTest(G1_2 + G1_4).call()
         self.assertEqual(G1_6, result)
 
     def test_bw6_ecmul(self) -> None:
         """
         Check that [-8] == -2 * [4]
         """
-        result = BW6_INSTANCE.functions.testECMul(G1_4 + FR_MINUS_2).call()
+        result = BW6_INSTANCE.functions.ecMulTest(G1_4 + FR_MINUS_2).call()
         self.assertEqual(G1_MINUS_8, result)
 
     def test_bw6_ecpairing(self) -> None:
@@ -119,9 +119,9 @@ def test_bw6_ecpairing(self) -> None:
         # Note, return result here is uint256(1) or uint256(0) depending on the
         # pairing check result.
         points = G1_6 + G2_4 + G1_3 + G2_8 + G1_4 + G2_4 + G1_MINUS_8 + G2_8
-        result = BW6_INSTANCE.functions.testECPairing(points).call()
+        result = BW6_INSTANCE.functions.ecPairingTest(points).call()
         self.assertEqual(1, result)
 
         points = G1_6 + G2_4 + G1_3 + G2_8 + G1_4 + G2_4 + G1_MINUS_8 + G2_4
-        result = BW6_INSTANCE.functions.testECPairing(points).call()
+        result = BW6_INSTANCE.functions.ecPairingTest(points).call()
         self.assertEqual(0, result)

client/test_contracts/test_groth16_bls12_377_contract.py

@@ -74,8 +74,8 @@ def setUpClass() -> None:
         contracts_dir = get_contracts_dir()
         contract_instance_desc = InstanceDescription.deploy(
             web3,
-            join(contracts_dir, "Groth16BLS12_377_test.sol"),
-            "Groth16BLS12_377_test",
+            join(contracts_dir, "TestGroth16BLS12_377.sol"),
+            "TestGroth16BLS12_377",
             web3.eth.accounts[0],  # pylint: disable=no-member
             None,
             500000,

client/test_contracts/test_merkle_tree_contract.py

@@ -44,15 +44,15 @@ def setUpClass() -> None:
         _mktree_interface, mktree_instance = mock.deploy_contract(
             eth,
             deployer_eth_address,
-            "MerkleTreeMiMC7_test",
+            "TestMerkleTreeMiMC7",
             {'treeDepth': ZETH_MERKLE_TREE_DEPTH})
         global MKTREE_INSTANCE  # pylint: disable=global-statement
         MKTREE_INSTANCE = mktree_instance
 
     def test_tree_empty(self) -> None:
         mktree = MerkleTree.empty_with_depth(ZETH_MERKLE_TREE_DEPTH, MiMC7())
         expected_root = mktree.recompute_root()
-        root = MKTREE_INSTANCE.functions.testAddLeaves([], []).call()
+        root = MKTREE_INSTANCE.functions.addLeavesTest([], []).call()
         self.assertEqual(expected_root, root, "test_tree_empty")
 
     def _test_partial(self, num_entries: int, step: int = 1) -> None:
@@ -72,7 +72,7 @@ def _test_partial(self, num_entries: int, step: int = 1) -> None:
             print(f"_test_partial: num_entries={num_entries}, cut={cut}")
             first = leaves[:cut]
             second = leaves[cut:]
-            root = MKTREE_INSTANCE.functions.testAddLeaves(first, second).call()
+            root = MKTREE_INSTANCE.functions.addLeavesTest(first, second).call()
             self.assertEqual(
                 expected_root,
                 root,

client/test_contracts/test_mimc_contract.py

@@ -21,8 +21,8 @@ def setUpClass() -> None:
         contracts_dir = get_contracts_dir()
         contract_instance_desc = InstanceDescription.deploy(
             web3,
-            join(contracts_dir, "MiMC_test.sol"),
-            "MiMC_test",
+            join(contracts_dir, "TestMiMC.sol"),
+            "TestMiMC",
             web3.eth.accounts[0],  # pylint: disable=no-member
             None,
             500000,

client/zeth/core/constants.py

@@ -56,7 +56,7 @@ def bit_length_to_byte_length(bit_length: int) -> int:
 # Public value length (v_pub_in and v_pub_out)
 PUBLIC_VALUE_LENGTH: int = 64
 PUBLIC_VALUE_LENGTH_BYTES: int = bit_length_to_byte_length(PUBLIC_VALUE_LENGTH)
-PUBLIC_VALUE_MASK: int = (1 << PUBLIC_VALUE_LENGTH) - 1
+_PUBLIC_VALUE_MASK: int = (1 << PUBLIC_VALUE_LENGTH) - 1
 
 PHI_LENGTH: int = 256
 PHI_LENGTH_BYTES: int = bit_length_to_byte_length(PHI_LENGTH)

client/zeth/core/mixer_client.py

@@ -709,7 +709,7 @@ def joinsplit_sign(
         zksnark, pp, extproof, public_data)
 
     # If for_dispatch_call is set, omit proof from the signature. See
-    # MixerBase.sol.
+    # BaseMixer.sol.
     if not for_dispatch_call:
         h.update(proof_bytes)
 

client/zeth/core/zksnark.py

@@ -496,4 +496,4 @@ def _contract_name(zksnark_name: str, pp: PairingParameters) -> str:
     Given a snark name fragment (as used in contract naming conventions) and
     pairing parameters, determine the full contract name.
     """
-    return zksnark_name + PAIRING_NAME_TO_CONTRACT_NAME[pp.name] + "Mixer"
+    return "Mixer" + zksnark_name + PAIRING_NAME_TO_CONTRACT_NAME[pp.name]

zeth_contracts/contracts/BaseMerkleTree.sol

@@ -5,7 +5,7 @@
 pragma solidity ^0.8.0;
 
 /// Abstract Merkle tree implementation. Child classes should implement the
-/// hash function.
+/// _hash function.
 ///
 /// The Merkle tree implementation must trade-off complexity, storage,
 /// initialization cost, and update & root computation cost.
@@ -17,88 +17,88 @@ abstract contract BaseMerkleTree
 {
     // Depth of the merkle tree (should be set with the same depth set in the
     // cpp prover)
-    uint256 internal constant DEPTH = 32;
+    uint256 internal constant _DEPTH = 32;
 
-    // Number of leaves
-    uint256 internal constant MAX_NUM_LEAVES = 2**DEPTH;
+    // Maximum number of leaves in the tree
+    uint256 internal constant _MAX_NUM_LEAVES = 2**_DEPTH;
 
-    // Number of nodes
-    uint256 internal constant MAX_NUM_NODES = (MAX_NUM_LEAVES * 2) - 1;
+    // Maximum number of nodes in the tree
+    uint256 internal constant _MAX_NUM_NODES = (_MAX_NUM_LEAVES * 2) - 1;
 
-    uint256 internal constant MASK_LS_BIT = ~uint256(1);
+    uint256 internal constant _MASK_LS_BIT = ~uint256(1);
 
-    bytes32 internal constant DEFAULT_LEAF_VALUE = 0x0;
+    bytes32 internal constant _DEFAULT_LEAF_VALUE = 0x0;
 
     // Sparse array of populated leaves of the merkle tree.
-    // Unpopulated leaves have the DEFAULT_LEAF_VALUE.
-    bytes32[MAX_NUM_NODES] internal nodes;
+    // Unpopulated leaves have the _DEFAULT_LEAF_VALUE.
+    bytes32[_MAX_NUM_NODES] internal _nodes;
 
-    // Number of leaves populated in `nodes`.
-    uint256 internal numLeaves;
+    // Number of leaves populated in `_nodes`.
+    uint256 internal _numLeaves;
 
     /// Constructor
     constructor(uint256 treeDepth) {
         require (
-            treeDepth == DEPTH,
+            treeDepth == _DEPTH,
             "Invalid depth in BaseMerkleTree"
         );
-        initializeTree();
+        _initializeTree();
     }
 
     /// Appends a commitment to the tree, and returns its address
     function insert(bytes32 commitment) public {
         // If this require fails => the merkle tree is full, we can't append
         // leaves anymore.
         require(
-            numLeaves < MAX_NUM_LEAVES,
+            _numLeaves < _MAX_NUM_LEAVES,
             "Merkle tree full: Cannot append anymore"
         );
 
         // Address of the next leaf is the current number of leaves (before
         // insertion).  Compute the next index in the full set of nodes, and
         // write.
-        uint256 next_address = numLeaves;
-        ++numLeaves;
-        uint256 next_entry_idx = (MAX_NUM_LEAVES - 1) + next_address;
-        nodes[next_entry_idx] = commitment;
+        uint256 next_address = _numLeaves;
+        ++_numLeaves;
+        uint256 next_entry_idx = (_MAX_NUM_LEAVES - 1) + next_address;
+        _nodes[next_entry_idx] = commitment;
     }
 
-    /// Abstract hash function to be supplied by a concrete implementation of
+    /// Abstract _hash function to be supplied by a concrete implementation of
     /// this class.
-    function hash(bytes32 left, bytes32 right)
+    function _hash(bytes32 left, bytes32 right)
         internal
         virtual
         returns (bytes32);
 
-    function recomputeRoot(uint numNewLeaves) internal returns (bytes32) {
+    function _recomputeRoot(uint numNewLeaves) internal returns (bytes32) {
         // Assume `numNewLeaves` have been written into the leaf slots.
         // Update any affected nodes in the tree, up to the root, using the
         // default values for any missing nodes.
 
-        uint256 end_idx = numLeaves;
-        uint256 start_idx = numLeaves - numNewLeaves;
-        uint256 layer_size = MAX_NUM_LEAVES;
+        uint256 end_idx = _numLeaves;
+        uint256 start_idx = _numLeaves - numNewLeaves;
+        uint256 layer_size = _MAX_NUM_LEAVES;
 
         while (layer_size > 1) {
             (start_idx, end_idx) =
-                recomputeParentLayer(layer_size, start_idx, end_idx);
+                _recomputeParentLayer(layer_size, start_idx, end_idx);
             layer_size = layer_size / 2;
         }
 
-        return nodes[0];
+        return _nodes[0];
     }
 
-    function initializeTree() private {
+    function _initializeTree() private {
         // First layer
-        bytes32 default_value = DEFAULT_LEAF_VALUE;
-        nodes[2 * MAX_NUM_LEAVES - 2] = default_value;
-        uint256 layer_size = MAX_NUM_LEAVES / 2;
+        bytes32 default_value = _DEFAULT_LEAF_VALUE;
+        _nodes[2 * _MAX_NUM_LEAVES - 2] = default_value;
+        uint256 layer_size = _MAX_NUM_LEAVES / 2;
 
         // Subsequent layers
         while (layer_size > 0) {
-            default_value = hash(default_value, default_value);
+            default_value = _hash(default_value, default_value);
             uint256 layer_final_entry_idx = 2 * layer_size - 2;
-            nodes[layer_final_entry_idx] = default_value;
+            _nodes[layer_final_entry_idx] = default_value;
             layer_size = layer_size / 2;
         }
     }
@@ -118,7 +118,7 @@ abstract contract BaseMerkleTree
     ///
     /// Returns the start and end indices (within the parent layer) of touched
     /// parent nodes.
-    function recomputeParentLayer(
+    function _recomputeParentLayer(
         uint256 childLayerSize,
         uint256 childStartIdx,
         uint256 childEndIdx
@@ -131,10 +131,10 @@ abstract contract BaseMerkleTree
         // Start at the right and iterate left, so we only execute the
         // default_value logic once.  child_left_idx_rend (reverse-end) is the
         // smallest value of child_left_idx at which we should recompute the
-        // parent node hash.
+        // parent node _hash.
 
         uint256 child_left_idx_rend =
-            child_layer_start + (childStartIdx & MASK_LS_BIT);
+            child_layer_start + (childStartIdx & _MASK_LS_BIT);
 
         // If childEndIdx is odd, it is the RIGHT of a computation we need
         // to make. Do the computation using the default value, and move to
@@ -144,8 +144,8 @@ abstract contract BaseMerkleTree
         uint256 child_left_idx;
         if ((childEndIdx & 1) != 0) {
             child_left_idx = child_layer_start + childEndIdx - 1;
-            nodes[(child_left_idx - 1) / 2] =
-                hash(nodes[child_left_idx], nodes[2 * child_layer_start]);
+            _nodes[(child_left_idx - 1) / 2] =
+                _hash(_nodes[child_left_idx], _nodes[2 * child_layer_start]);
         } else {
             child_left_idx = child_layer_start + childEndIdx;
         }
@@ -155,8 +155,8 @@ abstract contract BaseMerkleTree
 
         while (child_left_idx > child_left_idx_rend) {
             child_left_idx = child_left_idx - 2;
-            nodes[(child_left_idx - 1) / 2] =
-                hash(nodes[child_left_idx], nodes[child_left_idx + 1]);
+            _nodes[(child_left_idx - 1) / 2] =
+                _hash(_nodes[child_left_idx], _nodes[child_left_idx + 1]);
         }
 
         return (childStartIdx / 2, (childEndIdx + 1) / 2);