search

w3c / did-core

W3C Decentralized Identifier Specification v1.0
https://www.w3.org/TR/did-core/
Other
405 stars 94 forks source link

Clean up DID Resolution section; enable testability via proof by construction #601

Closed msporny closed 3 years ago

msporny commented 3 years ago

This PR is intended to address issue #549 by adding to the abstract functions discussed in the DID Resolution and DID URL Dereferencing sections and ensuring that all statements are testable using a "proof by construction". That is, all statements that return abstract data models are testable due to additional normative statements that effectively state: "anything that returns an abstract data model MUST be serializable in one of the representations". Previously, that statement didn't apply to didDocument, the metadata structure, some of the dereferencing values, etc. This PR fixes that.

The PR also adds at-risk markers to some of the newer features requested by individuals (but not necessarily broadly requested by the WG -- such as nextUpdate, nextVersionId, equivalentId, and canonicalId. There are also a few bugs fixed in this version (such as stating that a contentType must be specified for the resolve call -- that returns an abstract didDocument... which is clearly wrong).

There are a LOT of reformatting changes in this PR, it might be best to look at the rendered preview for this PR.

Preview | Diff

OR13 commented 3 years ago

We may find it helpful to review / agree to https://github.com/w3c/did-core/pull/600 before attempting further changes here.

peacekeeper commented 3 years ago

There are also a few bugs fixed in this version (such as stating that a contentType must be specified for the resolve call

@msporny Where does the current text state that?

peacekeeper commented 3 years ago

@msporny thanks for this new PR, I agree with a lot of changes, but am also confused by some others. Maybe it would be simpler to break this up into multiple PRs so they are easier to review?

peacekeeper commented 3 years ago

This PR is not easy to review.. There are many many changes, most of which have nothing to do with the title of the PR (make resolution concrete and testable).

msporny commented 3 years ago

@OR13 wrote:

As long as we can agree to at least one testable resolution function which returns json / cbor I am good, I don't care what we call it.

At present, I believe both resolve and resolveRepresentation are testable. resolve is now testable because we say you must be able to express didDocument (which is a map) in a representation. resolveRepresentation is testable because it returns a representation.

I believe I have addressed all of your concerns, @OR13 (and merged your change suggestions).

@rhiaro, @TallTed, and @peacekeeper, I believe I've addressed all of your concerns (and merged your change suggestions).

@jricher, I merged the majority of your change suggestions. There are three changes that I didn't apply with reasoning on why each one didn't go in. Please re-review and let us know if the current state of the PR works for you.

msporny commented 3 years ago

@jricher, I merged the majority of your change suggestions. There are three changes that I didn't apply with reasoning on why each one didn't go in. Please re-review and let us know if the current state of the PR works for you.

I believe we have discussed all of your concerns with the current PR @jricher, and while you think some of these changes weaken the specification and/or are unnecessary, you would not object to the PR if it was merged at this time. Can you please confirm this so I can merge the PR?

jricher commented 3 years ago

@msporny My concerns have been noted and discussed, and I disagree with the conclusions and solutions to those concerns that are part of this PR. That said my opinion is just one of many in the working group, and so I am not attempting to block this pull request with just my own stance, regardless of how and why I disagree.

msporny commented 3 years ago

@msporny My concerns have been noted and discussed, and I disagree with the conclusions and solutions to those concerns that are part of this PR. That said my opinion is just one of many in the working group, and so I am not attempting to block this pull request with just my own stance, regardless of how and why I disagree.

Acknowledged, thank you.

I am going to pull this PR in with the following caveat -- we still have a few weeks left to improve this section if anyone has a better idea on how to strike the abstract/testable balance. There are also legitimate concerns about the Representation sections and how they apply to some of the language in this PR that needs to be addressed; are they about serializing DID Documents only, or any data expressed using the ADM? We're almost certainly headed for a 2nd PR... I've never seen a specification with 130+ machine-testable normative statements get it right the first time -- but we're going to try. This PR addresses all the outstanding testability concerns with the Resolution section, but in a way that causes almost all of us heartburn... so if anyone has any better ideas in the meantime, please feel free to raise a PR -- the worst that can happen is that the group decides to not take it up before CR.

msporny commented 3 years ago

Normative, multiple reviews via #587, #591, #592, and #601, changes requested and made, remaining objections are non-blocking, merging with caveat in https://github.com/w3c/did-core/pull/601#issuecomment-777092645.