feat: Public view functions (unconstrained can read public storage) #1421

LHerskind · 2023-08-04T08:59:38Z

Addresses #1375 by giving the unconstrained execution access to public storage through the node.

Good things:

Makes it simpler to read values

Bad things:

Because the simulator is much slower than reading the value, makes testing slower 😭.

The return value from view functions are an array of bigint but should be altered to be following the abi specification instead for easy access.

Checklist:

Remove the checklist to signal you've completed it. Enable auto-merge if the PR is ready to merge.

If the pull request requires a cryptography review (e.g. cryptographic algorithm implementations) I have added the 'crypto' tag.
I have reviewed my diff in github, line by line and removed unexpected formatting changes, testing logs, or commented-out code.
Every change is related to the PR description.
I have linked this pull request to relevant issues (if any exist).

ludamad · 2023-08-04T13:45:25Z

Would this need later constraints to be used in practice?

LHerskind · 2023-08-04T14:14:20Z

Would this need later constraints to be used in practice?

I don't think so. The intention is just to have unconstrained that can read public storage, as you can then use it to read balances from public without having to compute the storage slots manually for tests and wallets and whatever uses the contracts.

rahul-kothari · 2023-08-04T16:58:45Z

yarn-project/noir-contracts/src/contracts/lending_contract/src/main.nr

@@ -306,4 +306,22 @@ contract Lending {
        debt_loc.write(static_debt - amount);
        1
    }
+
+    unconstrained fn getTot(


consider a better name than tot

Unrelated to this Pr. The struct is called tot so just used that naming here. Planning to rewrite most of the lending contract when time permits.

rahul-kothari · 2023-08-04T16:59:56Z

yarn-project/noir-contracts/src/contracts/non_native_token_contract/src/main.nr

+        owner: Field,
+    ) -> Field {
+        let storage = Storage::init();
+        storage.public_balances.at(owner).read()


I assume if the user has nothing, this returns a 0 as opposed to undefined or whatever?

rahul-kothari · 2023-08-04T17:01:16Z

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

@@ -95,8 +91,8 @@ describe('e2e_public_token_contract', () => {
    expect(receipts.map(r => r.status)).toEqual(times(3, () => TxStatus.MINED));
    expect(receipts.map(r => r.blockNumber)).toEqual(times(3, () => receipts[0].blockNumber));

-    const balance = await cc.l2.loadPublic(contract.address, cc.l2.computeSlotInMap(balanceSlot, recipient.toField()));
-    expect(balance.value).toBe(mintAmount * 3n);
+    const balance = (await contract.methods.publicBalanceOf(recipient.toField()).view({ from: recipient }))[0];


is there a github issue to not have this return an array?

@iAmMichaelConnor have a fix in one of his branches.

rahul-kothari · 2023-08-04T17:05:08Z

yarn-project/acir-simulator/src/client/unconstrained_execution.ts

@@ -54,6 +56,26 @@ export class UnconstrainedFunctionExecution {
      },
      getL1ToL2Message: ([msgKey]) => this.context.getL1ToL2Message(fromACVMField(msgKey)),
      getCommitment: ([commitment]) => this.context.getCommitment(this.contractAddress, commitment),
+      storageRead: async ([slot], [numberOfElements]) => {


where is the oracle call in noir that tells the VM to call storageRead?

What do you mean? It is the same as with public execution? The .read() ends up calling oracles in here.
https://github.com/AztecProtocol/aztec-packages/blob/master/yarn-project/noir-libs/noir-aztec/src/oracle/storage.nr

Maddiaa0 · 2023-08-04T17:03:26Z

yarn-project/acir-simulator/src/client/simulator.ts

@@ -101,6 +102,7 @@ export class AcirSimulator {
    contractAddress: AztecAddress,
    portalContractAddress: EthAddress,
    historicRoots: PrivateHistoricTreeRoots,
+    aztecNode: AztecNode | undefined = undefined,


nicer is

Suggested change

aztecNode: AztecNode | undefined = undefined,

aztecNode?: AztecNode,

Maddiaa0 · 2023-08-04T17:03:37Z

yarn-project/acir-simulator/src/client/unconstrained_execution.ts

   * @returns The return values of the executed function.
   */
-  public async run(): Promise<any[]> {
+  public async run(aztecNode: AztecNode | undefined = undefined): Promise<any[]> {


Maddiaa0 · 2023-08-04T17:04:10Z

yarn-project/acir-simulator/src/client/unconstrained_execution.ts

@@ -54,6 +56,26 @@ export class UnconstrainedFunctionExecution {
      },
      getL1ToL2Message: ([msgKey]) => this.context.getL1ToL2Message(fromACVMField(msgKey)),
      getCommitment: ([commitment]) => this.context.getCommitment(this.contractAddress, commitment),
+      storageRead: async ([slot], [numberOfElements]) => {
+        if (aztecNode === undefined) {


Suggested change

if (aztecNode === undefined) {

if (!aztecNode) {

Maddiaa0 · 2023-08-04T17:05:36Z

yarn-project/acir-simulator/src/client/unconstrained_execution.ts

+            this.log(`Oracle storage read: slot=${storageSlot.toString()} value=undefined`);
+            throw new Error(`Oracle storage read: slot=${storageSlot.toString()} value=undefined`);
+          }
+          const frValue = Fr.fromBuffer(value);


toACVMField can take in a Buffer as an arg. It would be cleaner to just store values as buffers, they can then be converted in one step at the end

Mainly for consistency between the implementations in the executor and here. Also just made it super simple to handle the logging 🤷

Maddiaa0 · 2023-08-04T17:06:17Z

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

+  const getStorageSnapshot = async (contract: LendingContract, aztecNode: AztecRPC, account: Account) => {
+    const storageValues: { [key: string]: Fr } = {};
+    const accountKey = await account.key();
+    const toFields = (res: any) => res[0].map((v: number | bigint | Fr) => new Fr(v));


res should denote an array here

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

Maddiaa0

lgtm, tiny nit around code reuse, change if you want, otherwise shipit

Maddiaa0 · 2023-08-07T16:57:30Z

yarn-project/acir-simulator/src/client/unconstrained_execution.ts

+      storageRead: async ([slot], [numberOfElements]) => {
+        if (!aztecNode) {
+          this.log(`Aztec node is undefined, cannot read public storage`);
+          throw new Error(`Aztec node is undefined, cannot read public storage`);
+        }
+        const startStorageSlot = fromACVMField(slot);
+        const values = [];
+        for (let i = 0; i < Number(numberOfElements); i++) {
+          const storageSlot = startStorageSlot.value + BigInt(i);
+          const value = await aztecNode.getPublicStorageAt(this.contractAddress, storageSlot);
+          if (value === undefined) {
+            this.log(`Oracle storage read: slot=${storageSlot.toString()} value=undefined`);
+            throw new Error(`Oracle storage read: slot=${storageSlot.toString()} value=undefined`);
+          }
+          const frValue = Fr.fromBuffer(value);
+          this.log(`Oracle storage read: slot=${storageSlot.toString()} value=${frValue.toString()}`);
+          values.push(frValue);
+        }
+        return values.map(v => toACVMField(v));
+      },


I feel like the logs here could be tidied up, there is alot of repeated text

Suggested change

storageRead: async ([slot], [numberOfElements]) => {

if (!aztecNode) {

this.log(`Aztec node is undefined, cannot read public storage`);

throw new Error(`Aztec node is undefined, cannot read public storage`);

}

const startStorageSlot = fromACVMField(slot);

const values = [];

for (let i = 0; i < Number(numberOfElements); i++) {

const storageSlot = startStorageSlot.value + BigInt(i);

const value = await aztecNode.getPublicStorageAt(this.contractAddress, storageSlot);

if (value === undefined) {

this.log(`Oracle storage read: slot=${storageSlot.toString()} value=undefined`);

throw new Error(`Oracle storage read: slot=${storageSlot.toString()} value=undefined`);

}

const frValue = Fr.fromBuffer(value);

this.log(`Oracle storage read: slot=${storageSlot.toString()} value=${frValue.toString()}`);

values.push(frValue);

}

return values.map(v => toACVMField(v));

},

storageRead: async ([slot], [numberOfElements]) => {

if (!aztecNode) {

const errorLog = "Aztec node is undefined, cannot read public storage";

this.log(errorLog);

throw new Error(errorLog);

}

const makeLog = (slot, value) => `Oracle storage read: slot=${slot.toString()} value=${value.toString()}`;

const startStorageSlot = fromACVMField(slot);

const values = [];

for (let i = 0; i < Number(numberOfElements); i++) {

const storageSlot = startStorageSlot.value + BigInt(i);

const value = await aztecNode.getPublicStorageAt(this.contractAddress, storageSlot);

if (value === undefined) {

this.log(makeLog(storageSlot, "undefined"));

throw new Error(makeLog(storageSlot, "undefined"));

}

const frValue = Fr.fromBuffer(value);

this.log(makeLog(storageSlot, frValue.toString()));

values.push(frValue);

}

return values.map(v =

LHerskind force-pushed the lh/1375-public-view branch from a8128a6 to 7e7b88c Compare August 4, 2023 12:50

LHerskind force-pushed the lh/1375-public-view branch from 0c21ce6 to faa0903 Compare August 4, 2023 14:44

LHerskind marked this pull request as ready for review August 4, 2023 14:56

LHerskind requested review from rahul-kothari and Maddiaa0 August 4, 2023 14:56

rahul-kothari reviewed Aug 4, 2023

View reviewed changes

Maddiaa0 reviewed Aug 4, 2023

View reviewed changes

rahul-kothari approved these changes Aug 7, 2023

View reviewed changes

LHerskind linked an issue Aug 7, 2023 that may be closed by this pull request

feat: Support unconstrained public functions #1375

Closed

Maddiaa0 approved these changes Aug 7, 2023

View reviewed changes

LHerskind added 8 commits August 7, 2023 20:28

feat: unconstrained can read public state

1ecff5a

test: use view in public token

57eb236

chore: remove useless comment

49a80e8

test: use public views in tests

d68b140

chore: remove useless comment

dd4963b

fix: remove unneeded from

75c63b3

chore: address some comments

f897289

chore: minor cleanup

3efb52d

LHerskind force-pushed the lh/1375-public-view branch from 0d8d779 to 3efb52d Compare August 7, 2023 20:49

LHerskind merged commit 912c1b4 into master Aug 7, 2023

LHerskind deleted the lh/1375-public-view branch August 7, 2023 21:08

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Public view functions (unconstrained can read public storage) #1421

feat: Public view functions (unconstrained can read public storage) #1421

LHerskind commented Aug 4, 2023 •

edited

Loading

ludamad commented Aug 4, 2023

LHerskind commented Aug 4, 2023

rahul-kothari Aug 4, 2023

LHerskind Aug 4, 2023

rahul-kothari Aug 4, 2023

rahul-kothari Aug 4, 2023

LHerskind Aug 4, 2023

rahul-kothari Aug 4, 2023

LHerskind Aug 4, 2023

Maddiaa0 Aug 4, 2023

Maddiaa0 Aug 4, 2023

Maddiaa0 Aug 4, 2023

Maddiaa0 Aug 4, 2023

LHerskind Aug 4, 2023

Maddiaa0 Aug 4, 2023

Maddiaa0 left a comment

Maddiaa0 Aug 7, 2023

	aztecNode: AztecNode \| undefined = undefined,
	aztecNode?: AztecNode,

feat: Public view functions (unconstrained can read public storage) #1421

feat: Public view functions (unconstrained can read public storage) #1421

Conversation

LHerskind commented Aug 4, 2023 • edited Loading

Checklist:

ludamad commented Aug 4, 2023

LHerskind commented Aug 4, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Maddiaa0 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LHerskind commented Aug 4, 2023 •

edited

Loading