Spec changes for currency support #594
Conversation
5f234ba
to
0ba51d8
Compare
5e3860d
to
867eb16
Compare
|
This is a big one; I would suggest starting with the type stuff; one of the existing fields changed [=generate bid/bid=] is now a struct w/a currency tag... |
| @@ -2295,6 +2362,32 @@ An interest group ad is a [=struct=] with the following items: | |||
|
|
|||
| </dl> | |||
|
|
|||
| <h3 dfn-type=dfn>Currency tag</h3> | |||
| A currency tag is a [=string=] containing exactly 3 upper-case ASCII letters, or null. The null value is used to denote that the currency is unspecified. | |||
There was a problem hiding this comment.
dumb question: if there an RFC or other place that defines these 3 letter tags? Is it https://www.iso.org/iso-4217-currency-codes.html? Something else?
There was a problem hiding this comment.
ISO 4217 was the intent, but maybe the industry could agree on some other values, too. I hear they like doing things in cents, for example?
There was a problem hiding this comment.
OK, eventually we should link out to something... or invent a new thing (sure, why not). But that can be a follow up issue - I'll file a spec issue. 💸
| @@ -2490,6 +2612,8 @@ The output of running a Protected Audience `scoreAd()` script, is represented us | |||
| dictionary ScoreAdOutput { | |||
| required double desirability; | |||
| double bid; | |||
| DOMString bidCurrency; | |||
There was a problem hiding this comment.
AuctionAdConfig's sellerCurrency is a USVString - do we want bidCurrency to be DOMString? (Maybe there is some meaningful difference and I'm missing it)
There was a problem hiding this comment.
Well... If it's a USVString I have to implement the bad-surrogate-pairs check, and if it's DOMString I don't. Is there a rule saying what AuctionAdConfig should be using, given that's clearly a usual web API?
It's actually an /observable/ difference for SetBid, since if it were a USVString, and one passed one that wasn't valid for that, but valid for DOMString, that would throw before side effects of converting the modelingSignals field occur.
... It doesn't matter if one is using valid tags, of course.
There was a problem hiding this comment.
Alright - works for me.
Is there a rule saying what AuctionAdConfig should be using, given that's clearly a usual web API?
Not that I'm aware of. But maybe @domfarolino has more knowledge than me on this.
There was a problem hiding this comment.
Guidance for string types can be found in https://webidl.spec.whatwg.org/#idl-USVString and especially https://w3ctag.github.io/design-principles/#idl-string-types. If you use USVString you do not have to implement a bad-surrogate-pairs check, as USVString does that for you. Maybe you're referring to the implementation though (which doesn't actually use WebIDL so we'd have to manually implement that check as WebIDL would automatically)?
So I think the question comes down to whether we are doing any text processing on the value and need to ever dig into the individual scalar values?
There was a problem hiding this comment.
It seems odd to be inconsistent here without good reason though. At the very least it would be good to document why these two currency types can be different, assuming them being different is the appropriate outcome.
Co-authored-by: Mike Taylor <[email protected]>
SHA: a218d03 Reason: push, by miketaylr Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
I am not quite asking for a review yet; mostly just want to look at diffs of the combined thing.
Preview | Diff