chore: address comments

docs/docs/misc/migration_notes.md

@@ -19,10 +19,10 @@ This change was made to communicate that we do not constrain the value in circui
 
 ### [AztecJS] Simulate and get return values for ANY call
 Historically it have been possible to "view" `unconstrained` functions to simulate them and get the return values, but not for `public` nor `private` functions.
-This have lead to a lot of bad code where we have the same function implemented thrice, once in `private`, once in `public` and once in `unconstrained`.
+This has lead to a lot of bad code where we have the same function implemented thrice, once in `private`, once in `public` and once in `unconstrained`. 
 It is not possible to call `simulate` on any call to get the return values! 
-However, beware that it currently always return a Field array of size 4 for private and public.
-This will change to become similar to the return values of the `unconstrained` with proper return times.
+However, beware that it currently always returns a Field array of size 4 for private and public.  
+This will change to become similar to the return values of the `unconstrained` functions with proper return types.
 
 ```diff
 -    #[aztec(private)]

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

@@ -110,6 +110,10 @@ contract DocsExample {
 
     #[aztec(private)]
     fn get_shared_immutable_constrained_private_indirect() -> pub Leader {
+        // This is a private function that calls another private function
+        // and returns the response.
+        // Used to test that we can retrieve values through calls and 
+        // correctly return them in the simulation
         let ret = context.call_private_function_no_args(
             context.this_address(),
             FunctionSelector::from_signature("get_shared_immutable_constrained_private()")
@@ -118,16 +122,20 @@ contract DocsExample {
     }
 
     #[aztec(public)]
-    fn get_shared_immutable_constrained_indirect() -> pub Leader {
+    fn get_shared_immutable_constrained_public_indirect() -> pub Leader {
+        // This is a public function that calls another public function
+        // and returns the response.
+        // Used to test that we can retrieve values through calls and 
+        // correctly return them in the simulation
         let ret = context.call_public_function_no_args(
             context.this_address(),
-            FunctionSelector::from_signature("get_shared_immutable_constrained()")
+            FunctionSelector::from_signature("get_shared_immutable_constrained_public()")
         );
         Leader::deserialize([ret[0], ret[1]])
     }
 
     #[aztec(public)]
-    fn get_shared_immutable_constrained() -> pub Leader {
+    fn get_shared_immutable_constrained_public() -> pub Leader {
         storage.shared_immutable.read_public()
     }
 

yarn-project/aztec.js/src/contract/contract_function_interaction.ts

@@ -100,12 +100,12 @@ export class ContractFunctionInteraction extends BaseContractInteraction {
         authWitnesses: [],
       });
       const simulatedTx = await this.pxe.simulateTx(txRequest, false, options.from ?? this.wallet.getAddress());
-      return simulatedTx.privateReturnValues && simulatedTx.privateReturnValues[0];
+      return simulatedTx.privateReturnValues?.[0];
     } else {
       const txRequest = await this.create();
       const simulatedTx = await this.pxe.simulateTx(txRequest, true);
       this.txRequest = undefined;
-      return simulatedTx.publicReturnValues && simulatedTx.publicReturnValues[0];
+      return simulatedTx.publicReturnValues?.[0];
     }
   }
 }

yarn-project/circuit-types/src/interfaces/pxe.ts

@@ -146,6 +146,26 @@ export interface PXE {
    * Also throws if simulatePublic is true and public simulation reverts.
    */
   proveTx(txRequest: TxExecutionRequest, simulatePublic: boolean): Promise<Tx>;
+
+  /**
+   * Simulates a transaction based on the provided preauthenticated execution request.
+   * This will run a local simulation of private execution (and optionally of public as well), assemble
+   * the zero-knowledge proof for the private execution, and return the transaction object along
+   * with simulation results (return values).
+   *
+   *
+   * Note that this is used with `ContractFunctionInteraction::simulateTx` to bypass certain checks.
+   * In that case, the transaction returned is only potentially ready to be sent to the network for execution.
+   *
+   *
+   * @param txRequest - An authenticated tx request ready for simulation
+   * @param simulatePublic - Whether to simulate the public part of the transaction.
+   * @param msgSender - (Optional) The message sender to use for the simulation.
+   * @returns A simulated transaction object that includes a transaction that is potentially ready
+   * to be sent to the network for execution, along with public and private return values.
+   * @throws If the code for the functions executed in this transaction has not been made available via `addContracts`.
+   * Also throws if simulatePublic is true and public simulation reverts.
+   */
   simulateTx(txRequest: TxExecutionRequest, simulatePublic: boolean, msgSender?: AztecAddress): Promise<SimulatedTx>;
 
   /**

yarn-project/circuit-types/src/mocks.ts

@@ -7,7 +7,7 @@ import {
   computeContractClassId,
   getContractClassFromArtifact,
 } from '@aztec/circuits.js';
-import { type ContractArtifact } from '@aztec/foundation/abi';
+import { type ContractArtifact, type DecodedReturn } from '@aztec/foundation/abi';
 import { makeTuple } from '@aztec/foundation/array';
 import { times } from '@aztec/foundation/collection';
 import { randomBytes } from '@aztec/foundation/crypto';
@@ -19,7 +19,7 @@ import { EncryptedL2Log } from './logs/encrypted_l2_log.js';
 import { EncryptedFunctionL2Logs, EncryptedTxL2Logs, Note, UnencryptedTxL2Logs } from './logs/index.js';
 import { makePrivateKernelTailCircuitPublicInputs, makePublicCallRequest } from './mocks_to_purge.js';
 import { ExtendedNote } from './notes/index.js';
-import { Tx, TxHash } from './tx/index.js';
+import { SimulatedTx, Tx, TxHash } from './tx/index.js';
 
 /**
  * Testing utility to create empty logs composed from a single empty log.
@@ -54,6 +54,12 @@ export const mockTx = (seed = 1, logs = true) => {
   return tx;
 };
 
+export const mockSimulatedTx = (seed = 1, logs = true) => {
+  const tx = mockTx(seed, logs);
+  const dec: DecodedReturn = [1n, 2n, 3n, 4n];
+  return new SimulatedTx(tx, dec, dec);
+};
+
 export const randomContractArtifact = (): ContractArtifact => ({
   name: randomBytes(4).toString('hex'),
   functions: [],

yarn-project/circuit-types/src/tx/index.ts

@@ -1,4 +1,5 @@
 export * from './tx.js';
+export * from './simulated_tx.js';
 export * from './tx_hash.js';
 export * from './tx_receipt.js';
 export * from './processed_tx.js';

yarn-project/circuit-types/src/tx/simulated_tx.test.ts

@@ -0,0 +1,9 @@
+import { mockSimulatedTx } from '../mocks.js';
+import { SimulatedTx } from './simulated_tx.js';
+
+describe('simulated_tx', () => {
+  it('convert to and from json', () => {
+    const simulatedTx = mockSimulatedTx();
+    expect(SimulatedTx.fromJSON(simulatedTx.toJSON())).toEqual(simulatedTx);
+  });
+});

yarn-project/circuit-types/src/tx/simulated_tx.ts

@@ -0,0 +1,68 @@
+import { AztecAddress } from '@aztec/circuits.js';
+import { type ProcessReturnValues } from '@aztec/foundation/abi';
+
+import { Tx } from './tx.js';
+
+export class SimulatedTx {
+  constructor(
+    public tx: Tx,
+    public privateReturnValues?: ProcessReturnValues,
+    public publicReturnValues?: ProcessReturnValues,
+  ) {}
+
+  /**
+   * Convert a SimulatedTx class object to a plain JSON object.
+   * @returns A plain object with SimulatedTx properties.
+   */
+  public toJSON() {
+    const returnToJson = (data: ProcessReturnValues): string => {
+      const replacer = (key: string, value: any): any => {
+        if (typeof value === 'bigint') {
+          return value.toString() + 'n'; // Indicate bigint with "n"
+        } else if (value instanceof AztecAddress) {
+          return value.toString();
+        } else {
+          return value;
+        }
+      };
+      return JSON.stringify(data, replacer);
+    };
+
+    return {
+      tx: this.tx.toJSON(),
+      privateReturnValues: returnToJson(this.privateReturnValues),
+      publicReturnValues: returnToJson(this.publicReturnValues),
+    };
+  }
+
+  /**
+   * Convert a plain JSON object to a Tx class object.
+   * @param obj - A plain Tx JSON object.
+   * @returns A Tx class object.
+   */
+  public static fromJSON(obj: any) {
+    const returnFromJson = (json: string): ProcessReturnValues => {
+      if (json == undefined) {
+        return json;
+      }
+      const reviver = (key: string, value: any): any => {
+        if (typeof value === 'string') {
+          if (value.match(/\d+n$/)) {
+            // Detect bigint serialization
+            return BigInt(value.slice(0, -1));
+          } else if (value.match(/^0x[a-fA-F0-9]{64}$/)) {
+            return AztecAddress.fromString(value);
+          }
+        }
+        return value;
+      };
+      return JSON.parse(json, reviver);
+    };
+
+    const tx = Tx.fromJSON(obj.tx);
+    const privateReturnValues = returnFromJson(obj.privateReturnValues);
+    const publicReturnValues = returnFromJson(obj.publicReturnValues);
+
+    return new SimulatedTx(tx, privateReturnValues, publicReturnValues);
+  }
+}

yarn-project/circuit-types/src/tx/tx.ts

@@ -1,13 +1,11 @@
 import {
-  AztecAddress,
   ContractClassRegisteredEvent,
   PrivateKernelTailCircuitPublicInputs,
   Proof,
   PublicCallRequest,
   SideEffect,
   SideEffectLinkedToNoteHash,
 } from '@aztec/circuits.js';
-import { type ProcessReturnValues } from '@aztec/foundation/abi';
 import { arrayNonEmptyLength } from '@aztec/foundation/collection';
 import { BufferReader, serializeToBuffer } from '@aztec/foundation/serialize';
 
@@ -17,70 +15,6 @@ import { EncryptedTxL2Logs, UnencryptedTxL2Logs } from '../logs/tx_l2_logs.js';
 import { type TxStats } from '../stats/stats.js';
 import { TxHash } from './tx_hash.js';
 
-export class SimulatedTx {
-  constructor(
-    public tx: Tx,
-    public privateReturnValues?: ProcessReturnValues,
-    public publicReturnValues?: ProcessReturnValues,
-  ) {}
-
-  /**
-   * Convert a SimulatedTx class object to a plain JSON object.
-   * @returns A plain object with SimulatedTx properties.
-   */
-  public toJSON() {
-    const returnToJson = (data: ProcessReturnValues): string => {
-      const replacer = (key: string, value: any): any => {
-        if (typeof value === 'bigint') {
-          return value.toString() + 'n'; // Indicate bigint with "n"
-        } else if (value instanceof AztecAddress) {
-          return value.toString();
-        } else {
-          return value;
-        }
-      };
-      return JSON.stringify(data, replacer);
-    };
-
-    return {
-      tx: this.tx.toJSON(),
-      privateReturnValues: returnToJson(this.privateReturnValues),
-      publicReturnValues: returnToJson(this.publicReturnValues),
-    };
-  }
-
-  /**
-   * Convert a plain JSON object to a Tx class object.
-   * @param obj - A plain Tx JSON object.
-   * @returns A Tx class object.
-   */
-  public static fromJSON(obj: any) {
-    const returnFromJson = (json: string): ProcessReturnValues => {
-      if (json == undefined) {
-        return json;
-      }
-      const reviver = (key: string, value: any): any => {
-        if (typeof value === 'string') {
-          if (value.match(/\d+n$/)) {
-            // Detect bigint serialization
-            return BigInt(value.slice(0, -1));
-          } else if (value.match(/^0x[a-fA-F0-9]{64}$/)) {
-            return AztecAddress.fromString(value);
-          }
-        }
-        return value;
-      };
-      return JSON.parse(json, reviver);
-    };
-
-    const tx = Tx.fromJSON(obj.tx);
-    const privateReturnValues = returnFromJson(obj.privateReturnValues);
-    const publicReturnValues = returnFromJson(obj.publicReturnValues);
-
-    return new SimulatedTx(tx, privateReturnValues, publicReturnValues);
-  }
-}
-
 /**
  * The interface of an L2 transaction.
  */

yarn-project/end-to-end/src/e2e_state_vars.test.ts

@@ -34,6 +34,11 @@ describe('e2e_state_vars', () => {
     });
 
     it('private read of SharedImmutable', async () => {
+      // Initializes the shared immutable and then reads the value using an unconstrained function
+      // checking the return values with:
+      // 1. A constrained private function that reads it directly
+      // 2. A constrained private function that calls another private function that reads.
+
       await contract.methods.initialize_shared_immutable(1).send().wait();
 
       const a = await contract.methods.get_shared_immutable_constrained_private().simulate();
@@ -50,8 +55,12 @@ describe('e2e_state_vars', () => {
     });
 
     it('public read of SharedImmutable', async () => {
-      const a = await contract.methods.get_shared_immutable_constrained().simulate();
-      const b = await contract.methods.get_shared_immutable_constrained_indirect().simulate();
+      // Reads the value using an unconstrained function checking the return values with:
+      // 1. A constrained public function that reads it directly
+      // 2. A constrained public function that calls another public function that reads.
+
+      const a = await contract.methods.get_shared_immutable_constrained_public().simulate();
+      const b = await contract.methods.get_shared_immutable_constrained_public_indirect().simulate();
       const c = await contract.methods.get_shared_immutable().simulate();
 
       expect((a as any)[0]).toEqual((c as any)['account'].toBigInt());

yarn-project/pxe/src/pxe_service/pxe_service.ts

@@ -404,6 +404,11 @@ export class PXEService implements PXE {
     return await this.jobQueue.put(async () => {
       const timer = new Timer();
       const simulatedTx = await this.#simulateAndProve(txRequest, msgSender);
+      // We log only if the msgSender is undefined, as simulating with a different msgSender
+      // is unlikely to be a real transaction, and likely to be only used to read data.
+      // Meaning that it will not necessarily have produced a nullifier (and thus have no TxHash)
+      // If we log, the `getTxHash` function will throw.
+
       if (!msgSender) {
         this.log(`Processed private part of ${simulatedTx.tx.getTxHash()}`, {
           eventName: 'tx-pxe-processing',
@@ -619,6 +624,7 @@ export class PXEService implements PXE {
    *
    * @param txExecutionRequest - The transaction request to be simulated and proved.
    * @param signature - The ECDSA signature for the transaction request.
+   * @param msgSender - (Optional) The message sender to use for the simulation.
    * @returns An object tract contains:
    * A private transaction object containing the proof, public inputs, and encrypted logs.
    * The return values of the private execution