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

Add non-normative DID Resolution and Dereferencing sections to Architecture. #262

Merged
merged 14 commits into from
May 1, 2020

Conversation

msporny
Copy link
Member

@msporny msporny commented Apr 20, 2020

These are the non-normative changes from @jricher's resolution contract PR. Moving them out to make it easier to merge the bits everyone seems to agree on.

I've done a number of editorial fixes to bring it more in line with the way the DNS and URI specs talk about resolvers and dereferencing, align the language with the plan for the architecture section, move definitions of terms into the terminology section, and various spurious whitespace and margin overrun issues.

In theory, if you were fine with #253, you should be fine with this one as well as it's just editorial changes to #253.

PRs #263, #264, and #265 layer the normative language on top of this PR in more easily digestible section-based chunks.

/cc @jricher (since he's still not in the Github w3c wg list)


Preview | Diff

@msporny msporny changed the title Add DID Resolution and Dereferencing to Architecture section. Add DID Resolution and Dereferencing to Architecture section (non-normative). Apr 20, 2020
@msporny msporny force-pushed the jricher-resolution-nonnormative branch 2 times, most recently from 9ceb5c1 to 28b4eb4 Compare April 20, 2020 02:38
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.

Looks on the right track. I noticed the contract has been removed at this point, but I'm hoping it will come back with your addition of normative statements. I look forward to seeing where this goes.

<a>DID fragment</a> MUST be used as input to the <a>DID URL</a> dereferencing
algorithm for the target component in the <a>DID document</a> object. For more
information, see [[?DID-RESOLUTION]].
<a>DID fragment</a> MUST be used as input to the <a>DID URL dereferencing</a>
Copy link
Member

Choose a reason for hiding this comment

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

just catching this now. Would it be considered derefencing if I used did:example:123?id=key1? I know we still have to discuss the usage of id query parameter, but figured it should be considered here as well.

Copy link
Member Author

Choose a reason for hiding this comment

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

I noticed the contract has been removed at this point, but I'm hoping it will come back with your addition of normative statements.

Yes, that's planned in the next PR that layers on top of this one.

Copy link
Member Author

Choose a reason for hiding this comment

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

Would it be considered derefencing if I used did:example:123?id=key1?

Yes, given that id=key1 becomes a DID Parameter whose purpose is to dereference to a key in the DID Document.

That said, I think that doing this is a bad idea. :)

Copy link
Contributor

Choose a reason for hiding this comment

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

That's part of the separation of concerns between resolution and dereferencing here: the resolution takes only the DID while dereferencing takes in the whole URL. The way the functions and inputs are defined in #253 the query parameters are NOT passed as inputs to the resolution function -- the input metadata is for information outside of the URL, much like HTTP request headers.

Copy link
Member

@kdenhartog kdenhartog Apr 22, 2020

Choose a reason for hiding this comment

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

Would it be considered derefencing if I used did:example:123?id=key1?

Yes, given that id=key1 becomes a DID Parameter whose purpose is to dereference to a key in the DID Document.

That said, I think that doing this is a bad idea. :)

Would doing that require an extended document loader to handle that de-referencing which is why you're less than enthused about that approach?

Copy link
Member Author

Choose a reason for hiding this comment

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

It's the concept of "get me this thing that is expressed in the DID Document using this syntax that duplicates the use of fragment identifiers" that is problematic. If you want the key, get the DID Document, look for the key's fragment identifier, and you're done... w/o needing a DID Parameter for that feature. It can be done w/ a simple convenience function.

Copy link
Member

@kdenhartog kdenhartog Apr 24, 2020

Choose a reason for hiding this comment

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

could did:example:123?id=key1 be used as an iri as well if the producer chose to do so? I'm really just trying to understand the edge cases here. I stick to fragments here normally cause they're easier to deal with and commonly used by everyone.

index.html Show resolved Hide resolved
@iherman
Copy link
Member

iherman commented Apr 20, 2020

(Absolutely aside:

/cc @jricher (since he's still not in the Github w3c wg list)

He is now. No idea why this wasn't done before)

index.html Outdated
When producing and consuming DID documents that are in plain JSON (as noted by
the document metadata), the following rules MUST be followed.
When producing and consuming DID documents that are in plain JSON (as indicated by
a <code>content-type</code> of <code>application/json+did</code> in the resolver metadata), the following rules MUST be followed.
Copy link
Member

Choose a reason for hiding this comment

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

application/json+did does not sound right. Shouldn't it be application/did+json?

Copy link
Contributor

Choose a reason for hiding this comment

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

That's part of the discussion under #255

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, this is wrong per RFC6838 and should be fixed. Thanks for catching that @iherman.

The specific section is here:

https://tools.ietf.org/html/rfc6838#section-4.2.8

More elaboration here:

https://tools.ietf.org/html/rfc6839

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed in 4f1c79c.

When producing and consuming DID documents that are in JSON-LD (as noted by
the document metadata), the following rules MUST be followed.
When producing and consuming DID documents that are in JSON-LD (as indicated by
a <code>content-type</code> of <code>application/jsonld+did</code> in the resolver metadata), the following rules MUST be followed.
</p>
Copy link
Member

Choose a reason for hiding this comment

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

Same remark as above: application/json+did does not sound right. Shouldn't it be application/did+json?

Copy link
Member Author

Choose a reason for hiding this comment

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

Correct, that was wrong, thanks @iherman!

Fixed in 4f1c79c.

@iherman
Copy link
Member

iherman commented Apr 20, 2020

Just something weird... When I push on the 'preview' button, then what I get is not what I seem to see in the 'changes'. And the header above says:

msporny wants to merge 4 commits into master from jricher-resolution-nonnormative

are we sure this is a PR on the text of Justin, and a possible merge would not merge into the master? I am not sure that is what we want, do we?

Cc @msporny

@jricher
Copy link
Contributor

jricher commented Apr 20, 2020

-1

This ignores the most important part of #253 in an attempt to sideline the discussion there. Please close this pull request and add comments there.

Copy link
Contributor

@jricher jricher left a comment

Choose a reason for hiding this comment

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

This change set was lifted from #253 and ignores vital portions that were intertwined with these changes.

Strongly suggest we reject this PR and continue conversation.

index.html Outdated
resolution input options and resolution output metadata. The general process of
DID Resolution is described in <a href="#did-resolution"></a>. The details of
implementing a <a>DID resolver</a> is described in the DID Resolution
specification [[?DID-RESOLUTION]].
Copy link
Contributor

Choose a reason for hiding this comment

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

This pushes off all details to another specification without providing anything actionable by implementors. I believe that this is intentional, but the language is dangerous and slippery.

Copy link
Contributor

Choose a reason for hiding this comment

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

Also note that this is much more than an "editorial change" to #253, as claimed by the header of the pull request. This one statement drastically changes the nature and intent of the pull request.

Copy link
Member Author

Choose a reason for hiding this comment

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

@msporny
Copy link
Member Author

msporny commented Apr 21, 2020

msporny wants to merge 4 commits into master from jricher-resolution-nonnormative

are we sure this is a PR on the text of Justin, and a possible merge would not merge into the master? I am not sure that is what we want, do we?

If I wanted to do that, I'd have to fork Justin's fork, which is a fork of this repo. It's a more difficult than necessary way to merge the changes in.

What I did instead was to pull @jricher's PR directly into a branch in the WG repo (so all Editors could have access to the text), squashed his edits (removing unnecessary middle-edits and the normative bits that will be added in a separate PR), and then performed editorial fixes to the rest. I did a line-by-line comparison to his non-normative text in cee116a and then did my changes on top of that.

This will just let us merge the non-normative stuff, w/ Editor corrections, when we have a clean review on this PR rather than having to completely agree to the normative and non-normative parts all at the same time (which will take much longer than getting agreement on the non-normative portions and then the normative portions before we can merge anything).

Does that answer your question @iherman?

@msporny
Copy link
Member Author

msporny commented Apr 21, 2020

This ignores the most important part of #253 in an attempt to sideline the discussion there.

This PR pulls the non-normative portions out of #253, performs a number of editorial fixes on it and prepares it for merging into the master branch of the specification.

The normative portions of #253 are coming in a PR shortly after this one (edit, these exist now as #263, #264, and #265), as stated in the text opening this PR -- #262 (comment):

The next PR is going to layer the normative language in #253 on top of this PR.

It is not an attempt to sideline discussion, it is an attempt to split #253 into more easily digestible chunks so that it can be merged into the specification more rapidly.

@msporny msporny changed the title Add DID Resolution and Dereferencing to Architecture section (non-normative). Add non-normative DID Resolution and Dereferencing sections to Architecture. Apr 21, 2020
Copy link
Contributor

@jricher jricher left a comment

Choose a reason for hiding this comment

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

I don't agree with many of the changes made during the editorial process as they fundamentally change the nature and direction of several statements. Suggested improvements are included in this review.

<a>DID fragment</a> MUST be used as input to the <a>DID URL</a> dereferencing
algorithm for the target component in the <a>DID document</a> object. For more
information, see [[?DID-RESOLUTION]].
<a>DID fragment</a> MUST be used as input to the <a>DID URL dereferencing</a>
Copy link
Contributor

Choose a reason for hiding this comment

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

That's part of the separation of concerns between resolution and dereferencing here: the resolution takes only the DID while dereferencing takes in the whole URL. The way the functions and inputs are defined in #253 the query parameters are NOT passed as inputs to the resolution function -- the input metadata is for information outside of the URL, much like HTTP request headers.

@@ -630,7 +630,43 @@ <h2>DID Documents</h2>
</section>

<section>
<h2>DID Resolvers and DID Resolution</h2>
<h2>DID Resolvers</h2>
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 these two sections flow better as a single section, especially at this level of the document.

Copy link
Contributor

Choose a reason for hiding this comment

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

Or better yet, remove this section and incorporate its content into the resolution and dereferencing contract sections below.

Copy link
Member Author

Choose a reason for hiding this comment

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

As discussed on the call, leaving this as-is for now to talk about the "nouns" in the architecture, and then what the nouns do.

<h2>DID Resolvers</h2>

<p>
A <a>DID resolver</a> is a software and/or hardware component that takes a
Copy link
Contributor

Choose a reason for hiding this comment

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

It's confusing to talk about the component first and then the process that defines them, I would suggest that this be reversed. However, realizing that this section is defining components of the architecture and not actions of the architecture, this ordering might be desired. With that in mind, perhaps these pieces should be moved to the resolution section below instead.

Copy link
Member Author

Choose a reason for hiding this comment

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

As discussed on the call, leaving this as-is for now to talk about the "nouns" in the architecture, and then what the nouns do.

index.html Outdated Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
Comment on lines +157 to +168
that the controller of the <a>DID</a> decides that it
identifies. These new identifiers are designed to enable the controller
of a <a>DID</a> to prove control over it and to be implemented independently
of any centralized registry, identity provider, or certificate authority.
<a>DID</a>s are URLs that associate a <a>DID subject</a> with a
<a>DID document</a> allowing trustable interactions associated with that subject.
Each <a>DID document</a> can express cryptographic material, verification methods,
or <a>service endpoints</a>, which provide a set of mechanisms enabling a
<a>DID controller</a> to prove control of the <a>DID</a>. <a>Service
endpoints</a> enable trusted interactions associated with the <a>DID
subject</a>. A <a>DID document</a> might contain semantics about the subject
that it identifies. A <a>DID document</a> might contain the <a>DID subject</a>
Copy link
Contributor

Choose a reason for hiding this comment

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

Do these need to be in this specific pull request? There are several similar changes here. Space cleanup commits like this should be left to separate pull requests so as not to distract from discussion on content.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, agreed, ideally they would be. Merging anyway so that these are fixed when merged into the main document. Will rebase the other PRs so they show up there.

<h2>DID Dereferencers</h2>

<p>
A <a>DID URL dereferencer</a> is a software and/or hardware component that takes
Copy link
Contributor

Choose a reason for hiding this comment

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

Same comment about ordering above: define the process, then the component that provides that process.

Copy link
Member Author

Choose a reason for hiding this comment

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

As discussed on the call, leaving this as-is for now to talk about the "nouns" in the architecture, and then what the nouns do.

index.html Outdated Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
@jricher
Copy link
Contributor

jricher commented Apr 22, 2020

@msporny I believe the currently suggested text changes address the concerns raised and can be incorporated. I'll make parallel changes to the other pull requests separately.

@msporny
Copy link
Member Author

msporny commented Apr 23, 2020

@jricher, all suggestions have been merged. Given the positive reviews in #253, given that this PR is based on that one, and given that this is all non-normative, I'm going to merge this in the next 48 hours unless I hear objections. I'll double check to make sure this is okay to do w/ Chairs and Editors today.

@msporny msporny force-pushed the jricher-resolution-nonnormative branch from 2327b38 to 4183235 Compare April 23, 2020 16:25
@philarcher
Copy link
Contributor

Looking at this afresh today, I remain content with the revised wording in the DID Resolvers and DID Dereferencers sections.

But...

I just spotted this in the section on Generic DID Parameters:

"The exact processing rules for these parameters are specified in [DID-RESOLUTION]."

Sorry to be awkward but, again, I believe it is not right to include a sentence like that in a normative section. Formally, nothing is specified in DID-RESOLUTION. Reusing the language we've converged on in the other sections, how about changing that to

Additional considerations for processing these parameters are provided in [?DID-RESOLUTION]

@msporny
Copy link
Member Author

msporny commented Apr 23, 2020

Sorry to be awkward but, again, I believe it is not right to include a sentence like that in a normative section.

I have awkwardly applied your exact wording in 758b635.

Copy link
Contributor

@philarcher philarcher 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 @msporny!

index.html Outdated Show resolved Hide resolved
@msporny msporny force-pushed the jricher-resolution-nonnormative branch from 758b635 to ead9e79 Compare May 1, 2020 14:29
@msporny msporny force-pushed the jricher-resolution-nonnormative branch from ead9e79 to d9833cb Compare May 1, 2020 15:03
Copy link
Member Author

@msporny msporny left a comment

Choose a reason for hiding this comment

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

Ok, finally re-based on top of @peacekeeper's change set (with contributions from @brentzundel) to capture their contributions in the history. Applied @jricher's last change. We're good to go here.

Arguably non-normative, multiple positive reviews, many changes made as a result, no objections, merging (finally!).

@msporny msporny merged commit af5800c into master May 1, 2020
@msporny msporny deleted the jricher-resolution-nonnormative branch July 3, 2020 15:49
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.

6 participants