Skip to content

Multi-Transfer (12 transfers per tx) and E2E Test #1031

Merged
suyash67 merged 14 commits into
masterfrom
rk/multi-transfer
Aug 18, 2023
Merged

Multi-Transfer (12 transfers per tx) and E2E Test #1031
suyash67 merged 14 commits into
masterfrom
rk/multi-transfer

Conversation

@suyash67

@suyash67 suyash67 commented Jul 12, 2023 •
edited
Loading

Copy link
Copy Markdown
Contributor

Description

Multi-transfer contract demonstrates upto 12 transfers per transaction for applications like payroll. We add two e2e tests, one that spends notes to transfer tokens to 12 unique recipients, other that splits a single note into 12 smaller denomination notes. The second test also tests creation and spending of value notes in the same transaction.

Co-credit: @rahul-kothari, @jeanmon, @Cooper-Kunz, @jfecher.
Thanks to @LeilaWang for helping fix get_balance in aztec-noir.

Also resolves #987 (This PR ensures that we emit different unencrypted logs in the same transaction, @rahul-kothari wrote specific execution tests for this issue in #1026)

Checklist:

  • I have reviewed my diff in github, line by line.
  • Every change is related to the PR description.
  • I have linked this pull request to the issue(s) that it resolves.
  • There are no unexpected formatting changes, superfluous debug logs, or commented-out code.
  • The branch has been merged or rebased against the head of its merge target.
  • I'm happy for the PR to be merged at the reviewer's next convenience.

@suyash67 suyash67 force-pushed the rk/multi-transfer branch 3 times, most recently from 0da4574 to 429032d Compare July 19, 2023 19:22
@suyash67 suyash67 force-pushed the rk/multi-transfer branch from 429032d to 0f016cc Compare July 21, 2023 11:29
@suyash67 suyash67 force-pushed the rk/multi-transfer branch 2 times, most recently from 305f21b to 472db53 Compare August 8, 2023 12:09
@suyash67 suyash67 linked an issue Aug 8, 2023 that may be closed by this pull request
[Offsite] Batch and Multi Transfer for Payroll #1462
Closed
@suyash67 suyash67 force-pushed the rk/multi-transfer branch 3 times, most recently from 3989b6d to 9a9e461 Compare August 13, 2023 09:48
@suyash67 suyash67 marked this pull request as ready for review August 13, 2023 19:54
rahul-kothari
rahul-kothari suggested changes Aug 14, 2023
View reviewed changes

@rahul-kothari rahul-kothari left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

GREAT WORK MY MAN!
I'd consider removing debug log statements though!

Comment thread yarn-project/noir-libs/value-note/src/utils.nr Outdated
Comment thread yarn-project/noir-libs/value-note/src/utils.nr Outdated
Comment thread yarn-project/noir-libs/value-note/src/utils.nr Outdated
Comment thread yarn-project/noir-libs/value-note/src/utils.nr Outdated
Comment thread yarn-project/noir-libs/value-note/src/utils.nr Outdated
Comment thread yarn-project/end-to-end/src/e2e_multi_transfer.test.ts
Comment thread yarn-project/end-to-end/src/e2e_multi_transfer.test.ts
Comment thread yarn-project/end-to-end/src/e2e_multi_transfer.test.ts
Comment thread yarn-project/end-to-end/src/e2e_multi_transfer.test.ts Outdated
Comment thread yarn-project/end-to-end/src/e2e_multi_transfer.test.ts Outdated
iAmMichaelConnor
iAmMichaelConnor reviewed Aug 15, 2023
View reviewed changes
Comment thread yarn-project/noir-libs/value-note/src/filter.nr
[notes[0], notes[1]]
}

fn get_4_notes<P>(notes: [Option<ValueNote>; MAX_READ_REQUESTS_PER_CALL], _x: P) -> [Option<ValueNote>; 4] {

@iAmMichaelConnor iAmMichaelConnor Aug 15, 2023

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@sirasistant are we still unable to have a generic get_notes function? (generic over the number of notes)?
(Also, what's P?)

@sirasistant sirasistant Aug 17, 2023

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

It should be possible, using slices

fn get_p_notes<P>(notes: [Option<ValueNote>; MAX_READ_REQUESTS_PER_CALL], p: Field) ->  [Option<ValueNote>]

or changing the signature to pass an empty array of the correct size

fn get_p_notes<P>(notes: [Option<ValueNote>; MAX_READ_REQUESTS_PER_CALL], p: [Option<ValueNote>; P]) ->  [Option<ValueNote>; P]

@iAmMichaelConnor

iAmMichaelConnor commented Aug 15, 2023

Copy link
Copy Markdown
Contributor

This is coming along nicely! Nice one!
Please could you also add a README/big code block explaining what this example app does and why it's been written? (I'm not sure where such a README should live)

rahul-kothari and others added 10 commits August 18, 2023 09:00
@rahul-kothari @suyash67
add multi transfer (initial version)
e0de7c6 
simplify test.

name fix.

Use non-deterministic unenc log in zk token contract

Add multi-transfer test to cci.

Fix e2e build but multi transfer fails.

Fix batchTx call.

Co-authored-by: Jean M <jeanmon@users.noreply.github.com>

fix multi test compilation

contracts compile.

Fix e2e multi compilation.

minor syntax change.

minor syntax change.

contracts compile.

Fix e2e test compilation.

fix multi contract.

Fix e2e multi test compilation.

Remove files that must be gitignored.
@suyash67
Fix note filtering based on pending nullifers.
90b68f9
@suyash67
Fix ctrlv mistake.
fd75e4f
@suyash67
Fix isMined timeout :/:/:/
edc0ff1
@suyash67
Add comments.
1badb30
@suyash67
Remove unnecessary fn.
c3af44d
@suyash67
Simple comments.
fa1f817 
bump timeouts.

bump multi test timeout.
@suyash67
remove debug statements in nr.
045d2d4
@suyash67
rearrange (rahul's suggestion)
3ff9501
@suyash67
Remove spend_note_index.
5e8cadb
rahul-kothari
rahul-kothari reviewed Aug 18, 2023
View reviewed changes
Comment thread circuits/cpp/src/aztec3/circuits/kernel/private/native_private_kernel_circuit_ordering.cpp Outdated
Comment thread yarn-project/aztec-rpc/src/note_processor/note_processor.ts
indexOfTxInABlock * MAX_NEW_COMMITMENTS_PER_TX,
(indexOfTxInABlock + 1) * MAX_NEW_COMMITMENTS_PER_TX,
);
const newNullifiers = block.newNullifiers.slice(

@rahul-kothari rahul-kothari Aug 18, 2023

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

for my understanding - why did we move this earlier?

@suyash67 suyash67 Aug 18, 2023

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

We were fetching the newNullifiers from the L2 block inside the decryption logic. This means for each successfully decrypted note, we re-do the slicing on block.newNullifiers to get the same value over and over again. I moved it outside to fetch the newNullifiers for a transaction just once.

Comment thread yarn-project/end-to-end/src/e2e_multi_transfer.test.ts Outdated
rahul-kothari
rahul-kothari reviewed Aug 18, 2023
View reviewed changes
Comment thread yarn-project/end-to-end/src/e2e_multi_transfer.test.ts
expect(balance).toBe(expectedBalance);
};

/**

@rahul-kothari rahul-kothari Aug 18, 2023

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

love this - thank you for adding it!!!

jeanmon
jeanmon approved these changes Aug 18, 2023
View reviewed changes

@jeanmon jeanmon left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM. Great job!

dbanks12
dbanks12 approved these changes Aug 18, 2023
View reviewed changes

@dbanks12 dbanks12 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Lgtm!

@suyash67 suyash67 merged commit 3445480 into master Aug 18, 2023
@suyash67 suyash67 deleted the rk/multi-transfer branch August 18, 2023 13:34
@jfecher

jfecher commented Aug 18, 2023

Copy link
Copy Markdown
Contributor

Co-credit: @rahul-kothari, @jeanmon, @Cooper-Kunz, @jfecher.

Not sure what I did but you're welcome I guess 😅

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@sirasistant sirasistant sirasistant left review comments

@iAmMichaelConnor iAmMichaelConnor iAmMichaelConnor left review comments

@dbanks12 dbanks12 dbanks12 approved these changes

@jeanmon jeanmon jeanmon approved these changes

+1 more reviewer

@rahul-kothari rahul-kothari rahul-kothari approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

No open projects
A3
Archived in project

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

[Offsite] Batch and Multi Transfer for Payroll Add E2E for encrypted/unencrypted log ordering

7 participants

@suyash67 @iAmMichaelConnor @jfecher @sirasistant @rahul-kothari @dbanks12 @jeanmon