Add spec for multi-seller getInterestGroupAdAuctionData #1389

brusshamilton · 2025-01-22T22:29:09Z

brusshamilton · 2025-01-23T21:49:18Z

qingxinwu · 2025-01-24T03:33:16Z

spec.bs

  1. Let |requestId| be the [=string representation=] of a [=version 4 UUID=].
+  1. [=list/For each=] |config| in |configs|:


qingxinwu · 2025-01-24T03:35:16Z

spec.bs

-     and |config|'s [=auction data config/request size=] is null:
-     1. Let |requestSize| be 0.
-     1. [=list/For each=] |buyerConfig| of |config|'s [=auction data config/per buyer config=]'s [=map/values=]:
+  1. Let |configs| be a new empty [=list=].


When possible, be explict about the element type in the list.
nit: "new" is enough.

Suggested change

1. Let |configs| be a new empty [=list=].

1. Let |configs| be a new [=list=] of [=auction data configs=].

qingxinwu · 2025-01-24T05:57:00Z

spec.bs

-     1. Let |requestSize| be 0.
-     1. [=list/For each=] |buyerConfig| of |config|'s [=auction data config/per buyer config=]'s [=map/values=]:
+  1. Let |configs| be a new empty [=list=].
+  1. Let |per buyer configs| be the result of running [=parse per buyer auction data configs=] on


|perBuyerConfigs|

I thought the convention was that for IDL types we used camel case variables. For infra types we used lower space types?

infra types use lower space types, but variables of infra types use camel case.

qingxinwu · 2025-01-24T06:00:37Z

spec.bs

+  1. If |configIDL|["{{AdAuctionDataConfig/seller}}"] [=map/exists=]:
+    1. Let |seller| be the result of running [=parse an https origin=] on
+       |configIDL|["{{AdAuctionDataConfig/seller}}"].
+    1. Let |coordinator| be the result of running [=parse an https origin=] on


now, we need to check whether |configIDL|["{{AdAuctionDataConfig/coordinatorOrigin}}"] [=map/exists=] first, since it's no longer required in IDL. Set |coordinator| to a default coordinator when it does not exist.

qingxinwu · 2025-01-24T06:04:04Z

spec.bs

+  1. Otherwise:
+    1. If |configIDL|["{{AdAuctionDataConfig/sellers}}"] does not [=map/exist=], then [=exception/throw=] a {{TypeError}}.
+    1. If |configIDL|["{{AdAuctionDataConfig/coordinatorOrigin}}"] [=map/exists=], then [=exception/throw=] a {{TypeError}}.
+    1. [=list/For each=] |seller config| in |configIDL|["{{AdAuctionDataConfig/sellers}}"]:


|sellerConfig|. There are several variables defined like this. Please check and fix those too.

qingxinwu · 2025-01-24T06:04:43Z

spec.bs

+    1. [=list/For each=] |seller config| in |configIDL|["{{AdAuctionDataConfig/sellers}}"]:
+      1. Let |seller| be the result of running [=parse an https origin=] on
+        |seller config|["{{AdAuctionOneSeller/seller}}"].
+      1. If |configs| already [=list/contains=] a [=auction data config=] whose


Suggested change

1. If |configs| already [=list/contains=] a [=auction data config=] whose

1. If |configs| [=list/contains=] an [=auction data config=] whose

qingxinwu · 2025-01-24T06:05:59Z

spec.bs

+      1. If |configs| already [=list/contains=] a [=auction data config=] whose
+         [=auction data config/seller=] is equal to |seller|, then
+         [=exception/throw=] a {{TypeError}}.
+      1. Let |coordinator| be the result of running [=parse an https origin=] on


same, need to check if |seller config|["{{AdAuctionOneSeller/coordinatorOrigin}}"] exists?

spec.bs

qingxinwu · 2025-01-24T16:06:55Z

spec.bs

@@ -3786,20 +3791,72 @@ The <dfn for=Navigator method>getInterestGroupAdAuctionData(|configIDL|)</dfn> m
    1. [=list/Append=] |serverIg| to |igMap|[|owner|].
    1. If |ig|'s [=interest group/Private Aggregation coordinator=] is not null, then [=map/set=]
      |igPAggCoordinatorMap|[(|owner|, |name|)] to it.
-  1. Let |result| be a new {{AdAuctionData}}.
+  1. Let |results| be a new [=list=].


Suggested change

1. Let |results| be a new [=list=].

1. Let |results| be a new [=list=] of [=auction data seller results=].

qingxinwu · 2025-01-24T16:09:02Z

spec.bs

+    1. Set |requestContext|'s [=server auction request context/request context=] field to |context|.
+    1. [=map/Set=] |global|'s [=associated Document's=] [=node navigable's=]
+      [=traversable navigable's=] [=traversable navigable/saved Bidding and Auction request context=][|requestId|] to |requestContext|.
+  1. Let |result| be a new {{AdAuctionData}}.


a different |result| was used above. Let's change one of the names to make it more readable.

Changed the second one to |IDL results|.

dmdabbs · 2025-01-25T04:30:03Z

[sad] David Dabbs reacted to your message:

…

________________________________ From: brusshamilton ***@***.***> Sent: Thursday, January 23, 2025 9:49:42 PM To: WICG/turtledove ***@***.***> Cc: Subscribed ***@***.***> Subject: Re: [WICG/turtledove] Add spec for multi-seller getInterestGroupAdAuctionData (PR #1389) External to the Groupe / en provenance de l'extérieur du Groupe @qingxinwu<https://github.com/qingxinwu> PTAL — Reply to this email directly, view it on GitHub<#1389 (comment)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/AABK3LDH2AROHGVF5KV6F7D2MFPXNAVCNFSM6AAAAABVV7TDUGVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDMMJRGA4DONJRHE>. You are receiving this because you are subscribed to this thread.Message ID: ***@***.***>

------------------------------------------------------------------------ Disclaimer The information in this email and any attachments may contain proprietary and confidential information that is intended for the addressee(s) only. If you are not the intended recipient, you are hereby notified that any disclosure, copying, distribution, retention or use of the contents of this information is prohibited. When addressed to our clients or vendors, any information contained in this e-mail or any attachments is subject to the terms and conditions in any governing contract. If you have received this e-mail in error, please immediately contact the sender and delete the e-mail.

spec.bs

qingxinwu · 2025-01-27T05:11:34Z

spec.bs

  :  <dfn>request size</dfn>
  :: {{unsigned long}} or null. An optional field, containing the desired size
    for the returned encrypted request blob.
  :  <dfn>per buyer config</dfn>
  :: A [=map=] whose [=map/keys=] are [=origins=] and [=map/values=] are [=auction data buyer config=].
 </dl>

+An <dfn>auction data seller result</dfn> is a [=struct=] with the following [=struct/items=]:


nit: auction data per seller result?

qingxinwu · 2025-01-27T05:24:34Z

spec.bs

+    1. Let (|requestBlob|, |context|) be the result of serializing |igMap| with |config| and
+      |igPAggCoordinatorMap|. The serialization method may follow that described in
+      [Section 2.2.4 of Bidding and Auction Services](https://privacysandbox.github.io/draft-ietf-bidding-and-auction-services/draft-ietf-bidding-and-auction-services.html#name-generating-a-request).
+    1. Let |result| be a new [=auction data seller result=] with the following [=struct/items=]:


Yeah, that is possible, but I think this way is more clear.

qingxinwu · 2025-01-27T05:28:30Z

spec.bs

+          :: |seller result|'s [=auction data seller result/seller=]
+          :  {{AdAuctionPerSellerData/request}}
+          :: |seller result|'s [=auction data seller result/request=]
+        1. [=list/Append=] |IDLresult| to |result|["{{AdAuctionData/requests}}"].


similar to another comment. Probably cleaner to define Let |IDLresult| be a new {{AdAuctionPerSellerData}} whose seller is ..... ANd in if/else, just set their error or request field. Then outside of if/else, append |IDLResult| to requests.

This one is probably more clear to do it that way. Done.

qingxinwu · 2025-01-28T16:45:53Z

spec.bs

-          :  {{AdAuctionPerSellerData/error}}
-          :: |seller result|'s [=auction data seller result/error=]
-        1. [=list/Append=] |IDLresult| to |result|["{{AdAuctionData/requests}}"].
+      1. Let |IDLresult| be a new {{AdAuctionPerSellerData}} whose {{AdAuctionPerSellerData/seller}} |seller result|'s [=auction data per seller result/seller=].


Suggested change

1. Let |IDLresult| be a new {{AdAuctionPerSellerData}} whose {{AdAuctionPerSellerData/seller}} |seller result|'s [=auction data per seller result/seller=].

1. Let |IDLresult| be a new {{AdAuctionPerSellerData}} whose {{AdAuctionPerSellerData/seller}} is |seller result|'s [=auction data per seller result/seller=].

qingxinwu · 2025-01-28T16:46:53Z

spec.bs

-     1. Let |requestSize| be 0.
-     1. [=list/For each=] |buyerConfig| of |config|'s [=auction data config/per buyer config=]'s [=map/values=]:
+  1. Let |configs| be a new empty [=list=].
+  1. Let |per buyer configs| be the result of running [=parse per buyer auction data configs=] on


infra types use lower space types, but variables of infra types use camel case.

qingxinwu · 2025-01-28T16:52:07Z

spec.bs

+      1. Let |coordinator| be the result of running [=parse an https origin=] on
+        |configIDL|["{{AdAuctionDataConfig/coordinatorOrigin}}"].
+    1. Otherwise
+      1. Let |coordinator| be the [=user agent=]'s default coordinator.


let's say what coordinator it refers to, like "bidding and auction coordinator"? The spec has several different totally unrelated coordinators now.

qingxinwu · 2025-01-28T16:55:09Z

spec.bs

@@ -3865,7 +3863,7 @@ The <dfn for=Navigator method>getInterestGroupAdAuctionData(|configIDL|)</dfn> m
  To <dfn>parse and verify ad auction data config</dfn> given an
  {{AdAuctionDataConfig}} |configIDL| and [=origin=] |top_level_origin|:

-  1. Let |configs| be a new empty [=list=].
+  1. Let |configs| be a new empty [=list=] of [=auction data configs=].


nit

Suggested change

1. Let |configs| be a new empty [=list=] of [=auction data configs=].

1. Let |configs| be a new [=list=] of [=auction data configs=].

qingxinwu · 2025-01-28T16:55:39Z

spec.bs

+        1. Let |coordinator| be the result of running [=parse an https origin=] on
+          |seller config|["{{AdAuctionOneSeller/coordinatorOrigin}}"].
+      1. Otherwise:
+        1. Let |coordinator| be the [=user agent=]'s default coordinator.


same, be clearer about what coordinator it is.

qingxinwu · 2025-01-28T20:09:03Z

spec.bs

+  1. Let |IDLresults| be a new {{AdAuctionData}}.
+  1. [=map/Set=] |IDLresults|["{{AdAuctionData/requestId}}"] to |requestId|.
+  1. If |configIDL|[{{AdAuctionDataConfig/seller}}] [=map/exists=]:
+    1. Let |seller result| be |results|[0].


sellerResult

qingxinwu · 2025-01-28T20:10:36Z

spec.bs

+    1. Let |keyInfo| be the result of [=looking up the server encryption key=]
+       with |config|'s [=auction data config/seller=] and |seller|'s [=auction data config/coordinator=].
+    1. If |keyInfo| is failure:
+          1. Let |result| be a new [=auction data per seller result=] with the following [=struct/items=]:


wrong indention. The preview shows how it's rendered.

qingxinwu · 2025-01-28T20:14:37Z

spec.bs

+  1. [=list/For each=] |config| of |configs|:
+    1. Let |seller| be |config|'s [=auction data config/seller=].
+    1. If |config|'s [=auction data config/coordinator=] is not one of the [=implementation-defined=]
+       coordinators supported by this [=user agent=]:


Suggested change

coordinators supported by this [=user agent=]:

Bidding and Auction Services coordinators supported by this [=user agent=]:

qingxinwu · 2025-01-28T20:17:31Z

spec.bs

+    1. Set |IDLresults|["{{AdAuctionData/request}}"] to |requestBlob|.
+  1. Otherwise:
+    1. [=list/For each=] |seller result| in |results|:
+      1. Let |IDLresult| be a new {{AdAuctionPerSellerData}} whose {{AdAuctionPerSellerData/seller}}


{{AdAuctionPerSellerData/seller}} is string, while the infra one is [=origin=]. Need to do [=serialization of an origin|serialization=] (example)

qingxinwu · 2025-01-28T20:18:08Z

spec.bs

+An <dfn>auction data per seller result</dfn> is a [=struct=] with the following [=struct/items=]:
+<dl dfn-for="auction data per seller result">
+  :  <dfn>seller</dfn>
+  :: The origin of the seller that this result corresponds to. Will match one of


Suggested change

:: The origin of the seller that this result corresponds to. Will match one of

:: The [=origin=] of the seller that this result corresponds to. Will match one of

qingxinwu · 2025-01-28T20:39:46Z

spec.bs

+    1. Set |requestContext|'s [=server auction request context/request ID=] field to |requestId|.
+    1. Set |requestContext|'s [=server auction request context/request context=] field to |context|.
+    1. [=map/Set=] |global|'s [=associated Document's=] [=node navigable's=]
+      [=traversable navigable's=] [=traversable navigable/saved Bidding and Auction request context=][|requestId|] to |requestContext|.


this does not seem right, which keeps overwriting the requestId's context to the next config's context. Do we need to use |requestId| + |seller| as the key now? https://crsrc.org/c/content/browser/interest_group/ad_auction_page_data.h;drc=12855e28398f2355e8002664108acc3c289401e0;l=150

brusshamilton added 4 commits January 22, 2025 21:53

Add spec for multi-seller getInterestGroupAdAuctionData

b485e1b

Remove redundant filtering step

6928455

Merge remote-tracking branch 'upstream/main' into BAMultiseller

9527de3

Fix typo

d9621d3

JensenPaul added the spec Relates to the spec label Jan 23, 2025

Make getInterestGroupAdAuctionData argument optional

5ff27f1

qingxinwu reviewed Jan 24, 2025

View reviewed changes

rdgordon-index mentioned this pull request Jan 27, 2025

Sandbox team proposal of getInterestGroupAdAuctionData to accept more than one seller origin #1338

Closed

qingxinwu reviewed Jan 27, 2025

View reviewed changes

Address comments

9027437

qingxinwu reviewed Jan 28, 2025

View reviewed changes

make variables camel case

d3e50ea

qingxinwu reviewed Jan 28, 2025

View reviewed changes

		1. Let \|requestId\| be the [=string representation=] of a [=version 4 UUID=].
		1. [=list/For each=] \|config\| in \|configs\|:

	1. [=list/For each=] \|config\| in \|configs\|:
	1. [=list/For each=] \|config\| of \|configs\|:

	1. Let \|configs\| be a new empty [=list=].
	1. Let \|configs\| be a new [=list=] of [=auction data configs=].

	1. If \|configs\| already [=list/contains=] a [=auction data config=] whose
	1. If \|configs\| [=list/contains=] an [=auction data config=] whose

	1. Let \|results\| be a new [=list=].
	1. Let \|results\| be a new [=list=] of [=auction data seller results=].

	1. Let \|IDLresult\| be a new {{AdAuctionPerSellerData}} whose {{AdAuctionPerSellerData/seller}} \|seller result\|'s [=auction data per seller result/seller=].
	1. Let \|IDLresult\| be a new {{AdAuctionPerSellerData}} whose {{AdAuctionPerSellerData/seller}} is \|seller result\|'s [=auction data per seller result/seller=].

	coordinators supported by this [=user agent=]:
	Bidding and Auction Services coordinators supported by this [=user agent=]:

	:: The origin of the seller that this result corresponds to. Will match one of
	:: The [=origin=] of the seller that this result corresponds to. Will match one of

Add spec for multi-seller getInterestGroupAdAuctionData #1389

Are you sure you want to change the base?

Add spec for multi-seller getInterestGroupAdAuctionData #1389

Conversation

brusshamilton commented Jan 22, 2025 • edited by pr-preview bot Loading

brusshamilton commented Jan 23, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dmdabbs commented Jan 25, 2025 via email

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brusshamilton commented Jan 22, 2025 •

edited by pr-preview bot

Loading