refactor(consensus): the Context is passed as a param instead of being held as a field by SHC

[matan-starkware]

This change is 

[matan-starkware]

• refactor(consensus): change the broadcast fn in context to take 'ref mut self' #2241
• refactor(consensus): put expectations near the most relevant call site for SHC tests #2239
• refactor(consensus): the Context is passed as a param instead of being held as a field by SHC #2238 👈
• refactor(consensus): the list of validators is passed expliecitly the SHC #2237
• main

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

Join @matan-starkware and the rest of your teammates on Graphite

[codecov]

Codecov Report

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

Project coverage is 66.31%. Comparing base (537e11e) to head (069a8a8).

Files	Patch %	Lines
...g/papyrus_consensus/src/single_height_consensus.rs	0.00%	3 Missing ⚠️
crates/papyrus_node/src/main.rs	0.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2238      +/-   ##
==========================================
+ Coverage   66.29%   66.31%   +0.01%     
==========================================
  Files         139      139              
  Lines       18347    18341       -6     
  Branches    18347    18341       -6     
==========================================
- Hits        12163    12162       -1     
+ Misses       4889     4883       -6     
- Partials     1295     1296       +1     

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

[asmaastarkware]

Reviewed 6 of 6 files at r1, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @matan-starkware)

[ShahakShama]

Reviewed all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @matan-starkware)

---

crates/sequencing/papyrus_consensus/src/lib.rs line 39 at r1 (raw file):

#[allow(missing_docs)]
async fn run_height<BlockT: ConsensusBlock>(
    context: &impl ConsensusContext<Block = BlockT>,

IMO impl should be used only if you have this type in this 1 place. Since you're using it everywhere, IMO <ConsensusContextT: ConsensusContext> will be cleaner
Same in SHC

---

crates/sequencing/papyrus_consensus/src/single_height_consensus.rs line 57 at r1 (raw file):

    pub(crate) async fn start(
        &mut self,
        context: &impl ConsensusContext<Block = BlockT>,

You're choosing here that each function can have a different type of context instead of having the context type hardcoded in the struct with a phantom. I'm just making sure you're aware of the 2 different options here and that you chose the one more correct to your case

---

crates/sequencing/papyrus_consensus/src/single_height_consensus_test.rs line 63 at r1 (raw file):

    // Sends proposal and prevote.
    assert!(matches!(shc.start(&context,).await, Ok(None)));

why ,?

The base branch was changed.

[matan-starkware]

Reviewable status: 3 of 6 files reviewed, 3 unresolved discussions (waiting on @asmaastarkware and @ShahakShama)

---

crates/sequencing/papyrus_consensus/src/lib.rs line 39 at r1 (raw file):

Previously, ShahakShama wrote…

IMO impl should be used only if you have this type in this 1 place. Since you're using it everywhere, IMO <ConsensusContextT: ConsensusContext> will be cleaner
Same in SHC

Done.

---

crates/sequencing/papyrus_consensus/src/single_height_consensus.rs line 57 at r1 (raw file):

Previously, ShahakShama wrote…

You're choosing here that each function can have a different type of context instead of having the context type hardcoded in the struct with a phantom. I'm just making sure you're aware of the 2 different options here and that you chose the one more correct to your case

Yes I'm aware. My feeling was that context is basically an outside resource and the caller can pass it what it wants. It would be very weird for it to pass multiple types, but I felt that adding PhantomData to the struct definition increases complexity for a benefit that I am not sure exists.

---

crates/sequencing/papyrus_consensus/src/single_height_consensus_test.rs line 63 at r1 (raw file):

Previously, ShahakShama wrote…

why ,?

Done.

[ShahakShama]

Reviewed 3 of 3 files at r2, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @matan-starkware)

[asmaastarkware]

Reviewed 1 of 1 files at r3, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @matan-starkware)