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

Conformance feedback from BCGov - DCC Extension - BCMinesActPermitCredential #184

Open
PatStLouis opened this issue Dec 13, 2024 · 6 comments

Comments

@PatStLouis
Copy link

After reviewing our conformance results, I'm not confident that this test suite is in a suitable position to be decisive on UNTP conformance just yet, at least for the Digital Conformity Credential.

Here are the major points of contention I have.

  • Preventing any form of extension
    • The current schema testing has constants for the context and all types withing the data model. This is an anti-pattern to extensibility, which is one of the fundamental concept of UNTP.
  • Type validation too strict
    • For some of the nested objects, there is too strict validation on the types set. As an example, I can't set an "issuingParty" to a type of "Party", it must be set to "Identifier".
  • Inconsistent date values
    • The VCDM has a specific datetime format. Dates can be complicated, I would suggest to align with this format instead of requiring a different format for other dates within the DCC.
  • Required ID fields
    • The test suite expects all objects to have an id field. In our case, we simply do not have these values for all objects. For example, we do not have a url for our permits. We do however, have a url for our governance documents and legal acts. I wouldn't make id required for all fields, otherwise there needs to be guidance for implementers that do not have values for these fields.

This would be a first round of observations/topic I would like to raise. Allowing extensibility is the most critical thing to address.

@absoludity
Copy link
Contributor

absoludity commented Jan 20, 2025

Thanks for this feedback @PatStLouis . I'm new to the project (with gosource, met you briefly on a meeting last week), and am looking at ensuring the schemas used in the next UNTP release are extensible.

I've just started looking at the @context today and after reading the previous discussion as well as checking the specs, have suggested on slack (as you'll see - you're on the thread) that we use something like:

"@context": {
  "type": "array",
  "prefixItems": [
  {
    "type": "string",
    "const": "https://www.w3.org/ns/credentials/v2"
  },
  {
    "type": "string",
    "const": "https://test.uncefact.org/vocabulary/untp/dpp/0.5.0/"
  }
],

to ensure that we both:

  1. meet the VC-DM spec of the first item being "https://www.w3.org/ns/credentials/v2" [1], as well as
  2. propose following suite for DPP (and DCC etc.) by requiring the 2nd item in the context to be the related untp context.

That said: I think it'll get tricky to follow if we have conversations either on slack or long-running GH discussions, so am proposing that I'll collect what I think are all the extension issues raised with their proposed solutions in a google doc or similar (together with a draft test-untp PR where I can highlight and test the changes). in a draft PR of the test-untp repo where we can discuss and resolve each in context on the one PR, if that works for you? Like this https://github.com/absoludity/tests-untp/pull/1/files as a beginning. Let me know if you prefer a google doc (I just think it's easier to discuss them where we can test the results also as we go etc.).

Once we're happy with the changes, we'll pass them to Alastair.

[1] The VC-DM Schema isn't currently itself validating this (but rather just "contains"), so I've created an upstream PR at w3c/vc-data-model#1586

@PatStLouis
Copy link
Author

@absoludity I think its a great PR and will ensure the vcdm schema is closer to the spec. Its mandatory that the context array is ordered for proper jsonld (LinkedData) processing. A json LD processor will go through the ordered contexts and as terms are being defined/protected, it will prevent subsequent context from redefining some terms. This is important so lets say a permitNumber doesn't get redefined as a reportDocumentNumber. The contexts should follow the extensibility logic, so UNTP being an extension of the Verifiable Credential Data Model, any UNTP context uri would come second. And then if an implementer wants to create something such as a mining permit extension of untp, they would add their context as a third entry, etc etc. I think what you are proposing reflects this. It could go the extra mile and have:

...
      "minItems": 2,
      "uniqueItems": true
...

This would be a first step in ensuring the UNTP schemas allow for proper extensions.

@absoludity
Copy link
Contributor

absoludity commented Jan 20, 2025

Great, and yes, I did include those (minItems and uniqueItems) on the upstream proposal for the VCDM Schema (which has one two (thanks ;) ) approvals), and will update my test-untp PR too.

Ok, I'll move forward and continue documenting the changes with proposed fixes in the PR. Thanks.

@absoludity
Copy link
Contributor

I've split the second point, the issue about issuingParty to its own issue #201 as, in addition to the extensible JSON Schema type checking, it needs a change to the model, I believe.

@absoludity
Copy link
Contributor

OK, the diff with explanations of the required changes in the JSON Schema artifacts to fix two of the above four points is ready for you to take a look at absoludity#1 . If you're happy with that, I'll provide that to Alastair to make it happen. Thanks Patrick.

@absoludity
Copy link
Contributor

Hi @PatStLouis , just regarding the required ID fields below:

Required ID fields

* The test suite expects all objects to have an id field. In our case, we simply do not have these values for all objects. For example, we do not have a url for our permits. We do however, have a url for our governance documents and legal acts. I wouldn't make id required for all fields, otherwise there needs to be guidance for implementers that do not have values for these fields.

I just want to check that you're aware that these are all uri (not url) data, and can, for example, use values such as permit:com.example.mycompany.permits.1234 ? That is, they don't need to be resource locations (though great if they are), they can just be identifiers that fit the uri format. If that's the case, is it still an issue with these being required? I'd think you would want a unique identifier for a permit, for example (scoped to the company or similar).

JFYI, I've included this in the proposed model changes for the next minor UNTP version, not yet approved, but viewable at

https://github.com/absoludity/spec-untp/blob/add-0.6.0-reasoning/docs/untp-data-model/changes-v0.6.0.md#optional-id-urls-required-on-all-models

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

No branches or pull requests

2 participants