-
Notifications
You must be signed in to change notification settings - Fork 81
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 replica update sector info to new method #1376
Conversation
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## master #1376 +/- ##
==========================================
+ Coverage 90.80% 91.00% +0.19%
==========================================
Files 145 145
Lines 27402 27372 -30
==========================================
+ Hits 24883 24909 +26
+ Misses 2519 2463 -56
|
continue; | ||
} | ||
|
||
if update.replica_proof.len() > 4096 { |
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.
This might be a good opportunity to make this magic number a constant somewhere
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.
(not worth blocking Im planning to merge once tests pass)
.context_code(ExitCode::USR_ILLEGAL_STATE, "couldn't load update proof type")?; | ||
if update.update_proof_type != expected_proof_type { | ||
info!("unsupported update proof type {}", i64::from(update.update_proof_type)); | ||
continue; |
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.
nit: For a future where this is called by a snark pack aggregate super snap method -- Im suspicious we might want to just fail the whole thing early here. Don't think you need to do anything in this PR.
Intended as a pure refactor. This doesn't get all of the re-usable code out, but a big step.