refactor(network): use DataOrFin in db executor's Data

[ShahakShama]

This change is 

[codecov]

Codecov Report

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

Project coverage is 68.35%. Comparing base (22a338d) to head (cd84de7).

Files	Patch %	Lines
crates/papyrus_network/src/db_executor/mod.rs	71.42%	3 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2113      +/-   ##
==========================================
+ Coverage   68.34%   68.35%   +0.01%     
==========================================
  Files         132      132              
  Lines       17614    17604      -10     
  Branches    17614    17604      -10     
==========================================
- Hits        12038    12034       -4     
+ Misses       4243     4239       -4     
+ Partials     1333     1331       -2     

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

[asmaastarkware]

Reviewed 4 of 4 files at r1, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @matan-starkware and @ShahakShama)

---

crates/papyrus_network/src/db_executor/mod.rs line 64 at r1 (raw file):

            }
            Data::StateDiffChunk(maybe_state_diff_chunk) => {
                let state_diffs_response =

suggestion: rename the variable to data and use into as done above, or change the above conversions to use from

---

crates/papyrus_network/src/db_executor/test.rs line 79 at r1 (raw file):

                        Data::BlockHeaderAndSignature(DataOrFin(Some(signed_header))) => {
                            assert_eq!(signed_header.block_header.block_number.0, i as u64);
                            assert_eq!(*requested_data_type, DataType::SignedBlockHeader);

remove this assertion (you already validated it) or add 2 cases to this match and delete the above one

[ShahakShama]

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

---

crates/papyrus_network/src/db_executor/mod.rs line 64 at r1 (raw file):

Previously, asmaastarkware (asmaa-starkware) wrote…

suggestion: rename the variable to data and use into as done above, or change the above conversions to use from

I've changed the above to be like this one

---

crates/papyrus_network/src/db_executor/test.rs line 79 at r1 (raw file):

Previously, asmaastarkware (asmaa-starkware) wrote…

remove this assertion (you already validated it) or add 2 cases to this match and delete the above one

Done.

[asmaastarkware]

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

[matan-starkware]

Reviewed 2 of 4 files at r1, 2 of 2 files at r2, all commit messages.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @ShahakShama)

---

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

#[derive(Clone)]
pub enum Data {
    BlockHeaderAndSignature(DataOrFin<SignedBlockHeader>),

?

Suggestion:

SignedBlockHeader(DataOrFin<SignedBlockHeader>)

---

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

                            assert_eq!(*requested_data_type, DataType::StateDiff);
                        }
                    }

This is separate because of the none case?

Code quote:

                    match &data {
                        Data::BlockHeaderAndSignature(_) => {
                            assert_eq!(*requested_data_type, DataType::SignedBlockHeader);
                        }
                        Data::StateDiffChunk(_) => {
                            assert_eq!(*requested_data_type, DataType::StateDiff);
                        }
                    }

---

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

                        _ => {
                            assert_eq!(i, len - 1);
                        }

WDYT of this instead of 2 matches (safe for StateDiff)

Suggestion:

                    match data {
                        Data::BlockHeaderAndSignature(DataOrFin(maybe_header)) => {
                            assert_eq!(*requested_data_type, DataType::SignedBlockHeader);
                            if let Some(header) = maybe_header {
                                assert_eq!(header.block_header.block_number.0, i as u64);
                            }
                        }
                        Data::StateDiffChunk(DataOrFin(Some(_state_diff)))  => {
                            // TODO: check the state diff.
                        }
                        _ => {
                            assert_eq!(i, len - 1);
                        }

---

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

            protobuf::BlockHeadersResponse::decode(&data[..]).unwrap(),
        )
        .unwrap();

Suggestion:

        let data: DataOrFin<SignedBlockHeader> =
            protobuf::BlockHeadersResponse::decode(&data[..]).unwrap().try_into().unwrap();

[ShahakShama]

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

---

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

Previously, matan-starkware wrote…

?

You're right, but this enum is going to be erased in the next PR and I prefer avoiding more conflicts if the change isn't going to last anyway

---

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

Previously, matan-starkware wrote…

This is separate because of the none case?

yes

---

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

Previously, matan-starkware wrote…

WDYT of this instead of 2 matches (safe for StateDiff)

I actually like the two matches, and we plan to fill this TODO at some point

---

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

            protobuf::BlockHeadersResponse::decode(&data[..]).unwrap(),
        )
        .unwrap();

Why? Gilad's style guide says to prefer try_from over try_into if possible (IMO we should use try_into only if the type can be deduced without writing it)

[matan-starkware]

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

---

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

Previously, ShahakShama wrote…

Why? Gilad's style guide says to prefer try_from over try_into if possible (IMO we should use try_into only if the type can be deduced without writing it)

If that's what the style guide says then sure. I'm surprised it can't be deduced from:

        data_sender.unbounded_send(Data::BlockHeaderAndSignature(data)).unwrap();