refactor(batcher): refactor proposal manager

[yair-starkware]

This change is 

[codecov]

Codecov Report

Attention: Patch coverage is 10.93750% with 57 lines in your changes missing coverage. Please review.

Project coverage is 2.57%. Comparing base (79f4c46) to head (64cfda8).
Report is 112 commits behind head on main.

Files with missing lines	Patch %	Lines
crates/batcher/src/proposals_manager.rs	10.93%	55 Missing and 2 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #790       +/-   ##
==========================================
- Coverage   76.03%   2.57%   -73.46%     
==========================================
  Files         359      48      -311     
  Lines       38300    3181    -35119     
  Branches    38300    3181    -35119     
==========================================
- Hits        29120      82    -29038     
+ Misses       6845    3089     -3756     
+ Partials     2335      10     -2325     

Flag	Coverage Δ
	2.57% <10.93%> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

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

[Yael-Starkware]

Reviewed 1 of 4 files at r1.
Reviewable status: 1 of 4 files reviewed, 2 unresolved discussions (waiting on @ArniStarkware, @dafnamatsry, and @yair-starkware)

---

crates/batcher/src/proposals_manager.rs line 203 at r2 (raw file):

                        self.max_txs_per_mempool_request,
                        &mempool_tx_sender,
                    ) => {

wdyt about defining a streaming_future in advance?
same as you did for the building_future.
I think it will be more readable

Code quote:

                    fetch_result = Self::feed_more_mempool_txs(
                        &self.mempool_client,
                        self.max_txs_per_mempool_request,
                        &mempool_tx_sender,
                    ) => {

---

crates/batcher/src/proposals_manager.rs line 232 at r2 (raw file):

        for tx in mempool_txs {
            mempool_tx_sender.send(tx).await.map_err(|err| {
                // TODO: should we return the rest of the txs to the mempool?

yes, but I think it should be one flow together with cleaning up aborted proposals.
I wonder if this is the batcher responsibility or the orchestrator's. In this case the orchestrator may not have gotten all the transactions yet. we should probably discuss this in the broader context of how to return txs to the mempool.

Code quote:

// TODO: should we return the rest of the txs to the mempool?

[Yael-Starkware]

Reviewable status: 1 of 4 files reviewed, 3 unresolved discussions (waiting on @ArniStarkware, @dafnamatsry, and @yair-starkware)

---

crates/batcher/src/proposals_manager.rs line 196 at r2 (raw file):

            );
            pin!(building_future);
            loop {

I think the loop should be inside feed_more_mempool_txs.

Code quote:

loop {

[yair-starkware]

Reviewable status: 1 of 4 files reviewed, 3 unresolved discussions (waiting on @ArniStarkware, @dafnamatsry, and @Yael-Starkware)

---

crates/batcher/src/proposals_manager.rs line 196 at r2 (raw file):

Previously, Yael-Starkware (YaelD) wrote…

I think the loop should be inside feed_more_mempool_txs.

Done.

---

crates/batcher/src/proposals_manager.rs line 203 at r2 (raw file):

Previously, Yael-Starkware (YaelD) wrote…

wdyt about defining a streaming_future in advance?
same as you did for the building_future.
I think it will be more readable

Done.

[Yael-Starkware]

Reviewed 3 of 4 files at r8, all commit messages.
Reviewable status: 3 of 4 files reviewed, 2 unresolved discussions (waiting on @ArniStarkware, @dafnamatsry, and @yair-starkware)

---

crates/batcher/src/proposals_manager.rs line 210 at r8 (raw file):

            },
            builder_done = building_future => {
                info!("Block builder finished.");

The building future can also return with an error.

[yair-starkware]

Reviewable status: 3 of 4 files reviewed, 2 unresolved discussions (waiting on @ArniStarkware, @dafnamatsry, and @Yael-Starkware)

---

crates/batcher/src/proposals_manager.rs line 232 at r2 (raw file):

Previously, Yael-Starkware (YaelD) wrote…

yes, but I think it should be one flow together with cleaning up aborted proposals.
I wonder if this is the batcher responsibility or the orchestrator's. In this case the orchestrator may not have gotten all the transactions yet. we should probably discuss this in the broader context of how to return txs to the mempool.

For now I think the todo is ok.

---

crates/batcher/src/proposals_manager.rs line 210 at r8 (raw file):

Previously, Yael-Starkware (YaelD) wrote…

The building future can also return with an error.

I will handle it in another PR after we have more final API for the block builder

[yair-starkware]

crates/batcher/src/proposals_manager.rs line 232 at r2 (raw file):

Previously, yair-starkware (Yair) wrote…

For now I think the todo is ok.

Resolved in #541

[Yael-Starkware]

Reviewable status: 3 of 4 files reviewed, all discussions resolved (waiting on @ArniStarkware and @dafnamatsry)

[dafnamatsry]

Reviewed 3 of 4 files at r8, all commit messages.
Reviewable status: 3 of 4 files reviewed, all discussions resolved (waiting on @ArniStarkware)

---

crates/batcher/src/proposals_manager.rs line 181 at r8 (raw file):

impl BuildProposalTask {
    async fn run(mut self) -> ProposalsManagerResult<()> {
        // TODO: Should we use a different config for the stream buffer size?

Note:
You defined a different config in https://reviewable.io/reviews/starkware-libs/sequencer/541#-

Code quote:

// TODO: Should we use a different config for the stream buffer size?

---

crates/batcher/src/proposals_manager.rs line 227 at r8 (raw file):

        loop {
            // TODO: Get L1 transactions.
            let mempool_txs = match mempool_client.get_txs(max_txs_per_mempool_request).await {

Note:
handle the case of empty mempool_txs.

[dafnamatsry]

Reviewed 1 of 4 files at r1.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @ArniStarkware)