feat: add IntegrationTestSetup

[giladchase]

Most of the changes are extraction from the end-to-end test, with the
following differences:

• spawning tasks via TokioExecutor#spawn_with_handle, which the mock
  stores as a field to facilitate aborting the job (our main will
  likely do something similar to this).
• added abstract add/get tx methods, the add uses the GatewayClient
  util, and the get currently just a channel (and will use the future
  BatcherMock when ready).
• needed to make config in Gateway public, it doesn't have any
  invariants on it anyway so it should have no problem being pub i think.

commit-id:b41211a9

---

This change is 

[codecov-commenter]

Codecov Report

Attention: Patch coverage is 75.00000% with 1 line in your changes missing coverage. Please review.

Project coverage is 76.49%. Comparing base (0b9493f) to head (cbd6389).

Files	Patch %	Lines
crates/mempool_types/src/errors.rs	0.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #174      +/-   ##
==========================================
+ Coverage   75.73%   76.49%   +0.75%     
==========================================
  Files          23       23              
  Lines         952      953       +1     
  Branches      952      953       +1     
==========================================
+ Hits          721      729       +8     
+ Misses        187      180       -7     
  Partials       44       44              

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

[giladchase]

Was wondering if this should be split: the only way I can think of is to first create IntegrationTestSetup and in a separate PR to make the e2e use it.
But doing that might not make it clear that most of the logic in IntegrationTestSetup is extracted from the e2e test 🤔

Reviewable status: 0 of 7 files reviewed, all discussions resolved

[giladchase]

This PR is currently clashing with GatewayClient that Ayelet has merged, will integrate it here once I refactor it in a separate PR. Will post here soon.

Reviewable status: 0 of 7 files reviewed, all discussions resolved (waiting on @elintul and @MohammadNassar1)

[elintul]

Reviewed 1 of 4 files at r2, 1 of 1 files at r4, all commit messages.
Reviewable status: 2 of 7 files reviewed, all discussions resolved (waiting on @MohammadNassar1)

[elintul]

Reviewed 2 of 7 files at r1.
Reviewable status: 4 of 7 files reviewed, 5 unresolved discussions (waiting on @giladchase and @MohammadNassar1)

---

crates/tests-integration/Cargo.toml line 17 at r4 (raw file):

starknet_api.workspace = true
starknet_gateway = { path = "../gateway", version = "0.0", features = [
    "testing",

Can the trailing comma be removed, so the line doesn't break?

Suggestion:

"testing"

---

crates/tests-integration/src/integration_test_setup.rs line 22 at r4 (raw file):

pub struct IntegrationTestSetup {
    pub executor: TokioExecutor,

Not to be confused with transaction execution.

Suggestion:

task_executor

---

crates/tests-integration/src/integration_test_setup.rs line 40 at r4 (raw file):

            initialize_gateway_network_channels();

        // Build and run Gateway and initialize a gateway client.

Suggestion:

// Build and run gateway; initialize a gateway client.

---

crates/tests-integration/src/integration_test_setup.rs line 59 at r4 (raw file):

        let batcher_channels =
            BatcherToMempoolChannels { rx: rx_batcher_to_mempool, tx: tx_mempool_to_batcher };
        // Build and run mempool.

Suggestion:

            BatcherToMempoolChannels { rx: rx_batcher_to_mempool, tx: tx_mempool_to_batcher };
        
        // Build and run mempool.

---

crates/tests-integration/src/integration_test_setup.rs line 69 at r4 (raw file):

            gateway_client,
            tx_batcher_to_mempool,
            rx_mempool_to_batcher: Mutex::new(rx_mempool_to_batcher),

Why?

Code quote:

Mutex

[elintul]

Reviewed 2 of 4 files at r2, 1 of 1 files at r3.
Reviewable status: all files reviewed, 9 unresolved discussions (waiting on @giladchase and @MohammadNassar1)

---

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

    #[cfg(any(test, feature = "testing"))]
    pub async fn create_for_testing(network_component: GatewayNetworkComponent) -> Gateway {
        let stateless_transaction_validator_config =

Across snakecase code.

Suggestion:

tx

---

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

    pub async fn create_for_testing(network_component: GatewayNetworkComponent) -> Gateway {
        let stateless_transaction_validator_config =
            crate::config::StatelessTransactionValidatorConfig {

Why fully qualified?

Code quote:

crate::config::StatelessTransactionValidatorConfig

---

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

            crate::config::StatefulTransactionValidatorConfig::create_for_testing();

        let config = GatewayConfig {

Suggestion:

gateway_config

---

crates/tests-integration/tests/end_to_end_test.rs line 46 at r4 (raw file):

#[tokio::test]
async fn test_end_to_end() {
    let mock = IntegrationTestSetup::new().await;

Suggestion:

mock_running_system

[elintul]

Reviewable status: all files reviewed, 10 unresolved discussions (waiting on @giladchase and @MohammadNassar1)

---

crates/tests-integration/src/integration_test_setup.rs line 75 at r4 (raw file):

    }

    pub async fn assert_add_tx_success(&self, tx: &ExternalTransaction, expected: &str) {

Is this method needed?

Suggestion:

add_tx

[elintul]

Reviewed 5 of 5 files at r8, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @MohammadNassar1)

[elintul]

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

---

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

Previously, giladchase wrote…

yep, the left-most part of the fully-qualified thing is automatically imported: it's basically either crate, which is obviously always available, or the name of an external crate, like tokio, which are all auto-imported after they are included in cargo.toml.

Interesting, didn't know the last part, or haven't thought about it like that.

[giladchase]

Reviewable status: 2 of 9 files reviewed, 1 unresolved discussion (waiting on @elintul and @MohammadNassar1)

---

crates/tests-integration/src/integration_test_setup.rs line 81 at r5 (raw file):

Previously, giladchase wrote…

Other PR merged, should work now (had to add a missing async in the decleration though, was a bug)

Other PR merged, this PR passes now.

[elintul]

Reviewed 7 of 7 files at r9, all commit messages.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @giladchase and @MohammadNassar1)

---

crates/gateway/src/gateway.rs line 78 at r9 (raw file):

    #[cfg(any(test, feature = "testing"))]
    pub async fn create_for_testing(mempool_client: Arc<dyn MempoolClient>) -> Gateway {

Should we alias by now?

Code quote:

Arc<dyn MempoolClient>

---

crates/mempool_types/src/mempool_types.rs line 30 at r9 (raw file):

    pub state: AccountState,
}

Why the derive(Clone)s?

---

crates/tests-integration/src/integration_test_setup.rs line 25 at r9 (raw file):

    pub gateway_client: GatewayClient,
    // TODO(MockBatcher).
    pub batcher_mempool_client: MempoolClientImpl,

Oh no, maybe the trait should be called XClientInterface and the specific impl. XClient? 😬

Code quote:

MempoolClientImpl

---

crates/tests-integration/src/integration_test_setup.rs line 66 at r9 (raw file):

        Self {
            task_executor: executor,

Suggestion:

task_executor

[giladchase]

Reviewable status: 6 of 9 files reviewed, 3 unresolved discussions (waiting on @elintul and @MohammadNassar1)

---

crates/gateway/src/gateway.rs line 78 at r9 (raw file):

Previously, elintul (Elin) wrote…

Should we alias by now?

Good idea, adding in separate PR: https://reviewable.io/reviews/starkware-libs/mempool/237

---

crates/mempool_types/src/mempool_types.rs line 30 at r9 (raw file):

Previously, elintul (Elin) wrote…

Why the derive(Clone)s?

For the get_txs command in integration_test_setup.

I want to execute this call on a separate thread, to simulate it being called from an external batcher service (until we have a mockBatcher service ready at least).

To do that, I need to clone the mempool client, in order to pass it to the spawned task.

Since Clone is an Ok thing for MempoolRequest/Response to have (not too expensive), I added it without any cfg/testing thing.

---

crates/tests-integration/src/integration_test_setup.rs line 25 at r9 (raw file):

Previously, elintul (Elin) wrote…

Oh no, maybe the trait should be called XClientInterface and the specific impl. XClient? 😬

Can you xplain more plz, i'm not sure i follow 🤔
This is supposed to represent the "Mempool instance" that the (to be implemented MockBatcher) stores and issues commands to.
When we have mockBatcher, this field will be a field there, and called pub mempool_client or maybe just pub mempool.

[MohammadNassar1]

Reviewed 3 of 4 files at r10, all commit messages.
Reviewable status: 8 of 9 files reviewed, 5 unresolved discussions (waiting on @elintul and @giladchase)

---

crates/gateway/src/gateway.rs line 78 at r10 (raw file):

    #[cfg(any(test, feature = "testing"))]
    pub async fn create_for_testing(mempool_client: Arc<dyn MempoolClient>) -> Gateway {

Why it's in a production file?
can you move it to test_util?

Code quote:

 pub async fn create_for_testing

---

crates/tests-integration/tests/end_to_end_test.rs line 11 at r10 (raw file):

    let expected_tx_hash = mock_running_system.assert_add_tx_success(&invoke_tx()).await;

    // TODO: compare

What to compare?

Code quote:

 TODO: compare

[giladchase]

Reviewable status: 8 of 9 files reviewed, 4 unresolved discussions (waiting on @elintul and @MohammadNassar1)

---

crates/gateway/src/gateway.rs line 78 at r10 (raw file):

Previously, MohammadNassar1 (mohammad-starkware) wrote…

Why it's in a production file?
can you move it to test_util?

Done.

I initially thought about putting it there to make it easier to find, but since this one is only useful for integration testing, It's less justified.

---

crates/tests-integration/tests/end_to_end_test.rs line 11 at r10 (raw file):

Previously, MohammadNassar1 (mohammad-starkware) wrote…

What to compare?

derp

[elintul]

Reviewed 2 of 4 files at r10, 4 of 4 files at r11, all commit messages.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @giladchase and @MohammadNassar1)

---

crates/gateway/src/gateway.rs line 78 at r9 (raw file):

Previously, giladchase wrote…

Good idea, adding in separate PR: https://reviewable.io/reviews/starkware-libs/mempool/237

Thanks for thinking of a good name! Let Itay know? We've been struggling to find one.

---

crates/mempool_types/src/mempool_types.rs line 30 at r9 (raw file):

Previously, giladchase wrote…

For the get_txs command in integration_test_setup.

I want to execute this call on a separate thread, to simulate it being called from an external batcher service (until we have a mockBatcher service ready at least).

To do that, I need to clone the mempool client, in order to pass it to the spawned task.

Since Clone is an Ok thing for MempoolRequest/Response to have (not too expensive), I added it without any cfg/testing thing.

Problem is, transactions might be big; let's revisit this.

---

crates/tests-integration/src/integration_test_setup.rs line 25 at r9 (raw file):

Previously, giladchase wrote…

Can you xplain more plz, i'm not sure i follow 🤔
This is supposed to represent the "Mempool instance" that the (to be implemented MockBatcher) stores and issues commands to.
When we have mockBatcher, this field will be a field there, and called pub mempool_client or maybe just pub mempool.

Was wondering what's an in intuitive naming convention will be. Let's discuss offline.

---

crates/tests-integration/src/integration_test_utils.rs line 18 at r11 (raw file):

use starknet_mempool_types::mempool_types::MempoolClient;

pub async fn create_gateway(mempool_client: Arc<dyn MempoolClient>) -> Gateway {

Why not a static method of Gateway anymore?

Code quote:

create_gateway

[giladchase]

Reviewable status: 7 of 10 files reviewed, 5 unresolved discussions (waiting on @elintul and @MohammadNassar1)

---

crates/gateway/src/gateway.rs line 78 at r9 (raw file):

Previously, elintul (Elin) wrote…

Thanks for thinking of a good name! Let Itay know? We've been struggling to find one.

Pinged him about it now.
He's OOO so if he wants it changed we'll talk about it once he's back.

---

crates/tests-integration/src/integration_test_utils.rs line 18 at r11 (raw file):

Previously, elintul (Elin) wrote…

Why not a static method of Gateway anymore?

Mohammad brought it up, and I realized it is only useful for integration tests, so this seemed like a more natural place to look for it.

---

crates/mempool_types/src/mempool_types.rs line 30 at r9 (raw file):

Previously, elintul (Elin) wrote…

Problem is, transactions might be big; let's revisit this.

Okay it was a bug, clone wasn't really necessary, it's just the derive(Clone) that artificially forced them to be Clone.
Deriving manually fixes this.
Fixed.

[giladchase]

Reviewed 4 of 4 files at r13, all commit messages.
Reviewable status: 9 of 10 files reviewed, 2 unresolved discussions (waiting on @elintul and @MohammadNassar1)

[giladchase]

Reviewed 1 of 3 files at r12.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @elintul)