Skip to content

chore: @aztec/stdlib pt. 3: aztec-address out of foundation#12140

Merged
Thunkar merged 11 commits intomasterfrom
gj/cleanup_circuits_js_2
Feb 20, 2025
Merged

chore: @aztec/stdlib pt. 3: aztec-address out of foundation#12140
Thunkar merged 11 commits intomasterfrom
gj/cleanup_circuits_js_2

Conversation

@Thunkar
Copy link
Contributor

@Thunkar Thunkar commented Feb 20, 2025 •
edited
Loading

Final cleanup foundation and removal of all aztec-specific references. This paves the way for the new stdlib to be born from the remnants of circuits.js and circuit-types!

This has led to a single "weird thing":

@aztec/ethereum requires AztecAddress just for the deployment of l1 contracts. In the end, this is just an arg provided to viem that gets eventually stringified, so we only have the type to avoid making mistakes ourselves. Since this is a pretty internal method, to avoid circular dependencies I've turned it into a Fr, as that's our "flattened" AztecAddress representation. @alexghr @spalladino how bad is it?

Another option would be to move deployL1Contracts to circuits.js (soon to be stdlib), as this package pulls from ethereum and has access to AztecAddress.

@Thunkar Thunkar self-assigned this Feb 20, 2025
@Thunkar Thunkar requested review from benesjan, nventuro, sklppy88 and spalladino and removed request for dbanks12 and fcarreiro February 20, 2025 14:24
@Thunkar Thunkar changed the title @aztec/stdlib pt. 3: aztec-address out of foundation chore: @aztec/stdlib pt. 3: aztec-address out of foundation Feb 20, 2025
benesjan
benesjan approved these changes Feb 20, 2025
View reviewed changes
Copy link
Contributor

@benesjan benesjan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

alexghr
alexghr reviewed Feb 20, 2025
View reviewed changes
Copy link
Contributor

@alexghr alexghr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM other than using Fr.random for aztec addresses in tests. Maybe we can pick some random values manually for those tests that need them instead?

yarn-project/ethereum/src/deploy_l1_contracts.test.ts Outdated
genesisBlockHash = Fr.random();
initialValidators = times(3, EthAddress.random);
l2FeeJuiceAddress = await AztecAddress.random();
l2FeeJuiceAddress = Fr.random();
Copy link
Contributor

@alexghr alexghr Feb 20, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this might be a problem. I don't think every Fr is a valid Aztec address? (if so, this might be a source of flakes)

Copy link
Contributor

@alexghr alexghr Feb 20, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we can just use a hardcoded test value just to be sure?

Copy link
Contributor Author

@Thunkar Thunkar Feb 20, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Funnily enough, the contract accepts whatever even if it's not valid, so I left it as a reminder of poor validation 😬 . I can use a proper one though, solidity validation is outside of the scope of this test. Updated with a hardcoded value and comment!

Copy link
Collaborator

@ludamad ludamad Feb 20, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it a productive reminder :D sounds like a gotcha

@spalladino
Copy link
Contributor

spalladino commented Feb 20, 2025

@aztec/ethereum requires AztecAddress just for the deployment of l1 contracts. In the end, this is just an arg provided to viem that gets eventually stringified, so we only have the type to avoid making mistakes ourselves. Since this is a pretty internal method, to avoid circular dependencies I've turned it into a Fr, as that's our "flattened" AztecAddress representation. @alexghr @spalladino how bad is it?

I'm ok with it, but it feels like aztec/ethereum should have access to general basic types, like Eth and Aztec addresses. Maybe is this a symptom that we need a lower aztec/types package, that exposes stuff like AztecAddress, and that aztec/ethereum can depend on?

@Thunkar
Copy link
Contributor Author

Thunkar commented Feb 20, 2025

@aztec/ethereum requires AztecAddress just for the deployment of l1 contracts. In the end, this is just an arg provided to viem that gets eventually stringified, so we only have the type to avoid making mistakes ourselves. Since this is a pretty internal method, to avoid circular dependencies I've turned it into a Fr, as that's our "flattened" AztecAddress representation. @alexghr @spalladino how bad is it?

I'm ok with it, but it feels like aztec/ethereum should have access to general basic types, like Eth and Aztec addresses. Maybe is this a symptom that we need a lower aztec/types package, that exposes stuff like AztecAddress, and that aztec/ethereum can depend on?

Discussed via slack!

@Thunkar Thunkar merged commit 8831838 into master Feb 20, 2025
9 checks passed
@Thunkar Thunkar deleted the gj/cleanup_circuits_js_2 branch February 20, 2025 18:25
TomAFrench added a commit that referenced this pull request Feb 20, 2025
@TomAFrench
Merge branch 'master' into sync-noir
ad66e8d 
* master: (300 commits)
  fix(ci): don't have checks go green immediately (#12168)
  fix: ASSERTS that should throw (#12167)
  fix: retry rm operation in cleanup (#12162)
  chore: Fix linter errors (#12164)
  feat: Barretenberg C++ binary overhaul (#11459)
  fix: call install_hooks in bootstrap (#12159)
  chore: @aztec/stdlib pt. 3: aztec-address out of foundation (#12140)
  test: verify proving is resumed after broker crash (#11122)
  chore(ci3): update ci.md with swc notes (#12147)
  fix: don't try to get bench artifacts on external PR (#12157)
  feat: partial note handling in aztec-nr (#12122)
  fix: external fixes pt 2 (#12153)
  chore: fix message path (#12150)
  chore(ci3): refactor ci3.yml, fix external PR flow (#12037)
  fix: Do not try flushing txs in bot setup if not set (#12144)
  chore: Silence warns on invalid bootnode enr (#12135)
  fix: don't early-out on test fails (#12143)
  feat(avm): deduplicating event emitters (#12137)
  chore: @aztec/stdlib pt.2 -> remove @aztec/types (#12133)
  test: kill prover node and see it recover (#11118)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ludamad ludamad ludamad left review comments

@alexghr alexghr alexghr left review comments

@benesjan benesjan benesjan approved these changes

@dbanks12 dbanks12 dbanks12 approved these changes

@nventuro nventuro Awaiting requested review from nventuro

@sklppy88 sklppy88 Awaiting requested review from sklppy88

@spalladino spalladino Awaiting requested review from spalladino

Assignees

@Thunkar Thunkar

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@Thunkar @spalladino @ludamad @alexghr @benesjan @dbanks12