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.

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

refactor!: Notes implementing Packable<N> #12004

Merged
merged 17 commits into from
Feb 18, 2025

Conversation

benesjan
Copy link
Contributor

@benesjan benesjan commented Feb 14, 2025

Fixes #11981
+\ note macro cleanup

@benesjan benesjan changed the title refactor: Notes implementing Packable<N> refactor!: Notes implementing Packable<N> Feb 14, 2025
@benesjan benesjan added the e2e-all CI: Deprecated, use ci-full. Enable all master checks. label Feb 14, 2025
@benesjan benesjan marked this pull request as ready for review February 14, 2025 11:01
@benesjan benesjan marked this pull request as draft February 14, 2025 14:04
/// ...
/// }
/// }
comptime fn generate_note_interface(s: StructDefinition, note_type_id: Field) -> 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.

I separated original generate_note_interface to generate_note_interface_for_partial_note and generate_note_interface because for normal notes we can use poseidon2 which is much cheaper. It also makes it clearer what is partial notes related complexity and what is not.

quote { packed_content }, // "packed_content" is argument of NoteInterface::unpack_content
0,
packing_enabled,
);
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 this code was related to pack_content and unpack_content ^.

@@ -6,7 +6,6 @@ pub use crate::{
note_interface::{NoteInterface, NullifiableNote},
note_viewer_options::NoteViewerOptions,
retrieved_note::RetrievedNote,
utils::compute_note_hash_and_optionally_a_nullifier as utils_compute_note_hash_and_optionally_a_nullifier,
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 re-export was unused and I see no reason for having it. I think it was just forgottent.

pub(crate) global VALUE_NOTE_LEN: u32 = 3; // 3 plus a header.

// docs:start:value-note-def
#[note]
#[derive(Eq)]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should have done this in the PR down the stack as this became unblocked after nuking of the NoteHeader.

// Stores an ECDSA public key composed of two 32-byte elements
// TODO: Do we need to include a nonce, in case we want to read/nullify/recreate with the same pubkey value?
#[note_custom_interface]
#[note]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

With the separation of the packing functionality from the NoteInterface it no longer made sense to use the #[note_custom_interface] here because the only custom thing here was the packing.

@@ -61,7 +61,7 @@ use super::traits::{Deserialize, Packable, Serialize};
/// # Panics
/// - If the deserialization logic encounters a type it does not support.
/// - If an incorrect number of fields are consumed when deserializing a string.
pub comptime fn generate_deserialize_from_fields<N>(
pub comptime fn generate_deserialize_from_fields(
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The N here had no use.

@benesjan benesjan marked this pull request as ready for review February 14, 2025 14:22
@AztecBot
Copy link
Collaborator

AztecBot commented Feb 14, 2025

Docs Preview

Hey there! 👋 You can check your preview at https://67af767f287cbd166c251616--aztec-docs-dev.netlify.app

Copy link
Contributor

github-actions bot commented Feb 14, 2025

Changes to public function bytecode sizes

Generated at commit: 6b27ea94c138a8ed6f9f34f03262b4e7b14c7a46, compared to commit: aafc2249a53c1e9873fbaca40efd70cb55689a71

🧾 Summary (100% most significant diffs)

Program Bytecode size in bytes (+/-) %
TokenBlacklist::public_dispatch -479 ✅ -2.03%
TokenBlacklist::mint_private -251 ✅ -7.13%
TokenBlacklist::shield -495 ✅ -8.87%

Full diff report 👇
Program Bytecode size in bytes (+/-) %
TokenBlacklist::public_dispatch 23,100 (-479) -2.03%
TokenBlacklist::mint_private 3,269 (-251) -7.13%
TokenBlacklist::shield 5,086 (-495) -8.87%

///
/// For more details on the generated code, see the individual functions.
pub comptime fn note_custom_interface(s: StructDefinition) -> Quoted {
let note_properties = generate_note_properties(s);
let (packable_impl, note_packed_len) = derive_packable_if_not_implemented_and_get_len(s);
// TODO(https://github.com/AztecProtocol/aztec-packages/issues/12012): This is broken
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 an issue which I uncovered in this PR.

},
};

// TODO(#12008): Remove the need for the manual import of `Packable` trait here. This is a bug in 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.

This issue was already present before because we used Packable in the implementations of the pack_content and unpack_content function.s

@@ -249,6 +249,9 @@ describe('e2e_2_pxes', () => {
note = notes[0];
}

// TODO(#12013): We need to do this hack because NoteDao no longer populates noteTypeId
note.noteTypeId = TestContract.notes.ValueNote.id;
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 am not really sure why this got triggered now as even before the noteTypeId should have been non-zero but now it caused the test to fail when adding the nullified note.

Given that note type id will be soon nuked from NoteDao and ExtendedNote and given that we will nuke the addNullifiedNote function doesn't make sense to bother with.

@benesjan benesjan force-pushed the 02-14-refactor_notes_implementing_packable_n_ branch from de1cc6f to 4a8b6e2 Compare February 17, 2025 09:49
@benesjan benesjan force-pushed the 02-12-refactor_removing_noteheader_out_of_note branch from aafc224 to 14eb329 Compare February 17, 2025 09:49
@benesjan benesjan force-pushed the 02-14-refactor_notes_implementing_packable_n_ branch from 4a8b6e2 to 0c4115b Compare February 18, 2025 08:15
@benesjan benesjan force-pushed the 02-12-refactor_removing_noteheader_out_of_note branch from edd3cac to b6aa786 Compare February 18, 2025 08:15
@benesjan benesjan marked this pull request as draft February 18, 2025 08:16
@benesjan benesjan force-pushed the 02-14-refactor_notes_implementing_packable_n_ branch from 0c4115b to 37ce5ab Compare February 18, 2025 08:33
@benesjan benesjan force-pushed the 02-14-refactor_notes_implementing_packable_n_ branch 3 times, most recently from c8e8515 to d8c127f Compare February 18, 2025 09:20
@benesjan benesjan marked this pull request as ready for review February 18, 2025 09:34
Base automatically changed from 02-12-refactor_removing_noteheader_out_of_note to master February 18, 2025 14:06
@benesjan benesjan force-pushed the 02-14-refactor_notes_implementing_packable_n_ branch from d8c127f to a1afb7a Compare February 18, 2025 14:07
@benesjan benesjan removed the e2e-all CI: Deprecated, use ci-full. Enable all master checks. label Feb 18, 2025
Copy link
Contributor

@nventuro nventuro left a comment

Choose a reason for hiding this comment

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

nature is healing

Co-authored-by: Nicolás Venturo <nicolas.venturo@gmail.com>
@benesjan benesjan enabled auto-merge (squash) February 18, 2025 16:18
@benesjan benesjan merged commit 64e6b13 into master Feb 18, 2025
13 checks passed
@benesjan benesjan deleted the 02-14-refactor_notes_implementing_packable_n_ branch February 18, 2025 17:41
TomAFrench added a commit that referenced this pull request Feb 19, 2025
* 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
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Replace NoteInterface::pack_content and NoteInterface::unpack_content with standard Packable
3 participants