feat(batcher): add validate flow to proposal manager

[Yael-Starkware]

No description provided.

[reviewable-StarkWare]

This change is 

[Yael-Starkware]

• fix(batcher): fix send_proposal_content return status and add the relevant tests #2053 : 3 dependent PRs (#2126 , #2148 , #2162 )
• feat(batcher): add validate flow #2050
• refactor(batcher): extract the deadline function in order to share with the validate flow #2049
• refactor(batcher): add input channel size config #2042
• refactor(batcher): add batcher creation func to test #2046
• feat(batcher): add validate flow to proposal manager #1951 👈 (View in Graphite)
• refactor(batcher): add propose_tx_provider fixture #1996
• refactor(batcher): change BuildProposalError to GenerateProposalError #1986
• refactor(batcher): extract common logic into a function for validate flow #1985
• main

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

[codecov]

Codecov Report

Attention: Patch coverage is 75.00000% with 4 lines in your changes missing coverage. Please review.

Project coverage is 5.02%. Comparing base (e3165c4) to head (53eeb10).
Report is 382 commits behind head on main.

Files with missing lines	Patch %	Lines
crates/starknet_batcher/src/proposal_manager.rs	75.00%	0 Missing and 4 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #1951       +/-   ##
==========================================
- Coverage   40.10%   5.02%   -35.09%     
==========================================
  Files          26     140      +114     
  Lines        1895   16808    +14913     
  Branches     1895   16808    +14913     
==========================================
+ Hits          760     844       +84     
- Misses       1100   15896    +14796     
- Partials       35      68       +33     

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

[alonh5]

Reviewed 1 of 3 files at r1, all commit messages.
Reviewable status: 1 of 3 files reviewed, 3 unresolved discussions (waiting on @Yael-Starkware and @yair-starkware)

---

crates/batcher/src/proposal_manager.rs line 88 at r2 (raw file):

    ) -> Result<(), BuildProposalError>;

    // TODO delete allow dead code once the batcher uses this code.

Suggestion:

// TODO: delete allow dead code once the batcher uses this code.

---

crates/batcher/src/proposal_manager.rs line 96 at r2 (raw file):

        deadline: tokio::time::Instant,
        tx_receiver: tokio::sync::mpsc::Receiver<Transaction>,
    ) -> Result<(), BuildProposalError>;

If we want to use the same error we should change it's name.

Suggestion:

ProposalError

---

crates/batcher/src/proposal_manager.rs line 200 at r2 (raw file):

        )?;

        self.spawn_build_block_task(proposal_id, block_builder).await?;

Extracting into a function would be a great preliminary PR :)

[Yael-Starkware]

Reviewable status: 0 of 3 files reviewed, 2 unresolved discussions (waiting on @alonh5 and @yair-starkware)

---

crates/batcher/src/proposal_manager.rs line 96 at r2 (raw file):

Previously, alonh5 (Alon Haramati) wrote…

If we want to use the same error we should change it's name.

I think ProposalError is too general since we have GetProposalResultError and StartHeightError.

changed to GenerateProposalError, which is not perfect because it is similar to Build.

I think the the problem with the terminology comes from the api, we have build or validate proposal, while it should have been propose and validate. And then we could have used the word build as describing both.
wdyt?

btw , opened a separate PR for this name change.

---

crates/batcher/src/proposal_manager.rs line 200 at r2 (raw file):

Previously, alonh5 (Alon Haramati) wrote…

Extracting into a function would be a great preliminary PR :)

Done.

---

crates/batcher/src/proposal_manager.rs line 88 at r2 (raw file):

    ) -> Result<(), BuildProposalError>;

    // TODO delete allow dead code once the batcher uses this code.

done

[alonh5]

Reviewed 3 of 3 files at r3, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @Yael-Starkware and @yair-starkware)

---

crates/batcher/src/proposal_manager_test.rs line 233 at r3 (raw file):

    // Build and validate multiple proposal consecutively (awaiting on them to
    // make sure they finished successfully).
    build_proposal_and_await_completion(&mut proposal_manager, ProposalId(0)).await;

Maybe interleave them?

[Yael-Starkware]

Reviewable status: 0 of 3 files reviewed, all discussions resolved (waiting on @alonh5 and @yair-starkware)

---

crates/batcher/src/proposal_manager_test.rs line 233 at r3 (raw file):

Previously, alonh5 (Alon Haramati) wrote…

Maybe interleave them?

done, unfortunately lines are too long now to see it aligned nicely together.
Do you think it's worth shortening names for this?

[yair-starkware]

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

---

crates/batcher/src/proposal_manager_test.rs line 202 at r4 (raw file):

#[rstest]
#[tokio::test]
async fn validate_proposal_fails_without_start_height(

Consider using a test case in a single test

---

crates/batcher/src/proposal_manager_test.rs line 230 at r4 (raw file):

#[rstest]
#[tokio::test]
async fn validate_proposal_success(

same

[alonh5]

Reviewed 3 of 3 files at r4, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @Yael-Starkware)

---

crates/batcher/src/proposal_manager_test.rs line 233 at r3 (raw file):

Previously, Yael-Starkware (YaelD) wrote…

done, unfortunately lines are too long now to see it aligned nicely together.
Do you think it's worth shortening names for this?

Maybe remove the _and_await_completion suffix to the function names. I don't think it's important to the test reader in this case.

[alonh5]

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

---

crates/starknet_batcher/src/proposal_manager_test.rs line 119 at r5 (raw file):

}

async fn build_proposal_and_await_completion(

Remove these?

Code quote:

_and_await_completion

---

crates/starknet_batcher/src/proposal_manager_test.rs line 39 at r5 (raw file):

    tokio::sync::mpsc::unbounded_channel()
}

Restore blank line.

[Yael-Starkware]

Reviewable status: 3 of 6 files reviewed, all discussions resolved (waiting on @alonh5 and @yair-starkware)

---

crates/starknet_batcher/src/proposal_manager_test.rs line 39 at r5 (raw file):

Previously, alonh5 (Alon Haramati) wrote…

Restore blank line.

done.

---

crates/starknet_batcher/src/proposal_manager_test.rs line 119 at r5 (raw file):

Previously, alonh5 (Alon Haramati) wrote…

Remove these?

done.

---

crates/batcher/src/proposal_manager_test.rs line 233 at r3 (raw file):

Previously, alonh5 (Alon Haramati) wrote…

Maybe remove the _and_await_completion suffix to the function names. I don't think it's important to the test reader in this case.

done.

---

crates/batcher/src/proposal_manager_test.rs line 202 at r4 (raw file):

Previously, yair-starkware (Yair) wrote…

Consider using a test case in a single test

couldn't find a simple solution

---

crates/batcher/src/proposal_manager_test.rs line 230 at r4 (raw file):

Previously, yair-starkware (Yair) wrote…

same

same

[alonh5]

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