Add did key and did web examples #19

OR13 · 2021-01-23T22:44:33Z

💥 Error: 500 Internal Server Error 💥

PR Preview failed to build. (Last tried on Jan 23, 2021, 10:44 PM UTC).

More

PR Preview relies on a number of web services to run. There seems to be an issue with the following one:

🚨 Spec Generator - Spec Generator is the web service used to build specs that rely on ReSpec.


😭  Sorry, there was an error generating the HTML. Please report this issue!
Specification: https://rawcdn.githack.com/w3c/did-test-suite/56fd379907847a1a69e6d5c2629759909254f4a7/index.html?isPreview=true&publishDate=2021-01-23
ReSpec version: 26.0.0
File a bug: https://github.com/w3c/respec/
Error: Error: Evaluation failed: TypeError: Cannot destructure property 'rsDocToDataURL' of '(intermediate value)' as it is undefined.
    at evaluateHTML (__puppeteer_evaluation_script__:23:13)
    at ExecutionContext._evaluateInternal (/u/spec-generator/node_modules/puppeteer/lib/cjs/puppeteer/common/ExecutionContext.js:217:19)
    at processTicksAndRejections (internal/process/task_queues.js:93:5)
    at async ExecutionContext.evaluate (/u/spec-generator/node_modules/puppeteer/lib/cjs/puppeteer/common/ExecutionContext.js:106:16)
    at async generateHTML (/u/spec-generator/node_modules/respec/tools/respecDocWriter.js:208:12)
    at async toHTML (/u/spec-generator/node_modules/respec/tools/respecDocWriter.js:87:18)
    at async Object.generate [as respec] (/u/spec-generator/generators/respec.js:13:40)
    at async /u/spec-generator/server.js:89:44

If you don't have enough information above to solve the error by yourself (or to understand to which web service the error is related to, if any), please file an issue.

peacekeeper

This should be changed so that the examples for application/did+json don't contain @context, for reasons that have been discussed many times. It makes no sense to inject a JSON-LD specific entry into a plain JSON DID document.

OR13 · 2021-01-25T15:28:49Z

@peacekeeper take a look at https://did.key.transmute.industries/

https://github.com/transmute-industries/did-key.js

Are you saying that you intend to block implementations that are legal, but don't look the way that you want?

Why?

Isn't the point of tests to show what implementers are actually doing?

I am happy to include tests that don't contain an @context if you can find my an implementation of did-key (afaik there are none) or did-web (trivial for you to create) that does....

You are also welcome to create examples that look the way you want, but please don't block a PR that shows that an existing implementation is conformant, because you don't like the way the implementation shows conformance.

This will quickly escalate and slow down an already slow process of writing tests.... if you want to block legality of implementations did-core is the place to do that, not here.

These tests are not mean to be "whatever the editors want"... they are meant to reflect actual implementations and their support.

As an implementer of many did methods, I don't agree with your perspective, and I have written lots of code which intentionally does not support your perspective, and that code is legal. You are are welcome to write code that embodies your perspective and add examples of it here.

msporny · 2021-01-25T15:41:50Z

This should be changed so that the examples for application/did+json don't contain @context, for reasons that have been discussed many times. It makes no sense to inject a JSON-LD specific entry into a plain JSON DID document.

Digital Bazaar's DID Document producer implementations will contain @context for JSON-only representations. We do this because we use the same JSON Schema file to check for the correctness of application/did+json and application/did+ld+json.

Our experience is that developers often get content negotiation wrong and we want to provide a single serialization that will work for JSON (just ignore but preserve @context) and JSON-LD (which requires that @context is defined). Having two code paths that only differ in whether or not @context is generated also creates the additional issue of creating more code to audit (which increases what can go wrong in complex systems).

There is nothing in the specification that states that you can't put @context in a JSON-only document, and there are good reasons to do so. The intention of the test suite is to test what actual implementations are doing, and preserving @context in some JSON-only documents is an example of that in practice.

msporny

Ok to merge... I have concerns about some of the stuff in did:key and did:web -- BUT, those are more concerns around the DID Method and not concerns about the test suite. Thanks for the addition of your implementations, @OR13! :)

msporny · 2021-01-25T16:24:09Z

packages/did-core-test-server/generate-latest-respec-data.js

 const getReportResults = require('./services/getReportResults');
 const suitesInput = require('./suites/did-spec/default.json');

+const respecPath = path.resolve(__dirname, '../../index.html');
+
+const updateRespec = (suitesInput, suitesReportJson) => {


This is some wizardry right here -- I like it -- wonder if any other W3C WG is doing stuff like this, feels like a useful design pattern.

msporny · 2021-01-25T16:25:34Z

packages/did-core-test-server/suites/did-spec/default.json

+                {
+                  "@base": "did:key:z6MkhTZLPqg7kYwBe6y8utZZr1NfMViqXWep7rY7ogUSPgsB"
+                }


This makes me sad -- feels like something the did-key library implementation should do by setting the base instead of it being hard coded in the @context.

It is extraneous information most of the time... but experience with JSON-LD has taught us that it is better to be explicit about this sort of thing.

msporny · 2021-01-25T16:26:47Z

packages/did-core-test-server/suites/did-spec/default.json

              "verificationMethod": [
                {
-                  "id": "#key-0",
+                  "id": "#z6MkhTZLPqg7kYwBe6y8utZZr1NfMViqXWep7rY7ogUSPgsB",


Hrm, why relative identifiers in did:key? This is more of a did:key quibble -- I had thought that everyone was going to use fully expanded identifiers (because there is no reason to optimize for space in did:key, right?)

Yeah, -1 to relative identifiers -- at least not as the main example as it encourages people to choose the format that I expect will frustrate devs the most. If we really need this for some reason, it should be its own special case example.

Our implementation prefers relative IRIs, and we ALWAYs try and optimize for space. We have tests the prove our relative ref implementation if consistent and equivalent with the expanded representation.

Another reason we prefer the relative form is to match the structure of DIDs returned by sidetree based methods.

regarding the "main example" for a did method... which implementation is "the main one" remains an area for the test suite to address... my preference is that anything that is legal for the did method be tested, and therefore that implementations share a spec input for the method.... this will yield better alignment.

msporny · 2021-01-25T16:27:41Z

packages/did-core-test-server/suites/did-spec/default.json

-              "capabilityDelegation": ["#key-0"],
-              "keyAgreement": ["#key-1"]
+              "authentication": [
+                "#z6MkhTZLPqg7kYwBe6y8utZZr1NfMViqXWep7rY7ogUSPgsB"


Again, why not fully expanded?

Our implementation prefers relative IRIs always, they are equivalent when compared via n-quads.

The real reason is that we want a did-key representation that looks like sidetree based dids, for testing... similar to the use of did-key with did:v1 I imagine.

As long as our implementation and yours produce the same n-quads we consider the implementations to be equivalent.

We also write tests using VCs and resolvers to show that they implementations both work, even though the look different.

msporny · 2021-01-25T16:28:23Z

packages/did-core-test-server/suites/did-spec/default.json

                  }
                },
                {
-                  "id": "#key-1",
+                  "id": "#z6LSh8yKFPF2bLnD9mHQZMJL6zQQZsowYUeUaDd39rMLHBBx",
                  "type": "JsonWebKey2020",


Assume that the did-key library will take an input parameter to specify the cryptosuite to use to express the key material?

our implementation is here: https://github.com/transmute-industries/did-key.js

essentially you create a keypair instance, which stores an internal representation of the key as buffers and meta data, and then you can ask for a did+json or did+ld+json representation of any given did-key.

the identifier remains the the same for both, according to the did key method spec, but they are represented differently depending on the caller's preference for JWK vs base58

msporny · 2021-01-25T16:31:02Z

packages/did-core-test-server/suites/methods/did-key/defaultSuiteConfig.json

+            "verificationMethod": [
+              {
+                "id": "#z6MkhTZLPqg7kYwBe6y8utZZr1NfMViqXWep7rY7ogUSPgsB",
+                "type": "Ed25519VerificationKey2018",


Just as a heads-up, we're probably going to submit a Ed25519VerificationKey2020 for did:key with publicKeyMultibase -- I assume the test suite allows for multiple did:key submissions?

msporny · 2021-01-25T16:32:02Z

packages/did-core-test-server/suites/methods/did-key/did-key.spec.js

@@ -0,0 +1,27 @@
+let { suiteConfig } = global;


This is about the name of the file... probably want did-key-transmute.spec.js -- as DB is going to want to submit their own as well.

@msporny yes, this part of writing tests remains undefined.... IMO, the method conformance tests should ideally be a single collection, and represent all legal examples produced by real world implementations, for did-key, the problem is that implementations don't support all representations.... this can be solved by splitting the did-key by implementation, which appears to be in line with what @peacekeeper wants.

OR13 · 2021-01-25T17:51:05Z

In summary, I think the main issue which remains unaddressed is how multiple implementations can be submitted for the same method.

The current structure allows for this to easily be tested, but in terms of a single test suite in which all methods are tested there is a problem.

The current tests assume that it is the DID Method, not the implementation that supports representations.... meaning in order for an implementation to show conformance it must either support all representations supported by other implementations, or be tested in isolation.... Its obviously better if we can use his to make methods support representations consistently... but I have low confidence that implementers have the time or skill needed to accomplish this.... so perhaps we should refactor the did core test suite to be implementation oriented as opposed to method oriented.

OR13 · 2021-01-26T18:23:48Z

so perhaps we should refactor the did core test suite to be implementation oriented as opposed to method oriented.

any objections?

csuwildcat · 2021-01-26T21:38:03Z

No objections here. Thank you for writing tests.

OR13 · 2021-01-28T19:05:27Z

I plan on refactoring this to be "vendor oriented"... instead of "method oriented"... this will mean that for any given method, like did:key each vendor will be able to show a different level of conformance to did core... so some vendors will support did+cbor whereas others won't etc... This will make the report much longer.... since we will essentially be multiplying number of did methods * number of vendor implementations per did method.

TallTed · 2021-01-28T21:13:33Z

I suggest that the refactoring not be to "vendor oriented" but to "implementation oriented" as was previously expressed as the revised plan, replacing "[DID] method oriented".

It's entirely possible (though perhaps unlikely during our CR period[s]) that one vendor will have multiple implementations, probably covering different DID methods, but possibly even multiple implementations on one DID method.

This suggestion may be what you meant in recent comments, but I thought it worth certainty, instead of waiting until a lot of work had gone into refactoring to "vendor orientation" and then needing that work to be redone for "implementation orientation".

peacekeeper · 2021-01-28T21:38:31Z

Are you saying that you intend to block implementations that are legal, but don't look the way that you want?

The intention of the test suite is to test what actual implementations are doing

The test suite should be built around what the spec says, not around what certain opinionated implementations are doing.

Yes your implementations that inject @context into application/did+json are "legal", but this is not the most direct and intutive way of implementing the data model and the JSON representation.

Our production rules for JSON-LD say "inject @context". Our production rules for JSON don't say that. So why do you do it there?

The reason why you're doing this is because you don't like the ADM and you would rather just have JSON-LD. I would also rather just have JSON-LD! But now that we have the ADM and multiple representations, we should use them in the test suite as intended. Your argument "it's legal" is not wrong, but it undermines the intentions of the specification, it ignores many F2F meetings and special topic calls that led to the current data model, and will confuse everyone who looks at the tests. You're trying to force your disagreement with the ADM design into the test suite using - no offense - a defiant and shallow argument.

Maybe at a later point in time we can add those special cases where additional entries like @context or foo or whatever are injected into application/did+json, but initially the test suite should be based on the spec in a straightforward way.

So -1 to merging this as-is.

Add did key and did web examples

56fd379

OR13 requested a review from msporny as a code owner January 23, 2021 22:44

This was referenced Jan 23, 2021

Help review did core conformance tests w3c-ccg/did-method-key#25

Closed

Help review did test suite conformance w3c-ccg/did-method-web#23

Open

peacekeeper requested changes Jan 25, 2021

View reviewed changes

msporny approved these changes Jan 25, 2021

View reviewed changes

OR13 mentioned this pull request Jan 26, 2021

Align implementation with resolution metadata portion of the spec decentralized-identity/sidetree-reference-impl#14

Open

OR13 closed this Jan 30, 2021

OR13 mentioned this pull request Jan 30, 2021

Refactor test suite to be vendor oriented and add example #20

Closed

msporny deleted the feat/add-did-key-and-did-web-example branch May 2, 2021 20:53

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add did key and did web examples #19

Add did key and did web examples #19

OR13 commented Jan 23, 2021 •

edited by pr-preview bot

Loading

peacekeeper left a comment

OR13 commented Jan 25, 2021 •

edited

Loading

msporny commented Jan 25, 2021 •

edited

Loading

msporny left a comment

msporny Jan 25, 2021

OR13 Jan 25, 2021

msporny Jan 25, 2021

OR13 Jan 25, 2021

msporny Jan 25, 2021

dlongley Jan 25, 2021

OR13 Jan 25, 2021

OR13 Jan 25, 2021

msporny Jan 25, 2021

OR13 Jan 25, 2021

OR13 Jan 25, 2021

msporny Jan 25, 2021

OR13 Jan 25, 2021

msporny Jan 25, 2021

msporny Jan 25, 2021

OR13 Jan 25, 2021

OR13 commented Jan 25, 2021

OR13 commented Jan 26, 2021 •

edited

Loading

csuwildcat commented Jan 26, 2021

OR13 commented Jan 28, 2021 •

edited

Loading

TallTed commented Jan 28, 2021

peacekeeper commented Jan 28, 2021 •

edited

Loading

Add did key and did web examples #19

Add did key and did web examples #19

Conversation

OR13 commented Jan 23, 2021 • edited by pr-preview bot Loading

💥 Error: 500 Internal Server Error 💥

peacekeeper left a comment

Choose a reason for hiding this comment

OR13 commented Jan 25, 2021 • edited Loading

msporny commented Jan 25, 2021 • edited Loading

msporny left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

OR13 commented Jan 25, 2021

OR13 commented Jan 26, 2021 • edited Loading

csuwildcat commented Jan 26, 2021

OR13 commented Jan 28, 2021 • edited Loading

TallTed commented Jan 28, 2021

peacekeeper commented Jan 28, 2021 • edited Loading

OR13 commented Jan 23, 2021 •

edited by pr-preview bot

Loading

OR13 commented Jan 25, 2021 •

edited

Loading

msporny commented Jan 25, 2021 •

edited

Loading

OR13 commented Jan 26, 2021 •

edited

Loading

OR13 commented Jan 28, 2021 •

edited

Loading

peacekeeper commented Jan 28, 2021 •

edited

Loading