-
Notifications
You must be signed in to change notification settings - Fork 85
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
refactor(sync): make changes to allow easily adding more types to sync #1803
refactor(sync): make changes to allow easily adding more types to sync #1803
Conversation
436be8f
to
b1f7587
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1803 +/- ##
==========================================
+ Coverage 72.12% 72.14% +0.02%
==========================================
Files 139 141 +2
Lines 19925 20013 +88
Branches 19925 20013 +88
==========================================
+ Hits 14370 14439 +69
- Misses 3396 3406 +10
- Partials 2159 2168 +9 ☔ View full report in Codecov by Sentry. |
5901135
to
a2c85e6
Compare
b1f7587
to
f59b775
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please move some code out of this file and into sub modules.
I'm letting it up to you to decide what and to where
Reviewed 2 of 3 files at r1, all commit messages.
Reviewable status: 2 of 3 files reviewed, 3 unresolved discussions (waiting on @ShahakShama)
crates/papyrus_p2p_sync/src/lib.rs
line 118 at r1 (raw file):
loop { let data = data_stream.next().await.expect("Sync data stream should never end")?; data.write_to_storage(&mut self.storage_writer)?;
why not use map?
crates/papyrus_p2p_sync/src/lib.rs
line 127 at r1 (raw file):
} struct HeaderData {
use signedheader struct
crates/papyrus_p2p_sync/src/lib.rs
line 179 at r1 (raw file):
+ u64::try_from(num_blocks_per_query) .expect("Failed converting usize to u64"); debug!("Downloading blocks [{}, {})", current_block_number.0, end_block_number);
add data type to this printout
cf208d6
to
d88b072
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 2 of 3 files reviewed, 3 unresolved discussions (waiting on @nagmo-starkware)
crates/papyrus_p2p_sync/src/lib.rs
line 118 at r1 (raw file):
Previously, nagmo-starkware wrote…
why not use map?
The resulting code looks like:
let mut writer_stream = data_stream.map(|data_result| {
data_result?.write_to_storage(&mut self.storage_writer)?;
Ok::<(), P2PSyncError>(())
});
loop {
writer_stream.next().await.expect("Sync stream should never end")?;
}
IMO the previous code was prettier. If you disagree I can do the code above
crates/papyrus_p2p_sync/src/lib.rs
line 127 at r1 (raw file):
Previously, nagmo-starkware wrote…
use signedheader struct
Done.
crates/papyrus_p2p_sync/src/lib.rs
line 179 at r1 (raw file):
Previously, nagmo-starkware wrote…
add data type to this printout
Done.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed all commit messages.
Reviewable status: 2 of 3 files reviewed, 2 unresolved discussions (waiting on @ShahakShama)
crates/papyrus_p2p_sync/src/lib.rs
line 118 at r1 (raw file):
Previously, ShahakShama wrote…
The resulting code looks like:
let mut writer_stream = data_stream.map(|data_result| { data_result?.write_to_storage(&mut self.storage_writer)?; Ok::<(), P2PSyncError>(()) }); loop { writer_stream.next().await.expect("Sync stream should never end")?; }
IMO the previous code was prettier. If you disagree I can do the code above
what about this?
I'm fine with whatever you decide
Code snippet:
loop {
data_stream.map(|data_result| {
data_result?.write_to_storage(&mut self.storage_writer)
})
.next()
.await
.expect("Sync stream should never end")?;
}
crates/papyrus_p2p_sync/src/lib.rs
line 127 at r1 (raw file):
Previously, ShahakShama wrote…
Done.
I meant to remove the HeaderData struct completly
d88b072
to
c9d9fb2
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 2 of 3 files reviewed, 1 unresolved discussion (waiting on @nagmo-starkware)
crates/papyrus_p2p_sync/src/lib.rs
line 127 at r1 (raw file):
Previously, nagmo-starkware wrote…
I meant to remove the HeaderData struct completly
Done.
c9d9fb2
to
c4b7873
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
Reviewable status: 1 of 5 files reviewed, all discussions resolved (waiting on @nagmo-starkware)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 1 of 5 files reviewed, all discussions resolved
c4b7873
to
8507d96
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 1 of 2 files at r3, 1 of 1 files at r5, all commit messages.
Reviewable status: 3 of 5 files reviewed, all discussions resolved
This change is