search

w3c / vc-jose-cose

Verifiable Credentials Working Group — VC JSON Web Tokens specification
https://w3c.github.io/vc-jose-cose/
Other
31 stars 13 forks source link

Editorial and non editorial changes #68

Closed OR13 closed 1 year ago

OR13 commented 1 year ago

Lots of changes in this PR, since the document is in pretty rough shape.

I moved sections around, updated examples, adjusted old text copied from version 1.1, clarified how examples map to resolutions made regarding the core data model, and removed a should requirement for an unregistered and extraneous media type.

Preview | Diff

iherman commented 1 year ago

"preview" does not handle respec's includes properly, so it is pretty much useless here. This seems to work:

https://raw.githack.com/w3c/vc-jwt/fixes-and-examples/index.html

OR13 commented 1 year ago

@TallTed @andresuribe87 Thank you for your reviews!

iherman commented 1 year ago

The issue was discussed in a meeting on 2023-04-05

View the transcript ### 2. Work Item status updates/PRs. **Brent Zundel:** Moving on to work item status updates and PRs. **Manu Sporny:** On VC Data Model, there are six PRs that are effectively stuck, that we need to discuss how we get them unstuck. … Other PRs look fairly straightforward and simple. … Data integrity spec is fine, nothing stuck there, same for EdDSA and ECDSA. … Need to do first public working drafts for EdDSA and ECDSA. … StatusList2021, not a whole not of progress. If folks are looking for work, that would be a good place to start. **Brent Zundel:** FPWD is important so that other W3C groups can review and so that patent declaration has time. … Want to enter FPWD as soon as possible. _See github pull request [vc-jwt#68](https://github.com/w3c/vc-jwt/pull/68)._ **Orie Steele:** [https://github.com/w3c/vc-jwt/pull/68](https://github.com/w3c/vc-jwt/pull/68) is an open pull request on VC-JWT what we want to merge. … Document is still close to the format it was in when it was copied out of the core spec. … Talked to Tobias at IETF about BBS, still in the process of adopting it. … Happy to do that work. **Manu Sporny:** Just a note that I have sent a list of steps to Gabe, who is raising PR on transitioning from CCG to VC-JWT. > *Michael Prorock:* yep - on the lookout.
brentzundel commented 1 year ago

I'm not challenging the move, but am curious about the reasoning behind moving the transformation to an appendix.

Sakurann commented 1 year ago

I personally think because it is a way to transform (not THE way), it should be in the appendix?

OR13 commented 1 year ago

@brentzundel What @Sakurann said, it was also a change request on your original PR.

We feel it is important to be clear that different organizations might want to produce different JSON-LD based on their business use cases, at the face to face, we discussed this in depth, we don't want readers concluding there is "only one way" to map a JWT to a JSON-LD object.

OR13 commented 1 year ago

@msporny

High level comments on this PR:

  • It's huge and changes many normative statements simultaneously, please avoid massive PRs like this, it makes it very easy to miss important changes.

Yes, sadly this document started as a few sections cut out of the v1.1 specification and has received very few "large PRs" like this.... it needs a lot more contribution.

  • The use of data-include has broken the PR Preview, which means that all examples are not viewable inline and the spec now requires you to download and run it locally to see the updates/changes. I doubt that most of the reviewers are doing this.

Interesting, do you think we should no use data include moving forward? I thought it was a best practice to use the features of ReSpec, but perhaps this one is not good to use.

  • It adds CWT and COSE to the scope of VC-JWT, after feature freeze, which seems to be difficult to reconcile with scope of the VCWG Charter (and could lead to formal objections).

This PR does not add this, it moves the existing section, which was added in:

https://github.com/w3c/vc-jwt/pull/51/files#diff-0eb547304658805aad788d320f10bf1f292797b5e6d745a3bf617584da017051R707

  • It defines the 'transformation' algorithm in a way that won't be tested by this WG and thus we won't demonstrate interoperability on the transformation algorithm in this specification.

This PR does not define the transformation, it moves the transformation that was supplied in:

https://github.com/w3c/vc-jwt/pull/60

To the appendix, as was requested here:

https://github.com/w3c/vc-jwt/pull/60#issuecomment-1470905492

and partially implemented by @brentzundel

It took over an hour to review this PR, and I'm still not confident that I understand all of the details that are changing. It's difficult to approve a PR that introduces multiple questionable changes in scope and testing simultaneously under a vague description of "Editorial and non-editorial changes".

Thank you for your review.

I am sorry the document is not in better shape, it is not yet FPWD, perhaps we should delay FPWD and continue to make improvements to the document cc @brentzundel @Sakurann @selfissued .

If the document is not yet FPWD, is there any change to process that I should be following to help this document catch up in terms of quality?

Recall this documented started as a cut out of version 1.1 and has received relatively few changes since then, as most of our time has been spent discussing how the media types in the core data model interact with it.

I'm a strong +1 to rapidly updating the spec to make it better, so in that vein, I'd be a +1 to merging this, IF 1) issues are raised for all items of concern,

Thanks I think I have captured the major one here:

https://github.com/w3c/vc-jwt/issues/73

There are already several open issues that are addressing some of the comments you have left, specifically:

Happy to provide an update on the next call.

iherman commented 1 year ago

The issue was discussed in a meeting on 2023-04-12

View the transcript #### 3.4. Editorial and non editorial changes (pr vc-jwt#68) _See github pull request [vc-jwt#68](https://github.com/w3c/vc-jwt/pull/68)._ **Orie Steele:** for VC JWT request for FPWD made and resolved in a previous meeting. Follow ups to possible objections made but it doesn't appear there will be objections. Go forward and prosper. … Open pull request for vc-jwt. Some awkward sections cut from ver. 1. Have added around that some vision for enhancements and improvements for securing JSON-LD with JWTs but structure of doc difficult to revise. **Manu Sporny:** almost of the opinion to not do a PR for JWT until its in a shape for all to see it in. The specific concerns are the way the transformation algorithm is stated is non-testable. Why are we discussing something that is optional. … what are we doing with transformation algorithms, are the testable, etc. > *Dave Longley:* +1 to the idea that there's a lot that needs to change in the VC-JWT document to reach consensus and make something usable, but FPWD can happen first. **Orie Steele:** there are many things that need work in the PR. Media type discussion in the vc specs dir, and the securing specs referring to them, etc. > *Manu Sporny:* I'm not super concerned either way, but would prefer we get it in better shape before FPWD, but we can also do that after FPWD.. **Orie Steele:** mappings and interpretations of the mapping in the core model and in the spec dir are differing... … anything that needs done in this context? **Michael Jones:** supports the recommendation that Orie prepare it before it becomes a working draft. **Brent Zundel:** process info -- publishing as a FWD has patent implications. Working drafts do not represent a consensus of the WG beyond an agreement to work on the topic. … starts the clock on patent disclosures and says its a tech we as a working group want to pursue. … encourages folks to look through the PR that Orie has drafted, if something causes concern look for an issue that is already open on it, consider raising it later. > *Orie Steele:* I agree with JoeAndrieu, its not clear what you want me to do. **Joe Andrieu:** Sounds like it's both a working draft and editor's draft. > *Dave Longley:* +1 to brent's suggestion. > *Orie Steele:* happy to do whatever.. > *Manu Sporny:* +1 to "Orie go for it, we'll review, then FPWD" -- and time box the review to a week so people can't hold it up.. > *PhilF:* +1 to Orie getting it squared away first. **Brent Zundel:** should we ask Orie to finish it and then review the FPWD? … no objections to Orie shaping the document as an editor and presenting it to the group. **Orie Steele:** next time does Orie need to wait 7 days or what? **Brent Zundel:** with the work mode proposed Orie should be able raise PRs people think are needed. Concerns can be tracked in the issue.
OR13 commented 1 year ago

I have addressed the feedback provided.

Based on the call yesterday, I am merging this PR and will merge directly to main with no PRs until the document is ready to be called for FPWD.

TallTed commented 1 year ago

I am befuddled by the merge a minute after the request for review.

What are you asking @msporny and I to review?

msporny commented 1 year ago

What are you asking @msporny and I to review?

He cleared our request for changes and merged. I don't think there was an expectation that we'd re-review. @OR13 is now operating in a "commit to main" capacity and will notify the WG to do a full review of the spec, including all of his unreviewerd changes, before FPWD.