Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

refactor: function macros cleanup #12066

Merged
merged 8 commits into from
Feb 18, 2025
Merged

refactor: function macros cleanup #12066

merged 8 commits into from
Feb 18, 2025

Conversation

benesjan
Copy link
Contributor

@benesjan benesjan commented Feb 18, 2025 •
edited
Loading

  • Cleans up CallInterfaces by adding a "new(...)" function and hiding struct fields.
  • Attempts making stub_fn readable.

This was referenced Feb 18, 2025
Merged
Merged
Merged
Merged
@benesjan Graphite App
Copy link
Contributor Author

benesjan commented Feb 18, 2025 •
edited
Loading

This stack of pull requests is managed by Graphite. Learn more about stacking.

@benesjan benesjan mentioned this pull request Feb 18, 2025
Merged
@benesjan benesjan force-pushed the 02-18-refactor_call_interface_macros_cleanup branch from 0f6b664 to 2c5df4b Compare February 18, 2025 11:33
@benesjan benesjan added the e2e-all CI: Deprecated, use ci-full. Enable all master checks. label Feb 18, 2025
@benesjan benesjan marked this pull request as ready for review February 18, 2025 12:47
benesjan
benesjan commented Feb 18, 2025
View reviewed changes
noir-projects/aztec-nr/aztec/src/context/call_interfaces.nr
@@ -16,16 +16,35 @@ pub trait CallInterface<let N: u32> {
}

pub struct PrivateCallInterface<let N: u32, T> {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

As Nico proposed in a PR down the stack - makes sense to have the args private and instead use the new method. All the changes in this file are a reflection of that.

benesjan
benesjan commented Feb 18, 2025
View reviewed changes
noir-projects/aztec-nr/aztec/src/macros/functions/abi_export.nr
@@ -0,0 +1,35 @@
use std::meta::type_of;

pub(crate) comptime fn create_fn_abi_export(f: FunctionDefinition) -> Quoted {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Moved this function from intercaces.nr as that file was too cluttered. There is no code change here.

benesjan
benesjan commented Feb 18, 2025
View reviewed changes
noir-projects/aztec-nr/aztec/src/macros/functions/call_interface_stubs.nr

comptime global SERIALIZED_ARGS_SLICE_NAME: Quoted = quote { serialized_args };

pub comptime fn stub_fn(f: FunctionDefinition) -> Quoted {
Copy link
Contributor Author

@benesjan benesjan Feb 18, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

This function was originally in interfaces.nr but I have completely rewrote it because it was impossible to read. In hindsight I shouldn't have moved it in the same PR. Sorry for the bad diff. (The diff would be huge here anyway as I reshufled it)

benesjan
benesjan commented Feb 18, 2025
View reviewed changes
noir-projects/aztec-nr/aztec/src/macros/functions/call_interface_stubs.nr
let is_static_call = is_fn_view(f);
let is_void = f.return_type() == type_of(());

if is_fn_private(f) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is the main change here. I branch into individual cases and there I return the complete quote from there.

benesjan
benesjan commented Feb 18, 2025
View reviewed changes
noir-projects/aztec-nr/aztec/src/macros/functions/mod.nr
@@ -1,14 +1,10 @@
pub mod interfaces;
pub mod initialization_utils;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I found this file very hard to read so I decided to keep here only the public functions and moved the rest to other files.

benesjan
benesjan commented Feb 18, 2025
View reviewed changes
noir-projects/aztec-nr/aztec/src/macros/functions/mod.nr
// Marker attribute
}

comptime fn create_internal_check(f: FunctionDefinition) -> Quoted {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Moved these utility functions to utils.nr in the same folder.

benesjan
benesjan commented Feb 18, 2025
View reviewed changes
noir-projects/aztec-nr/aztec/src/macros/functions/stub_registry.nr
@@ -0,0 +1,19 @@
use std::{collections::umap::UHashMap, hash::{BuildHasherDefault, poseidon2::Poseidon2Hasher}};
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This got moved in here from interfaces.nr. The other change here is that instead of exposing the STUBS global I just expose the setter and getter. This makes it nicer to read as everywhere we either use only the getter or the setter.

Copy link
Contributor

Choose a reason for hiding this comment

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

lovely

benesjan
benesjan commented Feb 18, 2025
View reviewed changes
noir-projects/aztec-nr/aztec/src/macros/functions/utils.nr
@@ -0,0 +1,320 @@
use crate::macros::{
Copy link
Contributor Author

Choose a reason for hiding this comment

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

All the code here is just copied from mod.nr.

benesjan
benesjan commented Feb 18, 2025
View reviewed changes
noir-projects/aztec-nr/aztec/src/macros/utils.nr
@@ -148,7 +148,7 @@ comptime fn signature_of_type(typ: Type) -> Quoted {
}
}

trait AsStrQuote {
pub(crate) trait AsStrQuote {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Did this to fix warnings when importing it.

@benesjan benesjan force-pushed the 02-18-refactor_call_interface_macros_cleanup branch from b396777 to a1e5a72 Compare February 18, 2025 14:07
@benesjan benesjan force-pushed the 02-17-fix_missing_import_warnings_in_macros branch from 518ab62 to a21c5bc Compare February 18, 2025 14:07
@benesjan benesjan force-pushed the 02-18-refactor_call_interface_macros_cleanup branch 2 times, most recently from b16a373 to fc2655a Compare February 18, 2025 15:16
@benesjan benesjan force-pushed the 02-17-fix_missing_import_warnings_in_macros branch from 381501c to fb8222e Compare February 18, 2025 18:05
@benesjan benesjan force-pushed the 02-18-refactor_call_interface_macros_cleanup branch from eba9bf3 to 2c9a324 Compare February 18, 2025 18:05
Base automatically changed from 02-17-fix_missing_import_warnings_in_macros to master February 18, 2025 18:44
@benesjan benesjan force-pushed the 02-18-refactor_call_interface_macros_cleanup branch from 2c9a324 to ff5849b Compare February 18, 2025 18:56
@benesjan benesjan changed the title refactor: call interface macros cleanup refactor: function macros cleanup Feb 18, 2025
@benesjan benesjan requested a review from nventuro February 18, 2025 19:21
@benesjan benesjan merged commit a00fa0e into master Feb 18, 2025
15 checks passed
@benesjan benesjan deleted the 02-18-refactor_call_interface_macros_cleanup branch February 18, 2025 19:51
TomAFrench added a commit that referenced this pull request Feb 19, 2025
@TomAFrench
Merge branch 'master' into sync-noir
9deee5e 
* master: (245 commits)
  chore: Fix unbound CI variable on release image bootstrap (#12095)
  fix: dry run on grind (#12088)
  fix(spartan): eth-execution logging (#12094)
  fix: aws_handle_evict recovery & termination (#12086)
  chore: Use native acvm when available on orchestrator tests (#11560)
  refactor: function macros cleanup (#12066)
  refactor: remove `addNullifiedNote` from pxe (#11822)
  fix: `#[aztec]` macro warnings (#12038)
  refactor!: Notes implementing `Packable<N>` (#12004)
  chore(ops): add gcloud cli into devbox base image (#12082)
  fix: hotfix grinding
  fix: L1 deployment on reth (#12060)
  fix: Add missing bootstrap fast aliases (#12078)
  fix: hash_str caching (#12074)
  fix: Naive attempt to fix nightly deployments (#12079)
  fix: kind smoke (#12084)
  refactor!: nuking `NoteHeader` (#11942)
  fix: inject dockerhub creds (#12072)
  feat(docs): Note discovery concepts page (#11760)
  chore: cleanup libp2p logger (#12058)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nventuro nventuro nventuro approved these changes

Assignees
No one assigned
Labels
e2e-all CI: Deprecated, use ci-full. Enable all master checks.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@benesjan @nventuro