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: remove redundant e2e tests and organize #8561

Merged
merged 4 commits into from
Sep 17, 2024

Conversation

sklppy88
Copy link
Contributor

@sklppy88 sklppy88 commented Sep 16, 2024

This is phase 1 of dealing with the e2e tests, where I am handling first the redundant tests. The next phase will start to port over non-redundant tests.

For reference, the doc regarding the state of e2e tests are here: https://hackmd.io/QiOaGHBlS9W_A-ZCU6tpag?view

A quick summary of changes:

  • Consolidate, organize, and remove redundant cross_chain tests into the multi-stage test
  • Remove counter e2e test
  • Port and remove auth e2e test

Copy link
Contributor Author

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

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

@sklppy88 sklppy88 changed the title init refactor: remove redundant e2e tests and organize cross_chain_tests Sep 16, 2024
@sklppy88 sklppy88 force-pushed the ek/refactor/remove-redundant-e2e-tests branch 4 times, most recently from bed393f to b64b782 Compare September 16, 2024 12:54
@sklppy88 sklppy88 marked this pull request as ready for review September 16, 2024 12:54
@sklppy88 sklppy88 changed the title refactor: remove redundant e2e tests and organize cross_chain_tests refactor: remove redundant e2e tests and organize Sep 16, 2024
@sklppy88 sklppy88 force-pushed the ek/refactor/remove-redundant-e2e-tests branch 3 times, most recently from 6b048ad to c97804d Compare September 16, 2024 14:05
@AztecBot
Copy link
Collaborator

AztecBot commented Sep 16, 2024

Docs Preview

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

@sklppy88 sklppy88 force-pushed the ek/refactor/remove-redundant-e2e-tests branch from c97804d to 2462907 Compare September 16, 2024 14:26
@@ -1,27 +0,0 @@
import { getDeployedTestAccountsWallets } from '@aztec/accounts/testing';
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 test was moved to deploy_method.

@@ -98,4 +99,18 @@ describe('e2e_deploy_contract deploy method', () => {
it.skip('publicly deploys and calls a public function in a tx in the same block', async () => {
// TODO(@spalladino): Requires being able to read a nullifier on the same block it was emitted.
});

describe('regressions', () => {
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 test was moved from regressions.test.ts.

@@ -204,60 +167,6 @@ describe('e2e_cross_chain_messaging', () => {
);
await crossChainTestHarness.redeemShieldPrivatelyOnL2(bridgeAmount, secretForRedeemingMintedNotes);
await crossChainTestHarness.expectPrivateBalanceOnL2(ownerAddress, bridgeAmount);
});

it("Bridge can't withdraw my funds if I don't give approval", async () => {
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 was redundant and existed in what was renamed to token_bridge_failure_cases.test.ts.

@@ -65,4 +68,37 @@ describe('e2e_public_cross_chain_messaging failures', () => {
l2Bridge.withWallet(user2Wallet).methods.claim_private(secretHash, bridgeAmount, secret).prove(),
).rejects.toThrow(`No non-nullified L1 to L2 message found for message hash ${wrongMessage.hash().toString()}`);
}, 60_000);

it("Can't claim funds publicly which were intended for private deposit from the token portal", async () => {
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 from e2e_cross_chain_messaging.test.ts.

).rejects.toThrow(`Unknown auth witness for message hash ${expectedBurnMessageHash.toString()}`);
});

it("Can't claim funds publicly if they were deposited privately", async () => {
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 was moved to token_bridge_failure_cases.test.ts..

@@ -1,36 +0,0 @@
import { type AccountWallet, type AztecAddress, type DebugLogger } from '@aztec/aztec.js';
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 was deleted because unnecessary as it was extremely simple and tested in TXe already.

@@ -1,126 +0,0 @@
import { type AccountWallet, AztecAddress, Fr, type PXE } from '@aztec/aztec.js';
Copy link
Contributor Author

Choose a reason for hiding this comment

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

These tests were ported to the TXe, and deleted because this is already covered by state_vars, and max_block_number.

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.

Thanks a lot for the consolidation of the l1<>l2 tests!

Comment on lines +6 to +10
#[test]
unconstrained fn main() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's add a todo mentioning this was simply ported from the e2e flow - at some point we'd want to rewrite this in TXE-style and e.g. check the error strings.

Copy link
Contributor Author

@sklppy88 sklppy88 Sep 17, 2024

Choose a reason for hiding this comment

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

Ah fair, added ! And I did it like this to avoid setup each time. It would be nice to also have assert_fails_with_x, I will look into improving this pattern in the next tests I port.

Comment on lines 46 to 47
// We advance block by 4, because we initially advanced block by one after setting the value. See below comment for explanation.
env.advance_block_by(4);
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's make the contract's delay be a constant in this file. This comment is quite cryptic if you are not also looking at the contract source at the same time.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good call, addressed !

Comment on lines +59 to +74
// Reading the value in public will work, because it will use the current block (7), and the current block is the block of change; but
// if we try to create a historical proof, we do not have access to block 7 yet, and have to build the proof off of block 6, but at this time, the value change will not have
// taken place yet, therefore we need to be at block 8 (building a tx to be included in block 8), for the historical proof to work, as it will have access to the full block 7
// where the value change takes effect.
// Note: We do not see this behavior in the e2e tests because setting the value inplicitly advances the block number by 1.
// 1 2 3 4 5 6 7 8 9
// | | | | | | | | |
// ^
// value change scheduled here
// ^
// get_authorized() (public) called here with block_number = 7
// ^
// get_authorized() (private) called here with block_number = 8
Copy link
Contributor

Choose a reason for hiding this comment

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

This is a very good explanation, but it ultimately shows that the current API is flawed. Users should not have to be exposed to this level of detail for something as simple as this. We should at least have call_private_at.

@sklppy88 sklppy88 force-pushed the ek/refactor/remove-redundant-e2e-tests branch from 2462907 to 7b49181 Compare September 17, 2024 05:02
…main.nr

Co-authored-by: Nicolás Venturo <nicolas.venturo@gmail.com>
@sklppy88 sklppy88 merged commit de2b775 into master Sep 17, 2024
50 checks passed
@sklppy88 sklppy88 deleted the ek/refactor/remove-redundant-e2e-tests branch September 17, 2024 05:36
spypsy pushed a commit that referenced this pull request Sep 17, 2024
🤖 I have created a release *beep* *boop*
---


<details><summary>aztec-package: 0.55.1</summary>

##
[0.55.1](aztec-package-v0.55.0...aztec-package-v0.55.1)
(2024-09-17)


### Features

* CI deploy on sepolia
([#8514](#8514))
([54f0344](54f0344))
* Tx verification & peer scoring on p2p layer. bot support for
EasyPrivateToken
([#8298](#8298))
([beb651f](beb651f))


### Miscellaneous

* Remove ARCHIVER_L1_START_BLOCK
([#8554](#8554))
([bc8d461](bc8d461))
</details>

<details><summary>barretenberg.js: 0.55.1</summary>

##
[0.55.1](barretenberg.js-v0.55.0...barretenberg.js-v0.55.1)
(2024-09-17)


### Miscellaneous

* **barretenberg.js:** Synchronize aztec-packages versions
</details>

<details><summary>aztec-packages: 0.55.1</summary>

##
[0.55.1](aztec-packages-v0.55.0...aztec-packages-v0.55.1)
(2024-09-17)


### Features

* `TXE::store_note_in_cache` --&gt; `TXE::add_note`
([#8547](#8547))
([5a6aaeb](5a6aaeb))
* Add a `comptime` string type for string handling at compile-time
(noir-lang/noir#6026)
([cd7983a](cd7983a))
* CI deploy on sepolia
([#8514](#8514))
([54f0344](54f0344))
* Default to outputting witness with file named after package
(noir-lang/noir#6031)
([cd7983a](cd7983a))
* Let LSP suggest trait impl methods as you are typing them
(noir-lang/noir#6029)
([cd7983a](cd7983a))
* NFT with "transient" storage shield flow
([#8129](#8129))
([578f67c](578f67c))
* Optimize allocating immediate amounts of memory
([#8579](#8579))
([e0185e7](e0185e7))
* Spartan iac
([#8455](#8455))
([16fba46](16fba46))
* Sync from aztec-packages (noir-lang/noir#6028)
([cd7983a](cd7983a))
* Tx verification & peer scoring on p2p layer. bot support for
EasyPrivateToken
([#8298](#8298))
([beb651f](beb651f))
* Unconstraining keys in unconstrained encryption
([#7912](#7912))
([eb9275a](eb9275a))
* Update args hash to be a flat poseidon
([#8571](#8571))
([0c54224](0c54224))
* Use poseidon for fn selectors
([#8239](#8239))
([41891db](41891db))


### Bug Fixes

* Disable side-effects for no_predicates functions
(noir-lang/noir#6027)
([cd7983a](cd7983a))
* Native world state test issues
([#8546](#8546))
([aab8773](aab8773))
* Remove special case for epoch 0
([#8549](#8549))
([b035d01](b035d01))
* Serialize AvmVerificationKeyData
([#8529](#8529))
([78c94a4](78c94a4))


### Miscellaneous

* 7791: Disable world_state test suite
([#8594](#8594))
([ee21583](ee21583))
* Add jq to aztec image
([#8542](#8542))
([a7fb791](a7fb791))
* Add sync suite
([#8550](#8550))
([ce0a9db](ce0a9db))
* **ci:** Action to redo typo PRs
([#8553](#8553))
([3ed5879](3ed5879))
* **ci:** Fix master
([#8534](#8534))
([47c368f](47c368f))
* **ci:** Fix redo-typo-pr.yml
([abf9802](abf9802))
* **ci:** Fix redo-typo-pr.yml
([#8555](#8555))
([7f1673c](7f1673c))
* **ci:** Hotfix
([ffd31aa](ffd31aa))
* **ci:** Hotfix arm ci
([979f267](979f267))
* **ci:** Optimize disk usage in arm run
([#8564](#8564))
([33e6aa4](33e6aa4))
* **ci:** Use labels and if branch=master to control jobs
([#8508](#8508))
([68a2226](68a2226))
* GitHub Actions Deployments to Amazon EKS
([#8563](#8563))
([6fae8f0](6fae8f0))
* Moves add gate out of aux
([#8541](#8541))
([c3ad163](c3ad163))
* Protogalaxy verifier matches prover 2
([#8477](#8477))
([58882b1](58882b1))
* Redo typo PR by ankushgoel27
([#8595](#8595))
([7ca6d24](7ca6d24))
* Redo typo PR by Ocheretovich
([#8559](#8559))
([c4296ba](c4296ba))
* Redo typo PR by Olexandr88
([#8560](#8560))
([e35d148](e35d148))
* Redo typo PR by skaunov
([#8557](#8557))
([8a1e7c3](8a1e7c3))
* Release Noir(0.34.0) (noir-lang/noir#5692)
([cd7983a](cd7983a))
* Remove ARCHIVER_L1_START_BLOCK
([#8554](#8554))
([bc8d461](bc8d461))
* Remove redundant e2e tests and organize
([#8561](#8561))
([de2b775](de2b775))
* Remove unused imports
([#8556](#8556))
([e11242e](e11242e))
* Replace relative paths to noir-protocol-circuits
([2336986](2336986))
* Replace relative paths to noir-protocol-circuits
([9668ed5](9668ed5))
</details>

<details><summary>barretenberg: 0.55.1</summary>

##
[0.55.1](barretenberg-v0.55.0...barretenberg-v0.55.1)
(2024-09-17)


### Bug Fixes

* Native world state test issues
([#8546](#8546))
([aab8773](aab8773))


### Miscellaneous

* 7791: Disable world_state test suite
([#8594](#8594))
([ee21583](ee21583))
* Moves add gate out of aux
([#8541](#8541))
([c3ad163](c3ad163))
* Protogalaxy verifier matches prover 2
([#8477](#8477))
([58882b1](58882b1))
</details>

---
This PR was generated with [Release
Please](https://github.com/googleapis/release-please). See
[documentation](https://github.com/googleapis/release-please#release-please).
AztecBot added a commit to AztecProtocol/barretenberg that referenced this pull request Sep 18, 2024
🤖 I have created a release *beep* *boop*
---


<details><summary>aztec-package: 0.55.1</summary>

##
[0.55.1](AztecProtocol/aztec-packages@aztec-package-v0.55.0...aztec-package-v0.55.1)
(2024-09-17)


### Features

* CI deploy on sepolia
([#8514](AztecProtocol/aztec-packages#8514))
([54f0344](AztecProtocol/aztec-packages@54f0344))
* Tx verification & peer scoring on p2p layer. bot support for
EasyPrivateToken
([#8298](AztecProtocol/aztec-packages#8298))
([beb651f](AztecProtocol/aztec-packages@beb651f))


### Miscellaneous

* Remove ARCHIVER_L1_START_BLOCK
([#8554](AztecProtocol/aztec-packages#8554))
([bc8d461](AztecProtocol/aztec-packages@bc8d461))
</details>

<details><summary>barretenberg.js: 0.55.1</summary>

##
[0.55.1](AztecProtocol/aztec-packages@barretenberg.js-v0.55.0...barretenberg.js-v0.55.1)
(2024-09-17)


### Miscellaneous

* **barretenberg.js:** Synchronize aztec-packages versions
</details>

<details><summary>aztec-packages: 0.55.1</summary>

##
[0.55.1](AztecProtocol/aztec-packages@aztec-packages-v0.55.0...aztec-packages-v0.55.1)
(2024-09-17)


### Features

* `TXE::store_note_in_cache` --&gt; `TXE::add_note`
([#8547](AztecProtocol/aztec-packages#8547))
([5a6aaeb](AztecProtocol/aztec-packages@5a6aaeb))
* Add a `comptime` string type for string handling at compile-time
(noir-lang/noir#6026)
([cd7983a](AztecProtocol/aztec-packages@cd7983a))
* CI deploy on sepolia
([#8514](AztecProtocol/aztec-packages#8514))
([54f0344](AztecProtocol/aztec-packages@54f0344))
* Default to outputting witness with file named after package
(noir-lang/noir#6031)
([cd7983a](AztecProtocol/aztec-packages@cd7983a))
* Let LSP suggest trait impl methods as you are typing them
(noir-lang/noir#6029)
([cd7983a](AztecProtocol/aztec-packages@cd7983a))
* NFT with "transient" storage shield flow
([#8129](AztecProtocol/aztec-packages#8129))
([578f67c](AztecProtocol/aztec-packages@578f67c))
* Optimize allocating immediate amounts of memory
([#8579](AztecProtocol/aztec-packages#8579))
([e0185e7](AztecProtocol/aztec-packages@e0185e7))
* Spartan iac
([#8455](AztecProtocol/aztec-packages#8455))
([16fba46](AztecProtocol/aztec-packages@16fba46))
* Sync from aztec-packages (noir-lang/noir#6028)
([cd7983a](AztecProtocol/aztec-packages@cd7983a))
* Tx verification & peer scoring on p2p layer. bot support for
EasyPrivateToken
([#8298](AztecProtocol/aztec-packages#8298))
([beb651f](AztecProtocol/aztec-packages@beb651f))
* Unconstraining keys in unconstrained encryption
([#7912](AztecProtocol/aztec-packages#7912))
([eb9275a](AztecProtocol/aztec-packages@eb9275a))
* Update args hash to be a flat poseidon
([#8571](AztecProtocol/aztec-packages#8571))
([0c54224](AztecProtocol/aztec-packages@0c54224))
* Use poseidon for fn selectors
([#8239](AztecProtocol/aztec-packages#8239))
([41891db](AztecProtocol/aztec-packages@41891db))


### Bug Fixes

* Disable side-effects for no_predicates functions
(noir-lang/noir#6027)
([cd7983a](AztecProtocol/aztec-packages@cd7983a))
* Native world state test issues
([#8546](AztecProtocol/aztec-packages#8546))
([aab8773](AztecProtocol/aztec-packages@aab8773))
* Remove special case for epoch 0
([#8549](AztecProtocol/aztec-packages#8549))
([b035d01](AztecProtocol/aztec-packages@b035d01))
* Serialize AvmVerificationKeyData
([#8529](AztecProtocol/aztec-packages#8529))
([78c94a4](AztecProtocol/aztec-packages@78c94a4))


### Miscellaneous

* 7791: Disable world_state test suite
([#8594](AztecProtocol/aztec-packages#8594))
([ee21583](AztecProtocol/aztec-packages@ee21583))
* Add jq to aztec image
([#8542](AztecProtocol/aztec-packages#8542))
([a7fb791](AztecProtocol/aztec-packages@a7fb791))
* Add sync suite
([#8550](AztecProtocol/aztec-packages#8550))
([ce0a9db](AztecProtocol/aztec-packages@ce0a9db))
* **ci:** Action to redo typo PRs
([#8553](AztecProtocol/aztec-packages#8553))
([3ed5879](AztecProtocol/aztec-packages@3ed5879))
* **ci:** Fix master
([#8534](AztecProtocol/aztec-packages#8534))
([47c368f](AztecProtocol/aztec-packages@47c368f))
* **ci:** Fix redo-typo-pr.yml
([abf9802](AztecProtocol/aztec-packages@abf9802))
* **ci:** Fix redo-typo-pr.yml
([#8555](AztecProtocol/aztec-packages#8555))
([7f1673c](AztecProtocol/aztec-packages@7f1673c))
* **ci:** Hotfix
([ffd31aa](AztecProtocol/aztec-packages@ffd31aa))
* **ci:** Hotfix arm ci
([979f267](AztecProtocol/aztec-packages@979f267))
* **ci:** Optimize disk usage in arm run
([#8564](AztecProtocol/aztec-packages#8564))
([33e6aa4](AztecProtocol/aztec-packages@33e6aa4))
* **ci:** Use labels and if branch=master to control jobs
([#8508](AztecProtocol/aztec-packages#8508))
([68a2226](AztecProtocol/aztec-packages@68a2226))
* GitHub Actions Deployments to Amazon EKS
([#8563](AztecProtocol/aztec-packages#8563))
([6fae8f0](AztecProtocol/aztec-packages@6fae8f0))
* Moves add gate out of aux
([#8541](AztecProtocol/aztec-packages#8541))
([c3ad163](AztecProtocol/aztec-packages@c3ad163))
* Protogalaxy verifier matches prover 2
([#8477](AztecProtocol/aztec-packages#8477))
([58882b1](AztecProtocol/aztec-packages@58882b1))
* Redo typo PR by ankushgoel27
([#8595](AztecProtocol/aztec-packages#8595))
([7ca6d24](AztecProtocol/aztec-packages@7ca6d24))
* Redo typo PR by Ocheretovich
([#8559](AztecProtocol/aztec-packages#8559))
([c4296ba](AztecProtocol/aztec-packages@c4296ba))
* Redo typo PR by Olexandr88
([#8560](AztecProtocol/aztec-packages#8560))
([e35d148](AztecProtocol/aztec-packages@e35d148))
* Redo typo PR by skaunov
([#8557](AztecProtocol/aztec-packages#8557))
([8a1e7c3](AztecProtocol/aztec-packages@8a1e7c3))
* Release Noir(0.34.0) (noir-lang/noir#5692)
([cd7983a](AztecProtocol/aztec-packages@cd7983a))
* Remove ARCHIVER_L1_START_BLOCK
([#8554](AztecProtocol/aztec-packages#8554))
([bc8d461](AztecProtocol/aztec-packages@bc8d461))
* Remove redundant e2e tests and organize
([#8561](AztecProtocol/aztec-packages#8561))
([de2b775](AztecProtocol/aztec-packages@de2b775))
* Remove unused imports
([#8556](AztecProtocol/aztec-packages#8556))
([e11242e](AztecProtocol/aztec-packages@e11242e))
* Replace relative paths to noir-protocol-circuits
([2336986](AztecProtocol/aztec-packages@2336986))
* Replace relative paths to noir-protocol-circuits
([9668ed5](AztecProtocol/aztec-packages@9668ed5))
</details>

<details><summary>barretenberg: 0.55.1</summary>

##
[0.55.1](AztecProtocol/aztec-packages@barretenberg-v0.55.0...barretenberg-v0.55.1)
(2024-09-17)


### Bug Fixes

* Native world state test issues
([#8546](AztecProtocol/aztec-packages#8546))
([aab8773](AztecProtocol/aztec-packages@aab8773))


### Miscellaneous

* 7791: Disable world_state test suite
([#8594](AztecProtocol/aztec-packages#8594))
([ee21583](AztecProtocol/aztec-packages@ee21583))
* Moves add gate out of aux
([#8541](AztecProtocol/aztec-packages#8541))
([c3ad163](AztecProtocol/aztec-packages@c3ad163))
* Protogalaxy verifier matches prover 2
([#8477](AztecProtocol/aztec-packages#8477))
([58882b1](AztecProtocol/aztec-packages@58882b1))
</details>

---
This PR was generated with [Release
Please](https://github.com/googleapis/release-please). See
[documentation](https://github.com/googleapis/release-please#release-please).
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.

3 participants