chore: Run nargo fmt on "formatting:fix" by LHerskind · Pull Request #3372 · AztecProtocol/aztec-packages

LHerskind · 2023-11-21T12:02:23Z

Run the nargo fmt command when executing formatting:fix so our eyes may rest in peace

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

sirasistant · 2023-11-21T12:19:50Z

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

-// docs:end:imports
-
-// docs:start:storage_struct

+    // docs:end:imports
+    // docs:start:storage_struct


Do you know if this will affect docs codegen?

Should now, I write it like that in many of the cases I have used

Looks like it removed some of the docs stuff though 💀 docs now building because it cannot find ends

LHerskind · 2023-11-21T14:05:11Z

The nargo fmt is currently deleting comments noir-lang/noir#3521 so this pr is just formatting them, and not adding it to the fix command anyway.

yarn-project/noir-contracts/src/contracts/card_game_contract/src/game.nr

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

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

AztecBot · 2023-11-21T16:31:43Z

Benchmark results

Metrics with a significant change:

note_trial_decrypting_time_in_ms (8): 5.21 (-72%)
note_trial_decrypting_time_in_ms (32): 89.8 (+563%)

Detailed results

All benchmarks are run on txs on the Benchmarking contract on the repository. Each tx consists of a batch call to create_note and increment_balance, which guarantees that each tx has a private call, a nested private call, a public call, and a nested public call, as well as an emitted private note, an unencrypted log, and public storage read and write.

This benchmark source data is available in JSON format on S3 here.

Values are compared against data from master at commit 5cfbb68b and shown if the difference exceeds 1%.

L2 block published to L1

Each column represents the number of txs on an L2 block published to L1.

Metric	8 txs	32 txs	128 txs
l1_rollup_calldata_size_in_bytes	45,444	179,588	716,132
l1_rollup_calldata_gas	222,972	868,088	3,447,968
l1_rollup_execution_gas	842,059	3,595,196	22,203,337
l2_block_processing_time_in_ms	1,994 (-2%)	7,668 (-2%)	30,166 (-1%)
note_successful_decrypting_time_in_ms	300 (-5%)	879 (-2%)	3,183 (-3%)
note_trial_decrypting_time_in_ms	⚠️ 5.21 (-72%)	⚠️ 89.8 (+563%)	140 (-1%)
l2_block_building_time_in_ms	20,841 (-2%)	82,063 (-1%)	326,220 (-1%)
l2_block_rollup_simulation_time_in_ms	12,143 (-2%)	47,532 (-1%)	188,257 (-2%)
l2_block_public_tx_process_time_in_ms	8,654 (-1%)	34,343 (-1%)	137,446 (-1%)

L2 chain processing

Each column represents the number of blocks on the L2 chain where each block has 16 txs.

Metric	5 blocks	10 blocks
node_history_sync_time_in_ms	21,957 (-1%)	42,925 (-1%)
note_history_successful_decrypting_time_in_ms	2,080 (-2%)	4,116 (-1%)
note_history_trial_decrypting_time_in_ms	127	159 (+2%)
node_database_size_in_bytes	1,630,156	1,095,644 (-1%)
pxe_database_size_in_bytes	29,748	59,307

Circuits stats

Stats on running time and I/O sizes collected for every circuit run across all benchmarks.

Circuit	circuit_simulation_time_in_ms	circuit_input_size_in_bytes	circuit_output_size_in_bytes
private-kernel-init	767 (-1%)	61,697	18,905
private-kernel-ordering	127 (-1%)	24,297	8,153
base-rollup	1,752 (-1%)	656,428	873
root-rollup	169 (-1%)	4,072	1,097
private-kernel-inner	792 (-1%)	81,568	18,905
public-kernel-private-input	570 (-1%)	41,519	18,905
public-kernel-non-first-iteration	408 (-1%)	41,561	18,905
merge-rollup	15.4 (-1%)	2,592	873

Miscellaneous

Transaction sizes based on how many contracts are deployed in the tx.

Metric	0 deployed contracts	1 deployed contracts
tx_size_in_bytes	8,787	27,547

This reverts commit 0269cfd.

This reverts commit 21da461.

benesjan · 2023-11-22T08:03:55Z

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

-    note_interface: NoteInterface<Note, N>,
-    options: NoteViewerOptions<Note, N>,
-) -> [Option<Note>; MAX_NOTES_PER_PAGE] {
+unconstrained pub fn view_notes<Note, N>(storage_slot: Field, note_interface: NoteInterface<Note, N>, options: NoteViewerOptions<Note, N>) -> [Option<Note>; MAX_NOTES_PER_PAGE] {


@f01dab1e I know you are most likely aware of this but having line length set to 120 by default would be useful. Is max line length already supported?

Yes, this can be done. In noirfmt.toml:

max_width = 120

The list of settings is here: https://github.com/noir-lang/noir/blob/master/tooling/nargo_fmt/src/config.rs#L44-L53

benesjan · 2023-11-22T08:39:54Z

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

-        .select(0, points as Field)
-        .select(1, secret)
-        .select(2, account_address)
+pub fn create_exact_card_getter_options(points: u8, secret: Field, account_address: Field) -> NoteGetterOptions<CardNote, CARD_NOTE_LEN, Field> {


@f01dab1e Not wrapping function declaration looks like the only big issue with the formatter now. Everything else are minor issues.

If needed, we support ignoring elements through // noir-fmt:ignore

But it seems in this case we have a bug, as we are not taking into account the size of the function name

benesjan · 2023-11-22T08:45:59Z

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

+        // nested call to confirm that balance is zero
+        let _callStackItem3 = context.call_private_function(inputs.call_context.storage_contract_address,
+            get_note_zero_fn_selector,
+            [owner]);


@f01dab1e In this case doing this:

let _callStackItem3 = context.call_private_function( inputs.call_context.storage_contract_address, get_note_zero_fn_selector, [owner] );

would be nicer but it's a detail.

sent a fix noir-lang/noir#3526

chore: add nargo fmt to formatting fix in noir-contracts

7751b8a

LHerskind marked this pull request as ready for review November 21, 2023 12:16

LHerskind requested review from benesjan, rahul-kothari and sirasistant November 21, 2023 12:17

LHerskind enabled auto-merge (squash) November 21, 2023 12:18

sirasistant reviewed Nov 21, 2023

View reviewed changes

LHerskind disabled auto-merge November 21, 2023 12:23

kevaundray mentioned this pull request Nov 21, 2023

Tracking issue: nargo format on aztec-packages noir-lang/noir#3521

Closed

LHerskind added 3 commits November 21, 2023 13:58

chore: run formatting:fix

21da461

chore: format

0269cfd

chore: remove from formatting:fix until fixed

91f3d6c

LHerskind force-pushed the lh/noir-formatting branch from 78cf3f6 to 91f3d6c Compare November 21, 2023 14:03

rahul-kothari reviewed Nov 21, 2023

View reviewed changes

benesjan added 5 commits November 22, 2023 07:44

Revert "chore: format"

3467d8e

This reverts commit 0269cfd.

Revert "chore: run formatting:fix"

5fa96e7

This reverts commit 21da461.

Merge branch 'master' into lh/noir-formatting

9d81887

chore: formatting noir-contracts with newest version of nargo

188f029

chore: formatting aztec-nr

ef3a6ba

benesjan approved these changes Nov 22, 2023

View reviewed changes

benesjan merged commit a73c4aa into master Nov 22, 2023

benesjan deleted the lh/noir-formatting branch November 22, 2023 09:00

This was referenced Nov 22, 2023

chore: move the first element to a new line if the tactic is vertical for call expression noir-lang/noir#3526

Merged

chore(fmt): refactor the way we handle shapes in the formatter noir-lang/noir#3546

Merged

Conversation

LHerskind commented Nov 21, 2023

Checklist:

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!

LHerskind commented Nov 21, 2023

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

AztecBot commented Nov 21, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Benchmark results

L2 block published to L1

L2 chain processing

Circuits stats

Miscellaneous

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

AztecBot commented Nov 21, 2023 •

edited

Loading