chore: aztec js circulars

[charlielye]

• This attempts to unpick aztec.js.
• Unfortunately, there was some heavy spaghettification here. There was no clear separation between most of the modules, with a lot of circular references.
• I've had to move things around to get the structure sound, but now there are things in places that are named unintuitively. One major example being Wallet now being in the account module.
• I think simply renaming some things, or e.g. introducing another module with tighter scope is the next step.
• I flattened aztec.js a bit to try and make some sense. I also introduced create_account as the highest level module.
• This definitely is not "complete" in terms of fixing the names/comments etc, but if agreed it's a good first step we can merge and iterate.

Here are some notes I took as I picked apart. Each module and its dependencies:

account:

account_contract:
	account

contract:
	account

contract_deployer:
	contract

wallet:
	account
	account_contract
	contract

account_manager:
	contract
	contract_deployer
	wallet
	account
	account_contract

create_account:
	account_manager
	wallet

[AztecBot]

Benchmark results

Metrics with a significant change:

• note_history_trial_decrypting_time_in_ms (10): 349 (+151%)

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 bd5614c2 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	223,008	868,376	3,449,528
l1_rollup_execution_gas	842,095	3,595,484	22,205,190
l2_block_processing_time_in_ms	1,361 (+2%)	5,150 (-2%)	21,264
note_successful_decrypting_time_in_ms	367 (+9%)	1,195 (+14%)	3,935 (+5%)
note_trial_decrypting_time_in_ms	78.6 (+69%)	38.3 (+3%)	140 (+1%)
l2_block_building_time_in_ms	13,798 (+2%)	54,041	218,429 (+1%)
l2_block_rollup_simulation_time_in_ms	10,223 (+2%)	39,914	161,625
l2_block_public_tx_process_time_in_ms	3,543 (+1%)	14,058 (+1%)	56,555 (+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	15,831 (+3%)	30,402 (+4%)
note_history_successful_decrypting_time_in_ms	2,523 (-6%)	4,930 (-2%)
note_history_trial_decrypting_time_in_ms	84.0 (+13%)	⚠️ 349 (+151%)
node_database_size_in_bytes	3,913,265	4,259,585
pxe_database_size_in_bytes	29,940	59,499

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	200	43,109	20,441
private-kernel-ordering	115 (+1%)	25,833	9,689
base-rollup	2,063	244,144	881
root-rollup	88.0 (+4%)	4,088	889
private-kernel-inner	262	64,516	20,441
public-kernel-private-input	172 (+1%)	25,203	20,441
public-kernel-non-first-iteration	171 (+1%)	25,245	20,441
merge-rollup	9.99 (+2%)	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	10,323	26,309

[spalladino]

Looks good, I got a few questions before approving. Also, I'm curious if you spotted improvements on tsserver after this refactor, since it had been one of the main reasons for removing circular dependencies in other parts of the system.

[spalladino]

Suggested change

// import SchnorrAccountContractArtifact from '../accounts_contract/artifacts/schnorr_account_contract.json' assert { type: 'json' };

[spalladino]

Suggested change

// export const INITIAL_SANDBOX_ACCOUNT_CONTRACT_ABI = SchnorrAccountContractArtifact;

[spalladino]

I'm curious: what was the need for adding this..?

Also, shouldn't it return Promise<TContract>? It doesn't matter too much though, since it gets casted afterwards.

[charlielye]

It broke a circular dependency between contract, sent_tx, and deploy_tx.
TBH I wasn't aware that this DeployMethod was being constructed publically, hence this has actually affected e.g. our browser test. But I'm also not sure it should ever need to be constructed by an end user so expect that's us just providing a bad api.

[spalladino]

It broke a circular dependency between contract, sent_tx, and deploy_tx.

Don't we still have the circular dependency, since we are still importing type Contract? If so, and if the main reason for breaking circular deps is a cognitive and not a runtime performance one, then I'd argue that this change doesn't make code easier to follow (it's actually probably harder). Instead, it just pleases the linter, since it explicitly ignores type imports. But please disregard this comment if the circular dep was elsewhere and I missed it.

I wasn't aware that this DeployMethod was being constructed publically, hence this has actually affected e.g. our browser test. But I'm also not sure it should ever need to be constructed by an end user so expect that's us just providing a bad api.

Agree, this class probably shouldn't be used directly by the end user.

[spalladino]

Suggested change

	// {
	// "path": "../ethereum"
	// },
	// {
	// "path": "../foundation"
	// },
	// {
	// "path": "../l1-artifacts"
	// },
	// {
	// "path": "../noir-compiler"
	// },
	// {
	// "path": "../noir-contracts"
	// },

[spalladino]

Where are these changes coming from? I wouldn't expect a snapshot in noir-compiler to change due to reorganizing code in aztec.js.

[charlielye]

Some are from the postDeployCtor thing.
But you're right the others are sus. Maybe from me having an outdated local nargo.

a few moments later

I just realised our CI skips these tests if it can't find a nargo 🤷‍♂️
So the tests are broken without out these updates.

[spalladino]

Is this from an unrelated set of commits that found its way into this PR?

[charlielye]

Not sure where this came from.

[spalladino]

Same!

[charlielye]

This was deliberate. These don't exit anymore.

[spalladino]

I recall having to add artifacts to source control (horrible, I know) to avoid a bootstrapping issue. Are we sure removing them doesn't break bootstrapping? (maybe it was fixed by removing some unneeded dependencies?)

And more importantly: are we sure our build steps that generate these contracts are always run before this package is published, so that we don't accidentally publish aztec.js without these artifacts?

[charlielye]

I'll double check, but given that we build-all-the-things in yarn-project, and in the bootstrap script, I would assume these should be present and not need to be committed.

As a separate point of note, I don't think these should even be here? The account contracts are defined by users of our system. Is there a reason we're baking these into aztec.js?

I could kind of see them being in the CLI, if we assume cli to be a layer that wraps aztec.js, and the cli is a tool that, amongst other things, provides examples.

[spalladino]

Good point! We can probably move them elsewhere at this point. Not the CLI though: these contracts need to be available as a library, since they are used a lot in tests (to bootstrap accounts to interact with) or even in the sandbox itself.

[charlielye]

Looks good, I got a few questions before approving. Also, I'm curious if you spotted improvements on tsserver after this refactor, since it had been one of the main reasons for removing circular dependencies in other parts of the system.

(Firstly, apologies for the mess in this PR, I should have reviewed the diff more thoroughly before assigning. Will look into it shortly.)

I can't definitively say if there's a performance boost yet. I expect it will help. I also expect it can help with startup times, tree-shaking, etc.

However, to be clear, I don't think the above are the root reasons for getting rid of circular dependencies. Circular dependencies can lead to unclear responsibilities, bad interfaces, and convoluted unit tests. Our goal should be a well-structured codebase, where dependencies flow in one direction, creating a clean and understandable architecture. This applies at all levels of the stack, not just between projects, but also the code within. So it isn't just about performance; it's about good software design principles.

There is high level of cognitive overhead to understand circular code (I struggled to comprehend how things connected together). While this PR pulls some things apart and puts them in unintuitive places, I hope it's a step towards something more maintainable. Once you get rid of the circulars, and can actually see "how the code lies", it should be easier to say "oh, this thing actually just does this, and should be named as such" etc etc.

I think making circulars be errors and not warnings, could be a strategic move (once all resolved). It encourages deeper consideration of a code unit's role and dependencies, and could improve our code quality. I haven't enforced this in such a large team before however, so am happy to hear any thoughts.

[spalladino]

(Firstly, apologies for the mess in this PR, I should have reviewed the diff more thoroughly before assigning. Will look into it shortly.)

We've all been guilty of this :-P

I think making circulars be errors and not warnings, could be a strategic move (once all resolved). It encourages deeper consideration of a code unit's role and dependencies, and could improve our code quality. I haven't enforced this in such a large team before however, so am happy to hear any thoughts.

I think there are legitimate uses for circular deps in code, especially when writing user-facing APIs, where a handful of objects can act as a facade to more complex code. Circular deps seems like an easy way to help us identify code spaghettis, but I'm not sure if consistently removing them is the best way to improve code clarity. That said, I'm fine giving it a shot.