chore: Just nargo compile.

[charlielye]

Ok. Don't be scared. 262 files sounds horrific but it's mostly path updates and that kind of nonsense. The noir-contracts and noir-compiler changes are probably what to focus on.

• We update boxes to use our build of nargo, and the modified code-generator.
• We update paths in docs, as noir-contracts/src/contracts moved to noir-contracts/contracts, as src is now pure codegen output.
• Contracts are now imported e.g. import { ChildContractArtifact } from '@aztec/noir-contracts/Child';. You can still just import from top level index, but it's pretty cruel to ask the runtime to parse all the artifacts just to get one, they are huge.
• Contract files are now just named as per the name of the contract (i.e. not snake case). Less moving parts is better here. Given it's codegen output it's acceptable to allow the output names to be inconsistent.
• aztec.js is now responsible for copying the account contracts into itself, as opposed to have some other random module push code into it. But we just need to get rid of this baked account stuff at some point anyway.
• Got rid of lodash.zip in one place, and then restrained myself to not go further. But think we should remove the "trivial" lodash cases at some point.
• Tidied up yp/bootstrap a bit, it's basically in line with the dockerfile at this point. Will prob make dockerfile just call bootstrap as part of some other docker cleanup I'll do later.
• source-map-support in cli.
• Remove compile command from cli. We are just going to promote use of aztec-nargo.
• The ts and noir generators now expect nargo output as input, rather than our transformed abi. The ts generator outputs the transformed abi as part of it's generation.
• Delete all the script stuff from noir-contracts. src folder is now just the codegen output, and the codegen is done with a trivial script to call compile and the ts generator in noir-compiler.
• Added an unused script called transform_json_abi.sh that uses a tiny bit of jq to perform the transform. Probably to be deleted, especially if we just stop transforming the noir output and use it directly, but it served me as a useful tool at one point.

[socket-security]

Removed dependencies detected. Learn more about Socket for GitHub ↗︎

🚮 Removed packages: @types/[email protected], [email protected]

[AztecBot]

Benchmark results

Metrics with a significant change:

• circuit_simulation_time_in_ms (public-kernel-private-input): 203 (+18%)
• circuit_simulation_time_in_ms (public-kernel-non-first-iteration): 200 (+17%)
• circuit_input_size_in_bytes (private-kernel-ordering): 30,953 (+20%)
• circuit_input_size_in_bytes (public-kernel-private-input): 31,891 (+27%)
• circuit_input_size_in_bytes (public-kernel-non-first-iteration): 31,933 (+26%)
• circuit_output_size_in_bytes (private-kernel-init): 25,561 (+25%)
• circuit_output_size_in_bytes (private-kernel-ordering): 14,809 (+53%)
• circuit_output_size_in_bytes (private-kernel-inner): 25,561 (+25%)
• circuit_output_size_in_bytes (public-kernel-private-input): 25,561 (+25%)
• circuit_output_size_in_bytes (public-kernel-non-first-iteration): 25,561 (+25%)
• tx_size_in_bytes (0): 15,487 (+50%)
• tx_size_in_bytes (1): 31,879 (+21%)
• note_history_successful_decrypting_time_in_ms (5): 2,944 (+17%)
• l2_block_public_tx_process_time_in_ms (8): 4,311 (+22%)
• l2_block_public_tx_process_time_in_ms (32): 17,131 (+22%)
• l2_block_public_tx_process_time_in_ms (128): 68,262 (+22%)

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 ec2e36e6 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,960	868,100	3,449,444
l1_rollup_execution_gas	867,069	3,746,153	23,306,209
l2_block_processing_time_in_ms	1,353 (+3%)	5,178 (+2%)	21,517 (+1%)
note_successful_decrypting_time_in_ms	369 (+10%)	1,150 (-1%)	4,276 (+12%)
note_trial_decrypting_time_in_ms	79.1 (+73%)	39.9 (+10%)	141 (-3%)
l2_block_building_time_in_ms	14,979 (+7%)	59,666 (+7%)	239,191 (+7%)
l2_block_rollup_simulation_time_in_ms	10,634 (+1%)	42,448 (+2%)	170,643 (+2%)
l2_block_public_tx_process_time_in_ms	⚠️ 4,311 (+22%)	⚠️ 17,131 (+22%)	⚠️ 68,262 (+22%)

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	15,232 (+1%)	29,373
note_history_successful_decrypting_time_in_ms	⚠️ 2,944 (+17%)	5,372 (+9%)
note_history_trial_decrypting_time_in_ms	96.2 (+21%)	214 (-4%)
node_database_size_in_bytes	3,620,198	3,726,484
pxe_database_size_in_bytes	29,923	59,478

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	210 (+5%)	44,709 (+4%)	⚠️ 25,561 (+25%)
private-kernel-ordering	137 (+19%)	⚠️ 30,953 (+20%)	⚠️ 14,809 (+53%)
base-rollup	1,076 (+1%)	128,374 (+4%)	881
root-rollup	84.7 (+3%)	4,088	889
private-kernel-inner	287 (+10%)	71,236 (+10%)	⚠️ 25,561 (+25%)
public-kernel-private-input	⚠️ 203 (+18%)	⚠️ 31,891 (+27%)	⚠️ 25,561 (+25%)
public-kernel-non-first-iteration	⚠️ 200 (+17%)	⚠️ 31,933 (+26%)	⚠️ 25,561 (+25%)
merge-rollup	9.80 (+1%)	2,608	881

Miscellaneous

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

Metric	0 deployed contracts	1 deployed contracts
tx_size_in_bytes	⚠️ 15,487 (+50%)	⚠️ 31,879 (+21%)

[ludamad]

LGTM but we can get more eyes on this

[Maddiaa0]

Does this perform the noir output transformations now?

edit: codegen now does it - this is a breaking change?

[Maddiaa0]

ticket needs to be created for this

[Maddiaa0]

I think this should be available separately, it is likely other components will need this abi that do not use a ts interface. e.g. alternative implementations

[charlielye]

We will unify the abi's. But yes it's been tweaked in subsequent PR to allow only outputting transformed abi.

[Maddiaa0]

issue

[Maddiaa0]

#3807 here it is

[Maddiaa0]

this needs to be mkdir -p it will fail if it already exists

[Maddiaa0]

The noir compiler itself does not know about aztec abis, since aztec-nargo exists anyway it should probably know this

issue required

[Maddiaa0]

#3812 guess this is close enough?

[rahul-kothari]

should we just delete/rename this method to avoid confusion? Says compiler actions but doesn't do any compiling

[rahul-kothari]

Personally, think it is weird to ask user to use nargo to compile but aztec-cli to generate noir interface.

[charlielye]

Will be cleaned up in later PRs.

[rahul-kothari]

should we just delete this to avoid confusion?

Btw this is the code that ends up doing compile time checks that we are currently skipping

[rahul-kothari]

What happens to noir-compiler/src/index.ts? Porbably can delete it too + delete WASM code?

[spalladino]

Love to see all the code removed! I'm worried about the breaking changes though, especially requiring typescript wrappers just to generate the artifacts for aztecjs.

[spalladino]

This is also a breaking change for users, since they have to update their tomls, right? Given we're probably moving noir-contracts again soon (given the folder restructuring), should we roll back this change so we don't have our users modifying their deps paths too often? cc @rahul-kothari

[rahul-kothari]

Oh yea good point - need a list of all the smaller breaking changes in this PR!
Would be nice if we can roll back ye

[spalladino]

Just discussed with Charlie: this only moves example contracts, and not aztec-nr or any widely used dependency, so it's not a major impact. We should be fine leaving it as is.

[spalladino]

Ouch, we're going to argue so much about which lodash usage is replaceable and which one isn't :-P

[charlielye]

Probably :)

I'd say if it's trivially implementable as above, don't use. I get zip is more descriptive (or is it? Someone might have to go read the lodash docs to understand what zip does) but it also just seems like bloat to add an entire dependency to achieve, even if it is a granular dependency. I think this is still perfectly readable and there's no hidden code being executed.

Lodash just seems like an old javascript focused frontend library (with debounce, and throttle etc and get style functions that take string based property paths). It's just not very... modern in it's approach. It was designed to fill a stdlib gap and provide browser compatibility. And deepClone stuff can be a footgun, only cloning plain data and not classes. I've seen when you put something like lodash into hands of developers it starts to get used everywhere, and looking through what lodash offers most of it is obsolete in a modern TS node.js environment.

I mean, take a look at this, and start clicking through the implementation:
https://github.com/lodash/lodash/blob/main/src/zip.ts

Do we really want that, to achieve the same thing as what I did above?

And what happened to version 5 that was meant to land 2 years ago?
https://github.com/lodash/lodash/wiki/Roadmap

I'd say if we have some clear code that can't be "done better" to avoid lodash, maybe a granular dependency can be warranted, but I'd want us to always try very hard to see if there is some neat technique in a given tempting case, to just avoid.

[spalladino]

(continuing just for the sake of the argument, I've already caved and submitted a PR that removes much lodash usage)

I agree that parts of lodash were meant to fill in gaps, like flatMap, every, or other functions that eventually made it. And I agree that get-style functions, while really popular in other languages, don't make much sense since optional chaining and nullish coalescing were introduced into the language, and probably don't play nicely with typescript (assuming no one abused string literal types to make it work).

But I think there's value in many of the higher-order functions, as they improve readability (if you're used to it, granted), and most importantly they reduce bugs. Lodash is battle-tested enough that it's more likely that there's a bug in a seemingly simple function you wrote yourself, than if you pull a lodash dependency, not to mention the time saved.

Still, lodash if one of the few libraries that I trust like this. In pretty much any other scenario, I'd go with reimplementing, especially if it's production code.

[charlielye]

I'm not anti high level functions that there is a risk of bugging if we do it ourself.
Although, I'd probably check the implementation, and if it's mess of backwards compatible js, I would also be tempted to just lift the implementation out of lodash, strip it back to a bare TS implementation, and put it into an appropriately named foundation module.

You mentioned times in your PR. It's a one liner, and I agree it's a little verbose to type every time. This would be particularly easy to put into an array module in foundation for example.

[spalladino]

Done! #3877

[spalladino]

Should we drop the eslint required docs check? I've been seeing this used quite often.

[charlielye]

Please yes.

[spalladino]

IIUC, this means that the only way to load a contract into Aztec.js is by generating the typescript wrapper beforehand, right? Doesn't this break the workflow for js-only (ie non-ts) projects? (eg here)

[spalladino]

Cleaning ./src doesn't feel right

[charlielye]

This entire project should go away soon. But given there was no other code left src seemed fine?

[spalladino]

It'd be nice to retain a way to compile just a single contract. We have enough contracts that rebuilding everything is significantly slower than rebuilding a single one.

[charlielye]

Agreed. Tiny script change solves that. Will solve in subsequent PR.