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

Procedures update for optional features #158

Merged
merged 15 commits into from
Apr 28, 2024
Merged

Conversation

Wind4Greg
Copy link
Collaborator

@Wind4Greg Wind4Greg commented Apr 16, 2024

This PR addresses issues: #153, #156 and also #150 and #157.

It does this by clarifying which optional feature, if any, is being used via an featureOption variable, feature specific base proof and derived proof header bytes, and extending procedure/function descriptions to clarify inputs/outputs. In addition informative summary tables have been added to the section describing the optional features.

Note that some optional features are dependent on IETF APIs that are yet to be frozen. These have been notated.


Preview | Diff

index.html Outdated Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
@Wind4Greg Wind4Greg requested a review from TallTed April 18, 2024 16:35
index.html Outdated Show resolved Hide resolved
</li>
<li>
If |featureOption| equals `"baseline"`:
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we might want to shorten this to just feature (or another single word name like "mode" or something that we come to consensus on), but I wouldn't hold up this PR for it. I think this PR is an approvement and am in support.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Hi @dlongley, agree. Picking good names is hard. Currently have a lot of names with option in it. Simpler the better.

</ol>
</li>
<li>
If |featureOption| equals `"anonymous_holder_binding"`:
Copy link
Contributor

Choose a reason for hiding this comment

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

Using snake_case here is inconsistent with our other names, so I'd prefer it be camel case or hyphenated (or whatever else matches our current style elsewhere), but again wouldn't hold up the PR for that.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Hi @dlongley, this is a bit of a mess and extends to variable names, and the IETF specs too (mixing camel and snake case). I would be nice to choose camel or snake case (or whatever) throughout or have a established naming convention for functions, variable, constants, constant strings, etc. I'm more than happy to clean up everything according to a nice convention. Does W3C have anything written? I'm not strongly optioned on what the conventions should be.

Wind4Greg and others added 14 commits April 28, 2024 11:56
…g inputs, outputs and such for issuer, holder, and verifier.
…ns and return different items based on headers.
…r, and formatting improvements.

Co-authored-by: Ted Thibodeau Jr <[email protected]>
…r, and formatting improvements.

Co-authored-by: Ted Thibodeau Jr <[email protected]>
@msporny
Copy link
Member

msporny commented Apr 28, 2024

Normative, multiple reviews, changes requested and made, no objections, merging.

@msporny msporny merged commit fe8f091 into w3c:main Apr 28, 2024
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants