chore(ref-imp): remove controller and add context to deactivated docu… #1006
Conversation
isaacJChen
requested review from
csuwildcat,
OR13,
tplooker and
troyronda
as code owners
| @@ -18,7 +18,6 @@ | |||
| "verificationMethod": [ | |||
| { | |||
| "id": "#publicKeyModel1Id", | |||
| "controller": "", | |||
There was a problem hiding this comment.
please read did core before making changes to test vectors.... https://w3c.github.io/did-core/#verification-methods
The properties MUST include the id, type, controller, and specific verification method properties, and MAY include additional properties.
I ideally you would also have tests that showed that JSON-LD was valid.
There was a problem hiding this comment.
@OR13 This change is my fault. I read the spec but I looked at wrong section (controller at top level of the document). Sorry about this. Can the controller be empty though?
There was a problem hiding this comment.
the controller should equal didDocument.id in order for verifiable credentials to be valid.... see: https://github.com/OR13/did-params-and-you/blob/master/examples/case-0-legal-working.json#L35
and our version of sidetree: https://github.com/transmute-industries/sidetree.js/blob/main/packages/core/src/DocumentComposer.ts#L65
If his repo had CI tests that used the result of test vectors, with verifiable credentials, it would be obvious when breaking changes like this were introduced.
btw, the reason that "" is legal here, is because of @base / relative IRIs in sidetree did documents....
You can check the nquads to see that its the same...
if the iri is blank, it will get mapped to the didDocument.id... making what we had before legal (but only for relative ref based did documents).
There was a problem hiding this comment.
So it is legal because it will be populated when processing @base - thank you for this clarification.
There was a problem hiding this comment.
I strongly recommend against returning the empty string for the controller property.... if makes sidetree look different than every other did method, and its not guaranteed to work accross JSON-LD implementations... the controller property should match the didDocument.id.
There was a problem hiding this comment.
Thanks for catching it. I will have a PR out fixing this soon.
…ment