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(batcher): rename build_proposal => propose_block and validate_proposal => validate_block #2162

Conversation

Yael-Starkware
Copy link
Contributor

No description provided.

@reviewable-StarkWare
Copy link

This change is Reviewable

@Yael-Starkware Yael-Starkware marked this pull request as ready for review November 19, 2024 08:50
@Yael-Starkware Yael-Starkware force-pushed the 11-19-chore_batcher_rename_build_proposal_propose_block_and_validate_proposal_validate_block branch from ffbdf2c to fac67dc Compare November 19, 2024 08:52
@Yael-Starkware Yael-Starkware changed the base branch from yael/fix_return_status_and_tests to graphite-base/2162 November 19, 2024 08:52
Copy link
Collaborator

@alonh5 alonh5 left a comment

Choose a reason for hiding this comment

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

:lgtm:

Is this all the locations we want to change? What about the generate error?

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


crates/starknet_batcher/src/batcher.rs line 60 at r1 (raw file):

    proposal_manager: Box<dyn ProposalManagerTrait>,
    propose_streams: HashMap<ProposalId, OutputStreamReceiver>,
    validate_streams: HashMap<ProposalId, InputStreamSender>,

Suggestion:

    propose_tx_streams: HashMap<ProposalId, OutputStreamReceiver>,
    validate_tx_streams: HashMap<ProposalId, InputStreamSender>,

Copy link

codecov bot commented Nov 19, 2024

Codecov Report

Attention: Patch coverage is 45.00000% with 11 lines in your changes missing coverage. Please review.

Project coverage is 12.92%. Comparing base (e3165c4) to head (564faf2).
Report is 494 commits behind head on main.

Files with missing lines Patch % Lines
crates/starknet_batcher_types/src/communication.rs 0.00% 6 Missing ⚠️
crates/starknet_batcher/src/communication.rs 0.00% 4 Missing ⚠️
...us_orchestrator/src/sequencer_consensus_context.rs 75.00% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##             main    #2162       +/-   ##
===========================================
- Coverage   40.10%   12.92%   -27.19%     
===========================================
  Files          26      188      +162     
  Lines        1895    24020    +22125     
  Branches     1895    24020    +22125     
===========================================
+ Hits          760     3104     +2344     
- Misses       1100    20560    +19460     
- Partials       35      356      +321     

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


🚨 Try these New Features:

@Yael-Starkware Yael-Starkware force-pushed the 11-19-chore_batcher_rename_build_proposal_propose_block_and_validate_proposal_validate_block branch from fac67dc to 145844f Compare November 19, 2024 09:05
@Yael-Starkware Yael-Starkware changed the base branch from graphite-base/2162 to main November 19, 2024 09:05
@Yael-Starkware Yael-Starkware force-pushed the 11-19-chore_batcher_rename_build_proposal_propose_block_and_validate_proposal_validate_block branch 2 times, most recently from 564b3c2 to bd4f174 Compare November 19, 2024 09:24
@Yael-Starkware Yael-Starkware force-pushed the 11-19-chore_batcher_rename_build_proposal_propose_block_and_validate_proposal_validate_block branch from bd4f174 to 564faf2 Compare November 19, 2024 09:27
Copy link
Contributor Author

@Yael-Starkware Yael-Starkware left a comment

Choose a reason for hiding this comment

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

these are all the locations as far as I could think of/search for.
I think GenerateProposalError name is ok.

Reviewable status: 6 of 9 files reviewed, all discussions resolved (waiting on @alonh5)


crates/starknet_batcher/src/batcher.rs line 60 at r1 (raw file):

    proposal_manager: Box<dyn ProposalManagerTrait>,
    propose_streams: HashMap<ProposalId, OutputStreamReceiver>,
    validate_streams: HashMap<ProposalId, InputStreamSender>,

done.

Copy link
Collaborator

@alonh5 alonh5 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 3 of 3 files at r2, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @Yael-Starkware)

@Yael-Starkware Yael-Starkware merged commit cd68f7e into main Nov 19, 2024
16 checks passed
@github-actions github-actions bot locked and limited conversation to collaborators Nov 21, 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.

3 participants