chore: add audit comments

backend/README.md

@@ -48,6 +48,10 @@ SIGNATURE_VERIFICATION_MESSAGE="Summa proof of solvency for CryptoExchange" carg
 
 ### Generating Verifiers for Backend
 
+/*
+    Can we replace this whole process with a bash script
+*/
+
 The following steps are optional and are only required if you need to update the verifier contracts for the backend:
 
 1. **Build the Verifier Contracts**:

backend/examples/summa_solvency_flow.rs

@@ -38,13 +38,24 @@ async fn main() -> Result<(), Box<dyn Error>> {
     )
     .unwrap();
 
+    /*
+        I think that this part related to `address_hashes` can be safely removed.
+        The example should be as simple as possible.
+    */
+
     // Retrieve hashed addresses using the `keccak256` method.
     let address_hashes = address_ownership_client
         .get_ownership_proofs()
         .iter()
         .map(|x| keccak256(encode(&[Token::String(x.cex_address.clone())])))
         .collect::<Vec<[u8; 32]>>();
 
+    /*
+        If I understand correctly, the function `dispatch_proof_of_address_ownership` returns a `Result<(), Box<dyn Error>>`.
+        When we call unwrap on the result, it will panic if the result is an error.
+        Is that correct?
+    */
+
     // Dispatch the proof of address ownership.
     // the `dispatch_proof_of_address_ownership` function sends a transaction to the Summa contract.
     address_ownership_client
@@ -97,9 +108,23 @@ async fn main() -> Result<(), Box<dyn Error>> {
     )
     .unwrap();
 
+    /*
+       I still get the error:
+       ``assert_eq` of unit values detected. This will always succeed
+       for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#unit_cmp
+       `#[deny(clippy::unit_cmp)]` on by defaultclippyClick for full compiler diagnostic`
+
+       I think that, similarly to what you do with `dispatch_proof_of_address_ownership` about, we can remove the `assert_eq` statement.
+       Instead, by just calling round.dispatch_solvency_proof().await.unwrap(), it will panic if the result is an error.
+
+    */
     // Sends the solvency proof, which should ideally complete without errors.
     assert_eq!(round.dispatch_solvency_proof().await.unwrap(), ());
 
+    /*
+        Remove the follwing println statement.
+    */
+
     // You can also use the `solvency_proof_submitted_filter` method to check if the solvency proof is submitted.
     // println!("{:?}", summa_contract
     //     .solvency_proof_submitted_filter()
@@ -111,6 +136,10 @@ async fn main() -> Result<(), Box<dyn Error>> {
 
     // 3. Generate Inclusion Proof
     //
+    /*
+        Remove the follwing comment. It seems that what you are doing here is different from what you would do in a production setup.
+    */
+
     // In a production setup, the CEX should first dispatch the solvency proof to update the Merkle sum tree's root before generating any inclusion proofs.
     // Otherwise, users might distrust the provided `root_hash` in the inclusion proof, as it hasn't been published on-chain.
     let inclusion_proof = round.get_proof_of_inclusion(USER_INDEX).unwrap();
@@ -140,6 +169,15 @@ async fn main() -> Result<(), Box<dyn Error>> {
 
     let public_inputs = downloaded_inclusion_proof.get_public_inputs();
 
+    /*
+        Why leaf_hash is public_inputs[0][0] while mst_root is public_inputs[1]?
+        Shouldn't public_inputs be a vector with 2 elements?
+    */
+
+    /*
+        I would specify that the balances are the balances of the user on the CEX at `snapshot_time`.
+    */
+
     // Verify the `leaf_hash` from the proof file.
     // It's assumed that both `user_name` and `balances` are provided by the CEX.
     let user_name = "dxGaEAii".to_string();
@@ -151,6 +189,10 @@ async fn main() -> Result<(), Box<dyn Error>> {
         generate_leaf_hash::<N_ASSETS>(user_name.clone(), balances.clone())
     );
 
+    /*
+        This whole conversion seems unnecessary to me. Look at the comment in `Round`
+    */
+
     // Before verifying `root_hath`, convert type of `proof` and `public_inputs` to the type of `Bytes` and `Vec<U256>`.
     let proof: Bytes = Bytes::from(inclusion_proof.get_proof().clone());
     let public_inputs: Vec<U256> = inclusion_proof

backend/src/apis/address_ownership.rs

@@ -10,6 +10,9 @@ pub struct AddressOwnership {
 }
 
 impl AddressOwnership {
+    /*
+       Build signer outside of the constructor and pass it in as a parameter.
+    */
     pub fn new(
         signer_key: &str,
         chain_id: u64,
@@ -29,6 +32,11 @@ impl AddressOwnership {
         &self.address_ownership_proofs
     }
 
+    /*
+        If I understand correctly, an error in submit_proof_of_address_ownership is propagated to the caller.
+        Is that correct?
+    */
+
     // This function dispatches the proof of address ownership. Before calling this function,
     // ensure externally that the provided `addresses` in `address_ownership_proof` are not already registered
     // on the Summa contract.

backend/src/apis/round.rs

@@ -44,6 +44,11 @@ impl SolvencyProof {
     }
 }
 
+/*
+   Since we are now using the `MstInclusionProof` only for on-chain verification,
+   why don't we replace the data type with something that can be directly consumed by the contract?
+   Similar to `SolvencyProof`.
+*/
 #[derive(Debug, Clone, Serialize, Deserialize)]
 pub struct MstInclusionProof {
     public_inputs: Vec<Vec<Fp>>,
@@ -66,6 +71,9 @@ pub struct Snapshot<const LEVELS: usize, const N_ASSETS: usize, const N_BYTES: u
     trusted_setup: [SetupArtifacts; 2],
 }
 
+/*
+   `timestamp` should be a unix timestamp. Is it enough to use `u64`?
+*/
 pub struct Round<const LEVELS: usize, const N_ASSETS: usize, const N_BYTES: usize> {
     timestamp: u64,
     snapshot: Snapshot<LEVELS, N_ASSETS, N_BYTES>,
@@ -78,6 +86,9 @@ where
     [usize; N_ASSETS + 1]: Sized,
     [usize; 2 * (1 + N_ASSETS)]: Sized,
 {
+    /*
+       Build signer outside of the constructor and pass it in as a parameter.
+    */
     pub fn new(
         signer_key: &str,
         chain_id: u64,
@@ -104,6 +115,10 @@ where
         self.timestamp
     }
 
+    /*
+        If I understand correctly, an error in submit_proof_of_solvency is propagated to the caller.
+        Is that correct?
+    */
     pub async fn dispatch_solvency_proof(&mut self) -> Result<(), Box<dyn Error>> {
         let proof: SolvencyProof = match self.snapshot.generate_proof_of_solvency() {
             Ok(p) => p,
@@ -202,6 +217,10 @@ where
         let circuit =
             MstInclusionCircuit::<LEVELS, N_ASSETS, N_BYTES>::init(self.mst.clone(), user_index);
 
+        /*
+            Why can't we get the `calldata` using `gen_proof_solidity_calldata` similar to `generate_proof_of_solvency`?
+        */
+
         // Currently, default manner of generating a inclusion proof for solidity-verifier.
         let proof = gen_evm_proof_shplonk(
             &self.trusted_setup[0].0,

backend/src/contracts/signer.rs

@@ -41,6 +41,9 @@ impl SummaSigner {
         }
     }
 
+    /*
+    This method is never used. Can it be removed?
+     */
     pub fn get_deployment_address<P: AsRef<Path>>(
         path: P,
         chain_id: u64,

backend/src/tests.rs

@@ -132,6 +132,9 @@ mod test {
             .dispatch_proof_of_address_ownership()
             .await;
 
+        /*
+            Replace it with  assert!(ownership_submitted_result.is_ok());
+        */
         assert_eq!(ownership_submitted_result.is_ok(), true);
 
         let logs = summa_contract
@@ -200,6 +203,9 @@ mod test {
             },
         ];
 
+        /*
+        Check comment in `examples/summa_solvency_flow.rs` for the following line
+         */
         assert_eq!(round.dispatch_solvency_proof().await.unwrap(), ());
 
         // After sending transaction of proof of solvency, logs should be updated
@@ -224,6 +230,10 @@ mod test {
         );
 
         // Test inclusion proof
+
+        /*
+               This whole conversion seems unnecessary to me. Look at the comment in `Round`
+        */
         let inclusion_proof = round.get_proof_of_inclusion(0).unwrap();
         let proof = Bytes::from(inclusion_proof.get_proof().clone());
         let public_inputs: Vec<U256> = inclusion_proof
@@ -244,7 +254,7 @@ mod test {
             .await
             .unwrap();
 
-        assert_eq!(verified, true);
+        assert!(verified);
 
         drop(anvil);
     }