Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
[spec] Introduce StorageInterestGroup and "get storage interest group… #1299
[spec] Introduce StorageInterestGroup and "get storage interest group… #1299
Changes from all commits
94acaac
4dd06a8
1416773
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
{{GenerateBidInterestGroup/sellerCapabilities}}
is not an[=ordered map=]
of byte sequences, it's arecord<USVString, sequence<DOMString>>
. You need to convert it.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Similarly,
record
andordered_map
(== map) should be "implicitly compatible". And the following implicitly conversions should also hold: set (== ordered set) == list == sequenceThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes,
record
andordered_map
are "implicitly compatible", but the actual capabilities are stored internally as abyte sequence
, not asequence<DOMString>
. So we need some conversion here. That might be kind of difficult since I don't think we actually specify how the enum is encoded into abyte sequence
other than that it only takes a bit.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I filed #1316, as I feel the right approach is to define those enum fields as strings directly. Besides, in the current specification, when these fields are being set, there's no conversion either: "If capabilityString is "interest-group-counts" or "latency-stats", then append capabilityString to sellerCapabilities."
Thus, it looks like it's best to treat these fields as strings now? This improves readability and aligns with the direction of the proposed specification.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How are you handling failure here? (and other conversion failures in this algorithm)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm assuming the data storage only contains valid serialized values, so we shouldn't expect any deserialization/conversion failures. Perhaps technically it's still better to handle failures due to database corruption? but even so, I assume we don't have to include this in the specification. Please correct me if I made any wrong assumptions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think specifying how failures are handled is probably one of the most important aspects of a specification. If you look at all the issues with undefined behavior in C/C++...
Please specify how failures are handled. I agree that the
user bidding signals
should be valid JSON, but there are still failure modes where it isn't (such as when the encoder and decoder are using a different JSON version).There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done. Added "return failure" for failed JSON parsing and base64 encode.