Closed msporny closed 10 months ago
This PR doesn't do the job. Putting a question mark in the references doesn't remove the references. Please remove them entirety, as agreed to at TPAC.
That is not what was agreed to at TPAC. The agreement was to make the references non-normative, which this PR does. Please review the minutes:
https://github.com/w3c/vc-data-integrity/issues/191#issuecomment-1720718132
Specifically:
Brent Zundel: appreciate that there's a feature at risk tag. is the intention, should multibase not be standardized by the time this moves from CR to REC, would the current link be moved to a non normative reference? Sebastian Crane: if that satisfies Mike I am happy to make that compromise myself and make it an informative reference. we are waiting for the spec to catch up. Sebastian Crane: (to summarize) - we are going to, in the cryptosuites, to have a specific encoding, for the spec text we are using a format compatible with multibase, so that if it becomes a standard we will use it normatively. we won't publish a normative ref if it's not ready by REC.
There was no opposition to that summary during the meeting. I do expect there to be opposition to removing all non-normative references to multiformats from the specification. If you would like to make that suggestion, please raise a PR (though I expect it to have a large number of downvotes since it weakens the specification by not pointing to useful, stable documentation that exists today in a non-normative fashion).
Sorry but I do not understand how adding a ? removes the normative dependency
It is a ReSpec shortcut: https://respec.org/docs/#normative-vs-informative-references
It was my understanding from the discussion at TPAC was that the resolution was going to be that the references were going to be removed entirely until such time as there is a multibase standard to reference.
As you can see from the quoted text above, that understanding was incorrect. The agreement was to make the reference non-normative, which this PR does.
And that it would be replaced by in-spec statements that these values are strings beginning with either "u" or "z" and followed respectively by either a base64url-encoded value or a base58btc-encoded value.
That spec text has existed in the DI spec for some time now:
https://w3c.github.io/vc-data-integrity/#resource-integrity
Promote these to be normative and ditch the reference to the individual Internet Draft.
The references to the I-Ds, which are non-normative now, are at the same level as linking to a Wikipedia article, or some content on the web, that is, they are informative in nature and helpful because they explain Multihash and Multibase in more depth than we do in the current specification. Removing access to documentation that is helpful to developers that want to learn more about how each technology works would be harmful to developers trying to understand more about each technology.
Replacing the normative reference with an informative reference effectively leaves proofValue
undefined. That's not OK, since it's a required field. This PR needs to be updated to normatively define the contents of the proofValue
field.
I think the proofValue
issue can be addressed without a normative reference to specific text encodings.
Something like:
"Based on the @type
and cryptoSuite
used by the proof, the specification defining the crypto suite can define the semantics of proofValue
, the JSON value MUST be a string."
Then site base58btc normatively as the string encoding, but just copy the alphabet used in to the spec.
[@selfissued] This PR needs to be updated to normatively define the contents of the
proofValue
field
I'm sure an explicit change suggestion would be welcome, and more likely to satisfy you than most if not all alternatives, though it may not be 100% accepted by the rest of the WG, who may yet make comments or suggest additional changes thereon (e.g., if your change request includes fully removing the informative references to [[?MULTIBASE]]
and/or [[?MULTIHASH]]
which have already replaced the normative references to [[MULTIBASE]]
and [[MULTIHASH]]
, I expect the informative references will be restored).
@selfissued wrote:
Replacing the normative reference with an informative reference effectively leaves proofValue undefined. That's not OK, since it's a required field. This PR needs to be updated to normatively define the contents of the proofValue field.
Fixed in dd44d6b1513b96ae72c96ac245f65507290a301e.
"Based on the @type and cryptoSuite used by the proof, the specification defining the crypto suite can define the semantics of proofValue, the JSON value MUST be a string."
If we use a "string" datatype, it creates the following two issues:
proofValue
is a multibase header or just so happens to be a value that starts with something that looks like a multibase value.proofValue
to each cryptosuite, so we'd end up effectively repeating the same text here in every cryptosuite.Then site base58btc normatively as the string encoding, but just copy the alphabet used in to the spec.
That alphabet exists in the spec today: https://w3c.github.io/vc-data-integrity/#example-an-integrity-protected-image-that-is-associated-with-an-object
... but I moved it into its own section to make it more clear about the normative definition based on what @selfissued was asking for. That should address @TallTed's concern about SHOULD vs. MUST (it's a MUST now because the spec provides the normative values).
I believe the latest changes should address each of your concerns, requesting a re-review from each of you.
The issue was discussed in a meeting on 2023-09-26
The issue was discussed in a meeting on 2023-09-27
With the current setup, §2.3.1.2 is a normative section that does not have normative references. I do not think that is o.k. I believe that this section must be flagged as informative or marked as "at risk". The latter may force us to remove the section if Multibase does not receive a normative stamp, which may harm already deployed software. I would therefore avoid that, and suggest marking the section as informative for now, making it clear (in a note) that if the IETF stamp is there, then this become normative (provided we have enough test results). Because
publicKeyJwk
is also a (normative) possibility to express the verification method, we do not really have a normative problem expressing that.
Having read the meeting minutes of the last call (see also https://github.com/w3c/vc-data-integrity/pull/196#issuecomment-1738307954) I realize I misread the text. The intention of the spec is to normatively define the portion of MULTIBASE that is used by us. That is fine. However, the normative definition should then not refer to the MULTIBASE document; the text should stand by itself. It may (should) refer to it informatively to show the background for reasons of compatibility, and it is also o.k. to refer to do it in the initial note. At the moment, it is not clear how the 'z' and 'u' characters should be used, for example (without going back to the MULTIBASE document).
My proposal is to
(The same comments apply to Multihash, b.t.w.)
@iherman are you saying this URL needs to change in the v2 context ^
@selfissued @OR13 I have made the specific change requested on the call yesterday in this commit: https://github.com/w3c/vc-data-integrity/pull/196/commits/7bb2e688e4491a06f316e089877bee1799f84df5
I have also gone through and removed any other reference to [[?MULTIBASE]] or [[?MULTICODEC]] in normative statements as well to address your concerns in case they exist anywhere else in the specification:
b9d5d96dfb27d86b43063f56c49cdd181d3b212f
I also took @selfissued's advice and modified the issue marker to note that compatibility between what we're defining in the specification, the Multiformats community, and the I-Ds at IETF (which might go standards track in a future Multiformats WG) are a desired outcome.
@iherman I have also attempted to address your concerns in b9d5d96dfb27d86b43063f56c49cdd181d3b212f:
put there a complete, comprehensive definition for the features we use
I think this is done as of the latest commit?
give background information in the initial note, with a non-normative reference to Multibase
I think this is done as of the latest commit.
the feature should then NOT be at risk! The exact text may change, but the feature, as used in DI is not at risk at all...
Yes, agree.
the text in Datatype specification must also be changed: the datatype is defined via §2.3.1.2 and not via the Multibase spec (but that can also change later)
I have updated that text in b9d5d96dfb27d86b43063f56c49cdd181d3b212f
I think this resolves all issues raised on the call yesterday. Requesting re-review from @OR13, @selfissued, and @iherman .
- https://pr-preview.s3.amazonaws.com/w3c/vc-data-integrity/pull/196.html#multibase
- https://github.com/w3c/vc-data-model/blob/main/contexts/credentials/v2#L313
@iherman are you saying this URL needs to change in the v2 context ^
I do not understand the question, @OR13.
@msporny,
put there a complete, comprehensive definition for the features we use
I think this is done as of the latest commit?
I tried to look at this, and I do not see any change. What I see (I downloaded your branch to my local machine) is:
I try to understand the table without looking at the IETF document, and I do not understand. What does 'u' and 'z' mean, how they are used, how does the multibase value reall looks like? It is not said.
give background information in the initial note, with a non-normative reference to Multibase
I think this is done as of the latest commit.
You mean in the introductory issue box? +1 to that.
the feature should then NOT be at risk! The exact text may change, but the feature, as used in DI is not at risk at all...
Yes, agree.
But the issue box still says "Feature at risk". This should be removed (also in the multihash section)
the text in Datatype specification must also be changed: the datatype is defined via §2.3.1.2 and not via the Multibase spec (but that can also change later)
I have updated that text in b9d5d96
Yes, that looks good to me.
I think this resolves all issues raised on the call yesterday. Requesting re-review from @OR13, @selfissued, and @iherman .
- https://pr-preview.s3.amazonaws.com/w3c/vc-data-integrity/pull/196.html#multibase
- https://github.com/w3c/vc-data-model/blob/main/contexts/credentials/v2#L313
@iherman are you saying this URL needs to change in the v2 context ^
I do not understand the question, @OR13.
@iherman proofValue
is normatively defined via the JSON-LD context to be:
"@type": "https://w3id.org/security#multibase"
W3C is not defining multibase as an RDF type, W3ID.org (a separate legal entity with some overlap in governance, but not equivalent to this working group or W3C) is defining multibase as an RDF type... @iherman I am asking you if this is a problem or not based on the other changes being made in this PR, from the perspective of W3C.
- https://pr-preview.s3.amazonaws.com/w3c/vc-data-integrity/pull/196.html#multibase
- https://github.com/w3c/vc-data-model/blob/main/contexts/credentials/v2#L313
@iherman are you saying this URL needs to change in the v2 context ^
I do not understand the question, @OR13.
@iherman
proofValue
is normatively defined via the JSON-LD context to be:"@type": "https://w3id.org/security#multibase"
W3C is not defining multibase as an RDF type, W3ID.org (a separate legal entity with some overlap in governance, but not equivalent to this working group or W3C) is defining multibase as an RDF type... @iherman I am asking you if this is a problem or not based on the other changes being made in this PR, from the perspective of W3C.
The definition of the multibase RDF Type is defined in the DI spec of W3C. It is an entry in the security vocabulary and, so far, the WG agreement is that this vocabulary's URI is https://w3id.org/security
. There is nothing special about multibase in this respect. Put it another way, there is no difference between that datatype in the vocabulary and, say, the proof
property. This PR has no effect on these whatsoever.
the definition of the multibase RDF Type is defined in the DI spec of W3C.
@OR13 , I forgot to make a reference to where this happens: look at §2.9.3 in the branch of this PR.
@iherman wrote:
I try to understand the table without looking at the IETF document, and I do not understand. What does 'u' and 'z' mean, how they are used, how does the multibase value reall looks like? It is not said.
This has been fixed in 2651487fc6fd4d29fa7d805d43f1b873ecadc42a and 02121e37066827d60bcb902c1dffa115484d4383.
You mean in the introductory issue box? +1 to that.
Yes.
But the issue box still says "Feature at risk". This should be removed (also in the multihash section)
Fixed in 55638820b0aace3b8c8fc4ef29ea4339c886c47f.
Does that address all of your concerns, @iherman ?
The latest changes answered all my concerns on this PR. To avoid the hassle of yet another PR, I would prefer to update the specification text for multihash in this PR, too, but I leave this to the editors.
That was done in https://github.com/w3c/vc-data-integrity/commit/02121e37066827d60bcb902c1dffa115484d4383
The issue was discussed in a meeting on 2023-10-04
@selfissued re-ping to re-review... ideally, you approve before we merge this. It's all green on reviews so far.
The issue was discussed in a meeting on 2023-10-11
The issue was discussed in a meeting on 2023-10-11
Normative, multiple reviews, changes requested and made, no objections, merging.
This PR has been raised to address issue #191, which requested the removal of Multibase from the list of normative dependencies for the vc-data-integrity specification. I went ahead and removed Multihash as well from the list of normative dependencies as I expect the request applies to that specification as well.
Preview | Diff