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

Remove DataIntegrityProof from v2 context #1091

Closed
wants to merge 1 commit into from

Conversation

OR13
Copy link
Contributor

@OR13 OR13 commented Apr 18, 2023

Addresses part of #1090


Preview | Diff

"cryptosuite": "https://w3id.org/security#cryptosuite",
"proofValue": {
"@id": "https://w3id.org/security#proofValue",
"@type": "https://w3id.org/security#multibase"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Related PR in Data Integrity

w3c/vc-data-integrity#92

Copy link
Contributor

@dlongley dlongley 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 doing this since we have said that it makes developer's lives easier for many common cases in a variety of other discussions.

If we don't want to do this here, perhaps we should also reconsider, for example, separating out the core context into two: one that has @vocab and one that doesn't -- to avoid impacting those that do not want to deal with its usage.

I think we should figure out which we're prioritizing: ideological / abstraction boundary purity or ease of common use. It seems these things are sometimes in conflict with one another.

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 for the following reasons:

  • We already had consensus to merge this a while ago.
  • We changed the way we did Data Integrity, based on @OR13's concerns, to make it easier, which resulted in this definition in the core context, so we could move toward having a single context.
  • There is very little cost (just a few extra bytes in a context that's perma-cached) to having this in the core context, and then only to those that use JSON-LD.

Overall, this feels like an appeal to "academic purity", which overrides the practical benefits of having this in here and it would make those using Data Integrity have to do unnecessary, extra work for no practical benefit.

@OR13
Copy link
Contributor Author

OR13 commented Apr 19, 2023

My proposed changes result in a consistent experience for data integrity implementers, and they save cost for non data integrity implementers.

v1 and v1.1

"@context": [
    "https://www.w3.org/2018/credentials/v1",
    "https://w3id.org/security/data-integrity/v1"
  ],

v2

"@context": [
    "https://www.w3.org/ns/credentials/v2",
    "https://w3id.org/security/data-integrity/v1"
  ],

using data integrity to secure arbitrary json

"@context": [
    {"title": "https://schema.org/title"},
    "https://w3id.org/security/data-integrity/v1"
]

Copy link
Member

@TallTed TallTed left a comment

Choose a reason for hiding this comment

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

I'm with @msporny [see 1] and @dlongley [see 2]

@OR13
Copy link
Contributor Author

OR13 commented Apr 25, 2023

Related issue: w3c/vc-data-integrity#93 (review)

@OR13
Copy link
Contributor Author

OR13 commented Apr 25, 2023

Also w3c/vc-data-integrity#92

@decentralgabe
Copy link
Contributor

I don't see the harm in keeping properties in the context; though I can see an argument made for the context matching required properties from the spec.

Copy link
Contributor

@mprorock mprorock left a comment

Choose a reason for hiding this comment

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

This section of the context (or an equivalent) seems like is should be in the data integrity context, not in the core context

Approving, contingent on the data integrity side having coverage for this in their context

Copy link
Contributor

@selfissued selfissued left a comment

Choose a reason for hiding this comment

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

Since DataIntegrityProof isn't needed for many VCs, it makes sense to remove it. In general, we should be aligning the base context with the elements that are intended for use in all VCs.

@m00sey
Copy link
Contributor

m00sey commented May 5, 2023

-1 agree with @dlongley , @TallTed , @msporny

Copy link
Contributor

@decentralgabe decentralgabe left a comment

Choose a reason for hiding this comment

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

agree with @selfissued's comments

@msporny
Copy link
Member

msporny commented May 18, 2023

Since DataIntegrityProof isn't needed for many VCs

Please provide proof of this statement given that @OR13, @mprorock, @decentralgabe, and @selfissued's entire argument for this PR hinges on it.

While it might be true that VCs that your organizations are issuing don't require DataIntegrityProof, it's also true that there are other organizations issuing VCs that plan to make significant use of DataIntegrityProof.

As was mentioned on the call today, this property is included (as well as name and description) because it helps a subset of developers and it is not a great burden to developers that don't plan to use DataIntegrityProof.

In an attempt to put this in perspective, the same argument that argues for the removal of DataIntegrityProof applies to credentialSchema. We could assert that there is no demonstration of widespread usage of the feature and that only a very small subset of the community is going to find the property useful; therefore, we should remove it from the core context. This is, of course, a questionable argument given that the cost for developers that are not going to use credentialSchema is quite low, and the benefit for those using it is high. We should optimize for things that help developers.

If the WG is going to argue for the removal of DataIntegrityProof on these grounds, we should be consistent and argue for the removal of credentialSchema as well. Doing so, of course, would be inadvisable.

Copy link
Contributor

@peacekeeper peacekeeper left a comment

Choose a reason for hiding this comment

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

Considering that the base media type is application/vc+ld+json and that proof is a core property, I think it also makes sense to keep DataIntegrityProof in the v2 context, in order to be able to easily add proofs to a JSON-LD VC.

Removing it I think could potentially also lead to interoperability problems between Data Integrity proofs and VC-JWT, since one type of proof would require an additional context and the other one wouldn't.

@iherman
Copy link
Member

iherman commented May 22, 2023

As someone put it, in this sounds like an "academic purity". I would say that even in terms of academic purity it does not hold, because the @context file does not define anything. It is a tool to make the life of developers, who care about the Linked Data aspect of VC-s easier, whilst it is totally opaque for those who care of a pure JSON only.

To put things to extremes, "academic purity" might be to say: do not use an external @context file at all. Instead, along the lines proposed in #1091 (comment), use

{
    "@context" : {
        "title": "https://schema.org/title",
        "issuer" :  {
            "@id" : "https://www.w3.org/2018/credentials#issuer",
            "@type": "@id",
        },
        "DataIntegrityProof" : "https://w3id.org/security#DataIntegrityProof",
         
        "@vocab": "https://www.w3.org/ns/credentials/issuer-dependent#"
    }
}

requiring all developers to list all terms they use in a specific VC instance. Linked Data-wise, that would be equivalent to the usage of the external @context file. It would also make the life of developers miserable. At best, each developer would create his/her own @context file, at worst, they would drop JSON-LD altogether.

-1 to this proposal.

}
}
},
"cryptosuite": "https://w3id.org/security#cryptosuite",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Keeping this in the v2 context, will allow anyone to create new crypto suites with no W3C review after the TR is published... here are some examples:

{
    "type": "DataIntegrityProof",
    "cryptosuite": "bitcoin-2023",
    "created": "2020-11-05T19:23:24Z",
    "verificationMethod": "https://ldi.example/issuer/1#z6MkjLrk3gKS2nnkeWcmcxi
      ZPGskmesDpuwRBorgHxUXfxnG",
    "proofPurpose": "assertionMethod",
    "proofValue": "z4oey5q2M3XKaxup3tmzN4DRFTLVqpLMweBrSxMY2xHX5XTYVQeVbY8nQA
      VHMrXFkXJpmEcqdoDwLWxaqA3Q1geV6"
  }
{
    "type": "DataIntegrityProof",
    "cryptosuite": "ethereum-2023",
    "created": "2020-11-05T19:23:24Z",
    "verificationMethod": "https://ldi.example/issuer/1#z6MkjLrk3gKS2nnkeWcmcxi
      ZPGskmesDpuwRBorgHxUXfxnG",
    "proofPurpose": "assertionMethod",
    "proofValue": "z4oey5q2M3XKaxup3tmzN4DRFTLVqpLMweBrSxMY2xHX5XTYVQeVbY8nQA
      VHMrXFkXJpmEcqdoDwLWxaqA3Q1geV6"
  }
{
    "type": "DataIntegrityProof",
    "cryptosuite": "ecdsa-p521",
    "created": "2020-11-05T19:23:24Z",
    "verificationMethod": "https://ldi.example/issuer/1#z6MkjLrk3gKS2nnkeWcmcxi
      ZPGskmesDpuwRBorgHxUXfxnG",
    "proofPurpose": "assertionMethod",
    "proofValue": "z4oey5q2M3XKaxup3tmzN4DRFTLVqpLMweBrSxMY2xHX5XTYVQeVbY8nQA
      VHMrXFkXJpmEcqdoDwLWxaqA3Q1geV6"
  }

Similar to all predicate extension points in the core data model, cryptosuite is an extension point, unlike other predicates, this one is not bound to any RDF types.

@iherman
Copy link
Member

iherman commented May 25, 2023

The issue was discussed in a meeting on 2023-05-24

  • no resolutions were taken
View the transcript

1.6. Add JSON Web Token Examples (pr vc-status-list-2021#60)

See github pull request vc-status-list-2021#60.

Orie Steele: We seem to have a difference of opinion regarding adding examples to the specification. And I would like to merge the examples as-is. I don't agree with the perspective that every example in our spec needs multiple context values.
… I don't agree with that and in light of bundling the Data Integrity context.
… This adds informative examples, can we merge this, please, thank you.

Greg Berstein: Please!

Manu Sporny: -1 to merge until the WG discusses it. With all the examples it keeps coming up and we need to have a consistent way that we're doing examples and figure out what it is so the editors can apply that uniformly across all the specs.

Phillip Long: is this a special topics call?

Kristina Yasuda: I'm up for spending 5-10 minutes on this call on this.

Orie Steele: lets remove the DataIntegrityProof RDF class from the core context... and make the examples include multiple contexts.

Manu Sporny: I don't know if we'll get through it in 5 minutes. The proposal is to just use the example in the core spec.
… Use it as an example across all the specs that need to use an example. That way we're just using an example that we've agreed to use across all the specs and we don't need to do variations of that in every other specification.

See github pull request vc-data-model#1091.

Kristina Yasuda: Are you fundamentally not ok with the example, Manu?

Manu Sporny: Yes.

Kristina Yasuda: Anyone else who has strong opinions about what examples we use?

Orie Steele: All the examples conformant to the core data model.

Michael Prorock: My question is along the same line... what precisely is wrong with the examples?
… At least by my read and going and running the example code it all checks out fine.

Manu Sporny: One of the things is that we have a suspension false entry in here, I don't know where that property came from, I think Orie said that's an artifact in the PR and it's not in the data model.
… There's a concern over the use of the undefined / issuer-independent context / vocab.
… Now we're using the issuer-dependent terms in the example.
… There are a number of people that said in the other PR that's not a good practice.
… We've had people say that they would rather the examples use the examples context instead of the issuer-defined one.
… I don't know why we're doing in status list when core does something else.

Dave Longley: A lot of developers will come along and look at examples to get things started, and there are such things as good examples and bad examples.

Orie Steele: IMO a good example is one that uses the minimal required fields.
it is bad form to include optional stuff in example, and bad form to include DataIntegrityProof in the core v2 context.

Dave Longley: You can create examples that are bad examples, which would be conformant, but wouldn't look good. It's not enough to say "We created an example and it validates so it's fine." If we get people started off on he wrong foot, or encouraging market failures, we're making mistakes. We should not be flippant about the examples we put in the spec. They're important, developers will look at these examples and maybe not the text.
… Saying "this verifies" is not enough, these are entry points into the spec and possibly exit points.

Kristina Yasuda: Sounds like we're leaving this issue open for now.
… Anything on the BBS cryptosuite stuff?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
discuss DO NOT MERGE PR contains something that should not be merged.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants