feat: addressing Nico's router comments (#8384)

cspell.json

@@ -217,6 +217,7 @@
     "REALDIR",
     "realpath",
     "rebundle",
+    "reentrancy",
     "repr",
     "Reserialize",
     "retag",
@@ -278,6 +279,7 @@
     "Validium",
     "vals",
     "viem",
+    "Vyper",
     "wasms",
     "webassembly",
     "WITGEN",

docs/docs/aztec/glossary/call_types.md

@@ -116,22 +116,34 @@ It is also possible to create public functions that can _only_ be invoked by pri
 
 A common pattern is to enqueue public calls to check some validity condition on public state, e.g. that a deadline has not expired or that some public value is set.
 
-#include_code enqueueing /noir-projects/noir-contracts/contracts/router_contract/src/main.nr rust
+#include_code enqueueing /noir-projects/noir-contracts/contracts/router_contract/src/utils.nr rust
 
-Note that this reveals what public function is being called on what contract.
-For this reason we've created a canonical router contract which implements some of the checks commonly performed.
-This conceals what contract performed the public call as the `context.msg_sender()` in the public function is the router itself (since the router's private function enqueued the public call).
+Note that this reveals what public function is being called on what contract, and perhaps more importantly which contract enqueued the call during private execution.
+For this reason we've created a canonical router contract which implements some of the checks commonly performed: this conceals the calling contract, as the `context.msg_sender()` in the public function will be the router itself (since it is the router that enqueues the public call).
 
 An example of how a deadline can be checked using the router contract follows:
 
 #include_code call-check-deadline /noir-projects/noir-contracts/contracts/crowdfunding_contract/src/main.nr rust
 
+`privately_check_timestamp` and `privately_check_block_number` are helper functions around the call to the router contract:
+
+#include_code helper_router_functions /noir-projects/noir-contracts/contracts/router_contract/src/utils.nr rust
+
 This is what the implementation of the check timestamp functionality looks like:
 
 #include_code check_timestamp /noir-projects/noir-contracts/contracts/router_contract/src/main.nr rust
 
+:::note
+Note that the router contract is not currently part of the [aztec-nr repository](https://github.com/AztecProtocol/aztec-nr).
+To add it as a dependency point to the aztec-packages repository instead:
+```toml
+[dependencies]
+aztec = { git = "https://github.com/AztecProtocol/aztec-packages/", tag = "#include_aztec_version", directory = "noir-projects/noir-contracts/contracts/router_contract/src" }
+```
+:::
+
 Even with the router contract achieving good privacy is hard.
-This is especially the case when the value being checked is unique and stored in the contract's public storage.
+For example, if the value being checked against is unique and stored in the contract's public storage, it's then simple to find private transactions that are using that value in the enqueued public reads, and therefore link them to this contract.
 For this reason it is encouraged to try to avoid public function calls and instead privately read [Shared State](../../reference/developer_references/smart_contract_reference/storage/shared_state.md) when possible.
 
 ### Public Execution
@@ -174,11 +186,11 @@ No correctness is guaranteed on the result of `simulate`! Correct execution is e
 
 #### `prove`
 
-This creates and returns a transaction request, which includes proof of correct private execution and side-efects. The request is not broadcast however, and no gas is spent. It is typically used in testing contexts to inspect transaction parameters or to check for execution failure.
+This creates and returns a transaction request, which includes proof of correct private execution and side-effects. The request is not broadcast however, and no gas is spent. It is typically used in testing contexts to inspect transaction parameters or to check for execution failure.
 
 #include_code local-tx-fails /yarn-project/end-to-end/src/guides/dapp_testing.test.ts typescript
 
-Like most Ethereum libraries, `prove` also simulates public execution to try to detect runtime errors that would only occur once the transaction is picked up by the sequencer. This makes `prove` very useful in testing environments, but users shuld be wary of both false positives and negatives in production environments, particularly if the node's data is stale. Public simulation can be skipped by setting the `skipPublicSimulation` flag.
+Like most Ethereum libraries, `prove` also simulates public execution to try to detect runtime errors that would only occur once the transaction is picked up by the sequencer. This makes `prove` very useful in testing environments, but users should be wary of both false positives and negatives in production environments, particularly if the node's data is stale. Public simulation can be skipped by setting the `skipPublicSimulation` flag.
 
 #### `send`
 

docs/docs/tutorials/codealong/contract_tutorials/crowdfunding_contract.md

@@ -93,7 +93,7 @@ aztec = { git="https://github.com/AztecProtocol/aztec-packages/", tag="#include_
 A word about versions:
 
 - Choose the aztec packages version to match your aztec sandbox version
-- Check that your `compiler_version` in Nargo.toml is satisified by your aztec compiler - `aztec-nargo -V`
+- Check that your `compiler_version` in Nargo.toml is satisfied by your aztec compiler - `aztec-nargo -V`
 
 Inside the Crowdfunding contract definition, use the dependency that defines the address type `AztecAddress` (same syntax as Rust)
 
@@ -135,9 +135,9 @@ We read the deadline from public storage in private and use the router contract
 
 #include_code call-check-deadline /noir-projects/noir-contracts/contracts/crowdfunding_contract/src/main.nr rust
 
-We do the check via the router contract to conceal which contract is performing the check (This is achieved by calling a private function on the router contract which then enqueues a call to a public function on the router contract. This then results in the msg_sender in the public call being the router contract.)
+We perform this check via the router contract to not reveal which contract is performing the check - this is achieved by calling a private function on the router contract which then enqueues a call to a public function on the router contract. The result is that `msg_sender` in the public call will then be the router contract.
 Note that the privacy here is dependent upon what deadline value is chosen by the Crowdfunding contract deployer.
-If it's unique to this contract, then we are leaking a privacy.
+If it's unique to this contract, then there'll be a privacy leak regardless, as third parties will be able to observe a deadline check against the Crowdfunding deadline, and therefore infer that the associated transaction is interacting with it.
 
 Now conclude adding all dependencies to the `Crowdfunding` contract:
 

l1-contracts/src/core/libraries/ConstantsGen.sol

@@ -144,7 +144,7 @@ library Constants {
   uint256 internal constant FEE_JUICE_ADDRESS =
     10248142274714515101077825679585135641434041564851038865006795089686437446849;
   uint256 internal constant ROUTER_ADDRESS =
-    8135649085127523915405560812661632604783066942985338123941332115593181690668;
+    7268799613082469933251235702514160327341161584122631177360064643484764773587;
   uint256 internal constant AZTEC_ADDRESS_LENGTH = 1;
   uint256 internal constant GAS_FEES_LENGTH = 2;
   uint256 internal constant GAS_LENGTH = 2;

noir-projects/aztec-nr/aztec/src/note/note_getter/mod.nr

@@ -7,7 +7,7 @@ use crate::note::{
     utils::compute_note_hash_for_read_request
 };
 use crate::oracle;
-use crate::utils::comparison::assert_comparison;
+use crate::utils::comparison::compare;
 
 mod test;
 
@@ -51,11 +51,8 @@ fn check_note_fields<let N: u32>(
         let select = selects.get_unchecked(i).unwrap_unchecked();
         let value_field = extract_property_value_from_selector(serialized_note, select.property_selector);
 
-        assert_comparison(
-            value_field,
-            select.comparator,
-            select.value.to_field(),
-            "Mismatch return note field."
+        assert(
+            compare(value_field, select.comparator, select.value.to_field()), "Mismatch return note field."
         );
     }
 }
@@ -125,7 +122,7 @@ fn constrain_get_notes_internal<Note, let N: u32, let M: u32, PREPROCESSOR_ARGS,
     let filter_args = options.filter_args;
     let filtered_notes = filter_fn(opt_notes, filter_args);
 
-    let notes = crate::utils::collapse::collapse_array(filtered_notes);
+    let notes = crate::utils::collapse_array(filtered_notes);
     let mut note_hashes: BoundedVec<Field, MAX_NOTE_HASH_READ_REQUESTS_PER_CALL> = BoundedVec::new();
 
     // We have now collapsed the sparse array of Options into a BoundedVec. This is a more ergonomic type and also

noir-projects/aztec-nr/aztec/src/note/note_getter_options.nr

@@ -86,7 +86,7 @@ impl<Note, let N: u32, let M: u32, PREPROCESSOR_ARGS, FILTER_ARGS> NoteGetterOpt
     // This method adds a `Select` criterion to the options.
     // It takes a property_selector indicating which field to select,
     // a value representing the specific value to match in that field, and
-    // a comparator (For possible values of comparators, please see the Comparator enum above)
+    // a comparator (For possible values of comparators, please see the Comparator enum from `utils::comparison`)
     pub fn select<T>(
         &mut self,
         property_selector: PropertySelector,

noir-projects/aztec-nr/aztec/src/utils/comparison.nr

@@ -16,137 +16,89 @@ global Comparator = ComparatorEnum {
     GTE: 6,
 };
 
-pub fn assert_comparison<let N: u32>(lhs: Field, operation: u8, rhs: Field, error_msg: str<N>) {
+pub fn compare(lhs: Field, operation: u8, rhs: Field) -> bool {
     // Values are computed ahead of time because circuits evaluate all branches
     let is_equal = lhs == rhs;
     let is_lt = lhs.lt(rhs);
 
     if (operation == Comparator.EQ) {
-        assert(is_equal, error_msg);
+        is_equal
     } else if (operation == Comparator.NEQ) {
-        assert(!is_equal, error_msg);
+        !is_equal
     } else if (operation == Comparator.LT) {
-        assert(is_lt, error_msg);
+        is_lt
     } else if (operation == Comparator.LTE) {
-        assert(is_lt | is_equal, error_msg);
+        is_lt | is_equal
     } else if (operation == Comparator.GT) {
-        assert(!is_lt & !is_equal, error_msg);
+        !is_lt & !is_equal
     } else if (operation == Comparator.GTE) {
-        assert(!is_lt, error_msg);
+        !is_lt
+    } else {
+        assert(false, "Invalid operation");
+        false // Noir would complain without boolean value here
     }
 }
 
 mod test {
-    use super::assert_comparison;
+    use super::compare;
     use super::Comparator;
 
     #[test]
-    fn test_assert_comparison_happy_path() {
+    fn test_compare() {
         let lhs = 10;
         let rhs = 10;
-        assert_comparison(lhs, Comparator.EQ, rhs, "Expected lhs to be equal to rhs");
+        assert(compare(lhs, Comparator.EQ, rhs), "Expected lhs to be equal to rhs");
 
         let lhs = 10;
         let rhs = 11;
-        assert_comparison(lhs, Comparator.NEQ, rhs, "Expected lhs to be not equal to rhs");
+        assert(compare(lhs, Comparator.NEQ, rhs), "Expected lhs to be not equal to rhs");
 
         let lhs = 10;
         let rhs = 11;
-        assert_comparison(lhs, Comparator.LT, rhs, "Expected lhs to be less than rhs");
+        assert(compare(lhs, Comparator.LT, rhs), "Expected lhs to be less than rhs");
 
         let lhs = 10;
         let rhs = 10;
-        assert_comparison(
-            lhs,
-            Comparator.LTE,
-            rhs,
-            "Expected lhs to be less than or equal to rhs"
-        );
+        assert(compare(lhs, Comparator.LTE, rhs), "Expected lhs to be less than or equal to rhs");
 
         let lhs = 11;
         let rhs = 10;
-        assert_comparison(lhs, Comparator.GT, rhs, "Expected lhs to be greater than rhs");
+        assert(compare(lhs, Comparator.GT, rhs), "Expected lhs to be greater than rhs");
 
         let lhs = 10;
         let rhs = 10;
-        assert_comparison(
-            lhs,
-            Comparator.GTE,
-            rhs,
-            "Expected lhs to be greater than or equal to rhs"
-        );
+        assert(compare(lhs, Comparator.GTE, rhs), "Expected lhs to be greater than or equal to rhs");
 
         let lhs = 11;
         let rhs = 10;
-        assert_comparison(
-            lhs,
-            Comparator.GTE,
-            rhs,
-            "Expected lhs to be greater than or equal to rhs"
-        );
-    }
+        assert(compare(lhs, Comparator.GTE, rhs), "Expected lhs to be greater than or equal to rhs");
 
-    #[test(should_fail_with="Expected lhs to be equal to rhs")]
-    fn test_assert_comparison_fail_eq() {
         let lhs = 10;
         let rhs = 11;
-        assert_comparison(lhs, Comparator.EQ, rhs, "Expected lhs to be equal to rhs");
-    }
+        assert(!compare(lhs, Comparator.EQ, rhs), "Expected lhs to be not equal to rhs");
 
-    #[test(should_fail_with="Expected lhs to be not equal to rhs")]
-    fn test_assert_comparison_fail_neq() {
         let lhs = 10;
         let rhs = 10;
-        assert_comparison(lhs, Comparator.NEQ, rhs, "Expected lhs to be not equal to rhs");
-    }
+        assert(!compare(lhs, Comparator.NEQ, rhs), "Expected lhs to not be not equal to rhs");
 
-    #[test(should_fail_with="Expected lhs to be less than rhs")]
-    fn test_assert_comparison_fail_lt() {
         let lhs = 11;
         let rhs = 10;
-        assert_comparison(lhs, Comparator.LT, rhs, "Expected lhs to be less than rhs");
-    }
+        assert(!compare(lhs, Comparator.LT, rhs), "Expected lhs to not be less than rhs");
 
-    #[test(should_fail_with="Expected lhs to be less than or equal to rhs")]
-    fn test_assert_comparison_fail_lte() {
         let lhs = 11;
         let rhs = 10;
-        assert_comparison(
-            lhs,
-            Comparator.LTE,
-            rhs,
-            "Expected lhs to be less than or equal to rhs"
-        );
-    }
+        assert(!compare(lhs, Comparator.LTE, rhs), "Expected lhs to not be less than or equal to rhs");
 
-    #[test(should_fail_with="Expected lhs to be greater than rhs")]
-    fn test_assert_comparison_fail_gt() {
         let lhs = 10;
         let rhs = 10;
-        assert_comparison(lhs, Comparator.GT, rhs, "Expected lhs to be greater than rhs");
-    }
+        assert(!compare(lhs, Comparator.GT, rhs), "Expected lhs to not be greater than rhs");
 
-    #[test(should_fail_with="Expected lhs to be greater than or equal to rhs")]
-    fn test_assert_comparison_fail_gte() {
         let lhs = 10;
         let rhs = 11;
-        assert_comparison(
-            lhs,
-            Comparator.GTE,
-            rhs,
-            "Expected lhs to be greater than or equal to rhs"
-        );
-    }
+        assert(!compare(lhs, Comparator.GTE, rhs), "Expected lhs to not be greater than or equal to rhs");
 
-    #[test(should_fail_with="Expected lhs to be greater than or equal to rhs")]
-    fn test_assert_comparison_fail_gte_2() {
         let lhs = 10;
         let rhs = 11;
-        assert_comparison(
-            lhs,
-            Comparator.GTE,
-            rhs,
-            "Expected lhs to be greater than or equal to rhs"
-        );
+        assert(!compare(lhs, Comparator.GTE, rhs), "Expected lhs to not be greater than or equal to rhs");
     }
 }

noir-projects/aztec-nr/aztec/src/utils/mod.nr

@@ -1,4 +1,6 @@
-mod collapse;
+mod collapse_array;
 mod comparison;
 mod point;
 mod test;
+
+use crate::utils::collapse_array::{collapse_array, verify_collapse_hints};

noir-projects/aztec-nr/aztec/src/utils/test.nr

@@ -1,4 +1,4 @@
-use super::collapse::{collapse_array, verify_collapse_hints};
+use super::collapse_array::{collapse_array, verify_collapse_hints};
 
 #[test]
 fn collapse_empty_array() {

noir-projects/noir-contracts/contracts/app_subscription_contract/src/main.nr

@@ -7,12 +7,12 @@ contract AppSubscription {
     use aztec::{
         prelude::{AztecAddress, Map, PrivateMutable, SharedImmutable},
         encrypted_logs::encrypted_note_emission::{encode_and_encrypt_note, encode_and_encrypt_note_with_keys},
-        keys::getters::get_current_public_keys,
-        protocol_types::constants::{MAX_FIELD_VALUE, ROUTER_ADDRESS}, utils::comparison::Comparator
+        keys::getters::get_current_public_keys, protocol_types::constants::MAX_FIELD_VALUE,
+        utils::comparison::Comparator
     };
     use authwit::auth::assert_current_call_valid_authwit;
     use token::Token;
-    use router::Router;
+    use router::utils::privately_check_block_number;
 
     #[aztec(storage)]
     struct Storage {
@@ -54,7 +54,7 @@ contract AppSubscription {
 
         // We check that the note is not expired. We do that via the router contract to conceal which contract
         // is performing the check.
-        Router::at(ROUTER_ADDRESS).check_block_number(note.expiry_block_number, Comparator.GT).call(&mut context);
+        privately_check_block_number(Comparator.LT, note.expiry_block_number, &mut context);
 
         payload.execute_calls(&mut context, storage.target_address.read_private());
     }
@@ -86,12 +86,13 @@ contract AppSubscription {
             nonce
         ).call(&mut context);
 
-        // Assert that the `expiry_block_number - SUBSCRIPTION_DURATION_IN_BLOCKS < current_block_number`.
+        // Assert that the `current_block_number > expiry_block_number - SUBSCRIPTION_DURATION_IN_BLOCKS`.
         // --> We do that via the router contract to conceal which contract is performing the check.
-        Router::at(ROUTER_ADDRESS).check_block_number(
+        privately_check_block_number(
+            Comparator.GT,
             expiry_block_number - SUBSCRIPTION_DURATION_IN_BLOCKS,
-            Comparator.LT
-        ).call(&mut context);
+            &mut context
+        );
 
         let subscriber_keys = get_current_public_keys(&mut context, subscriber);
         let msg_sender_ovpk_m = get_current_public_keys(&mut context, context.msg_sender()).ovpk_m;

noir-projects/noir-contracts/contracts/crowdfunding_contract/src/main.nr

@@ -4,17 +4,16 @@ contract Crowdfunding {
 
     // docs:start:all-deps
     use dep::aztec::{
-        protocol_types::{address::AztecAddress, constants::ROUTER_ADDRESS},
         encrypted_logs::encrypted_note_emission::encode_and_encrypt_note_with_keys,
-        keys::getters::get_current_public_keys, state_vars::{PrivateSet, SharedImmutable},
-        note::note_getter_options::Comparator
+        keys::getters::get_current_public_keys, prelude::{AztecAddress, PrivateSet, SharedImmutable},
+        utils::comparison::Comparator
     };
     use dep::aztec::unencrypted_logs::unencrypted_event_emission::encode_event;
     // docs:start:import_valuenote
     use dep::value_note::value_note::ValueNote;
     // docs:end:import_valuenote
     use dep::token::Token;
-    use router::Router;
+    use router::utils::privately_check_timestamp;
     // docs:end:all-deps
 
     #[aztec(event)]
@@ -60,7 +59,7 @@ contract Crowdfunding {
         // is performing the check.
         // docs:start:call-check-deadline
         let deadline = storage.deadline.read_private();
-        Router::at(ROUTER_ADDRESS).check_timestamp(deadline, Comparator.GT).call(&mut context);
+        privately_check_timestamp(Comparator.LT, deadline, &mut context);
         // docs:end:call-check-deadline
 
         // docs:start:do-transfer