refactor: storing `&mut context` in state vars #1926

benesjan · 2023-09-01T09:22:23Z

Note: I run the non-contract Noir files through rustfmt so some of the changes are purely formatting related.

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).

iAmMichaelConnor · 2023-09-01T15:45:36Z

Thanks very much for this @benesjan
I think this will make the syntax look very pretty indeed, when we're able to "hide" the Storage::init(...) call using Sean's new macro approach.

I just merged a big PR from Leila, which adds lots of docs relating to state variables and storage syntax. Please can you rebase and maybe search for new instances of Storage::init which might need to be updated in the example code which gets embedded into the new docs?

benesjan · 2023-09-01T20:13:50Z

I just merged a big PR from Leila, which adds lots of docs relating to state variables and storage syntax. Please can you rebase and maybe search for new instances of Storage::init which might need to be updated in the example code which gets embedded into the new docs?

@iAmMichaelConnor Just finished doing all the changes.

spalladino

Amazing work Jan, thanks for taking this on!

spalladino · 2023-09-04T11:38:43Z

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

    }
    // docs:end:state_vars-check_return_notes

    // docs:start:functions-UncontrainedFunction
    unconstrained fn get_total_points(account: Field) -> u8 {
-        let storage = Storage::init();
+        let storage = Storage::init(Option::none(), Option::none());


I was going to suggest adding a method like Storage::init_view() that didn't take any params and set both Option::nones under the hood. But if we're going to hide this behind a macro, no need to polish it up, right?

Yes, I think it doesn't make sense to implement because of the macro. It would just make Storage a bit more complex and the gain would not be there.

spalladino · 2023-09-04T11:39:50Z

yarn-project/noir-contracts/src/contracts/docs_example_contract/src/actions.nr

-use dep::aztec::context::{
-    PrivateContext,
-};
+use dep::aztec::constants_gen::{MAX_NOTES_PER_PAGE, MAX_READ_REQUESTS_PER_CALL};


Did you run a code formatter or something? If so, how? We should get it into the CI so we ensure all our code gets formatted.

I used rustfmt but it was a semi-manual process because rustfmt fails on unconstrained methods and global vars. How I made it work is that I first renamed unconstrained get_balance to get_balance_unconstrained and removed the global vars and then reverted the changes. The formatting became too ugly so it was worth the hassle but this makes it impossible to use in CI without bigger modifications of rustfmt.

But this actually makes me curious whether to create noirfmt it would be sufficient to create some simple pre and post processors which would make and revert the changes I did manually... 🤔

FWIW I found a tracking issue in Noir, which doesn't seem prioritised atm: noir-lang/noir#1580

As for using rustfmt, it's an interesting idea, but I'm worried that it'll keep diverging as we add more non-rust features to Noir. Also, removing noir-specific keywords is easy, but adding them back is not, given the file changes introduced by rustfmt. I think we should probably wait.

spalladino · 2023-09-04T11:44:11Z

yarn-project/noir-libs/easy-private-state/src/easy_private_state.nr


        // Emit the newly created encrypted note preimages via oracle calls.
        let owner_key = get_public_key(owner);
+        let _context = self.context.unwrap();


Nit: avoid using variable names starting with underscore, since they are (or are in most languages, not sure if Rust also) skipped for unused variable checks. As an alternative, you can rename the field to maybeContext, and the unwrapped version to context.

spalladino · 2023-09-04T11:49:01Z

yarn-project/noir-libs/noir-aztec/src/state_vars/public_state.nr

+        // Note: Passing the contexts to new(...) just to have an interface compatible with a Map.
+        _: Option<&mut PrivateContext>,
+        _: Option<&mut PublicContext>,


I'm on the fence about this: should we pass both optional contexts to every state var type? Or just the ones they actually need, like in ImmutableSingleton? Still, when we introduce traits, this problem will go away.

iAmMichaelConnor · 2023-09-05T10:48:21Z

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

+    fn init(
+        private_context: Option<&mut PrivateContext>,
+        public_context: Option<&mut PublicContext>,
+    ) -> Self {
        Storage {
-            balances: Map::new(1, |s| Set::new(s, ValueNoteMethods)),
-            claims: Set::new(2, ClaimNoteMethods),
+            balances: Map::new(
+                private_context,
+                public_context,
+                1, // Storage slot
+                |private_context, public_context, slot| {
+                    Set::new(private_context, public_context, slot, ValueNoteMethods)
+                },
+            ),
+            claims: Set::new(private_context, public_context, 2, ClaimNoteMethods),


I hadn't appreciated how ugly storage declarations would become with this new change. Declaring storage like this doesn't make for a very nice user experience, does it? (I know I initially proposed the change, to clean up calls to methods (by removing the context as a parameter to those calls)... but this is an ugly trade off, and I'm not sure if it's a good one, in hindsight).
I returned to this PR after a breaking change was flagged on discord @benesjan @spalladino - this PR caused a breaking change to storage declarations, and someone was struggling with declaring a double-mapping with this new syntax (and I can understand why :) )

Declaring a double mapping has become this monstrosity, haha:

Storage { double_mapping: Map::new( private_context, public_context, 1, |private_context, public_context, slot1| Map::new( private_context, public_context, slot1, |private_context, public_context, slot2| Set::new( private_context, public_context, slot2, ValueNoteMethods ) ) ) }

This might have to be classified as "unacceptable syntax"?

Edit: especially when compared to: mapping(uint256 => mapping(uint256 => uint256)) doubleMapping; in Solidity.

Agree it's horrible, but bear in mind that, as soon as we get traits, we can work with a single context. So the example above would become this. And we can even introduce a DoubleMap struct that abstracts it if we wanted to.

Storage { double_mapping: Map::new(context, 1, |context, slot1| Map::new(context, slot1, |context, slot2| Set::new(context, slot2, ValueNoteMethods) ) ) }

I still think the change is worth it. I'd expect developers to spend more time in contract logic that in storage declaration. And by moving the context into storage initialisation (which we can then hide with a macro), we can keep moving explicit references to context out of the picture, for cleaner contract code.

TLDR: Can you push the Noir team to get traits implemented soon? :-P

I don't think they'll be implemented for some weeks (at the earliest). Perhaps we could wrap the private_context, public_context "tuple" in a Context type, just to reduce the verbosity of this.

struct Context { private_context: Option<PrivateContext>, public_context: Option<PublicContext>, _is_private: bool }

And then pass this ugly thing between state variables instead?

A double mapping is a nice idea too!

TLDR: Can you push the Noir team to get traits implemented soon? :-P

I just asked, for you:

Perhaps we could wrap the private_context, public_context "tuple" in a Context type, just to reduce the verbosity of this.

I think that sould be easy to do (annoying, but easy) until we do get traits. Do you want to create an issue and assign it to Otters?

🤖 I have created a new Aztec Packages release --- ## [0.1.0-alpha50](v0.1.0-alpha49...v0.1.0-alpha50) (2023-09-05) ### ⚠ BREAKING CHANGES * update to acvm 0.24.0 ([#1925](#1925)) ### Features * **892:** add hints for matching transient read requests with correspondi… ([#1995](#1995)) ([0955bb7](0955bb7)) * Add support for assert messages & runtime call stacks ([#1997](#1997)) ([ac68837](ac68837)) * **Aztec.nr:** Kernel return types abstraction ([#1924](#1924)) ([3a8e702](3a8e702)) * **ci:** use content hash in build system, restrict docs build to *.ts or *.cpp ([#1953](#1953)) ([0036e07](0036e07)) * do not allow slot 0 in `noir-libs` ([#1884](#1884)) ([54094b4](54094b4)), closes [#1692](#1692) * throwing when submitting a duplicate tx of a settled one ([#1880](#1880)) ([9ad768f](9ad768f)), closes [#1810](#1810) * typos, using Tx.clone functionality, better naming ([#1976](#1976)) ([00bca67](00bca67)) ### Bug Fixes * add retry_10 around ensure_repo ([#1963](#1963)) ([0afde39](0afde39)) * Adds Mac cross compile flags into barretenberg ([#1954](#1954)) ([3aaf91e](3aaf91e)) * bb meta-data ([#1960](#1960)) ([712e0a0](712e0a0)) * **bb.js:** (breaking change) bundles bb.js properly so that it works in the browser and in node ([#1855](#1855)) ([1aa6f59](1aa6f59)) * Benchmark preset uses clang16 ([#1902](#1902)) ([4f7eeea](4f7eeea)) * build ([#1906](#1906)) ([8223be1](8223be1)) * **ci:** Incorrect content hash in some build targets ([#1973](#1973)) ([0a2a515](0a2a515)) * circuits should not link openmp with -DMULTITHREADING ([#1929](#1929)) ([cd1a685](cd1a685)) * compilation on homebrew clang 16.06 ([#1937](#1937)) ([c611582](c611582)) * docs preprocessor line numbers and errors ([#1883](#1883)) ([4e7e290](4e7e290)) * ensure CLI command doesn't fail due to missing client version ([#1895](#1895)) ([88086e4](88086e4)) * error handling in acir simulator ([#1907](#1907)) ([165008e](165008e)) * Fix off by one in circuits.js when fetching points from transcript ([#1993](#1993)) ([cec901f](cec901f)) * format.sh issues ([#1946](#1946)) ([f24814b](f24814b)) * master ([#1981](#1981)) ([6bfb053](6bfb053)) * More accurate c++ build pattern ([#1962](#1962)) ([21c2f8e](21c2f8e)) * polyfill by bundling fileURLToPath ([#1949](#1949)) ([1b2de01](1b2de01)) * Set correct version of RPC & Sandbox when deploying tagged commit ([#1914](#1914)) ([898c50d](898c50d)) * typescript lookup of aztec.js types ([#1948](#1948)) ([22901ae](22901ae)) * unify base64 interface between mac and linux (cherry-picked) ([#1968](#1968)) ([ee24b52](ee24b52)) * Update docs search config ([#1920](#1920)) ([c8764e6](c8764e6)) * update docs search keys ([#1931](#1931)) ([03b200c](03b200c)) ### Miscellaneous * **1407:** remove forwarding witnesses ([#1930](#1930)) ([cc8bc8f](cc8bc8f)), closes [#1407](#1407) * **1879:** add use of PrivateKernelPublicInputs in TS whenever relevant ([#1911](#1911)) ([8d5f548](8d5f548)) * acir tests are no longer base64 encoded ([#1854](#1854)) ([7fffd16](7fffd16)) * Add back double verify proof to test suite ([#1986](#1986)) ([f8688d7](f8688d7)) * add CLI test to canary flow ([#1918](#1918)) ([cc68958](cc68958)), closes [#1903](#1903) * Add safemath noir testing ([#1967](#1967)) ([cb1f1ec](cb1f1ec)) * **Aztec.nr:** remove implicit imports ([#1901](#1901)) ([c7d5190](c7d5190)) * **Aztec.nr:** Remove the open keyword from public functions ([#1917](#1917)) ([4db8603](4db8603)) * **ci:** build docs on every pr ([#1955](#1955)) ([c200bc5](c200bc5)) * Enable project-specific releases for dockerhub too ([#1721](#1721)) ([5d2c082](5d2c082)) * reduce max circuit size in bb binary ([#1942](#1942)) ([c61439b](c61439b)) * Reference noir master for acir tests ([#1969](#1969)) ([86b72e1](86b72e1)) * remove debug output from `run_acir_tests` script ([#1970](#1970)) ([74c83c5](74c83c5)) * storing `&mut context` in state vars ([#1926](#1926)) ([89a7a3f](89a7a3f)), closes [#1805](#1805) * sync bb master ([#1947](#1947)) ([eed58e1](eed58e1)) * update to acvm 0.24.0 ([#1925](#1925)) ([e728304](e728304)) * Update to acvm 0.24.1 ([#1978](#1978)) ([31c0a02](31c0a02)) * updating docs to clang16 ([#1875](#1875)) ([a248dae](a248dae)) ### Documentation * **keys:** Complete addresses are now broadcast ([#1975](#1975)) ([92068ad](92068ad)), closes [#1936](#1936) * limitations, privacy, roadmap ([#1759](#1759)) ([0cdb27a](0cdb27a)) * put dev docs before spec ([#1944](#1944)) ([f1b29cd](f1b29cd)) * storage and state variables ([#1725](#1725)) ([fc72f84](fc72f84)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please). Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>

benesjan marked this pull request as draft September 1, 2023 09:22

benesjan added 21 commits September 1, 2023 15:01

WIP

10b1679

WIP

36665a5

using Option

a294e68

non native token

5b7ece0

child

55fbdb3

escrow

4154ea6

public token

cef297a

child

50591cf

price feed

d84ddb2

private token fix

ff0bbf0

WIP

078a3b4

ecdsa

f956a3f

schnorr account

f06ea56

schnorr

179aece

WIP

719be2a

pokeable

4726622

pending commitments

4e098e0

private token airdrop

8a727b6

native token

248e171

WIP

7eae394

cleanup

3754c87

benesjan force-pushed the janb/storing-context-in-state-vars branch from 1978f38 to 3754c87 Compare September 1, 2023 15:01

benesjan marked this pull request as ready for review September 1, 2023 15:22

benesjan enabled auto-merge (squash) September 1, 2023 15:23

benesjan added 4 commits September 1, 2023 18:16

Merge branch 'master' into janb/storing-context-in-state-vars

ae0df88

WIP

66b9eb6

fix

4e71326

formatting

ba53c99

fix

f8edcbf

benesjan requested a review from spalladino September 1, 2023 20:18

ludamad force-pushed the master branch from e2c543c to 0036e07 Compare September 3, 2023 22:02

spalladino approved these changes Sep 4, 2023

View reviewed changes

benesjan merged commit 89a7a3f into master Sep 4, 2023

benesjan deleted the janb/storing-context-in-state-vars branch September 4, 2023 11:49

github-actions bot mentioned this pull request Sep 4, 2023

chore(master): release 0.1.0-alpha50 #1900

Merged

iAmMichaelConnor reviewed Sep 5, 2023

View reviewed changes

This was referenced Sep 6, 2023

Introduce Context struct in Noir as a pair of Optional private and public contexts #2074

Closed

Move Context into Noir interfaces initialiser #2075

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: storing `&mut context` in state vars #1926

refactor: storing `&mut context` in state vars #1926

benesjan commented Sep 1, 2023 •

edited

Loading

iAmMichaelConnor commented Sep 1, 2023

benesjan commented Sep 1, 2023 •

edited

Loading

spalladino left a comment

spalladino Sep 4, 2023

benesjan Sep 4, 2023

spalladino Sep 4, 2023

benesjan Sep 4, 2023

spalladino Sep 4, 2023

spalladino Sep 4, 2023

spalladino Sep 4, 2023

iAmMichaelConnor Sep 5, 2023

iAmMichaelConnor Sep 5, 2023 •

edited

Loading

spalladino Sep 5, 2023

iAmMichaelConnor Sep 5, 2023 •

edited

Loading

iAmMichaelConnor Sep 5, 2023

spalladino Sep 5, 2023

refactor: storing &mut context in state vars #1926

refactor: storing &mut context in state vars #1926

Conversation

benesjan commented Sep 1, 2023 • edited Loading

Checklist:

iAmMichaelConnor commented Sep 1, 2023

benesjan commented Sep 1, 2023 • edited Loading

spalladino left a 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

iAmMichaelConnor Sep 5, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iAmMichaelConnor Sep 5, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

refactor: storing `&mut context` in state vars #1926

refactor: storing `&mut context` in state vars #1926

benesjan commented Sep 1, 2023 •

edited

Loading

benesjan commented Sep 1, 2023 •

edited

Loading

iAmMichaelConnor Sep 5, 2023 •

edited

Loading

iAmMichaelConnor Sep 5, 2023 •

edited

Loading