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.
| </div> | ||
|
|
||
| <div algorithm> | ||
| To <dfn lt="obtain the Private Aggregation coordinator from a string">obtain the |
There was a problem hiding this comment.
@alexmturner Can we export this algorithm in PAgg spec so that we don't need to redefine it in this spec? There is a way to link to un-exported algorithms, but shared storage spec also needs it (and redefined it), so maybe easist to just export it. It'll be unfortunate to keep three copies of this algorithm in three different specs.
There was a problem hiding this comment.
It's only used by us, so my follow up change removes it from PAgg.
There was a problem hiding this comment.
shared storage spec also uses it
There was a problem hiding this comment.
That's its own implementation, using its own (different!) types?
There was a problem hiding this comment.
they seem to be the same (starting from step 3 of shared storage one). If the shared storage one moves the first two steps out and pass in a url, then it's the same with ours.
I'm not sure what the implementation does, but I guess it's the PAgg service handling obtaining the coordinator, and PA and SS just call the same function? If so, I feel this feature belongs to PAgg, not PA or SS.
There was a problem hiding this comment.
Ack -- put out a PR to export the string input version: patcg-individual-drafts/private-aggregation-api#163
spec.bs
Outdated
| :: An [=aggregation coordinator=]. Defaults to the [=default aggregation | ||
| coordinator=]. |
There was a problem hiding this comment.
nit
| :: An [=aggregation coordinator=]. Defaults to the [=default aggregation | |
| coordinator=]. | |
| :: An [=aggregation coordinator=]. Defaults to the [=default aggregation coordinator=]. |
spec.bs
Outdated
| 1. If |index| is equal to or greater than |interestGroupBuyers|' [=list/size=], | ||
| [=iteration/continue=]. | ||
|
|
||
| Note: [=iteration/Continue=] is used (instead of [=iteration/break=]) to match validation |
There was a problem hiding this comment.
I'm not sure if I understand this sentence correctly. Does it mean "to validate all given buyer keys"?
| Note: No error is thrown to allow forward compatibility if | ||
| additional report types are added later. | ||
|
|
||
| Issue: Should these strings be dash delimited? |
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.
spec.bs
Outdated
| : <dfn>required seller capabilities</dfn> | ||
| :: A [=set=] of [=seller capabilities=]. | ||
| The [=seller capabilities=] that each [=interest group=] must declare to participate in the | ||
| auction. Interest groups that don't declare all these capabilities will not participate in the | ||
| auction. | ||
| : <dfn>auction report buyer keys</dfn> | ||
| :: A [=map=] from buyer [=origins=] to {{bigint}}s. |
There was a problem hiding this comment.
can we add a sentence describing what each of the new fields are about?
The explainer has some descriptions.
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