WICG / turtledove

TURTLEDOVE
https://wicg.github.io/turtledove/
Other
538 stars 237 forks source link

Spec: Factor out trusted bidding signals batcher... #1163

Closed morlovich closed 6 months ago

morlovich commented 6 months ago

...to separate struct + routines, since it's a non-trivial piece of code in the middle of a complicated algorithm, and I am about to make it a bit more complicated still.

Also fix a bug in data-version handling: If different data versions are returned during the various fetches, route them to the appropriate IG rather than using the last version for everything.


Preview | Diff

morlovich commented 6 months ago

@xtlsheep would appreciate if you could take a look at this

domfarolino commented 6 months ago

If this is just a refactor I am tempted to say I don't need to look at this and we can proceed with @xtlsheep's LGTM unless there is some specific stuff you'd like me to review. Does that sound OK?

morlovich commented 6 months ago

If this is just a refactor I am tempted to say I don't need to look at this and we can proceed with @xtlsheep's LGTM unless there is some specific stuff you'd like me to review. Does that sound OK?

It's not completely mechanical (there is also some bugfix), but at any rate I would especially appreciate if you checked my map get syntax on line 1680, there is a high chance it's wrong.

xtlsheep commented 6 months ago

OK I've mostly looked at the changes I was asked specifically to look at (and quickly scanned the others). If @xtlsheep is OK with this change then I think we can land it.

Yeah the change LGTM

morlovich commented 6 months ago

@caraitto So this ended up having a rather non-trivial merge with updateIfOlderThanMs, so I would appreciate if you could take a look.

morlovich commented 6 months ago

On Thu, May 16, 2024 at 2:16 PM caraitto @.***> wrote:

@.**** commented on this pull request.

In spec.bs https://github.com/WICG/turtledove/pull/1163#discussion_r1603845315:

@@ -1721,8 +1680,13 @@ To generate and score bids given an [=auction config=] |auctionConfig

  1. Let |directFromSellerSignalsForBuyer| be the result of running [=get direct from seller signals for a buyer=] with |directFromSellerSignals|, and |ig|'s [=interest group/owner=].
      1. Let |dataVersion| be |trustedBiddingSignalsBatcher|'s [=trusted bidding signals batcher/data versions=]
    • [|ig|'s [=interest group/name=]].
      1. If |dataVersion| is not null, then [=map/set=] |browserSignals|["{{BiddingBrowserSignals/dataVersion}}"] to |dataVersion|.

Also, did @domfarolino https://github.com/domfarolino sign off?

On the pre-merge-with-your-stuff version.

Message ID: @.***>