spec: Move over the config/IG code changes from Private Aggregation spec. #1297
Conversation
Right now Private Aggregation monkey patches us a lot to implement our integration. This moves over the most mundane of the changes --- parsing of new IG and AuctionConfig fields --- to reduce the size of the monkeypatch and to practive how the process works. It's moved basically unchanged (except different indent/wrapping); I think the only substantative change is using our origin parser. https://github.com/morlovich/private-aggregation-api/tree/move-stuff-to-pa has the other half, it likely needs something with imports (I hope adding export to auction config/interest group on our end will help).
|
@alexmturner this will likely need your involvement as well. |
spec.bs
Outdated
| @@ -82,6 +82,11 @@ spec: Fenced Frame; urlPrefix: https://wicg.github.io/fenced-frame/ | |||
| spec: Private Aggregation API; urlPrefix: https://patcg-individual-drafts.github.io/private-aggregation-api | |||
There was a problem hiding this comment.
nit: I wonder if this should have spec: private-aggregation-api so that it isn't duplicated at the bottom in Terms defined by reference
There was a problem hiding this comment.
That helps a bit, though there is still kinda silly debug-details-enabled
There was a problem hiding this comment.
not really too important, but I think you can avoid this by doing [=debug details/enabled=] instead of [=debug-details-enabled|enabled=] inline and then
text: debug details
for: debug details
text: enabled
text: key
in the header. We do something similar in the Shared Storage spec here.
There was a problem hiding this comment.
Hmm. So this is kinda a sideways move. It needs ;url still --- that's actually broken in shared storage, too, e.g. it tries to take you to: https://patcg-individual-drafts.github.io/private-aggregation-api/#get-batching-scope-steps
rather than to #scoping-details-get-batching-scope-steps
... and the index ends up with entries for 'enabled' and 'key' inside the private aggregation section, which is probably worse than debug-details-enabled. Like the right thing would be to import the entire type (but I guess it may not know it because it's not exported, and it's kinda a generic name so maybe shouldn't be exported?)
There was a problem hiding this comment.
Ah oof, yeah that's not ideal. Maybe the answer here is to rename it then export it? What you have seems like a pretty reasonable option at least for now, though.
There was a problem hiding this comment.
probalby not related, but better to put the definition of "debug details" to the struct, instead of the section header. I.e., "A <dfn>debug details</dfn> is a [=struct=] ..." and rename the header's id to something different like "debug details header". (it actually applies to many of the type definitions in PAgg spec).
Also, bikeshed has the concept of namespace, like there's [=origin=] and [=url/origin=]. If we want, we can probably do data-dfn-for="pagg" when exporting a generic name.
spec.bs
Outdated
| @@ -82,6 +82,11 @@ spec: Fenced Frame; urlPrefix: https://wicg.github.io/fenced-frame/ | |||
| spec: Private Aggregation API; urlPrefix: https://patcg-individual-drafts.github.io/private-aggregation-api | |||
There was a problem hiding this comment.
probalby not related, but better to put the definition of "debug details" to the struct, instead of the section header. I.e., "A <dfn>debug details</dfn> is a [=struct=] ..." and rename the header's id to something different like "debug details header". (it actually applies to many of the type definitions in PAgg spec).
Also, bikeshed has the concept of namespace, like there's [=origin=] and [=url/origin=]. If we want, we can probably do data-dfn-for="pagg" when exporting a generic name.
|
This LGTM if the open comments get answered. |
|
Err, what is still open? |
|
Didn't notice you already addressed them. Feel free to resolve or reply "done" to those comments that are addressed. |
| :: A [=map=] from [=strings=] to {{AuctionReportBuyersConfig}}s. For buyer metrics delegated to be | ||
| reported to the seller via the [Private Aggregation API](https://github.com/patcg-individual-drafts/private-aggregation-api), | ||
| this determines how each metric bucket is chosen inside the buyer's space, and how to scale it. | ||
| <!-- TODO: this should probably use enums instead --> |
There was a problem hiding this comment.
drive by: should this be an issue instead?
…pec. (WICG#1297) * Move over the config/IG code changes from Private Aggregation spec. Right now Private Aggregation monkey patches us a lot to implement our integration. This moves over the most mundane of the changes --- parsing of new IG and AuctionConfig fields --- to reduce the size of the monkeypatch and to practive how the process works. It's moved basically unchanged (except different indent/wrapping); I think the only substantative change is using our origin parser. https://github.com/morlovich/private-aggregation-api/tree/move-stuff-to-pa has the other half, it likely needs something with imports (I hope adding export to auction config/interest group on our end will help). * Fix field names. * Alex feedback. * Apply Qingxin's feedback. * Merge with Alex's parsing helper refactor. --------- Co-authored-by: Maks Orlovich <[email protected]>
…ere. (#166) Fixes #43. See WICG/turtledove#1297 and WICG/turtledove#1312. Co-authored-by: Maks Orlovich <[email protected]>
Right now Private Aggregation monkey patches us a lot to implement our integration. This moves over the most mundane of the changes --- parsing of new IG and AuctionConfig fields --- to reduce the size of the monkeypatch and to practive how the process works.
It's moved basically unchanged (except different indent/wrapping); I think the only substantative change is using our origin parser.
https://github.com/morlovich/private-aggregation-api/tree/move-stuff-to-pa has the other half, it likely needs something with imports (I hope adding export to auction config/interest group on our end will help).
Preview | Diff