-
Notifications
You must be signed in to change notification settings - Fork 236
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
[SPEC] Deals #1275
[SPEC] Deals #1275
Conversation
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.
Thanks, Youssef!
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.
A few more comments. Thanks again, Youssef!
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.
Last few comments. Thanks, Youssef!
spec.bs
Outdated
1. Let |igAd| be the [=interest group ad=] from |generatedBid|'s [=generated bid/interest group=]'s | ||
[=interest group/ads=] whose [=interest group ad/render url=] is |generatedBid|'s | ||
[=generated bid/ad descriptor=]'s [=ad descriptor/url=]. | ||
1. If both of the following return true: |
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.
These four lines (7081-7084) seem to be an incomplete transition. Is this correct?
1. If [=query generated bid k-anonymity count=] given |generatedBid| returns true AND
(|generatedBid|'s [=generated bid/selected buyer and seller reporting ID=] does not [=map/exists|exist=] OR
[=query reporting ID k-anonymity count=] given |generatedBid|'s [=generated bid/interest group=], |igAd|,
and the |generatedBid|'s [=generated bid/selected buyer and seller reporting ID=] is true), then:
1. [=list/Append=] |generatedBid| to |bidsToScore|.
This formation allows you to also remove these two lines above:
1. Let |selectedReportingId| be a [=string=]-or-null that is set to null.
1. If |generatedBid|'s [=generated bid/selected buyer and seller reporting ID=] [=map/exists=], set |selectedReportingId| to it.
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.
Hmm This seemed to have been in a messed up state. I restructured it based on your comments. However I left creating the variables because I feel as though it makes that long if statement easier to understand. Let me know what you think.
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.
first round comments. Will take another look later.
almost there. |
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.
sorry for being slow on this. A few last nits, and two questions around k-anon (mostly double check to help me understand).
After these are addressed, it'll be ready.
LGTM |
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.
Thanks, Youssef. A few comments here, though I only reviewed the first half (up to line 3997), will revisit for the second half shortly. Thanks again!
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.
Thanks, Youssef.
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.
Thanks again, Youssef.
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.
Thanks, Youssef!
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.
Everything looks great! Just a few remaining small comments. Thanks, Youssef!
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.
Looks great. Thanks, Youssef!
SHA: bdc705b Reason: push, by JensenPaul Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Adding Spec changes for Deals support.
Preview | Diff