feat: add mock batcher and implement as a wrapper in integration test

[ayeletstarkware]

This change is 

[codecov-commenter]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 82.46%. Comparing base (26060ef) to head (9e57a3e).

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #332      +/-   ##
==========================================
+ Coverage   82.38%   82.46%   +0.08%     
==========================================
  Files          33       34       +1     
  Lines        1612     1620       +8     
  Branches     1612     1620       +8     
==========================================
+ Hits         1328     1336       +8     
  Misses        219      219              
  Partials       65       65              

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

[MohammadNassar1]

Reviewed 4 of 4 files at r1, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @ayeletstarkware and @giladchase)

---

crates/tests-integration/src/mock_batcher.rs line 10 at r1 (raw file):

#[derive(Clone)]
pub struct MockBatcher {
    mempool_client: MempoolClientImpl,

Why does it implement add_tx?
Maybe we should have a trait for Mempool <-> Batcher communication?

Code quote:

mempool_client: MempoolClientImpl,

[ayeletstarkware]

Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @giladchase)

---

crates/tests-integration/src/mock_batcher.rs line 15 at r1 (raw file):

impl MockBatcher {
    pub fn new(
        mempool: Sender<ComponentRequestAndResponseSender<MempoolRequest, MempoolResponse>>,

Should it be renamed to sender?

Code quote:

mempool

[giladchase]

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

---

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

    pub task_executor: TokioExecutor,
    pub gateway_client: GatewayClient,
    pub mock_batcher: MockBatcher,

Mock is an implementation constraint, the field should describe the role.
There is also consistency with other fields, which also describes roles (note that Tokio isn't mentioned in the executor field).

Suggestion:

    pub batcher: MockBatcher,

---

crates/tests-integration/src/mock_batcher.rs line 10 at r1 (raw file):

Why does it implement add_tx?

This was the requirement.

The requirement was to have a trait that represents the mempool fully, so that it can either be represented by the infra communication layer, or by the mempool itself.

Maybe we should have a trait for Mempool <-> Batcher communication?

I see why you're referring this, but it'd contradict the above requirement.

---

crates/tests-integration/src/mock_batcher.rs line 15 at r1 (raw file):

Previously, ayeletstarkware (Ayelet Zilber) wrote…

Should it be renamed to sender?

See how the gateway communicates with the mempool, name it similarly.

As a second step after merging this, before the step of creating the channel for communicating between the batcher and the integrationtestsetup, see how much work it would be to make the batcher use the same client that the gateway used in order to communicate with the mempool (unless that's already the case?).

[MohammadNassar1]

Reviewed 3 of 3 files at r2, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @giladchase)

---

crates/tests-integration/src/mock_batcher.rs line 10 at r1 (raw file):

Previously, giladchase wrote…

Why does it implement add_tx?

This was the requirement.

The requirement was to have a trait that represents the mempool fully, so that it can either be represented by the infra communication layer, or by the mempool itself.

Maybe we should have a trait for Mempool <-> Batcher communication?

I see why you're referring this, but it'd contradict the above requirement.

The requirement is a bit weird, as we now have a Batcher that can send a transaction!

[MohammadNassar1]

Reviewed 3 of 3 files at r3, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @ayeletstarkware and @giladchase)

---

crates/tests-integration/Cargo.toml line 25 at r3 (raw file):

starknet_gateway = { path = "../gateway", version = "0.0", features = ["testing"] }
starknet_mempool = { path = "../mempool", version = "0.0" }
starknet_mempool_infra = { path = "../mempool_infra", version = "0.0" }

Out of curiosity, what's the difference between them?

Code quote:

starknet_mempool_infra = { path = "../mempool_infra", version = "0.0" }

[ayeletstarkware]

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

---

crates/tests-integration/Cargo.toml line 25 at r3 (raw file):

Previously, MohammadNassar1 (mohammad-starkware) wrote…

Out of curiosity, what's the difference between them?

i have no idea

[giladchase]

Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @MohammadNassar1)

---

crates/tests-integration/src/mock_batcher.rs line 10 at r1 (raw file):

Previously, MohammadNassar1 (mohammad-starkware) wrote…

The requirement is a bit weird, as we now have a Batcher that can send a transaction!

That you're describing, i think, is an authentication scheme, with users and roles.

This needs to be added separately at some point, perhaps even into the mempool itself, but it could also be added at the infra level.

In any case, this is a separate abstraction.
One which is independent from the infra itself, and the mempol's API.

The alternative, of warding things off at the API level, would probably make our type system more convoluted than it needs to be.
It can be done, and it'd work, but since we're gonna have to do some kind of user roles at some point, we might as well put that behavior there, which is a more natural location for it.

[giladchase]

Reviewed 1 of 4 files at r1, 3 of 3 files at r3, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @ayeletstarkware and @MohammadNassar1)

---

crates/tests-integration/Cargo.toml line 25 at r3 (raw file):

Previously, ayeletstarkware (Ayelet Zilber) wrote…

i have no idea

The version thing isn't really necessary for us yet, it is basically to appease crates.io, which doesn't support path dependencies.

So when we do publish, which we haven't thus far, we'll need to assign the current version in addition to the path, for every path dependency we have.