feat: replace private token in testing by LHerskind · Pull Request #2304 · AztecProtocol/aztec-packages

LHerskind · 2023-09-14T16:05:09Z

Fixes #2301.

Replaces the private token in most testing. Not altering docs and some of the cli_docs testing.

Checklist:

Remove the checklist to signal you've completed it. Enable auto-merge if the PR is ready to merge.

If the pull request requires a cryptography review (e.g. cryptographic algorithm implementations) I have added the 'crypto' tag.
I have reviewed my diff in github, line by line and removed unexpected formatting changes, testing logs, or commented-out code.
Every change is related to the PR description.
I have linked this pull request to relevant issues (if any exist).

Maddiaa0

lgtm, have a few nits, just points for discussion though rather than issues

Maddiaa0 · 2023-09-16T19:20:35Z

  });

-  describe('private token contract', () => {
+  describe('stateful test contract contract', () => {


nit: double contract

What do you mean by double contract?

It says contract twice

stateful test contract contract

Maddiaa0 · 2023-09-16T22:04:30Z

+aztec-cli send _initialize \
+  --args $ALICE \
+  --contract-abi TokenContractAbi \
+  --contract-address 0x2d23acefa3ce07b3c308caf78d86c064cdf8957bcea48b38753cf58441796c8c \


probably worth making the address a param if its used alot / fixed. especially since its in the docs

Josh has a PR open for this and we will probably nuke this file so I think it's ok to not do that change here.

As mentioned in #2304 (comment), nuke it.

Maddiaa0 · 2023-09-16T22:06:37Z

+    const secret = Fr.random();
+    const secretHash = await computeMessageSecretHash(secret);
+
+    await token.methods.mint_private(50, secretHash).send().wait();


in general all of these could probably be sped up in the tests by sending the promises together and doing a Promise.all.
Or is this not doable as we require the mint private to happen before the redeem

If done with promise all it still got in as separate txs, so it cannot perform the second part that depends on the first. Might be replaces with requests I guess to do it in the same tx.

Nvm, we can't do it in same tx because the mint_private is a public call and we need the result in private.

Maddiaa0 · 2023-09-16T22:09:37Z

-    token = await Contract.deploy(owner, PrivateTokenArtifact, [100n, owner.getAddress()]).send().deployed();
+
+    token = await Contract.deploy(owner, TokenContractAbi, []).send().deployed();
+    await token.methods._initialize({ address: owner.getAddress() }).send().wait();


hmm, we might want to move the docs:start:setup to another example now, as this has rough syntax with the initialise

It's ugly but I feel that if we explain it in the docs why it's necessary to call here it's ok given that we'll change in hopefully not-too-distant future. @LHerskind do you know if the work allowing for public calls from constructor has been mapped out?

Think it might be fine to point out that you need to initialise if you need any private values set etc, and that it is horribly insecure but it will be addressed when constructor can do calls.

benesjan

Looks good 👍

benesjan · 2023-09-18T08:48:21Z

-    token = await Contract.deploy(owner, PrivateTokenArtifact, [100n, owner.getAddress()]).send().deployed();
+
+    token = await Contract.deploy(owner, TokenContractAbi, []).send().deployed();
+    await token.methods._initialize({ address: owner.getAddress() }).send().wait();


It's ugly but I feel that if we explain it in the docs why it's necessary to call here it's ok given that we'll change in hopefully not-too-distant future. @LHerskind do you know if the work allowing for public calls from constructor has been mapped out?

benesjan · 2023-09-18T09:08:13Z

+aztec-cli send _initialize \
+  --args $ALICE \
+  --contract-abi TokenContractAbi \
+  --contract-address 0x2d23acefa3ce07b3c308caf78d86c064cdf8957bcea48b38753cf58441796c8c \


Josh has a PR open for this and we will probably nuke this file so I think it's ok to not do that change here.

LHerskind · 2023-09-18T09:11:37Z

@LHerskind do you know if the work allowing for public calls from constructor has been mapped out?

Not sure, maybe @iAmMichaelConnor and @PhilWindle started on it?

iAmMichaelConnor · 2023-09-18T11:01:19Z

@LHerskind do you know if the work allowing for public calls from constructor has been mapped out?

Not sure, maybe @iAmMichaelConnor and @PhilWindle started on it?

There's just a github issue at the moment, sitting there for someone to pick up and discuss, then build, when they have time. #2249

🤖 I have created a release *beep* *boop* --- <details><summary>aztec-packages: 0.7.6</summary> ## [0.7.6](aztec-packages-v0.7.5...aztec-packages-v0.7.6) (2023-09-18) ### Features * New api to get note nonces ([#2327](#2327)) ([8f5eb28](8f5eb28)) * Replace private token in testing ([#2304](#2304)) ([934ba96](934ba96)) ### Bug Fixes * Exit with error log when COMMIT_TAG is not set properly on canary ([#2371](#2371)) ([68fe053](68fe053)) * Preserve public function call ordering in account entrypoint ([#2348](#2348)) ([5b2cf75](5b2cf75)) * Return output-debug flag ([#2364](#2364)) ([af86580](af86580)) * Revert "fix: strip leading 'v' from dockerhub tags" ([#2367](#2367)) ([53bc041](53bc041)) * Stale CLI docs ([#2336](#2336)) ([f38873b](f38873b)) * Strip leading 'v' from dockerhub tags ([#2360](#2360)) ([a4bb05c](a4bb05c)) ### Miscellaneous * Added docs for artifact files ([#2362](#2362)) ([6d3ba3f](6d3ba3f)), closes [#2190](#2190) * **aztec_noir:** Remove inputs from consume l1 to l2 message ([#2354](#2354)) ([2235f7c](2235f7c)) * Remove "as unknown" casts for ABIs where possible ([#2331](#2331)) ([bf2651e](bf2651e)) * Script to extract tag version ([#2368](#2368)) ([4b686b0](4b686b0)) * Share e2e code with canary ([#2299](#2299)) ([21224de](21224de)) </details> <details><summary>barretenberg.js: 0.7.6</summary> ## [0.7.6](barretenberg.js-v0.7.5...barretenberg.js-v0.7.6) (2023-09-18) ### Miscellaneous * **barretenberg.js:** Synchronize aztec-packages versions </details> <details><summary>barretenberg: 0.7.6</summary> ## [0.7.6](barretenberg-v0.7.5...barretenberg-v0.7.6) (2023-09-18) ### Miscellaneous * **barretenberg:** Synchronize aztec-packages versions </details> --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please).

LHerskind force-pushed the lh/remove_private_token branch from 9864fd1 to 87aa0a0 Compare September 14, 2023 16:08

LHerskind force-pushed the lh/removing_native_token branch from 2b379f7 to e284b94 Compare September 15, 2023 11:36

LHerskind force-pushed the lh/remove_private_token branch from 87aa0a0 to 7c8dc95 Compare September 15, 2023 11:55

Base automatically changed from lh/removing_native_token to master September 15, 2023 11:59

LHerskind force-pushed the lh/remove_private_token branch 2 times, most recently from 17466aa to 33225d8 Compare September 16, 2023 07:20

LHerskind changed the title ~~feat: remove private token~~ feat: replace private token in testing Sep 16, 2023

LHerskind added 6 commits September 16, 2023 16:35

fix: replace private token in tests

9103eab

refactor: replace private_token in sample_dapp e2e

f4fffdf

feat: add stateful test contract for simulator tests

c77fc90

test: fix missing abi

825702d

test: Update more e2e tests

69b1bfe

feat: update up-quick-start

9fc02a8

LHerskind force-pushed the lh/remove_private_token branch from b57a57e to 9fc02a8 Compare September 16, 2023 16:44

LHerskind requested review from Maddiaa0, benesjan and rahul-kothari September 16, 2023 16:52

LHerskind marked this pull request as ready for review September 16, 2023 18:40

Maddiaa0 reviewed Sep 16, 2023

View reviewed changes

benesjan approved these changes Sep 18, 2023

View reviewed changes

LHerskind merged commit 934ba96 into master Sep 18, 2023

LHerskind deleted the lh/remove_private_token branch September 18, 2023 09:14

AztecBot mentioned this pull request Sep 18, 2023

chore(master): Release 0.7.6 #2349

Merged

Conversation

LHerskind commented Sep 14, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist:

Uh oh!

Maddiaa0 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Maddiaa0 Sep 17, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

benesjan left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

LHerskind commented Sep 18, 2023

Uh oh!

iAmMichaelConnor commented Sep 18, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

LHerskind commented Sep 14, 2023 •

edited

Loading

Maddiaa0 Sep 17, 2023 •

edited

Loading