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(network): unite Data with SignedBlockHeader #2041

Merged
merged 1 commit into from
May 26, 2024

Conversation

ShahakShama
Copy link
Collaborator

@ShahakShama ShahakShama commented May 23, 2024

This change is Reviewable

Copy link

codecov bot commented May 23, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 71.06%. Comparing base (06e7fd1) to head (306c685).

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2041      +/-   ##
==========================================
+ Coverage   71.03%   71.06%   +0.02%     
==========================================
  Files         129      129              
  Lines       16885    16863      -22     
  Branches    16885    16863      -22     
==========================================
- Hits        11995    11983      -12     
+ Misses       3551     3542       -9     
+ Partials     1339     1338       -1     

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

@ShahakShama ShahakShama force-pushed the shahak/unite_SignedBlockHeader branch from fda2d72 to a8bb486 Compare May 23, 2024 09:15
Copy link
Collaborator Author

@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.

Reviewable status: 0 of 9 files reviewed, all discussions resolved (waiting on @DvirYo-starkware and @matan-starkware)


crates/papyrus_network/src/converters/test.rs line 0 at r1 (raw file):
Moved to crates/papyrus_network/src/converters/protobuf_conversion/header_test.rs

@ShahakShama ShahakShama force-pushed the shahak/unite_SignedBlockHeader branch from a8bb486 to 306c685 Compare May 23, 2024 10:14
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 8 of 9 files at r1, 1 of 1 files at r2, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @DvirYo-starkware and @ShahakShama)


crates/papyrus_network/src/converters/protobuf_conversion/header.rs line 270 at r2 (raw file):

}

impl From<Option<SignedBlockHeader>> for protobuf::BlockHeadersResponse {

So if I understand all the deletions below, we previously were using an internal type Data in parallel to the external type BlockHeadersResponse which represented some things which don't belong in the proto BlockHeadersResponse. This change semantically aligned the internal type with the external type so we only hold header relevant information?

Code quote:

impl From<Option<SignedBlockHeader>> for protobuf::BlockHeadersResponse

crates/papyrus_network/src/network_manager/test.rs line 152 at r2 (raw file):

                }
                None => (Data::Fin(DataType::SignedBlockHeader), true),
            };

I find it more readable to break into multiple lines

Suggestion:

        let decoded_data = protobuf::BlockHeadersResponse::decode_length_delimited(&data[..])
            .unwrap()
            .try_into()
            .unwrap();
        let (data, is_fin) = match decoded_data {
            Some(signed_block_header) => {
                (Data::BlockHeaderAndSignature(signed_block_header), false)
            }
            None => (Data::Fin(DataType::SignedBlockHeader), true),
        };

@ShahakShama ShahakShama added this pull request to the merge queue May 26, 2024
Merged via the queue into main with commit 2136c4d May 26, 2024
18 of 19 checks passed
@ShahakShama ShahakShama deleted the shahak/unite_SignedBlockHeader branch May 26, 2024 11:58
@github-actions github-actions bot locked and limited conversation to collaborators May 28, 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.

None yet

2 participants