Skip to content
This repository has been archived by the owner on Dec 26, 2024. It is now read-only.

feat(network): add Transaction Protocol #2135

Merged
merged 3 commits into from
Jun 25, 2024

Conversation

asmaastarkware
Copy link
Contributor

@asmaastarkware asmaastarkware commented Jun 23, 2024

This change is Reviewable

Copy link

codecov bot commented Jun 23, 2024

Codecov Report

Attention: Patch coverage is 13.55932% with 51 lines in your changes missing coverage. Please review.

Project coverage is 65.62%. Comparing base (43eea8f) to head (213c364).

Files Patch % Lines
crates/papyrus_network/src/db_executor/mod.rs 25.00% 21 Missing ⚠️
...tes/papyrus_protobuf/src/converters/transaction.rs 0.00% 18 Missing ⚠️
crates/papyrus_node/src/main.rs 0.00% 11 Missing ⚠️
crates/papyrus_network/src/network_manager/mod.rs 0.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2135      +/-   ##
==========================================
- Coverage   65.72%   65.62%   -0.11%     
==========================================
  Files         135      135              
  Lines       17834    17875      +41     
  Branches    17834    17875      +41     
==========================================
+ Hits        11722    11730       +8     
- Misses       4825     4859      +34     
+ Partials     1287     1286       -1     

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

Copy link
Contributor

@ShahakShama ShahakShama left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 1 files at r1, 7 of 7 files at r2, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @asmaastarkware)


crates/papyrus_network/src/db_executor/mod.rs line 220 at r2 (raw file):

        for (transaction, transaction_output) in transactions.iter().zip(transaction_outputs.iter())
        {
            result.push((transaction.clone(), transaction_output.clone()));

Use into_iter and remove clone


crates/papyrus_network/src/db_executor/test.rs line 29 at r2 (raw file):

const BUFFER_SIZE: usize = 10;

// TODO(shahak): Add test for state_diff_query_positive_flow.

Add test for transactions, or add a TODO to add such test (maybe it's better to add a TODO in order to keep this PR thin)
I think the best way to do this without code duplication is to have a function that receives the storage reader and the expected items and runs the test, and then call it for headers and for transactions
This function can run all the tests, not just the positive flow one


crates/papyrus_node/src/main.rs line 304 at r2 (raw file):

        network_manager.register_sqmr_subscriber(Protocol::SignedBlockHeader);
    let state_diff_client_channels = network_manager.register_sqmr_subscriber(Protocol::StateDiff);
    let transaction_client_channels =

Remove this line. @eitanm-starkware will add it when he adds transaction as a sync protocol

@asmaastarkware asmaastarkware force-pushed the asmaa/add_transaction_protocol branch from 0df65bf to 7e40497 Compare June 24, 2024 05:49
Copy link
Contributor Author

@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, 3 unresolved discussions (waiting on @eitanm-starkware and @ShahakShama)


crates/papyrus_network/src/db_executor/mod.rs line 220 at r2 (raw file):

Previously, ShahakShama wrote…

Use into_iter and remove clone

Done.


crates/papyrus_network/src/db_executor/test.rs line 29 at r2 (raw file):

Previously, ShahakShama wrote…

Add test for transactions, or add a TODO to add such test (maybe it's better to add a TODO in order to keep this PR thin)
I think the best way to do this without code duplication is to have a function that receives the storage reader and the expected items and runs the test, and then call it for headers and for transactions
This function can run all the tests, not just the positive flow one

Done.


crates/papyrus_node/src/main.rs line 304 at r2 (raw file):

Previously, ShahakShama wrote…

Remove this line. @eitanm-starkware will add it when he adds transaction as a sync protocol

Done.

@ShahakShama ShahakShama force-pushed the shahak/return_network_manager_tests branch from c60153f to 7813706 Compare June 24, 2024 05:55
Copy link
Contributor

@ShahakShama ShahakShama left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 7 files at r3, 10 of 10 files at r4, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @asmaastarkware)

a discussion (no related file):
Blocking until the base PR is merged


@ShahakShama ShahakShama force-pushed the shahak/return_network_manager_tests branch from 7813706 to 06c7a95 Compare June 24, 2024 12:25
Base automatically changed from shahak/return_network_manager_tests to shahak/register_db_executor_through_sqmr_subscriber June 25, 2024 04:55
Base automatically changed from shahak/register_db_executor_through_sqmr_subscriber to shahak/db_executor_channels June 25, 2024 04:56
Base automatically changed from shahak/db_executor_channels to shahak/remove_data June 25, 2024 04:57
Base automatically changed from shahak/remove_data to main June 25, 2024 05:28
@asmaastarkware asmaastarkware force-pushed the asmaa/add_transaction_protocol branch from 7e40497 to 12396a3 Compare June 25, 2024 08:27
Copy link
Contributor

@ShahakShama ShahakShama left a comment

Choose a reason for hiding this comment

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

Reviewed 10 of 10 files at r6, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @asmaastarkware)


crates/papyrus_network/src/db_executor/mod.rs line 109 at r6 (raw file):

                    }
                }
                result = self.state_diff_queries_receiver.next() => {

Why did you change this? keep the same code for all protocols

@asmaastarkware asmaastarkware force-pushed the asmaa/add_transaction_protocol branch from 12396a3 to 4e4eaf0 Compare June 25, 2024 13:21
Copy link
Contributor

@ShahakShama ShahakShama 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 1 of 1 files at r7, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @asmaastarkware)

@asmaastarkware asmaastarkware enabled auto-merge June 25, 2024 13:27
@asmaastarkware asmaastarkware added this pull request to the merge queue Jun 25, 2024
Merged via the queue into main with commit 9c88127 Jun 25, 2024
33 of 34 checks passed
@asmaastarkware asmaastarkware deleted the asmaa/add_transaction_protocol branch June 25, 2024 13:37
@github-actions github-actions bot locked and limited conversation to collaborators Jun 27, 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.

2 participants