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: passing partial note logs through transient storage #9356

Merged
merged 11 commits into from
Oct 25, 2024
Merged

refactor: passing partial note logs through transient storage #9356

merged 11 commits into from
Oct 25, 2024

Conversation

benesjan
Copy link
Contributor

@benesjan benesjan commented Oct 23, 2024 •
edited
Loading

Partial notes log gets passed via transient storage to the public function where it the public values get appended to the log and emitted via the unencrypted log stream. I refactored the partial notes related macros so that it's not all disgusting.

Note for the reviewer

I included a lot of comments in the code in this PR and if you check the files from bottom down all should hopefully make sense.

This was referenced Oct 23, 2024
Merged
Merged
Merged
Closed
Closed
@benesjan Graphite App
Copy link
Contributor Author

benesjan commented Oct 23, 2024 •
edited
Loading

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

Join @benesjan and the rest of your teammates on Graphite Graphite

@benesjan benesjan mentioned this pull request Oct 23, 2024
Merged
@benesjan benesjan force-pushed the 10-23-refactor_passing_partial_note_logs_through_transient_storage branch from 006ab0a to a0b45b4 Compare October 23, 2024 18:21
@benesjan benesjan changed the base branch from 10-10-refactor_updating_nft_flows to graphite-base/9356 October 24, 2024 14:10
@benesjan benesjan force-pushed the 10-23-refactor_passing_partial_note_logs_through_transient_storage branch 2 times, most recently from 42381d1 to d0bd136 Compare October 24, 2024 16:38
@benesjan benesjan force-pushed the 10-23-refactor_passing_partial_note_logs_through_transient_storage branch from d0bd136 to 3d3c8d3 Compare October 24, 2024 17:04
@benesjan benesjan changed the base branch from graphite-base/9356 to master October 24, 2024 17:04
benesjan
benesjan commented Oct 24, 2024
View reviewed changes
yarn-project/simulator/src/public/enqueued_call_simulator.ts
@@ -283,48 +283,75 @@ export class EnqueuedCallSimulator {
callContext: result.executionRequest.callContext,
proverAddress: AztecAddress.ZERO,
argsHash: computeVarArgsHash(result.executionRequest.args),
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added the error messages below as the message thrown in padArray was not descriptive enough.

@benesjan benesjan force-pushed the 10-23-refactor_passing_partial_note_logs_through_transient_storage branch from 1cf6f85 to f586e44 Compare October 24, 2024 18:10
benesjan
benesjan commented Oct 24, 2024
View reviewed changes
noir-projects/aztec-nr/aztec/src/encrypted_logs/payload.nr Outdated
@@ -13,13 +13,52 @@ use crate::{
keys::point_to_symmetric_key::point_to_symmetric_key,
};

pub fn compute_encrypted_log<let P: u32, let M: u32>(
pub fn compute_encrypted_note_log<let P: u32, let M: u32>(
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added compute_encrypted_note_log and compute_encrypted_event_log wrapper functions which just call compute_encrypted_log with different offset.

@benesjan benesjan force-pushed the 10-23-refactor_passing_partial_note_logs_through_transient_storage branch 2 times, most recently from 3ae2f41 to fb79008 Compare October 24, 2024 19:25
benesjan
benesjan commented Oct 24, 2024
View reviewed changes
noir-projects/noir-contracts/contracts/token_contract/src/main.nr
@@ -24,7 +24,6 @@ contract Token {
encrypted_event_emission::encode_and_encrypt_event_unconstrained,
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 changes in this file are a bit unfortunate because originally we didn't have to deal with "transient" storage as we were able to just pass the values as args to the teardown function.

I decided to use the transient storage approach here as well because:

  1. I forgot about this flow and I stumbled upon it only after I changed the macros 😂 (and after the change we cannot instantiate finalization payload from raw values as it expects context and loading from storage),
  2. I think we might need to go with the transient storage approach because that will allow us to eventually not force user to define the note log length manually which I think is extremely important (deriving that manually essentially makes the dev a compiler).

@benesjan benesjan changed the base branch from master to 10-25-chore_bumping_constants October 25, 2024 14:32
@benesjan benesjan mentioned this pull request Oct 25, 2024
Merged
Base automatically changed from 10-25-chore_bumping_constants to master October 25, 2024 18:15
@benesjan benesjan force-pushed the 10-23-refactor_passing_partial_note_logs_through_transient_storage branch from 8841d78 to 1e49301 Compare October 25, 2024 18:15
nventuro
nventuro approved these changes Oct 25, 2024
View reviewed changes
noir-projects/aztec-nr/aztec/src/encrypted_logs/payload.nr Outdated Show resolved Hide resolved
noir-projects/aztec-nr/aztec/src/encrypted_logs/payload.nr Outdated Show resolved Hide resolved
noir-projects/aztec-nr/aztec/src/encrypted_logs/payload.nr Outdated Show resolved Hide resolved
noir-projects/aztec-nr/aztec/src/macros/notes/mod.nr Outdated Show resolved Hide resolved
noir-projects/aztec-nr/aztec/src/macros/notes/mod.nr Outdated Show resolved Hide resolved
noir-projects/aztec-nr/aztec/src/macros/notes/mod.nr Show resolved Hide resolved
noir-projects/noir-contracts/contracts/nft_contract/src/main.nr Show resolved Hide resolved
yarn-project/pxe/src/note_processor/note_processor.test.ts Show resolved Hide resolved
yarn-project/pxe/src/note_processor/note_processor.ts
const excludedIndices: Set<number> = new Set();
for (const functionLogs of txFunctionLogs) {
Copy link
Contributor

Choose a reason for hiding this comment

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

These diffs are easy to read anyway with the 'hide whitespace` review flag

@nventuro
Copy link
Contributor

Looks like we're ready to be done with this for a while 😅 thanks a lot for pushing through the pain!

The only thing I don't fully understand is why you say we'd not need to know the length if we were using transient storage.

@benesjan benesjan marked this pull request as draft October 25, 2024 21:21
@benesjan benesjan force-pushed the 10-23-refactor_passing_partial_note_logs_through_transient_storage branch from 07072c9 to 6b42b41 Compare October 25, 2024 21:32
@benesjan benesjan marked this pull request as ready for review October 25, 2024 21:42
@benesjan benesjan enabled auto-merge (squash) October 25, 2024 21:53
@benesjan benesjan merged commit 8835b31 into master Oct 25, 2024
103 of 104 checks passed
@benesjan benesjan deleted the 10-23-refactor_passing_partial_note_logs_through_transient_storage branch October 25, 2024 22:08
@AztecBot AztecBot mentioned this pull request Oct 25, 2024
Open
@ludamad
Copy link
Collaborator

ludamad commented Oct 26, 2024

This appears to have broken two e2e contract tests

@benesjan
Copy link
Contributor Author

@ludamad submitted a fix

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
None yet
Projects
None yet
Milestone
No milestone
Development

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