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
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion docs/docs/aztec/glossary/call_types.md
Original file line number Diff line number Diff line change
Expand Up @@ -178,7 +178,7 @@ This is used to get a result out of an execution, either private or public. It c

#include_code public_getter /noir-projects/noir-contracts/contracts/auth_contract/src/main.nr rust

#include_code simulate_public_getter yarn-project/end-to-end/src/e2e_auth_contract.test.ts typescript
#include_code simulate_function yarn-project/end-to-end/src/composed/docs_examples.test.ts typescript

:::warning
No correctness is guaranteed on the result of `simulate`! Correct execution is entirely optional and left up to the client that handles this request.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ Similarly we have discovered some anti-patterns too (like privacy leakage) that
We call this the "authentication witness" pattern or authwit for short.

- Approve someone in private domain:
#include_code authwit_to_another_sc /yarn-project/end-to-end/src/e2e_cross_chain_messaging.test.ts typescript
#include_code authwit_to_another_sc /yarn-project/end-to-end/src/e2e_cross_chain_messaging/token_bridge_private.test.ts typescript

Here you approve a contract to burn funds on your behalf.

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -141,14 +141,14 @@ This fetches the wallets from the sandbox and deploys our cross chain harness on

Paste the private flow test below the setup:

#include_code e2e_private_cross_chain /yarn-project/end-to-end/src/e2e_cross_chain_messaging.test.ts typescript
#include_code e2e_private_cross_chain /yarn-project/end-to-end/src/e2e_cross_chain_messaging/token_bridge_private.test.ts typescript

## Public flow test

Paste the public flow below the private flow:

```ts
#include_code e2e_public_cross_chain /yarn-project/end-to-end/src/e2e_public_cross_chain_messaging/deposits.test.ts raw
#include_code e2e_public_cross_chain /yarn-project/end-to-end/src/e2e_cross_chain_messaging/token_bridge_public.test.ts raw
})
```

Expand All @@ -165,7 +165,7 @@ Note - you might have a jest error at the end of each test saying "expected 1-2

## Next Steps

### Follow a more detailed Aztec.js tutorial
### Follow a more detailed Aztec.js tutorial

Follow the tutorial [here](../../../aztecjs-getting-started.md).

Expand Down
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
mod test;

// Test contract showing basic public access control that can be used in private. It uses a SharedMutable state variable to
// publicly store the address of an authorized account that can call private functions.
contract Auth {
Expand All @@ -12,7 +14,7 @@ contract Auth {
// Admin can change the value of the authorized address via set_authorized()
admin: PublicImmutable<AztecAddress>,
// docs:start:shared_mutable_storage
authorized: SharedMutable<AztecAddress, CHANGE_AUTHORIZED_DELAY_BLOCKS>,
authorized: SharedMutable<AztecAddress, CHANGE_AUTHORIZED_DELAY_BLOCKS>,
// docs:end:shared_mutable_storage
}

Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
mod main;
mod utils;
123 changes: 123 additions & 0 deletions noir-projects/noir-contracts/contracts/auth_contract/src/test/main.nr
Original file line number Diff line number Diff line change
@@ -0,0 +1,123 @@
use crate::test::utils;
use crate::Auth;

use dep::aztec::prelude::AztecAddress;

global CHANGE_AUTHORIZED_DELAY_BLOCKS = 5;

// TODO (#8588): These were ported over directly from e2e tests. Refactor these in the correct TXe style.
#[test]
unconstrained fn main() {
Comment on lines +9 to +10
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.

// Setup without account contracts. We are not using authwits here, so dummy accounts are enough
let (env, auth_contract_address, admin, to_authorize, other) = utils::setup();

let authorized_is_unset_initially = || {
env.impersonate(admin);
let authorized = env.call_public(Auth::at(auth_contract_address).get_authorized());
assert_eq(authorized, AztecAddress::from_field(0));
};
authorized_is_unset_initially();

let non_admin_cannot_set_unauthorized = || {
env.impersonate(other);
env.assert_public_call_fails(Auth::at(auth_contract_address).set_authorized(to_authorize));
};
non_admin_cannot_set_unauthorized();

let admin_sets_authorized = || {
env.impersonate(admin);
env.call_public(Auth::at(auth_contract_address).set_authorized(to_authorize));
env.advance_block_by(1);

let scheduled_authorized = env.call_public(Auth::at(auth_contract_address).get_scheduled_authorized());
assert_eq(scheduled_authorized, to_authorize);
};
admin_sets_authorized();

let authorized_is_not_yet_effective = || {
env.impersonate(to_authorize);
let authorized: AztecAddress = env.call_public(Auth::at(auth_contract_address).get_authorized());
assert_eq(authorized, AztecAddress::zero());

env.assert_private_call_fails(Auth::at(auth_contract_address).do_private_authorized_thing());
};
authorized_is_not_yet_effective();

let authorized_becomes_effective_after_delay = || {
env.impersonate(to_authorize);

// We advance block by 4, because the delay is 5, and we initially advanced block by one after setting the value. See below comment for explanation.
env.advance_block_by(CHANGE_AUTHORIZED_DELAY_BLOCKS - 1);
let authorized: AztecAddress = env.call_public(Auth::at(auth_contract_address).get_authorized());
assert_eq(authorized, to_authorize);

let authorized_in_private: AztecAddress = env.call_private(Auth::at(auth_contract_address).get_authorized_in_private());
assert_eq(authorized_in_private, AztecAddress::zero());

// We need to always advance the block one more time to get the current value in private, compared to the value in public.
// To see why let's see this diagram.
// When we schedule a change in public, lets say we are at block 2 (building a tx to be included in block 2), which means the latest committed block is block 1.
// Thus, the value change will be set to block 7 (2 + 5).
// If we now advance our env by 5 blocks, we will be at block 7 (building a tx to be included in block 7), which means the latest committed block is block 6.
// 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
Comment on lines +62 to +74
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.

env.advance_block_by(1);
let authorized_in_private_again: AztecAddress = env.call_private(Auth::at(auth_contract_address).get_authorized_in_private());
assert_eq(authorized_in_private_again, to_authorize);

env.call_private_void(Auth::at(auth_contract_address).do_private_authorized_thing());
};
authorized_becomes_effective_after_delay();

let authorize_other = || {
env.impersonate(admin);
env.call_public(Auth::at(auth_contract_address).set_authorized(other));
env.advance_block_by(1);

let scheduled_authorized = env.call_public(Auth::at(auth_contract_address).get_scheduled_authorized());
assert_eq(scheduled_authorized, other);

let authorized: AztecAddress = env.call_public(Auth::at(auth_contract_address).get_authorized());
assert_eq(authorized, to_authorize);

env.impersonate(to_authorize);
env.call_private_void(Auth::at(auth_contract_address).do_private_authorized_thing());

env.impersonate(other);
env.assert_private_call_fails(Auth::at(auth_contract_address).do_private_authorized_thing());
};
authorize_other();

let authorized_becomes_effective_after_delay_again = || {
env.impersonate(to_authorize);

// We advance the block by 4 again like above
env.advance_block_by(CHANGE_AUTHORIZED_DELAY_BLOCKS - 1);
let authorized: AztecAddress = env.call_public(Auth::at(auth_contract_address).get_authorized());
assert_eq(authorized, other);

let authorized_in_private: AztecAddress = env.call_private(Auth::at(auth_contract_address).get_authorized_in_private());
assert_eq(authorized_in_private, to_authorize);

env.advance_block_by(1);
let authorized_in_private_again: AztecAddress = env.call_private(Auth::at(auth_contract_address).get_authorized_in_private());
assert_eq(authorized_in_private_again, other);

env.assert_private_call_fails(Auth::at(auth_contract_address).do_private_authorized_thing());

env.impersonate(other);
env.call_private_void(Auth::at(auth_contract_address).do_private_authorized_thing());
};
authorized_becomes_effective_after_delay_again();
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
use dep::aztec::{prelude::AztecAddress, test::helpers::test_environment::TestEnvironment};

use crate::Auth;

pub fn setup() -> (&mut TestEnvironment, AztecAddress, AztecAddress, AztecAddress, AztecAddress) {
let mut env = TestEnvironment::new();

let admin = env.create_account();
let to_authorize = env.create_account();
let other = env.create_account();

let initializer_call_interface = Auth::interface().constructor(admin);

let auth_contract = env.deploy_self("Auth").with_public_initializer(initializer_call_interface);
let auth_contract_address = auth_contract.to_address();
env.advance_block_by(1);
(&mut env, auth_contract_address, admin, to_authorize, other)
}
10 changes: 2 additions & 8 deletions yarn-project/end-to-end/Earthfile
Original file line number Diff line number Diff line change
Expand Up @@ -152,9 +152,6 @@ e2e-cheat-codes:
e2e-counter-contract:
DO +E2E_TEST --test=./src/e2e_counter_contract.test.ts

e2e-cross-chain-messaging:
DO +E2E_TEST --test=./src/e2e_cross_chain_messaging.test.ts

e2e-crowdfunding-and-claim:
DO +E2E_TEST --test=./src/e2e_crowdfunding_and_claim.test.ts

Expand Down Expand Up @@ -230,11 +227,8 @@ e2e-fees-fee-juice-payments:
e2e-fees-account-init:
DO +E2E_TEST --test=./src/e2e_fees/account_init.test.ts

e2e-public-cross-chain-messaging:
DO +E2E_TEST --test=./src/e2e_public_cross_chain_messaging

e2e-public-to-private-messaging:
DO +E2E_TEST --test=./src/e2e_public_to_private_messaging.test.ts
e2e-cross-chain-messaging:
DO +E2E_TEST --test=./src/e2e_cross_chain_messaging

e2e-state-vars:
DO +E2E_TEST --test=./src/e2e_state_vars.test.ts
Expand Down
126 changes: 0 additions & 126 deletions yarn-project/end-to-end/src/e2e_auth_contract.test.ts

This file was deleted.

36 changes: 0 additions & 36 deletions yarn-project/end-to-end/src/e2e_counter_contract.test.ts

This file was deleted.

Loading
Loading