Updates the pre-rotation and witnesses sections with feedback from the first set of updates #159

swcurran · 2024-12-11T23:54:26Z

Adjusts the recently changes pre-rotation and witness section, addressing a number of issues:

Issue Should we support turning off pre-rotation and if so, how #151 adds that pre-rotation can be turned off, how (empty list) and the implications
Issue Update the data structure and what is signed by witness proofs #152 updates the did-witness.json data model to make the versionId what is signed (also issue Is it sufficient for a witness to sign only the versionId of the entry? #142)
Issue Is the spec clear on how a valid witness proof applies to earlier versions? #141 Does it make sense to only keep the proofs about the two most recent log update entries? #143 Adjust the did-witness.json file to minimize the number of proofs #144 clarifying language on when and why to remove proofs and when there will be 1 or 2 proofs from a witness. Tricky language!
Issue Specify requirements on witness key types #149 the type of Data Integrity cryptosuite that the witnesss must use
Issue Should we allow disabling a witness #140 how to disable the use of witnesses for a DID

The goal is that this PR should complete the changes needed in v0.5 -- including the renaming.

Please review carefully!

…e first set of updates Signed-off-by: Stephen Curran <[email protected]>

brianorwhatever · 2024-12-12T18:11:19Z

I am not totally comfortable with the disable prerotation feature. See comment in #151

Signed-off-by: Stephen Curran <[email protected]>

spec/specification.md

Signed-off-by: Stephen Curran <[email protected]>

… nextKeyHashes, updateKeys or witnesses Signed-off-by: Stephen Curran <[email protected]>

brianorwhatever · 2024-12-19T19:40:54Z

So with the new null option for nextKeyHashes what about undefined aka the parameter is left off. This should definitely be a failure case right?

andrewwhitehead · 2024-12-19T19:45:42Z

So with the new null option for nextKeyHashes what about undefined aka the parameter is left off. This should definitely be a failure case right?

If a parameter is left off it should carry on with its old value, although in this case the update would be invalid because we require it to be changed (once set).

My perspective was that parameters that have never been set default to null and they can be reset to null (since undefined isn't supported in JSON). Although we could also define default (implicit) values for each parameter.

swcurran · 2024-12-19T19:57:10Z

Need to get to a final answer on this.

We could add to the spec that all parameters are initialized to null.
We could add the default to null and remove the empty list “[]” option for the array items, leaving only null — used initially and when reset.

As noted in the spec, a missing value in the parameters means “unchanged from the previous value”. As Andrew notes, some parameters add rules of when that a missing value is unacceptable — e.g. parameters that MUST be in the first entry, and the pre-rotation parameters that MUST be in every entry when active.

After writing this, my leaning is 2 above.

…ated using null and define defaults for parameters Signed-off-by: Stephen Curran <[email protected]>

…hanges

swcurran · 2024-12-20T18:44:06Z

Added commit to limit array parameters (witness, nextKeyHashes, updateKeys) to be deactivated by null, removing the option of using of empty list []. Also went through the parameters and formalized their default values where not previously explicit, and how to use null to deactive them (where needed).

Signed-off-by: Stephen Curran <[email protected]>

swcurran · 2024-12-20T18:47:25Z

Add a last tweak -- made explicit that they array parameters MUST NOT be set to an empty list.

brianorwhatever

All of my comments are of the "take it or leave it" type

brianorwhatever · 2024-12-20T19:48:41Z

spec/specification.md

-  - If not explicitly set in the first [[ref: log entry]], it is set to `false`.
-  - Once the value has been set to `false`, it cannot be set back to `true`.
+  - If not explicitly set in the first [[ref: log entry]], it **MUST** be set to `false`.
+  - Once the value has been explicitly to `false` in a [[ref: DID log entry]], it **MUST NOT** be set back to `true`.


Suggested change

- Once the value has been explicitly to `false` in a [[ref: DID log entry]], it **MUST NOT** be set back to `true`.

- Once the value has been explicitly set to `false` in a [[ref: DID log entry]], it **MUST NOT** be set back to `true`.

brianorwhatever · 2024-12-20T19:51:21Z

spec/specification.md

 - `portable`: A boolean (`true` / `false`) indicating if the DID is portable and
  thus can be renamed to change the Web location of the DID.
  - The value can **ONLY** be set to `true` in the first [[ref: log entry]], the initial version of the DID.
-  - If not explicitly set in the first [[ref: log entry]], it is set to `false`.
-  - Once the value has been set to `false`, it cannot be set back to `true`.
+  - If not explicitly set in the first [[ref: log entry]], it **MUST** be set to `false`.


I think this kind of reads funny as it's not really a MUST it is just a fact.

Suggested change

- If not explicitly set in the first [[ref: log entry]], it **MUST** be set to `false`.

- If not explicitly set in the first [[ref: log entry]], its value is set to `false`.

I think leaving it as being testable makes sense. Leaving this one.

brianorwhatever · 2024-12-20T19:52:32Z

spec/specification.md

@@ -610,12 +638,21 @@ properties are defined below.
  - The process for generating the hashes and additional details for using [[ref: pre-rotation]] are defined in the
    [Pre-Rotation Key Hash Generation and Verification](#pre-rotation-key-hash-generation-and-verification)
    section of this specification.
-  - Once the [[ref: parameter]] `nextKeyHashes` has been set, the [[ref: Key Pre-Rotation]] feature becomes active and the property **MUST** be present in any future [[ref: DID log entry]].
+  - If not explicitly set in the first [[ref: DID Log entry]], its value **MUST** be `null`.


Same as above

Suggested change

- If not explicitly set in the first [[ref: DID Log entry]], its value **MUST** be `null`.

- If not explicitly set in the first [[ref: DID Log entry]], its value is `null`.

brianorwhatever · 2024-12-20T19:53:22Z

spec/specification.md

+    pre-rotation. For additional details about turning off [[ref: pre-rotation]]
+    see the [pre-rotation](#pre-rotation-key-hash-generation-and-verification)
+    section of this specification.
+  - `nextKeyHashes` **MUST NOT** be set to an empty list `[]`.


Suggested change

- `nextKeyHashes` **MUST NOT** be set to an empty list `[]`.

- `nextKeyHashes` **MUST** have at least 1 entry and **MUST NOT** be set to an empty list `[]`.

Fixed in all three places.

brianorwhatever · 2024-12-20T19:53:50Z

spec/specification.md

-  be deactivated. See the [deactivate (revoke)](#deactivate-revoke) section of
-  this specification for more details.
+  - If the `witness` property is not set in the first [[ref: DID log entry]],
+    its value **MUST** be null.


Suggested change

its value **MUST** be null.

its value is null.

brianorwhatever · 2024-12-20T19:54:42Z

spec/specification.md

-  this specification for more details.
+  - If the `witness` property is not set in the first [[ref: DID log entry]],
+    its value **MUST** be null.
+  - `witness` **MUST NOT** be set to an empty list `[]`.


Suggested change

- `witness` **MUST NOT** be set to an empty list `[]`.

- `witness` **MUST** have at least 1 entry and **MUST NOT** be set to an empty list `[]`.

brianorwhatever · 2024-12-20T19:55:44Z

spec/specification.md

+  - If the `witness` property is not set in the first [[ref: DID log entry]],
+    its value **MUST** be null.
+  - `witness` **MUST NOT** be set to an empty list `[]`.
+- `deactivated`: A JSON boolean that **MUST** be initialized to `false` and


Suggested change

- `deactivated`: A JSON boolean that **MUST** be initialized to `false` and

- `deactivated`: A JSON boolean that is initialized to `false` and

brianorwhatever · 2024-12-20T19:55:57Z

spec/specification.md

+    its value **MUST** be null.
+  - `witness` **MUST NOT** be set to an empty list `[]`.
+- `deactivated`: A JSON boolean that **MUST** be initialized to `false` and
+  **SHOULD** be set to `true` when the DID is to be deactivated but remains


Suggested change

**SHOULD** be set to `true` when the DID is to be deactivated but remains

**MUST** be set to `true` when the DID is to be deactivated but remains

Left as is. In the deactivate section we left it. If a DID is "deactivated" by removing the JSONL file, for example, no need to set deactivate to true. I wanted to leave that open.

Signed-off-by: Stephen Curran <[email protected]>

Updates the pre-rotation and witnesses sections with feedback from th…

e9caa3f

…e first set of updates Signed-off-by: Stephen Curran <[email protected]>

swcurran requested review from a team December 11, 2024 23:54

swcurran added 2 commits December 13, 2024 11:54

Update to cover versionId, versionTime, versionNumber

33ed5f0

Signed-off-by: Stephen Curran <[email protected]>

Clarify deactivation with pre-rotation, update the changelog

dc4d9b7

Signed-off-by: Stephen Curran <[email protected]>

brianorwhatever reviewed Dec 18, 2024

View reviewed changes

spec/specification.md Outdated Show resolved Hide resolved

spec/specification.md Outdated Show resolved Hide resolved

spec/specification.md Outdated Show resolved Hide resolved

spec/specification.md Outdated Show resolved Hide resolved

spec/specification.md Outdated Show resolved Hide resolved

swcurran added 2 commits December 17, 2024 16:55

Fixing typos from review

9a65074

Signed-off-by: Stephen Curran <[email protected]>

Formalized that null can be used as well as empty list to indicate no…

e13e35b

… nextKeyHashes, updateKeys or witnesses Signed-off-by: Stephen Curran <[email protected]>

swcurran requested review from brianorwhatever and a team December 19, 2024 23:57

swcurran added 3 commits December 20, 2024 08:12

Merge branch 'main' into v05-changes

fa5c5d0

Clarify that array parameters must be initialized to null and deactiv…

eed6af2

…ated using null and define defaults for parameters Signed-off-by: Stephen Curran <[email protected]>

Merge branch 'v05-changes' of github.com:swcurran/didwebvh into v05-c…

a9321a0

…hanges

Made explicit that the array parameters MUST NOT be set to empty list

3fb9c54

Signed-off-by: Stephen Curran <[email protected]>

brianorwhatever previously approved these changes Dec 20, 2024

View reviewed changes

Fixed typos and added clarifications based on feedback

4f62d0d

Signed-off-by: Stephen Curran <[email protected]>

swcurran dismissed brianorwhatever’s stale review via 4f62d0d December 20, 2024 20:09

brianorwhatever approved these changes Dec 20, 2024

View reviewed changes

swcurran merged commit b9ec8d7 into decentralized-identity:main Dec 20, 2024
1 check passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Updates the pre-rotation and witnesses sections with feedback from the first set of updates #159

Updates the pre-rotation and witnesses sections with feedback from the first set of updates #159

swcurran commented Dec 11, 2024

brianorwhatever commented Dec 12, 2024

brianorwhatever commented Dec 19, 2024

andrewwhitehead commented Dec 19, 2024 •

edited

Loading

swcurran commented Dec 19, 2024

swcurran commented Dec 20, 2024

swcurran commented Dec 20, 2024

brianorwhatever left a comment

brianorwhatever Dec 20, 2024

swcurran Dec 20, 2024

brianorwhatever Dec 20, 2024

swcurran Dec 20, 2024

brianorwhatever Dec 20, 2024

swcurran Dec 20, 2024

brianorwhatever Dec 20, 2024

swcurran Dec 20, 2024

brianorwhatever Dec 20, 2024

swcurran Dec 20, 2024

brianorwhatever Dec 20, 2024

swcurran Dec 20, 2024

brianorwhatever Dec 20, 2024

swcurran Dec 20, 2024

brianorwhatever Dec 20, 2024

swcurran Dec 20, 2024

	- Once the value has been explicitly to `false` in a [[ref: DID log entry]], it MUST NOT be set back to `true`.
	- Once the value has been explicitly set to `false` in a [[ref: DID log entry]], it MUST NOT be set back to `true`.

	- If not explicitly set in the first [[ref: log entry]], it MUST be set to `false`.
	- If not explicitly set in the first [[ref: log entry]], its value is set to `false`.

	- `nextKeyHashes` MUST NOT be set to an empty list `[]`.
	- `nextKeyHashes` MUST have at least 1 entry and MUST NOT be set to an empty list `[]`.

	- `witness` MUST NOT be set to an empty list `[]`.
	- `witness` MUST have at least 1 entry and MUST NOT be set to an empty list `[]`.

	- `deactivated`: A JSON boolean that MUST be initialized to `false` and
	- `deactivated`: A JSON boolean that is initialized to `false` and

	SHOULD be set to `true` when the DID is to be deactivated but remains
	MUST be set to `true` when the DID is to be deactivated but remains

Updates the pre-rotation and witnesses sections with feedback from the first set of updates #159

Updates the pre-rotation and witnesses sections with feedback from the first set of updates #159

Conversation

swcurran commented Dec 11, 2024

brianorwhatever commented Dec 12, 2024

brianorwhatever commented Dec 19, 2024

andrewwhitehead commented Dec 19, 2024 • edited Loading

swcurran commented Dec 19, 2024

swcurran commented Dec 20, 2024

swcurran commented Dec 20, 2024

brianorwhatever 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

andrewwhitehead commented Dec 19, 2024 •

edited

Loading