test: implement end to end test #106

MohammadNassar1 · 2024-05-07T11:04:33Z

This change is

codecov-commenter · 2024-05-07T11:36:01Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 51.00%. Comparing base (6312ee3) to head (6426c74).

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #106      +/-   ##
==========================================
+ Coverage   45.30%   51.00%   +5.70%     
==========================================
  Files          19       19              
  Lines         682      694      +12     
  Branches      682      694      +12     
==========================================
+ Hits          309      354      +45     
+ Misses        358      322      -36     
- Partials       15       18       +3

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

ayeletstarkware

Reviewed 5 of 15 files at r1, 9 of 11 files at r2, 2 of 2 files at r3, all commit messages.
Reviewable status: all files reviewed, 8 unresolved discussions (waiting on @MohammadNassar1)

crates/gateway/src/gateway.rs line 120 at r2 (raw file):

    // Send response.

    let internal_transaction = create_tx_for_testing();

why use "create_for_testing" outside of tests?

crates/gateway/src/utils.rs line 131 at r2 (raw file):

}

pub fn create_tx_for_testing() -> InternalTransaction {

add a todo to change to ThinTransaction?

crates/gateway/tests/end_to_end_test.rs line 140 at r2 (raw file):

    let gateway = Gateway {
        config: gateway_config.clone(),

is the clone necessary?

crates/gateway/tests/end_to_end_test.rs line 165 at r2 (raw file):

    let response: Response<Body> = client.request(request).await.unwrap();

    assert_eq!(response.status(), StatusCode::default());

what is the default?

Code quote:

StatusCode::default()

crates/gateway/tests/end_to_end_test.rs line 136 at r3 (raw file):

        validate_non_zero_l1_gas_fee: true,
        max_calldata_length: 10,
        max_signature_length: 2,

why did you choose these numbers?

Code quote:

        max_calldata_length: 10,
        max_signature_length: 2,

crates/gateway/tests/end_to_end_test.rs line 184 at r3 (raw file):

#[tokio::test]
async fn test_end_to_end() {
    let (network_gateway, network_mempool) = initialize_network_channels();

Perhaps 'network_gateway_to_mempool' would be clearer, or adding a comment would help.

Code quote:

(network_gateway, network_mempool

crates/gateway/tests/end_to_end_test.rs line 196 at r3 (raw file):

    let mempool = Arc::new(Mutex::new(Mempool::new([], network_mempool)));

    let mempool_clone = mempool.clone();

Is this necessary?

crates/gateway/tests/routing_test.rs line 60 at r3 (raw file):

        ..Default::default()
    };
    let (tx_gateway_2_mempool, _rx_gateway_2_mempool) = channel::<GatewayMessage>(1);

I think "to" looks better, don't you?

Code quote:

MohammadNassar1

Reviewable status: 9 of 17 files reviewed, 5 unresolved discussions (waiting on @ayeletstarkware)

crates/gateway/src/gateway.rs line 120 at r2 (raw file):

Previously, ayeletstarkware (Ayelet Zilber) wrote…

why use "create_for_testing" outside of tests?

It's temporal.
Added a TODO.

crates/gateway/tests/end_to_end_test.rs line 140 at r2 (raw file):

Previously, ayeletstarkware (Ayelet Zilber) wrote…

is the clone necessary?

No, removed :)

crates/gateway/tests/end_to_end_test.rs line 165 at r2 (raw file):

Previously, ayeletstarkware (Ayelet Zilber) wrote…

what is the default?

StatusCode::OK.
Updated.

crates/gateway/tests/end_to_end_test.rs line 136 at r3 (raw file):

Previously, ayeletstarkware (Ayelet Zilber) wrote…

why did you choose these numbers?

These are the lengths of the invoke json.

crates/gateway/tests/end_to_end_test.rs line 196 at r3 (raw file):

Previously, ayeletstarkware (Ayelet Zilber) wrote…

Is this necessary?

Yes, borrowing issue...

Itay-Tsabary-Starkware

Reviewable status: 9 of 17 files reviewed, 7 unresolved discussions (waiting on @ayeletstarkware and @MohammadNassar1)

crates/gateway/src/gateway_test.rs line 55 at r4 (raw file):

        .max_signature_length = TOO_SMALL_SIGNATURE_LENGTH;

    let (tx_gateway_to_mempool, _rx_gateway_to_mempool) = channel::<GatewayToMempoolMessage>(1);

Change to _ if unused.

Code quote:

 _rx_gateway_to_mempool

crates/gateway/tests/end_to_end_test.rs line 115 at r4 (raw file):

}

const TEST_FILES_FOLDER: &str = "./tests/fixtures";

Move to top of file?

Code quote:

const TEST_FILES_FOLDER: &str = "./tests/fixtures";

MohammadNassar1

Reviewable status: 9 of 17 files reviewed, 7 unresolved discussions (waiting on @ayeletstarkware and @Itay-Tsabary-Starkware)

crates/gateway/src/gateway_test.rs line 55 at r4 (raw file):

Previously, Itay-Tsabary-Starkware wrote…

Change to _ if unused.

_ closes the channel (the receiver was dropped). In this case, the test fails on SendError.

MohammadNassar1

Reviewable status: 9 of 17 files reviewed, 7 unresolved discussions (waiting on @ayeletstarkware and @Itay-Tsabary-Starkware)

crates/gateway/tests/end_to_end_test.rs line 115 at r4 (raw file):

Previously, Itay-Tsabary-Starkware wrote…

Move to top of file?

Done.

ArniStarkware

Reviewable status: 9 of 17 files reviewed, 9 unresolved discussions (waiting on @ayeletstarkware, @Itay-Tsabary-Starkware, and @MohammadNassar1)

crates/gateway/src/gateway.rs line 46 at r5 (raw file):

    gateway_state: GatewayState,
    network: Arc<GatewayNetworkComponent>,
}

Why Arc?
Is it because of this? A workaround for enabling clone for the state. Is it possible to derive Clone for the network component?

Suggestion:

#[derive(Clone)]
pub struct AppState {
    gateway_state: GatewayState,
    network: GatewayNetworkComponent,
}

crates/gateway/src/gateway.rs line 59 at r5 (raw file):

    // TODO(Arni, 7/5/2024): Change this function to accept GatewayConfig.
    /// Sets up the router with the specified routes for the server.

Please delete :).
I have another TODO in this file - about Squashing stateless_transaction_validator_config into network config.

Code quote:

    // TODO(Arni, 7/5/2024): Change this function to accept GatewayConfig.
    /// Sets up the router with the specified routes for the server.

MohammadNassar1

Reviewable status: 9 of 17 files reviewed, 9 unresolved discussions (waiting on @ArniStarkware, @ayeletstarkware, and @Itay-Tsabary-Starkware)

crates/gateway/src/gateway.rs line 46 at r5 (raw file):

Previously, ArniStarkware (Arnon Hod) wrote…

Why Arc?
Is it because of this? A workaround for enabling clone for the state. Is it possible to derive Clone for the network component?

Correct.
Note that GatewayNetworkComponent doesn't implement Clone, as the receiver doesn't implement Clone since there is a single channel to receive.

crates/gateway/src/gateway.rs line 59 at r5 (raw file):

Previously, ArniStarkware (Arnon Hod) wrote…

Please delete :).
I have another TODO in this file - about Squashing stateless_transaction_validator_config into network config.

Not sure I get it :)
I didn't create this task. Are you saying that the PR addresses this specific todo?

ArniStarkware

Reviewable status: 9 of 17 files reviewed, 8 unresolved discussions (waiting on @ayeletstarkware, @Itay-Tsabary-Starkware, and @MohammadNassar1)

crates/gateway/src/gateway.rs line 59 at r5 (raw file):

Previously, MohammadNassar1 (mohammad-starkware) wrote…

Not sure I get it :)
I didn't create this task. Are you saying that the PR addresses this specific todo?

Yes, This PR addresses this issue. Thank you.

ArniStarkware

Reviewable status: 9 of 17 files reviewed, 9 unresolved discussions (waiting on @ayeletstarkware, @Itay-Tsabary-Starkware, and @MohammadNassar1)

crates/gateway/src/gateway.rs line 67 at r5 (raw file):

        };

        // A workaround for enabling clone for state.

Please remove.

Please address this issue here: https://reviewable.io/reviews/starkware-libs/mempool/121#-NxftI5t39GATgcQc3Nk
This is not a workaround, it is a design choice (That I like) :)

Code quote:

        // A workaround for enabling clone for state.

MohammadNassar1

Reviewable status: 9 of 17 files reviewed, 9 unresolved discussions (waiting on @ArniStarkware, @ayeletstarkware, and @Itay-Tsabary-Starkware)

crates/gateway/src/gateway.rs line 59 at r5 (raw file):

Previously, ArniStarkware (Arnon Hod) wrote…

Yes, This PR addresses this issue. Thank you.

Done.

crates/gateway/src/gateway.rs line 67 at r5 (raw file):

Previously, ArniStarkware (Arnon Hod) wrote…

Please remove.

Please address this issue here: https://reviewable.io/reviews/starkware-libs/mempool/121#-NxftI5t39GATgcQc3Nk

This is not a workaround, it is a design choice (That I like) :)

Done.

elintul

Reviewed 3 of 9 files at r8, 2 of 5 files at r9, 6 of 7 files at r11, all commit messages.
Reviewable status: 17 of 18 files reviewed, 8 unresolved discussions (waiting on @ayeletstarkware, @Itay-Tsabary-Starkware, and @MohammadNassar1)

crates/gateway/src/gateway.rs line 70 at r11 (raw file):

// Gateway handlers.
/// Checks if the gateway is alive.

Remove.

Code quote:

/// Checks if the gateway is alive.

crates/mempool/src/mempool.rs line 57 at r11 (raw file):

    }

    pub fn empty_mempool(network: MempoolNetworkComponent) -> Self {

Suggestion:

empty

MohammadNassar1

Reviewable status: 15 of 18 files reviewed, 7 unresolved discussions (waiting on @ayeletstarkware, @elintul, and @Itay-Tsabary-Starkware)

crates/gateway/src/gateway.rs line 98 at r7 (raw file):

Previously, elintul (Elin) wrote…

A thinner access would be nice.

Any suggestion? :)

MohammadNassar1

Reviewable status: 15 of 18 files reviewed, 2 unresolved discussions (waiting on @ayeletstarkware, @elintul, and @Itay-Tsabary-Starkware)

crates/gateway/src/gateway.rs line 120 at r2 (raw file):

Previously, MohammadNassar1 (mohammad-starkware) wrote…

It's temporal.
Added a TODO.

Stale

elintul

Reviewed 1 of 3 files at r12, 2 of 4 files at r13, all commit messages.
Reviewable status: 16 of 18 files reviewed, 4 unresolved discussions (waiting on @ayeletstarkware, @Itay-Tsabary-Starkware, and @MohammadNassar1)

crates/gateway/src/gateway.rs line 70 at r14 (raw file):

// Gateway handlers.
async fn is_alive() -> GatewayResult<String> {

Suggestion:

// Gateway handlers.

async fn is_alive() -> GatewayResult<String> {

crates/gateway/tests/end_to_end_test.rs line 26 at r11 (raw file):

const TEST_FILES_FOLDER: &str = "./tests/fixtures";

pub fn create_default_account() -> Account {

Change to derive::Default, can be done in another PR.

Code quote:

Account

crates/mempool/src/mempool.rs line 157 at r14 (raw file):

                self.batcher_tx.send(txs).await?;
                Ok(())
            }

Waiting for this PR to be rebased over Batcher network component.

MohammadNassar1

Reviewable status: 16 of 18 files reviewed, 3 unresolved discussions (waiting on @ayeletstarkware, @elintul, and @Itay-Tsabary-Starkware)

crates/mempool/src/mempool.rs line 157 at r14 (raw file):

Previously, elintul (Elin) wrote…

Waiting for this PR to be rebased over Batcher network component.

Done.

MohammadNassar1

Reviewable status: 12 of 18 files reviewed, 3 unresolved discussions (waiting on @ayeletstarkware, @elintul, and @Itay-Tsabary-Starkware)

crates/gateway/tests/end_to_end_test.rs line 26 at r11 (raw file):

Previously, elintul (Elin) wrote…

Change to derive::Default, can be done in another PR.

Removed, as it's not in use anymore.

elintul

Reviewed 4 of 6 files at r15, all commit messages.
Reviewable status: 16 of 18 files reviewed, 2 unresolved discussions (waiting on @ayeletstarkware, @Itay-Tsabary-Starkware, and @MohammadNassar1)

crates/gateway/src/gateway.rs line 17 at r15 (raw file):

use crate::starknet_api_test_utils::get_sender_address;
use crate::stateless_transaction_validator::StatelessTransactionValidator;
use crate::utils::external_tx_to_thin_tx;

Will this be moved to SN API?

Code quote:

crate::utils::external_tx_to_thin_tx

MohammadNassar1

Reviewable status: 16 of 18 files reviewed, 2 unresolved discussions (waiting on @ayeletstarkware, @elintul, and @Itay-Tsabary-Starkware)

crates/gateway/src/gateway.rs line 17 at r15 (raw file):

Previously, elintul (Elin) wrote…

Will this be moved to SN API?

ThinTx is temporal.
When we move to use InternalTransaction, the converter can be moved to SN_API.

Itay-Tsabary-Starkware

Reviewed 4 of 11 files at r2, 2 of 9 files at r8, 2 of 5 files at r9, 4 of 7 files at r11, 6 of 6 files at r15, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @ayeletstarkware and @elintul)

ArniStarkware

Reviewed 1 of 9 files at r8, 1 of 7 files at r11, 4 of 6 files at r15, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @ayeletstarkware, @elintul, and @MohammadNassar1)

crates/gateway/tests/end_to_end_test.rs line 121 at r15 (raw file):

    let (tx_batcher_to_mempool, rx_batcher_to_mempool) = channel::<BatcherToMempoolMessage>(1);
    let (tx_mempool_to_batcher, mut rx_mempool_to_batcher) = channel::<MempoolToBatcherMessage>(1);

Extract to a function? initizalize_mempool_network_channels.

Or even include the creating of the batcher_channels.

Code quote:

    let (tx_batcher_to_mempool, rx_batcher_to_mempool) = channel::<BatcherToMempoolMessage>(1);
    let (tx_mempool_to_batcher, mut rx_mempool_to_batcher) = channel::<MempoolToBatcherMessage>(1);

giladchase

Reviewed 4 of 11 files at r2, 1 of 8 files at r5, 2 of 12 files at r7, 3 of 9 files at r8, 2 of 5 files at r9, 6 of 7 files at r11.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @ayeletstarkware, @elintul, and @MohammadNassar1)

crates/gateway/tests/end_to_end_test.rs line 92 at r7 (raw file):

    // Ensure the server has time to start up
    sleep(Duration::from_millis(1000)).await;

In general better avoid long sleeps, it slows down tests and can create false negatives in case there are network issues.

Polling + timeouts are probably better, but we can address this issue later.

Code quote:

    // Ensure the server has time to start up
    sleep(Duration::from_millis(1000)).await;

crates/gateway/tests/end_to_end_test.rs line 93 at r7 (raw file):

    // Ensure the server has time to start up
    sleep(Duration::from_millis(1000)).await;
    (ip, port)

Later, not in this PR.

Rationale: they are always used together i think.

Suggestion:

    SocketAddr::from((ip, port))

crates/gateway/tests/end_to_end_test.rs line 105 at r7 (raw file):

    let request = Request::builder()
        .method("POST")
        .uri(format!("http://{}", SocketAddr::from((ip, port))) + "/add_transaction")

Later

Suggestion:

async fn send_and_verify_transaction(
    socket_addr: SocketAddr,
    json_file_path: &Path,
    expected_response: &str,
) {
    let tx_json = fs::read_to_string(json_file_path).unwrap();
    let uri = format!("http://{socket_addr}/add_transaction");
    let request = Request::post(uri)

crates/gateway/tests/end_to_end_test.rs line 111 at r7 (raw file):

    // Create a client
    let client = Client::new();

Later.

other comments that can be understood from the good variable naming

Suggestion:

    let client = Client::new();

crates/gateway/tests/end_to_end_test.rs line 136 at r7 (raw file):

    // Initialize Mempool.
    let mempool = Arc::new(Mutex::new(Mempool::new([], mempool_to_gateway_network)));

Suggestion:

    let mempool = Arc::new(Mutex::new(Mempool::empty_mempool(mempool_to_gateway_network)));

crates/gateway/tests/end_to_end_test.rs line 147 at r7 (raw file):

    // Wait for the listener to receive the transactions.
    sleep(Duration::from_secs(2)).await;

Same (nonblocking) commment on sleeps as above.

Code quote:

    sleep(Duration::from_secs(2)).await;

crates/mempool/src/mempool.rs line 57 at r11 (raw file):

    }

    pub fn empty_mempool(network: MempoolNetworkComponent) -> Self {

Canonical naming

Suggestion:

 pub fn empty(network: MempoolNetworkComponent) -> Self {

elintul

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @ayeletstarkware and @MohammadNassar1)

MohammadNassar1

Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @ayeletstarkware)

crates/gateway/tests/end_to_end_test.rs line 136 at r7 (raw file):

    // Initialize Mempool.
    let mempool = Arc::new(Mutex::new(Mempool::new([], mempool_to_gateway_network)));

Stale?

crates/mempool/src/mempool.rs line 57 at r11 (raw file):

Previously, giladchase wrote…

Canonical naming

stale?

MohammadNassar1 force-pushed the mohammad/end-to-end-test/implement-end-to-end-test branch 2 times, most recently from 48fd378 to 8cfce13 Compare May 7, 2024 11:33

MohammadNassar1 force-pushed the mohammad/end-to-end-test/implement-end-to-end-test branch 4 times, most recently from 3930178 to a8f09b9 Compare May 8, 2024 08:24

MohammadNassar1 force-pushed the mohammad/end-to-end-test/implement-simple-test-with-network-component branch from 808f1f6 to 48e38a3 Compare May 8, 2024 10:32

ayeletstarkware reviewed May 8, 2024

View reviewed changes

MohammadNassar1 force-pushed the mohammad/end-to-end-test/implement-simple-test-with-network-component branch from 48e38a3 to cf8371d Compare May 8, 2024 11:11

MohammadNassar1 changed the base branch from mohammad/end-to-end-test/implement-simple-test-with-network-component to main May 8, 2024 14:19

MohammadNassar1 force-pushed the mohammad/end-to-end-test/implement-end-to-end-test branch from a8f09b9 to 7ebf605 Compare May 8, 2024 14:43

MohammadNassar1 commented May 8, 2024

View reviewed changes

Itay-Tsabary-Starkware requested changes May 9, 2024

View reviewed changes

MohammadNassar1 commented May 9, 2024

View reviewed changes

MohammadNassar1 force-pushed the mohammad/end-to-end-test/implement-end-to-end-test branch from 7ebf605 to e6c3916 Compare May 9, 2024 14:28

MohammadNassar1 commented May 9, 2024

View reviewed changes

ArniStarkware requested review from ayeletstarkware and Itay-Tsabary-Starkware May 12, 2024 06:57

ArniStarkware requested changes May 12, 2024

View reviewed changes

MohammadNassar1 commented May 12, 2024

View reviewed changes

ArniStarkware mentioned this pull request May 12, 2024

refactor: move app function to gateway method #121

Merged

ArniStarkware requested changes May 12, 2024

View reviewed changes

MohammadNassar1 force-pushed the mohammad/end-to-end-test/implement-end-to-end-test branch from e6c3916 to d340ca4 Compare May 12, 2024 09:52

MohammadNassar1 changed the base branch from main to mohammad/gateway/add-network-to-app-method May 12, 2024 09:53

MohammadNassar1 commented May 12, 2024

View reviewed changes

MohammadNassar1 force-pushed the mohammad/gateway/add-network-to-app-method branch 2 times, most recently from 1738915 to 829c021 Compare May 12, 2024 12:53

MohammadNassar1 changed the base branch from mohammad/gateway/add-network-to-app-method to main May 12, 2024 13:24

MohammadNassar1 force-pushed the mohammad/end-to-end-test/implement-end-to-end-test branch from bcad4c0 to 9ea0417 Compare May 20, 2024 12:44

MohammadNassar1 changed the base branch from mohammad/mempool/implement-netowrk-receive to mohammad/gateway/add-tx/send-through-channel May 20, 2024 12:45

MohammadNassar1 force-pushed the mohammad/gateway/add-tx/send-through-channel branch 3 times, most recently from 0cb907c to e277030 Compare May 21, 2024 06:24

MohammadNassar1 changed the base branch from mohammad/gateway/add-tx/send-through-channel to main May 21, 2024 09:01

elintul reviewed May 21, 2024

View reviewed changes

MohammadNassar1 force-pushed the mohammad/end-to-end-test/implement-end-to-end-test branch from 9ea0417 to 903ca0e Compare May 21, 2024 09:27