Skip to content
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

Add spec for multi-seller getInterestGroupAdAuctionData #1389

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

brusshamilton
Copy link
Contributor

@brusshamilton brusshamilton commented Jan 22, 2025

@JensenPaul JensenPaul added the spec Relates to the spec label Jan 23, 2025
@brusshamilton
Copy link
Contributor Author

@qingxinwu PTAL

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit

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

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=].
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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=].

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
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

|perBuyerConfigs|

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
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

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}}"]:
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

coordinators supported by this [=user agent=]:
1. Let |result| be a new [=auction data seller result=] with the following [=struct/items=]:
: [=auction data seller result/seller=]
:: |seller|
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not defined. Need to define it as |config|'s seller somewhere above.

@@ -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=].
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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}}.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@dmdabbs
Copy link
Contributor

dmdabbs commented Jan 25, 2025 via email

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
spec Relates to the spec
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants