[Spec] Add private aggregation support for B&A response #1344
Conversation
|
IETF spec that handles paggResponse field: privacysandbox/draft-ietf-bidding-and-auction-services#15 |
|
@morlovich Mind taking a look? Thanks! |
| get batching scope steps</a>. | ||
|
|
||
| Note: Each non-reserved |event| will have a different [=batching scope=] | ||
| that is created later. |
There was a problem hiding this comment.
this is copied from on-device PAgg part. I don't see a place that creates the batching scope for non-reserved events, IIUC
There was a problem hiding this comment.
I think it may be because those are sent, well, on event?
| @@ -6042,6 +6135,8 @@ event, PAExtendedHistogramContribution contribution)</dfn> method steps are: | |||
| reserved event types are added later. | |||
| 1. If |event| is "`reserved.once`" and |function| is [=worklet function/report-result=] or | |||
| [=worklet function/report-win=], [=exception/throw=] a {{TypeError}}. | |||
| 1. If |event| does not [=string/start with=] "`reserved.`", and |function| is "`scoreAd()`" or | |||
| "`report-result`", return. | |||
There was a problem hiding this comment.
I do not understand this change.
|
|
||
| <div algorithm> | ||
| To <dfn>commit server response private aggregation contributions</dfn> given a | ||
| [=server auction response=] |contributionsMap|, a [=reporting context=] |reportingContext|, a |
There was a problem hiding this comment.
I don't think server auction response is the right type here?
| 1. Return the result of running [=get or create a batching scope=] given |reportingOrigin|, | ||
| |coordinator| and |reportingContext|. | ||
| : <a spec="private-aggregation-api" for="scoping details">get debug scope steps</a> | ||
| :: An algorithm that returns a new [=debug scope=]. |
There was a problem hiding this comment.
So technically you don't need a |scopingDetails| here since you're not integrating with contributeToHistogram[OnEvent], though it might be worthwhile for consistency?
| get batching scope steps</a>. | ||
|
|
||
| Note: Each non-reserved |event| will have a different [=batching scope=] | ||
| that is created later. |
There was a problem hiding this comment.
I think it may be because those are sent, well, on event?
| 1. [=list/For each=] |contribution| of |contributions|: | ||
| 1. Let |entry| be a new [=on event contribution entry=] with the items: | ||
| : [=on event contribution entry/contribution=] | ||
| :: |contribution| |
There was a problem hiding this comment.
It would be nice to have some sort of an assert here that it doesn't have any baseValues?
| :: |reportingOrigin| | ||
| 1. If |eventToContributionsMap|[|event|] does not [=map/exist=], set | ||
| |eventToContributionsMap|[|event|] to « |entry| ». | ||
| 1. Otherwise, [=list/append=] |eventToContributionsMap|[|event|] with |entry|. |
There was a problem hiding this comment.
....append ... with ... is kinda strange. Append |entry| to ... ?
| 1. If |event| does not [=string/start with=] "`reserved.`": | ||
| 1. [=list/For each=] |onEventEntry| of |contributions|: | ||
| 1. If |onEventEntry|'s [=on event contribution entry/server filtered=] is false and |bidId| is | ||
| not |winnerId|, then [=list/remove=] |onEventEntry| from |contributions|. |
There was a problem hiding this comment.
I think I would prefer if you checked it inside the existing loop over contributions, and just used continue as everything else, rather than have a different pre-filtering loop.
| 1. [=list/For each=] |onEventEntry| of |contributions|: | ||
| 1. If |onEventEntry|'s [=on event contribution entry/server filtered=] is false and |bidId| is | ||
| not |winnerId|, then [=list/remove=] |onEventEntry| from |contributions|. | ||
| 1. If |onEventEntry|'s [=on event contribution entry/server filtered=] is false: |
There was a problem hiding this comment.
...and you don't even have onEventEntry at this level? I think you probably just want to move the reserved.win/reserved.loss check into the "For each onEventEntry of contributions :" loop, since now it needs to check the server filtered bit.... except shouldn't things that are server-filtered only be using reserved.always anyway?
...I am not sure you actually need the server-filtered bid if the part about them always having reserved.always are true.
(The way losing components is supposed to be handled is by not having a winnerId be set on them).
Client side support of private aggregation reports for auctions run on bidding and auction service.
Also fix a bug that non-reserved events should be ignored for seller scripts.
Preview | Diff