refactor: use mutable refs in noir-contracts by iAmMichaelConnor · Pull Request #1099 · AztecProtocol/aztec-packages

iAmMichaelConnor · 2023-07-18T11:03:46Z

Description

Refactoring:

Pass context everywhere by mutable reference
Push to arrays by mutable reference
And a small change that I initially made: moving call from PrivateCallStackItem and PublicCallStackItem to context.

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.

iAmMichaelConnor · 2023-07-18T16:20:26Z

Blocked by noir-lang/noir#1961, so converting this PR back to 'draft'.

iAmMichaelConnor · 2023-07-29T09:24:23Z

Soon to be unblocked by noir-lang/noir#2087

iAmMichaelConnor · 2023-07-29T21:02:23Z

Only compiles with Jake's PR's branch. Need to wait until that's merged before merging this.

Edit: it's merged!

iAmMichaelConnor · 2023-07-31T15:53:24Z

Not sure why these tests are failing. They're working locally...

spalladino

Thanks for tackling this, it was a major change! I really like the result, and agree with moving the call_function methods into context.

One thing I hadn't noticed before is how some methods silently add the header to a note, which is now made explicit since they require a mutable reference to it. Does it make sense to use different types to represent notes with and without headers, to make it more clear which methods expect the header to be set, and which ones set it? Not for this PR of course.

spalladino · 2023-07-31T16:39:52Z

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

        let balances = storage.balances;

-        context = balances.at(owner).add(context, initial_supply, owner);
+        balances.at(owner).add(&mut context, initial_supply, owner);


Yay, new syntax! Just to check: the compiler will complain if we don't include this &mut keyword, right?

Correct! It results in something slightly more verbose and cumbersome than I'd have liked, because many end users won't be familiar with references, but now they have to be. But I guess Noir emulates Rust syntax, so this is unavoidable.
I think it's to prevent foot guns. The developer has to consciously say "I am aware that the context I am passing to this function could get mutated"

spalladino · 2023-07-31T16:40:50Z

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

        //*********************************/
        initial_supply: Field, 
-        owner: Field,
+        owner: Field


Curious if you removed this manually or you have a noir formatter set up

This must have been a manual change.

spalladino · 2023-07-31T16:43:53Z

yarn-project/noir-libs/noir-aztec/src/note/note_getter.nr

    let is_dummy = note_interface.is_dummy;
    if is_dummy(note) == false {
-        check_note_header(context, storage_slot, note_interface, note);
+        check_note_header(*context, storage_slot, note_interface, note);


Hold on, why *? Do we need to manually dereference the mutable reference?

Yeah, since check_note_header doesn't mutate the context, we want to pass in a Context type rather than an &mut Context type

ludamad · 2023-07-31T16:52:07Z

I like overall how we decided to expose a pretty low level interface and now we're cleaning it up as we go (versus making everything magic from the start, I suppose) LGTM at a glance, really like the change

iAmMichaelConnor · 2023-07-31T16:53:59Z

One thing I hadn't noticed before is how some methods silently add the header to a note, which is now made explicit since they require a mutable reference to it. Does it make sense to use different types to represent notes with and without headers, to make it more clear which methods expect the header to be set, and which ones set it? Not for this PR of course.

I'll tag @LeilaWang on this comment, because Leila did a lot of the lovely work introducing headers.

iAmMichaelConnor force-pushed the mc/mutable-refs-refactor branch from 82999ea to f6c114a Compare July 18, 2023 11:12

iAmMichaelConnor linked an issue Jul 18, 2023 that may be closed by this pull request

Tidy Noir Contract examples to make use of mutable references #1055

Closed

iAmMichaelConnor requested review from LeilaWang and sirasistant July 18, 2023 11:22

iAmMichaelConnor marked this pull request as ready for review July 18, 2023 11:22

iAmMichaelConnor force-pushed the mc/mutable-refs-refactor branch from f6c114a to 55e2ebe Compare July 18, 2023 12:04

iAmMichaelConnor marked this pull request as draft July 18, 2023 16:20

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

iAmMichaelConnor force-pushed the mc/mutable-refs-refactor branch 2 times, most recently from 843bd9a to c8de12d Compare July 20, 2023 21:19

iAmMichaelConnor force-pushed the mc/mutable-refs-refactor branch from 7af2051 to ec762d0 Compare July 29, 2023 17:29

iAmMichaelConnor changed the title ~~Mc/mutable refs refactor~~ refactor: use mutable refs in noir-contracts Jul 29, 2023

iAmMichaelConnor added the T-refactor Type: this code needs refactoring label Jul 31, 2023

iAmMichaelConnor force-pushed the mc/mutable-refs-refactor branch from 06ac22e to 5a2c2c3 Compare July 31, 2023 14:51

iAmMichaelConnor marked this pull request as ready for review July 31, 2023 14:52

refactor(noir-contracts): introduce mutable references everywhere

cfb690b

iAmMichaelConnor force-pushed the mc/mutable-refs-refactor branch from dad2a3f to cfb690b Compare July 31, 2023 16:21

iAmMichaelConnor mentioned this pull request Jul 31, 2023

feat(noir-contracts): Option<T> for get_notes #1272

Merged

6 tasks

spalladino approved these changes Jul 31, 2023

View reviewed changes

ludamad approved these changes Jul 31, 2023

View reviewed changes

iAmMichaelConnor merged commit 0bd1772 into master Jul 31, 2023

iAmMichaelConnor deleted the mc/mutable-refs-refactor branch July 31, 2023 16:54

Conversation

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

Uh oh!

Description

Checklist:

Uh oh!

iAmMichaelConnor commented Jul 18, 2023

Uh oh!

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

Uh oh!

Uh oh!

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

Uh oh!

Uh oh!

iAmMichaelConnor commented Jul 31, 2023

Uh oh!

spalladino left a comment

Choose a reason for hiding this comment

Uh oh!

spalladino Jul 31, 2023

Choose a reason for hiding this comment

Uh oh!

iAmMichaelConnor Jul 31, 2023

Choose a reason for hiding this comment

Uh oh!

spalladino Jul 31, 2023

Choose a reason for hiding this comment

Uh oh!

iAmMichaelConnor Jul 31, 2023

Choose a reason for hiding this comment

Uh oh!

spalladino Jul 31, 2023

Choose a reason for hiding this comment

Uh oh!

iAmMichaelConnor Jul 31, 2023

Choose a reason for hiding this comment

Uh oh!

ludamad commented Jul 31, 2023

Uh oh!

iAmMichaelConnor commented Jul 31, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

iAmMichaelConnor commented Jul 18, 2023 •

edited

Loading

iAmMichaelConnor commented Jul 29, 2023 •

edited

Loading

iAmMichaelConnor commented Jul 29, 2023 •

edited

Loading