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

Fix examples and point to specifications #105

Merged
merged 3 commits into from
Jul 9, 2023
Merged

Conversation

msporny
Copy link
Member

@msporny msporny commented Jun 30, 2023

This PR fixes the introductory examples and points to the respective specifications so that people can find the cryptosuites used in the examples.


Preview | Diff

@msporny msporny changed the base branch from main to msporny-add-related June 30, 2023 15:38
@msporny msporny requested a review from mprorock as a code owner June 30, 2023 15:38
Copy link
Collaborator

@Wind4Greg Wind4Greg left a comment

Choose a reason for hiding this comment

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

Very nice update with examples from both ECDSA and EdDSA signatures and two different canonicalization schemes.

index.html Outdated
verifiable digital proof by presumptively transforming the input data using the
JSON Canonicalization Scheme [[?RFC8785]] and then digitally signing it using an
Ed25519 elliptic curve signature.
The proof above uses the `jcs-eddsa-2022` cryptography suite [[?DI-EDDSA]] that
Copy link
Member

Choose a reason for hiding this comment

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

I generally find things are better comprehended when the description (and any relevant warnings, etc.) precedes (rather than follows) the examples. This will take some minutes to adjust document-wide, along with changing all the above to below. I can do, within this PR, or as a standalone.....

Copy link
Member Author

@msporny msporny Jun 30, 2023

Choose a reason for hiding this comment

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

Agree, @TallTed, thank you! Feel free to do that in this PR or one that layers on top of this one. Up to you, I'll take the changes either way.

Copy link
Member Author

Choose a reason for hiding this comment

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

Nevermind, @TallTed, I went ahead an implemented your suggestion in 7575750.

index.html Outdated
Comment on lines 556 to 560
"verificationMethod": "https://di.example/issuer#z6MkjLrk3gKS2nnkeWcmcxi
ZPGskmesDpuwRBorgHxUXfxnG",
"proofPurpose": "assertionMethod",
"proofValue": "zQeVbY4oey5q2M3XKaxup3tmzN4DRFTLVqpLMweBrSxMY2xHX5XTYV8nQA
pmEcqaqA3Q1gVHMrXFkXJeV6doDwLWx"
Copy link
Member

Choose a reason for hiding this comment

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

This makes it look like the inline whitespace in the verificationMethod and proofValue will be ignored by tools that make use of these values. I'm pretty sure that's not the reality, so I think the above should become the following:

Suggested change
"verificationMethod": "https://di.example/issuer#z6MkjLrk3gKS2nnkeWcmcxi
ZPGskmesDpuwRBorgHxUXfxnG",
"proofPurpose": "assertionMethod",
"proofValue": "zQeVbY4oey5q2M3XKaxup3tmzN4DRFTLVqpLMweBrSxMY2xHX5XTYV8nQA
pmEcqaqA3Q1gVHMrXFkXJeV6doDwLWx"
"verificationMethod": "https://di.example/issuer#z6MkjLrk3gKS2nnkeWcmcxiZPGskmesDpuwRBorgHxUXfxnG",
"proofPurpose": "assertionMethod",
"proofValue": "zQeVbY4oey5q2M3XKaxup3tmzN4DRFTLVqpLMweBrSxMY2xHX5XTYV8nQApmEcqaqA3Q1gVHMrXFkXJeV6doDwLWx"

It might make sense to put   between the keys and the values, so that the wraps are guaranteed to be in the long strings....

Copy link
Member Author

@msporny msporny Jul 9, 2023

Choose a reason for hiding this comment

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

I attempted a fix in 3b1631f. The benefit is that people can now just copy-paste the examples AND the line wrapping happens automatically on small screens. The downside is that the examples are less readable w/o the indentation.

If we want to do better here, we will need to write a respec plugin that automatically wraps and indents long lines in <pre> HTML tags, which would be a non-trivial undertaking if we wanted highlighting to work as well. CSS is hard.

Copy link
Member

Choose a reason for hiding this comment

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

wbr works for me

Base automatically changed from msporny-add-related to main July 9, 2023 16:27
@msporny msporny force-pushed the msporny-update-example-refs branch from 7575750 to c7c2a21 Compare July 9, 2023 16:29
@msporny
Copy link
Member Author

msporny commented Jul 9, 2023

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

@msporny msporny merged commit 15fc2a3 into main Jul 9, 2023
1 check passed
@msporny msporny deleted the msporny-update-example-refs branch July 9, 2023 18:02
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.

None yet

5 participants