Skip to content

Add respec-vc proof/JWT rendering to many other examples. #835

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

Merged
merged 1 commit into from
Nov 20, 2021

Conversation

msporny
Copy link
Member

@msporny msporny commented Nov 13, 2021

This PR builds on PR #834 by just applying respec-vc to all other examples that it can be applied to at this time.


Preview | Diff

@github-pages github-pages bot temporarily deployed to github-pages November 14, 2021 00:06 Inactive
@github-pages github-pages bot temporarily deployed to github-pages November 14, 2021 00:49 Inactive
@msporny msporny force-pushed the msporny-examples-signed branch from e121946 to 9d434d4 Compare November 14, 2021 00:50
@github-pages github-pages bot temporarily deployed to github-pages November 14, 2021 00:52 Inactive
Copy link
Member

@TallTed TallTed left a comment

Choose a reason for hiding this comment

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

Looks good at a quick skim.

But I noticed that the paragraph describing Figure 5 and Figure 6 has a few colored-and-outlined words which are near illegible, which problem I thought we resolved months ago with substantial rewrite of this paragraph including splitting it into two... And that makes me very concerned that there may be related and/or other issues that may need re-resolution.

@David-Chadwick
Copy link
Contributor

Whilst the JWT example is no doubt correct, it would be more helpful to readers if the additional tab could be (or another one added) which shows the transformation of the credential into the JWT payload (prior to signing and base64 encoding) as in example 28. The reason being is that the transformation is the part that is specified in the VC specification, whilst the signing and base64 encoding are already standardised in referenced RFCs and we do not modify these specs.

@msporny
Copy link
Member Author

msporny commented Nov 14, 2021

Whilst the JWT example is no doubt correct, it would be more helpful to readers if the additional tab could be (or another one added) which shows the transformation of the credential into the JWT payload (prior to signing and base64 encoding) as in example 28. The reason being is that the transformation is the part that is specified in the VC specification, whilst the signing and base64 encoding are already standardised in referenced RFCs and we do not modify these specs.

I've raised an issue to track this since it has more to do with respec-vc: #836

Let's take the discussion around JWT enhancements to the current examples there.

@msporny msporny force-pushed the msporny-examples-signed branch from 9d434d4 to 7790d6c Compare November 14, 2021 18:06
@msporny msporny force-pushed the msporny-all-examples-signed branch from 1d68b8b to 90d16e3 Compare November 14, 2021 18:07
Copy link
Member

@kdenhartog kdenhartog left a comment

Choose a reason for hiding this comment

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

From a spec perspective, everything looks good to me. There's some things I'd like to better understand about the implementation to help maintain it and improve a bit more is all.

Copy link
Member

@iherman iherman left a comment

Choose a reason for hiding this comment

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

This comment is just to make your life bit more difficult :-)

See, e.g., example. The "subject" of the example is the credentialSubject property, duly highlighted in the example. However, if I look at the signed example, that highlight disappears, and the reader is lost as to exactly what happened: one expects to see the top part of the example to be exactly the same, including the highlight. Would it be possible to make it so?

@msporny
Copy link
Member Author

msporny commented Nov 15, 2021

However, if I look at the signed example, that highlight disappears, and the reader is lost as to exactly what happened: one expects to see the top part of the example to be exactly the same, including the highlight. Would it be possible to make it so?

Yes, one could apply a custom diff'ing algorithm to the pre-signed and signed output and then apply some sort of patching algorithm to match the highlights between the unsigned and signed output up. Another option would be to just pluck the proof field the signed output and tack it onto the end. The former is probably a week or two of coding, the latter would be a few hours... but then you run the risk of not tacking on the proof at the right place. There would have to be some regexes involved as well, I imagine. It's not entirely straightforward and so is left as an enhancement for the future. :)

@iherman
Copy link
Member

iherman commented Nov 16, 2021

However, if I look at the signed example, that highlight disappears, and the reader is lost as to exactly what happened: one expects to see the top part of the example to be exactly the same, including the highlight. Would it be possible to make it so?

Yes, one could apply a custom diff'ing algorithm to the pre-signed and signed output and then apply some sort of patching algorithm to match the highlights between the unsigned and signed output up. Another option would be to just pluck the proof field the signed output and tack it onto the end. The former is probably a week or two of coding, the latter would be a few hours... but then you run the risk of not tacking on the proof at the right place. There would have to be some regexes involved as well, I imagine. It's not entirely straightforward and so is left as an enhancement for the future. :)

I told you my goal is to make your life more difficult :-)

@TallTed
Copy link
Member

TallTed commented Nov 16, 2021

The issue I referenced above was #777, which was addressed by PRs #785 and #786, but those no longer appear to have been merged? Unless maybe this PR is against the wrong base?

@msporny
Copy link
Member Author

msporny commented Nov 16, 2021

The issue I referenced above was #777, which was addressed by PRs #785 and #786, but those no longer appear to have been merged? Unless maybe this PR is against the wrong base?

Hrm, this PR is in the main history: a0f4863 but this one is not: e0ec8de

@TallTed can you please open an issue to track this, we have to make sure we figure out what's going on here. Losing commits like this is a big deal/problem. The Editors are going to have to do some git bisect or git blame to figure out which PR overwrote those PRs. I'm noting some git cherry-picking going on and some main branch reverting, so its possible that there was some out of order application of the PRs.

@msporny
Copy link
Member Author

msporny commented Nov 20, 2021

I'm merging this PR into PR #834 (give that there are no objections, I need to clean up the JWT encoding, and because that other PR isn't going to be merged until all of this is correct). This is an administrative move to merge two PRs into one before we send out the 14 day merge announcement to the CCG.

@msporny msporny merged commit cd60128 into msporny-examples-signed Nov 20, 2021
@msporny msporny deleted the msporny-all-examples-signed branch November 20, 2021 21:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants