-
Notifications
You must be signed in to change notification settings - Fork 26
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 and cleaup daphne and daphne_worker crates #395
Conversation
a73b374
to
2cad5b8
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.
Beautiful work. Only one blocker: Let's drop ed09478, as I strongly suspect we're going to want MockAggregator
in the public API soon in order to support interop testing. This is especially important after we sunset daphne_worker
(in the public repo).
pub taskprov_version: TaskprovVersion, | ||
/// Is the taskprov extension allowed and which taskprov draft should be used? | ||
#[serde(default)] | ||
pub taskprov_version: Option<TaskprovVersion>, |
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.
Don't forget to make the corresponding change to the testing/staging/prod configs.
@@ -162,7 +162,16 @@ pub trait DapLeader<S>: DapAuthorizedSender<S> + DapAggregator<S> { | |||
.map_err(|e| DapAbort::from_codec_error(e, task_id.clone()))?; | |||
debug!("report id is {}", report.report_metadata.id); | |||
|
|||
resolve_taskprov(self, task_id, req, Some(&report.report_metadata)).await?; | |||
if let Some(taskprov_version) = self.get_global_config().taskprov_version { |
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.
if you are checking this for every use of resolve_taskprov, maybe move the check into resolve_taskprov itself?
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.
I thought about that, the reason I kept it out is:
- in roles/helper.rs you'd have to check twice
- you can't forget to check because you need the value inside the option to call
resolve_taskprov
2cad5b8
to
667f81a
Compare
667f81a
to
d7eca8b
Compare
d7eca8b
to
55fe71e
Compare
This is a collection of mostly unrelated commits that clean up the codebase a
bit, they should be reviewed individually instead of as a single unit.