addressing feedback

Author: sklppy88

noir-projects/noir-contracts/contracts/inclusion_proofs_contract/src/test.nr

@@ -15,7 +15,8 @@ pub unconstrained fn setup(
     let owner = env.create_account(1);
     env.impersonate(owner);
 
-    // Advance a block so we know that at block 2 our contract has not been deployed yet.
+    // We need to advance two blocks here, because we want to deploy our contract at block 3.
+    // This is because we will do tests later that prove the non inclusion of values and of the contract itself at block 2.
     env.advance_block_by(2);
 
     // Deploy contract and initialize
@@ -30,42 +31,47 @@ pub unconstrained fn setup(
 #[test]
 unconstrained fn note_flow() {
     let (env, contract_address, owner) = setup(INITIAL_VALUE);
-
     env.impersonate(owner);
 
-    let block_number = env.block_number();
+    let note_creation_block_number = env.block_number();
 
     let NOTE_VALUE = 69;
     InclusionProofs::at(contract_address).create_note(owner, NOTE_VALUE).call(&mut env.private());
 
+    // We have to advance two blocks here because if we only advanced it by one, the current block would be 4, and the note creation block would be 3.
+    // But when we do not use the block_number, env.private would get the last block header, which is still 3. Fix this !
+    // TODO: env.block_number() should actually return the private context inputs block number, not the block that is currently being built !
+    // Change env.block_number to subtract one, then just use `env.block_number()`.
     env.advance_block_by(2);
 
     let current_contract_address = get_contract_address();
     cheatcodes::set_contract_address(contract_address);
 
+    // We fetch the note we created and make sure that it is valid.
     let note = InclusionProofs::get_note(owner);
     cheatcodes::set_contract_address(current_contract_address);
 
     assert(note.owner.eq(owner));
     assert(note.value.eq(NOTE_VALUE));
 
+    // Each of these tests (note inclusion, note non-nullification, and validity (inclusion & non-nullification)) check the assertion at the block of creation of note, as well as at the "current" block
     InclusionProofs::at(contract_address)
-        .test_note_inclusion(owner, true, block_number, false)
+        .test_note_inclusion(owner, true, note_creation_block_number, false)
         .call(&mut env.private());
     InclusionProofs::at(contract_address).test_note_inclusion(owner, false, 0, false).call(
         &mut env.private(),
     );
 
     InclusionProofs::at(contract_address)
-        .test_note_not_nullified(owner, true, block_number, false)
+        .test_note_not_nullified(owner, true, note_creation_block_number, false)
         .call(&mut env.private());
     InclusionProofs::at(contract_address).test_note_not_nullified(owner, false, 0, false).call(
         &mut env.private(),
     );
 
-    InclusionProofs::at(contract_address).test_note_validity(owner, true, block_number, false).call(
-        &mut env.private(),
-    );
+    InclusionProofs::at(contract_address)
+        .test_note_validity(owner, true, note_creation_block_number, false)
+        .call(&mut env.private());
     InclusionProofs::at(contract_address).test_note_validity(owner, false, 0, false).call(
         &mut env.private(),
     );
@@ -74,7 +80,6 @@ unconstrained fn note_flow() {
 #[test]
 unconstrained fn nullify_note_flow() {
     let (env, contract_address, owner) = setup(INITIAL_VALUE);
-
     env.impersonate(owner);
 
     let note_valid_block_number = env.block_number();
@@ -87,6 +92,7 @@ unconstrained fn nullify_note_flow() {
 
     env.advance_block_by(1);
 
+    // We test note inclusion at the note creation block and at current block
     InclusionProofs::at(contract_address)
         .test_note_inclusion(owner, true, note_valid_block_number, true)
         .call(&mut env.private());
@@ -95,6 +101,7 @@ unconstrained fn nullify_note_flow() {
         &mut env.private(),
     );
 
+    // We test note non-nullification and validity at the note creation block
     InclusionProofs::at(contract_address)
         .test_note_not_nullified(owner, true, note_valid_block_number, true)
         .call(&mut env.private());
@@ -106,7 +113,6 @@ unconstrained fn nullify_note_flow() {
 #[test(should_fail_with = "Assertion failed: Proving nullifier non-inclusion failed: low_nullifier.value < nullifier.value check failed")]
 unconstrained fn note_not_nullified_after_nullified() {
     let (env, contract_address, owner) = setup(INITIAL_VALUE);
-
     env.impersonate(owner);
 
     InclusionProofs::at(contract_address).create_note(owner, 5).call(&mut env.private());
@@ -127,7 +133,6 @@ unconstrained fn note_not_nullified_after_nullified() {
 #[test(should_fail_with = "Assertion failed: Proving nullifier non-inclusion failed: low_nullifier.value < nullifier.value check failed")]
 unconstrained fn note_not_nullified_after_nullified_no_block_number() {
     let (env, contract_address, owner) = setup(INITIAL_VALUE);
-
     env.impersonate(owner);
 
     InclusionProofs::at(contract_address).create_note(owner, 5).call(&mut env.private());
@@ -146,7 +151,6 @@ unconstrained fn note_not_nullified_after_nullified_no_block_number() {
 #[test(should_fail_with = "Assertion failed: Proving nullifier non-inclusion failed: low_nullifier.value < nullifier.value check failed")]
 unconstrained fn validity_after_nullified() {
     let (env, contract_address, owner) = setup(INITIAL_VALUE);
-
     env.impersonate(owner);
 
     InclusionProofs::at(contract_address).create_note(owner, 5).call(&mut env.private());
@@ -165,7 +169,6 @@ unconstrained fn validity_after_nullified() {
 #[test(should_fail_with = "Assertion failed: Proving nullifier non-inclusion failed: low_nullifier.value < nullifier.value check failed")]
 unconstrained fn validity_after_nullified_no_block_number() {
     let (env, contract_address, owner) = setup(INITIAL_VALUE);
-
     env.impersonate(owner);
 
     InclusionProofs::at(contract_address).create_note(owner, 5).call(&mut env.private());
@@ -184,8 +187,8 @@ unconstrained fn validity_after_nullified_no_block_number() {
 #[test(should_fail_with = "not found in NOTE_HASH_TREE")]
 unconstrained fn note_inclusion_fail_case() {
     let (env, contract_address, owner) = setup(INITIAL_VALUE);
-
     env.impersonate(owner);
+
     let random_owner = AztecAddress::from_field(dep::aztec::oracle::random::random());
 
     let block_number = env.block_number();
@@ -200,8 +203,8 @@ unconstrained fn note_inclusion_fail_case() {
 #[test(should_fail_with = "not found in NOTE_HASH_TREE")]
 unconstrained fn note_inclusion_fail_case_no_block_number() {
     let (env, contract_address, owner) = setup(INITIAL_VALUE);
-
     env.impersonate(owner);
+
     let random_owner = AztecAddress::from_field(dep::aztec::oracle::random::random());
 
     env.advance_block_by(1);
@@ -231,7 +234,6 @@ unconstrained fn nullifier_inclusion() {
 #[test]
 unconstrained fn nullifier_inclusion_public() {
     let (env, contract_address, owner) = setup(INITIAL_VALUE);
-
     env.impersonate(owner);
 
     let unsiloed_nullifier = 0xffffff;
@@ -248,9 +250,10 @@ unconstrained fn nullifier_inclusion_public() {
 #[test(should_fail_with = "Nullifier witness not found for nullifier")]
 unconstrained fn nullifier_non_existence() {
     let (env, contract_address, owner) = setup(INITIAL_VALUE);
+    env.impersonate(owner);
 
     let block_number = env.block_number() - 1;
-    env.impersonate(owner);
+
     let random_nullifier = dep::aztec::oracle::random::random();
 
     InclusionProofs::at(contract_address)
@@ -261,8 +264,8 @@ unconstrained fn nullifier_non_existence() {
 #[test(should_fail_with = "Nullifier witness not found for nullifier")]
 unconstrained fn nullifier_non_existence_no_block_number() {
     let (env, contract_address, owner) = setup(INITIAL_VALUE);
-
     env.impersonate(owner);
+
     let random_nullifier = dep::aztec::oracle::random::random();
 
     InclusionProofs::at(contract_address).test_nullifier_inclusion(random_nullifier, false, 0).call(
@@ -273,7 +276,6 @@ unconstrained fn nullifier_non_existence_no_block_number() {
 #[test]
 unconstrained fn historical_reads() {
     let (env, contract_address, owner) = setup(INITIAL_VALUE);
-
     env.impersonate(owner);
 
     let block_number = env.block_number() - 1;
@@ -298,7 +300,6 @@ unconstrained fn historical_reads() {
 #[test]
 unconstrained fn contract_flow() {
     let (env, contract_address, owner) = setup(INITIAL_VALUE);
-
     env.impersonate(owner);
 
     let block_number = env.block_number() - 1;
@@ -316,7 +317,6 @@ unconstrained fn contract_flow() {
 #[test(should_fail_with = "Nullifier witness not found for nullifier")]
 unconstrained fn test_contract_not_initialized() {
     let (env, contract_address, owner) = setup(INITIAL_VALUE);
-
     env.impersonate(owner);
 
     // We are using block number 2 because we know the contract has not been deployed nor initialized at this point.
@@ -328,11 +328,32 @@ unconstrained fn test_contract_not_initialized() {
 #[test(should_fail_with = "Nullifier witness not found for nullifier")]
 unconstrained fn test_contract_not_deployed() {
     let (env, contract_address, owner) = setup(INITIAL_VALUE);
-
     env.impersonate(owner);
 
     // We are using block number 2 because we know the contract has not been deployed nor initialized at this point.
     InclusionProofs::at(contract_address)
         .test_contract_inclusion(contract_address, 2, false, true)
         .call(&mut env.private());
 }
+
+#[test]
+unconstrained fn test_deployed_contract_not_deployed() {
+    let (env, contract_address, owner) = setup(INITIAL_VALUE);
+    env.impersonate(owner);
+
+    // We are using block number 2 because we know the contract has been deployed nor initialized at this point.
+    InclusionProofs::at(contract_address)
+        .test_contract_non_inclusion(contract_address, 3, true, false)
+        .call(&mut env.private());
+}
+
+#[test]
+unconstrained fn test_deployed_contract_not_initialized() {
+    let (env, contract_address, owner) = setup(INITIAL_VALUE);
+    env.impersonate(owner);
+
+    // We are using block number 2 because we know the contract has been deployed nor initialized at this point.
+    InclusionProofs::at(contract_address)
+        .test_contract_non_inclusion(contract_address, 3, false, true)
+        .call(&mut env.private());
+}