feat: internal keyword + lending contract and tests by LHerskind · Pull Request #978 · AztecProtocol/aztec-packages

LHerskind · 2023-07-06T13:25:31Z

Description

Adding checks to ensure that isInternal as introduced in noir-lang/noir#1873 behaves as expected.

Briefly, if isInternal = true then only the contract itself should be able to execute the call. There is a lending protocol example that uses this such that there are both a private and public entry with most logic being in the same internal function.

This PR are only strictly enforcing the check for private functions, as public where not checking the contract tree (see #1200 for more info).

Checklist:

I have reviewed my diff in github, line by line.
Every change is related to the PR description.
I have linked this pull request to the issue(s) that it resolves.
There are no unexpected formatting changes, superfluous debug logs, or commented-out code.
The branch has been merged or rebased against the head of its merge target.
I'm happy for the PR to be merged at the reviewer's next convenience.

netlify · 2023-07-25T19:49:21Z

✅ Deploy Preview for preeminent-bienenstitch-606ad0 canceled.

Name	Link
🔨 Latest commit	`f123135`
🔍 Latest deploy log	https://app.netlify.com/sites/preeminent-bienenstitch-606ad0/deploys/64c25bb6ea685a0008e6a02c

Maddiaa0

Looks good, most of what i have is nits and possible improvements we can make in next steps.

Maddiaa0 · 2023-07-26T11:11:36Z

circuits/cpp/src/aztec3/circuits/abis/function_leaf_preimage.hpp

    using uint32 = typename NCT::uint32;

    uint32 function_selector = 0;
+    boolean is_internal = false;


This being almost the same as function_data makes me think that the types should maybe be unified

Possibly, 3 are overlapping. But the function data is accesible inside noir as well, so maybe the leaf should include the function data: leaf = {function_data, vk_hash, acir_hash}. Seems more intuitive to have the is_constructor stored in the leaf as well. Don't recall how we right now are ensuring the constructors are not rerun @iAmMichaelConnor.

I like the sounds of that

I think perhaps the difference between function_data and function_leaf_preimage (at least, when things were initially designed... having quickly looked back at the diagram, which is basically an extension of my memory at this point)... is...

function_data is information about the function you wish to call, which becomes part of a tx_request. It's separate from any deployment information. Perhaps it should be renamed to target_function or entrypoint_function or something. Given this information, a node which has already synced can lookup the rest of the function's info (vk_hash, acir_hash, etc).

function_leaf_preimage is data required to prove existence of the function in the contracts tree. Some of that data comes from function_data, and some of that data is grabbed by the rpc server.

Some of this is faint memories, so I could be wrong, but I believe their separation was intentional.

Re storing is_constructor in the leaf. In the initial design, the constructor isn't included as a leaf in the tree (because it never needs to be called after deployment); so the constructor's information is only included in the constructor_hash which is included in the contract's address. I'm not sure if that's still the case in the code. I intend to write lots of specs soon, and we'll review all this.

Don't recall how we right now are ensuring the constructors are not rerun
We emit a nullifier = h(contract_address). You can only call a constructor at the time of deployment, and at deployment the nullifier gets emitted, so there's no way to call a constructor twice.

sounds good, it can stay as is!

Maddiaa0 · 2023-07-26T11:11:57Z

circuits/cpp/src/aztec3/circuits/abis/function_leaf_preimage.hpp

 #include "aztec3/utils/types/convert.hpp"
 #include "aztec3/utils/types/native_types.hpp"

+#include "barretenberg/common/serialize.hpp"


Why was this not neede before?

Probably not needed actually, but just leftovers from some fiddling in the middle.

Maddiaa0 · 2023-07-26T11:13:42Z

circuits/cpp/src/aztec3/circuits/kernel/public/common.hpp

    builder.do_assert(public_kernel_inputs.public_call.bytecode_hash != 0,
                      "Bytecode hash must be non-zero",
                      CircuitErrorCode::PUBLIC_KERNEL__BYTECODE_HASH_INVALID);
+


Can you add a cpp test testing this code path, similarly for the other assert

Yep, will do. It's tested in the TS as well.

Maddiaa0 · 2023-07-26T15:30:53Z

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

    });

-    it('parent should call child', async () => {
+    it.only('parent should call child', async () => {


Maddiaa0 · 2023-07-26T15:31:50Z

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

    targetArgs: Fr[],
    callerContext: CallContext,
  ): Promise<PublicCallRequest> {
+    const targetAbi = await this.context.db.getFunctionABI(targetContractAddress, targetFunctionSelector);


nicely done

Maddiaa0 · 2023-07-26T15:56:34Z

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

+      await tx.isMined(0, 0.1);
+      const receipt = await tx.getReceipt();
+      expect(receipt.status).toBe(TxStatus.MINED);
+      logger('Depositing 🥸 : 💰 -> 🏦');


Maddiaa0 · 2023-07-26T15:57:28Z

yarn-project/end-to-end/package.json

    "formatting": "run -T prettier --check ./src && run -T eslint ./src",
    "formatting:fix": "run -T prettier -w ./src",
-    "test": "DEBUG='aztec:*,wasm' NODE_NO_WARNINGS=1 node --experimental-vm-modules $(yarn bin jest) --runInBand --passWithNoTests --testTimeout=15000",
+    "test": "DEBUG='aztec:*e2e*,wasm' NODE_NO_WARNINGS=1 node --experimental-vm-modules $(yarn bin jest) --runInBand --passWithNoTests --testTimeout=15000",


was this commited on purpose?

Whoops. Will fix.

Maddiaa0 · 2023-07-26T15:57:54Z

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

+
+        let asset = storage.assets.at(0);
+
+        let tot = asset.read();


what does tot stand for?

Totals. Normally, would refer to it simply as the asset, but that gets somewhat weird with our storage accessing, maybe the asset becomes asset_loc and then I can do it.

Maddiaa0 · 2023-07-26T15:58:49Z

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

+
+        let (_callStackItem, mut context) = PublicCallStackItem::call(
+            inputs.call_context.storage_contract_address, 
+            1462609836,


damn we really need the sig utilities

Maddiaa0 · 2023-07-26T15:59:40Z

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

+    secret: Field,
+}
+
+impl Account {


this file could use some annotations, im not super familiar with lending protocols its hard to decipher the abbreviations

The lending protocol part of it is a bit shit right now, as there was not enough reads/write to do it proper. But just enough to hit the use for internal and stuff.

Maddiaa0

Looks good, most of what i have is nits and possible improvements we can make in next steps.

rahul-kothari · 2023-07-26T16:48:48Z

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

    });

-    it('parent should call child', async () => {
+    it.only('parent should call child', async () => {


think this got included by mistake

Maddiaa0

lgtm overall, i dont think the bb build system should be changed here though. One that is resolved lets send it

Maddiaa0 · 2023-07-27T11:40:49Z

circuits/cpp/barretenberg/build-system

Should the build system be changed in this pr?

Nope, should be fixed now.

Maddiaa0 · 2023-07-27T11:41:57Z

circuits/cpp/src/aztec3/circuits/abis/function_leaf_preimage.hpp

    using uint32 = typename NCT::uint32;

    uint32 function_selector = 0;
+    boolean is_internal = false;


sounds good, it can stay as is!

Maddiaa0 · 2023-07-27T11:45:11Z

yarn-project/noir-compiler/src/__snapshots__/index.test.ts.snap

      {
        "bytecode": "00000000020000000000000001000000010000000100000000000000000100000030644e72e131a029b85045b68181585d2833e84879b9709143e1f593f000000001000000000000000000000000000000000000000000000000000000000000000000002a",
        "functionType": "open",
+        "isInternal": undefined,


Im assuming this will get fixed over time, and as this folder is not under active development, this is fine for now

Maddiaa0

lgtm

LHerskind changed the title ~~[DO NOT MERGE] feat: noir lending contract and tests~~ [DO NOT MERGE] feat: internal keyword + lending contract and tests Jul 6, 2023

ludamad force-pushed the master branch from 2e89f3f to 12044b4 Compare July 19, 2023 23:33

LHerskind added 8 commits July 25, 2023 08:37

feat: noir contracts and test file

673284a

feat: include is_internal in kernels

c4b6264

feat: assume isInternal unless told otherwise + update snaps

684e14b

test: cleanup tests

46d6e9e

chore: update noir

0f62615

chore: recompile noir

bc58383

fix: rebase issues and clean test

876df6b

chore: prettier + update name

7aa3e02

LHerskind force-pushed the lh/noir-example-lending branch from 056ace0 to 7aa3e02 Compare July 25, 2023 19:49

LHerskind and others added 8 commits July 25, 2023 19:56

chore: fix snapshots

6191d5e

fix: add to ci

b3f6e88

Merge branch 'master' into lh/noir-example-lending

cfe0493

chore: minor fixes

1245143

fix: update noir FunctionData (cannot satisfy 💀)

3689ab4

fix: serialization

c6a2b12

Merge branch 'master' into lh/noir-example-lending

9b22e07

fix: bump lending contract

2c6102d

LHerskind marked this pull request as ready for review July 26, 2023 11:45

LHerskind changed the title ~~[DO NOT MERGE] feat: internal keyword + lending contract and tests~~ feat: internal keyword + lending contract and tests Jul 26, 2023

LHerskind mentioned this pull request Jul 26, 2023

feat: Public Kernel must check contract membership #1200

Open

LHerskind requested review from Maddiaa0 and rahul-kothari July 26, 2023 15:10

Maddiaa0 requested changes Jul 26, 2023

View reviewed changes

Maddiaa0 reviewed Jul 26, 2023

View reviewed changes

rahul-kothari reviewed Jul 26, 2023

View reviewed changes

LHerskind added 2 commits July 27, 2023 09:25

chore: address nits

25cf778

Merge branch 'master' into lh/noir-example-lending

746b74e

LHerskind added 2 commits July 27, 2023 10:42

chore: recompile noir

fac6b3c

Merge branch 'master' into lh/noir-example-lending

b41a7dc

Maddiaa0 reviewed Jul 27, 2023

View reviewed changes

chore: recompile noir

f123135

LHerskind enabled auto-merge (squash) July 27, 2023 12:55

Maddiaa0 approved these changes Jul 27, 2023

View reviewed changes

LHerskind merged commit ab19ce1 into master Jul 27, 2023

LHerskind deleted the lh/noir-example-lending branch July 27, 2023 13:03

Conversation

LHerskind commented Jul 6, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Checklist:

Uh oh!

netlify bot commented Jul 25, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

✅ Deploy Preview for preeminent-bienenstitch-606ad0 canceled.

Uh oh!

Maddiaa0 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Maddiaa0 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Maddiaa0 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Maddiaa0 left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

LHerskind commented Jul 6, 2023 •

edited

Loading

netlify bot commented Jul 25, 2023 •

edited

Loading