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 language regarding DID Method Update Operation #220

Merged

Conversation

OR13
Copy link
Contributor

@OR13 OR13 commented Mar 15, 2020

  • Add language about what an "Update" is.

Based on feedback here: #174


Preview | Diff

@OR13 OR13 requested review from msporny, peacekeeper, selfissued, awoie, dhh1128, dmitrizagidulin and talltree and removed request for msporny March 15, 2020 17:32
index.html Outdated Show resolved Hide resolved
Copy link
Member

@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.

-1 to this PR. The last discussion we had regarding this property was that it was metadata... which means that it exists in a relationship such as doc.didDocument.created or, and this would be the preferred thing, it lives outside of the DID Document. did:web is the only thing that seems to need it and @dmitrizagidulin noted that he was fine just defining it in the did:web method context and then we'll decide on whether or not to move it into DID Core after a year or two of deployment experience.

@OR13
Copy link
Contributor Author

OR13 commented Mar 15, 2020

@msporny I don't understand your objection. Both "created" and "updated" are currently marked as "SHOULD".

I agree with your objection, and I believe this PR is an attempt to move us towards the perspective you are sharing.

Would you prefer I modify the PR to remove "created" and "updated" entirely?

Please provide actionable feedback, I'm happy to make changes.

@OR13 OR13 requested a review from msporny March 15, 2020 18:12
@msporny
Copy link
Member

msporny commented Mar 15, 2020

Would you prefer I modify the PR to remove "created" and "updated" entirely?

Yes, please... you may be unaware that there were PRs already to do this (which failed, blocked by the metadata discussion):

#28
#27
#26

My expectation is that any PR you submit to remove created/updated/proof will be blocked for the same reason. The group is waiting on a resolution to the metadata discussion in order to resubmit those PRs.

Does that make sense?

@OR13
Copy link
Contributor Author

OR13 commented Mar 15, 2020

@msporny

Based on the content of this PR, I would not expect it to be blocked by those... This PR is a step towards removing metadata from the doc...

If any method decides to include such fields in the did document (did:web), but most others don't, then the spec should definitely not say "SHOULD", and IMO it should say MAY, and specify the timestamp format as it does currently...

otherwise we are inviting methods to put whatever "metadata" they want in the did document... we are currently encouraging them to do that, with the use of the word "SHOULD".

Removing the properties seems like a change more likely to be objected to than marking them as "MAY" when we know that only a few (1) DID Method wants to use them.

@msporny
Copy link
Member

msporny commented Mar 15, 2020

If any method decides to include such fields in the did document (did:web), but most others don't, then the spec should definitely not say "SHOULD", and IMO it should say MAY, and specify the timestamp format as it does currently...

We're still talking past each other.

A subset of the group believe that the DID Document MUST NOT specify created/updated metadata at the top level.

The options on the table right now seem to be:

  1. Put metadata at the top-level of the DID Document. This conflates metadata about the DID subject w/ metadata about the DID Document. This led to objections and the "metadata buckets" discussion.
  2. Put metadata in a didDocument property at the top level. This seems to be an acceptable compromise to both sides of the discussion. Only did:web seems to want to do this at present (based on the last call on this topic), so there was a suggestion for the did:web method to define this property and use it.
  3. Do not put metadata in the DID document, and instead make it part of a DID Resolution response, which is what some would prefer as the cleanest option.

Changing the language to MAY is choice # 1 above, which is the point of contention around metadata in the DID document... which led to objections last time, and I suspect will lead to objections this time (because the fundamentals haven't changed around the discussion).

In other words, "MAY" is RFC-speak for: "It's totally cool if you want to put metadata at the top level, go ahead and do it if you want to."

OR13 added 2 commits March 15, 2020 13:56
…hub.com:OR13/did-core into feat/address-174-regarding-metadata-and-updated
@OR13
Copy link
Contributor Author

OR13 commented Mar 15, 2020

@msporny Ahh I see... I have reverted those changes, the PR now does not comment on metadata properties, only on DID Method Operations.

@OR13 OR13 changed the title Add language regarding Update Add language regarding DID Method Update Operation Mar 15, 2020
Copy link
Member

@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.

Minor nits then good to go.

index.html Outdated Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
OR13 and others added 4 commits March 15, 2020 14:07
Co-Authored-By: Manu Sporny <[email protected]>
Co-Authored-By: Manu Sporny <[email protected]>
Co-Authored-By: Manu Sporny <[email protected]>
@OR13 OR13 requested a review from msporny March 15, 2020 19:10
@brentzundel
Copy link
Member

The current proposed text is confusing to me. The section seems to be about updating a DID Document, but the text states that it is possible to update a DID Document without it changing.
Perhaps adding an example of an update that doesn't result in changes to the DID Doc would clarify things.

@OR13
Copy link
Contributor Author

OR13 commented Mar 17, 2020

@brentzundel

The text is about the "Update Operation" which might result in an updated representation...

I'm not sure an example would be appropriate here, since it would certainly be DID Method Specific.... This seems to be a case where the implementation guideline would be helpful...

For the sake of clarifying, imagine creating an update operation that is signed which replaces key material without changing the material.. or creating an update operation which is not valid....

Both are updates according to sidetree protocol (did:ion, did:elem, etc...), neither results in a did document change...

I'd prefer not to introduce method specific examples in did core... it invites politics.

Maybe this text is not helpful at all...

@brentzundel
Copy link
Member

Perhaps just the sentence, "For example, an update operation which replaces key material without changing it could be a valid update that doesn't result in changes to the DID Document."

index.html Outdated
</p>

<p>
An update to a DID is any change in the data used to produce a DID Document.
Copy link
Member

@kdenhartog kdenhartog Mar 18, 2020

Choose a reason for hiding this comment

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

This sentence feels weird to me because it could be understood that the initial creation of a DID Document is an update.

Nothing => basic did Doc

This would be classified as an update using this language. Is that an intentional choice of language?

Copy link
Contributor Author

@OR13 OR13 Mar 18, 2020

Choose a reason for hiding this comment

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

It was intentional, I think the language aligns well with KERI / did:peer / did:ethr / sidetree, but I'm happy to change it if there is something stronger.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added "after creation" to address this concern.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can I resolve this thread?

@OR13
Copy link
Contributor Author

OR13 commented Mar 18, 2020

@brentzundel thanks, I added your language, much clearer.

Copy link
Member

@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.

Minor nits, can merge after they're resolved.

index.html Outdated Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
OR13 and others added 3 commits March 21, 2020 14:09
Co-Authored-By: Manu Sporny <[email protected]>
Co-Authored-By: Manu Sporny <[email protected]>
Co-Authored-By: Manu Sporny <[email protected]>
@OR13 OR13 dismissed msporny’s stale review March 21, 2020 19:10

Implemented requested changes

@OR13 OR13 requested a review from msporny March 21, 2020 19:10
@OR13
Copy link
Contributor Author

OR13 commented Mar 21, 2020

@msporny I have merged your changes, can you review again?

Copy link
Member

@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.

Editorial, all changes requested and made, multiple positive reviews, merging.

@msporny msporny merged commit a84f18c into w3c:master Mar 23, 2020
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.

4 participants