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

Make DID Resolution section concrete and testable (Typescript). #591

Closed
wants to merge 10 commits into from

Conversation

msporny
Copy link
Member

@msporny msporny commented Feb 2, 2021

This PR is intended to address issue #549 by converting the abstract functions discussed in the DID Resolution and DID URL Dereferencing sections into concrete, testable functions. The PR uses an Typescript interface to provide an example while keeping much of what we had the same as before. It also tries to strike a balance by providing at least one concrete realization of the interface without preventing other concrete interfaces from being realized.

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

dereference(USVString did,
DereferencingOptions dereferencingOptions): DereferencingResponse;
};
</pre>
Copy link
Member

Choose a reason for hiding this comment

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

That does not seem like a correct Typescript. Should it be

did: UVString, dereferencingOptions: DereferencingOptions

Copy link
Member Author

Choose a reason for hiding this comment

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

You are correct, fixed in latest push.

Copy link
Contributor

@peacekeeper peacekeeper left a comment

Choose a reason for hiding this comment

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

This ignores the feedback given in the other PR by @jricher and myself (see #587 (comment) and #587 (review)).

Adding JavaScript/TypeScript is a good idea, but only as an example, not to replace the abstract function definitions. Also, the resolve() and resolveRepresentation() functions should both be kept.

@OR13
Copy link
Contributor

OR13 commented Feb 2, 2021

I feel that "resolve() and resolveRepresentation()" should NOT both be kept, especially if we can get consensus on "resolveRepresentation" and its interface definition, since it is a superset.

I'd be fine keeping a single abstract function, and provide some concrete interface definitions in typescript / infa / webidl

resolveRepresentation ( did, did-resolution-input-metadata ) <br>
&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;-&gt; ( did-resolution-metadata, did-document-stream, did-document-metadata )
</code></p>
<pre class="example" title="A conforming resolver interface in Typescript ">
Copy link
Contributor

Choose a reason for hiding this comment

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

As @peacekeeper mentioned in his review, I think we should keep the abstract function definition in infra AND keep the typescript example provided here.

@peacekeeper
Copy link
Contributor

since it is a superset.

No it's not a superset. It is possible to implement resolve() in a way that internally calls resolveRepresentation(), and it's also possible to implement resolveRepresentation() in a way that internally calls resolve().

The long discussion process in the WG has shown that there are use cases for both functions.

Copy link
Contributor

@OR13 OR13 left a comment

Choose a reason for hiding this comment

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

We should keep a single abstract function definition using infra for resolve which should map to what we have previously called resolveRepresentation.

@OR13
Copy link
Contributor

OR13 commented Feb 2, 2021

I think practically the abstract definition for resolve is useless and confusing, whereas resolveRepresentation is useful, and should be kept, after being renamed to resolve.

I acknowledge the WG spent a lot of time on this... and I think we got it totally wrong, in a way that harms implementers.

@peacekeeper
Copy link
Contributor

@OR13 I understand your point of view if you are working mostly in a JavaScript environment, where the native data model = JSON, which is at the same time also a representation. In this environment there's no real difference between the abstract data model and the representation, therefore you also don't see a need for two separate functions.

But once you start thinking outside of a JavaScript environment, in a generic and implementation-agnostic way (which is what the spec should do), then there's a big difference between a function that returns a data model, and a function that returns a concrete representation.

@OR13
Copy link
Contributor

OR13 commented Feb 2, 2021

@peacekeeper the abstract function definition is what prevents us from having a language / representation specific view of resolution, I am supportive of preserving that definition in INFRA.

I am also supportive of providing helpful examples.

In summary I agree we should preserve the abstract definitions, and we should provide concrete examples, in as many representations / languages as we can.

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 PR removes functionality agreed upon by the WG after many hours of discussion.

This also ignores feedback provided in #587 to the same.

This PR should be rejected.

@msporny
Copy link
Member Author

msporny commented Feb 3, 2021

Closing in favor of PR #592.

@msporny msporny deleted the msporny-issue-549-remove-webidl branch February 21, 2021 21:24
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