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

Some suggestions and amendments #65

Merged
merged 8 commits into from
Oct 24, 2023
Merged

Conversation

henkvancann
Copy link
Contributor

I stopped after a while, checked the result locally.
Please let me know whether the teams appreciates this kind of contribution.

@swcurran
Copy link
Contributor

swcurran commented Oct 6, 2023

Updates like this are indeed welcome. Please make sure to sign (lower case -s) your commits with DCO — DCO - Developer Certificate of Origin - https://github.com/apps/dco. They must be signed to be accepted/merged.

Signed-off-by: Henk van Cann <hvancann@bcws.io>
Signed-off-by: Henk van Cann <hvancann@bcws.io>
Signed-off-by: Henk van Cann <hvancann@bcws.io>
Signed-off-by: Henk van Cann <hvancann@bcws.io>
Signed-off-by: Henk van Cann <hvancann@bcws.io>
Signed-off-by: Henk van Cann <hvancann@bcws.io>
spec/abstract.md Outdated
identifiers between web and non-web an easy step.

identifiers between web and non-web a manageable step.
<!-- Nothing is 'easy' in our line of work -->
Copy link
Contributor

Choose a reason for hiding this comment

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

I assume we won't include this line, it's just a comment for our review?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes

spec/abstract.md Show resolved Hide resolved
spec/core.md Outdated

However, this method introduces an important nuance about the target system. In
many DID methods, the target system equals a [[ref: verifiable data registry]] —
besides publishing data, its security attributes make that data trustworthy. In
this DID method, the target system's role is more limited. It is expected to
serve data about the DID, and to follow acknowledged cybersecurity best
practices to preserve good hygiene. However, the authenticity of data is
practices to preserve good hygiene.
<!-- What are these acknowledges rules and what is good hygiene? -->
Copy link
Contributor

Choose a reason for hiding this comment

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

I assume this won't be included, and that we can discuss them some time for better clarification. Consider using the github comments for discussion.
In terms of good hygiene.... thats a long topic that we probably need to handwave to keep the spec tight.

Copy link
Contributor Author

@henkvancann henkvancann Oct 16, 2023

Choose a reason for hiding this comment

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

I assume this won't be included,
Right, I suggest we clear all my html comments out: <!-- indeed all comments Henk finally have to go -->

and that we can discuss them some time for better clarification. Consider using the github comments for discussion.
Right, if I knew how in combination with PRs... Could you give an example?

In terms of good hygiene.... thats a long topic that we probably need to handwave to keep the spec tight.
Yes, there are more parts in the current specs, that I would shorten drastically and then link to a spot where there is an elaboration. Any ideas how to do this? My 2 cents: We could create a small more_on_|topic|.md per topic.

spec/core.md Outdated
6. Create the AID folder on the web server at the selected location, and place
the `did:web` DID document resource (`did.json`) and the [[ref: KERI event stream]] resource (`keri.cesr`)
into that folder. See section [Target System(s)](#target-systems) for further details about
the locations of these resources.

<!-- Why can't 2. be " Create a KERI AID and add it as the last element of the web URL for the DID" and 3. "Add the appropriate KERI events to the AID's KERI logs that will correspond to properties of the DID document, such as verification methods and service endpoints" ? The AID won't change if we add events to KEL/TEL. I am not 100% sure about this though. -->
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 I like the suggestion. Although in the github review view, i can't see the current 2, to compare. Can you convert this to a review comment on the PR, so that it points directly to 2 and so that it isn't in the PR itself, i'm just concerned that we'll forget to remove them, and it will be easier to see the current 2 that you refer to, from the review view.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added it in commit 68ab0a6

The `did:webs` method described in this spec separates these two questions and answers them distinctively. _Information about DIDs_ is still published on the web, but _its trustworthiness_ derives from mechanisms entirely governed by individual DID controllers. This preserves most of the delightful convenience of `did:web`, while drastically upgrading security and decentralized trust.
<!-- What is decentralized trust? We might need to describe edge cases that are either in or out this container term -->
Copy link
Contributor

Choose a reason for hiding this comment

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

yes, 'trust' feels abstract, so maybe shortcut it by saying something like 'upgrading security through authentic data that is end-verifiable' or something similar.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

spec/terminology.md Show resolved Hide resolved
@2byrds
Copy link
Contributor

2byrds commented Oct 16, 2023

Great progress @henkvancann ! Looking forward to strengthening it together!

Signed-off-by: Henk van Cann <hvancann@bcws.io>
spec/core.md Outdated
@@ -27,6 +27,7 @@ The ABNF definition of a `did:webs` DID is as follows:
webs-did = "did:webs:" domain-name ":" aid
webs-did = "did:webs:" domain-name * (":" path) ":" aid
```
`ABNF` stands for Augmented Backus-Naur Form, which is a formal notation used for describing syntax rules in various kinds of languages and protocols. We need to ensure that our implementation aligns with W3C standards and the ABNF rules specified in the DID documentation. These rules serve as a kind of "grammar" for DIDs, ensuring that they are formatted and used consistently.
Copy link
Contributor

Choose a reason for hiding this comment

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

If DID core refers to ABNF, then we should link there instead of adding this paragraph. Can you double check?

Copy link
Contributor

Choose a reason for hiding this comment

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

@henkvancann instead of a full definition, please add a [[def:]] in terminology with just the first sentence and perhaps add a link to this https://www.w3.org/TR/did-core/#did-syntax and then [[ref:]] that here.

Copy link
Contributor Author

@henkvancann henkvancann Oct 18, 2023

Choose a reason for hiding this comment

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

Yes, that's what I intend to do: one sentence + link. Some terms are just too complicated to define in one sentence. But it's what I strive for. See my Termtext PR72.
Btw: Feel free to add / change stuff you don't like afterwards. It's getting messy now with too may PRs, please accept a few first / then modify according to your wishes and then I'll be back on the front ;)
PR65 is the most relevant now to integrate, for it's coming back in all the following PR70-73. And it'll give much more oversight when PR65 is in.

Copy link
Contributor

@swcurran swcurran 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 to me. In reading the text I still see more things I would do, but lets get this merged and do more passes :-)

Thanks!

Signed-off-by: Henk van Cann <hvancann@bcws.io>
@dhh1128
Copy link
Contributor

dhh1128 commented Oct 24, 2023

I also vote to merge now.

Copy link
Contributor

@2byrds 2byrds left a comment

Choose a reason for hiding this comment

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

Thank you for the updates!

@2byrds 2byrds merged commit 14609dc into trustoverip:main Oct 24, 2023
1 check passed
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

4 participants