refactor(starknet_batcher): delete the proposal manager

[dafnamatsry]

No description provided.

[reviewable-StarkWare]

This change is 

[dafnamatsry]

• refactor(starknet_batcher): add tests and refactor batcher_test #2640
• refactor(starknet_batcher): delete the proposal manager #2618 👈 (View in Graphite)
• refactor(starknet_batcher): move around some types and utils to a common utils file #2622
• refactor(starknet_batcher): validate flow refactor #2441
• refactor(starknet_batcher): rename GetProposalResultError and remove the ProposalNotFound variant #2483
• refactor(starknet_batcher): add getters to the proposal manger #2480
• main

This stack of pull requests is managed by Graphite. Learn more about stacking.

[codecov]

Codecov Report

Attention: Patch coverage is 68.42105% with 24 lines in your changes missing coverage. Please review.

Project coverage is 6.31%. Comparing base (b3026d5) to head (cdaa5ee).

Files with missing lines	Patch %	Lines
crates/starknet_batcher/src/batcher.rs	68.42%	19 Missing and 5 partials ⚠️

Additional details and impacted files

@@                                                    Coverage Diff                                                     @@
##           12-10-refactor_starknet_batcher_move_around_some_types_and_utils_to_a_common_utils_file   #2618      +/-   ##
==========================================================================================================================
- Coverage                                                                                     6.45%   6.31%   -0.14%     
==========================================================================================================================
  Files                                                                                          146     145       -1     
  Lines                                                                                        17386   17335      -51     
  Branches                                                                                     17386   17335      -51     
==========================================================================================================================
- Hits                                                                                          1122    1095      -27     
+ Misses                                                                                       16188   16166      -22     
+ Partials                                                                                        76      74       -2     

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

[alonh5]

Reviewed 1 of 5 files at r2, 1 of 3 files at r3, 4 of 4 files at r4, all commit messages.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @dafnamatsry and @Yael-Starkware)

---

crates/starknet_batcher/src/proposal_manager.rs line 0 at r4 (raw file):
Delete files.

---

crates/starknet_batcher/src/batcher.rs line 289 at r4 (raw file):

            // TODO(dafna): at this point the proposal result must be an error, since it finsisehd
            // earlier than expected. Consider panicking instead of returning an error.
            Ok(_) => Err(BatcherError::ProposalAlreadyFinished { proposal_id }),

Okay, let's panic.

Code quote:

            // TODO(dafna): at this point the proposal result must be an error, since it finsisehd
            // earlier than expected. Consider panicking instead of returning an error.
            Ok(_) => Err(BatcherError::ProposalAlreadyFinished { proposal_id }),

---

crates/starknet_batcher/src/batcher.rs line 300 at r4 (raw file):

        debug!("Send proposal content done for {}", proposal_id);

        self.validate_tx_streams.remove(&proposal_id);

expect.

Code quote:

self.validate_tx_streams.remove(&proposal_id);

---

crates/starknet_batcher/src/batcher.rs line 304 at r4 (raw file):

            if let Some(proposal_task) = self.active_proposal_task.take() {
                proposal_task.join_handle.await.ok();
            }

Use await_active_proposal().

Code quote:

            if let Some(proposal_task) = self.active_proposal_task.take() {
                proposal_task.join_handle.await.ok();
            }

---

crates/starknet_batcher/src/batcher.rs line 422 at r4 (raw file):

        let mut active_proposal = self.active_proposal.lock().await;
        if let Some(active_proposal_id) = *active_proposal {
            return Err(BatcherError::ServerBusy {

This error name isn't very informative, should we change it? (In a different PR)

Code quote:

ServerBusy

[alonh5]

Reviewed 1 of 1 files at r5, all commit messages.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @dafnamatsry and @Yael-Starkware)

[dafnamatsry]

Reviewable status: 2 of 6 files reviewed, 5 unresolved discussions (waiting on @alonh5 and @Yael-Starkware)

---

crates/starknet_batcher/src/batcher.rs line 289 at r4 (raw file):

Previously, alonh5 (Alon Haramati) wrote…

Okay, let's panic.

Done.
Note that I deleted a test that simulated this scenario (something that cannot happen for real).

---

crates/starknet_batcher/src/batcher.rs line 300 at r4 (raw file):

Previously, alonh5 (Alon Haramati) wrote…

expect.

Done.

---

crates/starknet_batcher/src/batcher.rs line 304 at r4 (raw file):

Previously, alonh5 (Alon Haramati) wrote…

Use await_active_proposal().

Done.

---

crates/starknet_batcher/src/batcher.rs line 422 at r4 (raw file):

Previously, alonh5 (Alon Haramati) wrote…

This error name isn't very informative, should we change it? (In a different PR)

Yes, something like ProposalInProgress would be better. WDYT?

---

crates/starknet_batcher/src/proposal_manager.rs line at r4 (raw file):

Previously, alonh5 (Alon Haramati) wrote…

Delete files.

Done.

[alonh5]

Reviewed 4 of 4 files at r6, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @dafnamatsry and @Yael-Starkware)

---

crates/starknet_batcher/src/batcher.rs line 289 at r4 (raw file):

Previously, dafnamatsry wrote…

Done.
Note that I deleted a test that simulated this scenario (something that cannot happen for real).

I'm confused, how could it be simulated if it cannot happen?

---

crates/starknet_batcher/src/batcher.rs line 422 at r4 (raw file):

Previously, dafnamatsry wrote…

Yes, something like ProposalInProgress would be better. WDYT?

Sounds good. New PR?

[dafnamatsry]

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @alonh5 and @Yael-Starkware)

---

crates/starknet_batcher/src/batcher.rs line 289 at r4 (raw file):

Previously, alonh5 (Alon Haramati) wrote…

I'm confused, how could it be simulated if it cannot happen?

With a mock block builder. The test mocks a block builder that finish successfully. I the validate flow it can't finish successfully unless we get an explicit Finish request from the consensus.

---

crates/starknet_batcher/src/batcher.rs line 422 at r4 (raw file):

Previously, alonh5 (Alon Haramati) wrote…

Sounds good. New PR?

Yap, I'll open one.

[alonh5]

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

[alonh5]

Reviewed 2 of 2 files at r7, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @Yael-Starkware)