Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

chore(sequencing): remove ProposalChunk #2512

Merged
merged 1 commit into from
Dec 10, 2024

Conversation

guy-starkware
Copy link
Contributor

No description provided.

@reviewable-StarkWare
Copy link

This change is Reviewable

@guy-starkware guy-starkware marked this pull request as ready for review December 8, 2024 08:11
@guy-starkware guy-starkware force-pushed the 12-08-chore_sequencing_remove_proposalwrapper branch from 95f13f3 to b9fa56f Compare December 8, 2024 08:17
@guy-starkware guy-starkware force-pushed the 12-08-chore_sequencer_remove_proposalchunk_type branch from 84ca7d2 to 8a2c0ae Compare December 8, 2024 08:17
@guy-starkware guy-starkware force-pushed the 12-08-chore_sequencing_remove_proposalwrapper branch from b9fa56f to 4aec645 Compare December 8, 2024 08:39
@guy-starkware guy-starkware force-pushed the 12-08-chore_sequencer_remove_proposalchunk_type branch 2 times, most recently from 0eae84e to 78dd7ea Compare December 8, 2024 08:59
@guy-starkware guy-starkware force-pushed the 12-08-chore_sequencer_remove_proposalchunk_type branch from 78dd7ea to bb57c46 Compare December 8, 2024 10:09
@guy-starkware guy-starkware changed the base branch from 12-08-chore_sequencing_remove_proposalwrapper to main December 8, 2024 10:09
Copy link

github-actions bot commented Dec 8, 2024

Artifacts upload workflows:

Copy link

codecov bot commented Dec 8, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 17.99%. Comparing base (e3165c4) to head (307e662).
Report is 808 commits behind head on main.

Additional details and impacted files
@@             Coverage Diff             @@
##             main    #2512       +/-   ##
===========================================
- Coverage   40.10%   17.99%   -22.11%     
===========================================
  Files          26      119       +93     
  Lines        1895    14058    +12163     
  Branches     1895    14058    +12163     
===========================================
+ Hits          760     2530     +1770     
- Misses       1100    11257    +10157     
- Partials       35      271      +236     

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

Copy link
Contributor

@asmaastarkware asmaastarkware left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 5 of 5 files at r1, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @guy-starkware and @matan-starkware)


crates/sequencing/papyrus_consensus/src/types.rs line 31 at r1 (raw file):

    /// The chunks of content returned when iterating the proposal.
    // In practice I expect this to match the type sent to the network
    // (papyrus_protobuf::ConsensusMessage), and not to be specific to just the block's content.

Replace this with doc to ProposalPart (it should contain the ProposalInit + Fin, right?)

Code quote:

    /// The chunks of content returned when iterating the proposal.
    // In practice I expect this to match the type sent to the network
    // (papyrus_protobuf::ConsensusMessage), and not to be specific to just the block's content.

Copy link
Contributor

@asmaastarkware asmaastarkware left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @guy-starkware and @matan-starkware)


crates/sequencing/papyrus_consensus_orchestrator/src/sequencer_consensus_context.rs line 134 at r1 (raw file):

#[async_trait]
impl ConsensusContext for SequencerConsensusContext {
    // TODO(guyn): Switch to ProposalPart when done with the streaming integration.

does this comment still apply?

Copy link
Contributor Author

@guy-starkware guy-starkware left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @asmaastarkware and @matan-starkware)


crates/sequencing/papyrus_consensus/src/types.rs line 31 at r1 (raw file):

Previously, asmaastarkware (asmaa-starkware) wrote…

Replace this with doc to ProposalPart (it should contain the ProposalInit + Fin, right?)

Done.


crates/sequencing/papyrus_consensus_orchestrator/src/sequencer_consensus_context.rs line 134 at r1 (raw file):

Previously, asmaastarkware (asmaa-starkware) wrote…

does this comment still apply?

No, I'm removing it. Thanks!

@guy-starkware guy-starkware force-pushed the 12-08-chore_sequencer_remove_proposalchunk_type branch from bb57c46 to 8682fd1 Compare December 9, 2024 13:23
Copy link
Contributor

@asmaastarkware asmaastarkware left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:lgtm:

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

Copy link
Contributor

@matan-starkware matan-starkware left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 3 of 5 files at r1, 2 of 2 files at r2, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @guy-starkware)


crates/sequencing/papyrus_consensus/src/test_utils.rs line 21 at r2 (raw file):

#[derive(Debug, PartialEq, Clone)]
pub struct MockProposalPart(pub u64);

Please rename this FakeProposalPart or TestProposalPart. A Mock is something which you set expectations on. A Fake is a simplified version for the sake of testing.

Separate PR

Code quote:

#[derive(Debug, PartialEq, Clone)]
pub struct MockProposalPart(pub u64);

@guy-starkware guy-starkware force-pushed the 12-08-chore_sequencer_remove_proposalchunk_type branch from 8682fd1 to 7bc63e0 Compare December 10, 2024 07:31
Copy link
Contributor Author

@guy-starkware guy-starkware left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: 4 of 6 files reviewed, all discussions resolved (waiting on @asmaastarkware and @matan-starkware)


crates/sequencing/papyrus_consensus/src/test_utils.rs line 21 at r2 (raw file):

Previously, matan-starkware wrote…

Please rename this FakeProposalPart or TestProposalPart. A Mock is something which you set expectations on. A Fake is a simplified version for the sake of testing.

Separate PR

Done.

@guy-starkware guy-starkware force-pushed the 12-08-chore_sequencer_remove_proposalchunk_type branch 3 times, most recently from a5d57e5 to 2f802e2 Compare December 10, 2024 08:25
@guy-starkware guy-starkware force-pushed the 12-08-chore_sequencer_remove_proposalchunk_type branch from 2f802e2 to 307e662 Compare December 10, 2024 10:32
Copy link
Contributor Author

@guy-starkware guy-starkware left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

guy-starkware commented Dec 10, 2024

Merge activity

  • Dec 10, 9:07 AM EST: A user started a stack merge that includes this pull request via Graphite.
  • Dec 10, 9:08 AM EST: A user merged this pull request with Graphite.

@guy-starkware guy-starkware merged commit 13a0944 into main Dec 10, 2024
17 checks passed
@github-actions github-actions bot locked and limited conversation to collaborators Dec 12, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants