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

feat: add open-attestation schema #57

Merged
merged 1 commit into from
Dec 19, 2019

Conversation

Nebulis
Copy link
Contributor

@Nebulis Nebulis commented Nov 21, 2019

MAJOR RELEASE REQUESTED

Schema v2

I've added a v2 schema, it's the same as Tradetrust schema with added certificateStore object for OpenCerts document (https://github.com/TradeTrust/tradetrust-schema/blob/master/schema/1.0/schema.json)

The goal is to provide types, It's not possible to issue a v1 document (to help on adoption of the new schema), however it's possible to validate the schema

Schema v3

I've added open attestation schema v3 following https://github.com/Open-Attestation/adr/blob/master/generalised_verification_method.md. The schema is purposely closed for extension => we can make it less strict without breaking change. List of noticeable things:

  • issuers is not an array anymore. Consequence, proof.value is not an array anymore
  • issuer.id is a new mandatory field which must be an URI (per w3c spec)
  • issuer.identityProof.subject has been remove because it's not needed anymore
  • @context has been added for w3c compliance to help consumer provide terminology. It's an optional field
  • id has been updated to follow w3c compliance. It's an optional uri about the subject of the VC
  • reference has been added and is equal to the previous id
  • type has been added as the type of the VC (w3c compliance)
  • validFrom has been added (w3c compliance)
  • validUntil has been added (w3c compliance). It's an optional field
  • attachments.type has been updated to follow w3c compliance. It refers to a valid evidence type.
  • attachments.mimeType has been added and is equal to the previous attachments.type

Types

Automatic generation of types from the schema using quicktype. Types are not committed and are generated post installation (automatically)

Usage example

import {v2} from '@govtechsg/open-attestation'
const document: SignedDocument<v2.OpenAttestationDocument>

Improvement for SignedDocument and SchematisedDocument types. data properties are automatically transformed to string using when the SignedDocument type.
Improvement for getData function typings.

APIs changes

  • BREAKING CHANGE for issuedDocument and issueDocuments:
    • second parameter schema has been removed.
    • an optional second parameter has been added to provide options (passing down a link to an external custom schema the document has been built on)
    • function only checks for the validity against open-attestation v2 schema. The responsibility of the validity of a document against an external schema is left to the consumer.
  • Schema validation throw a custom error passing down the errors encountered.

@Nebulis Nebulis force-pushed the feat/add-open-attestation-schema branch 2 times, most recently from b8379aa to ce8f9fa Compare November 28, 2019 07:55
@Nebulis Nebulis changed the title feat: add open-attestation schema and allow external custom schema feat: add open-attestation schema Nov 28, 2019
@Nebulis Nebulis force-pushed the feat/add-open-attestation-schema branch 3 times, most recently from ded788d to 4fea34a Compare December 3, 2019 06:31
.eslintignore Show resolved Hide resolved
.eslintrc.json Show resolved Hide resolved
src/schema/1.0/schema.json Outdated Show resolved Hide resolved
src/schema/1.0/schema.json Outdated Show resolved Hide resolved
src/schema/1.0/schema.json Outdated Show resolved Hide resolved
@@ -73,12 +53,15 @@ export const obfuscateData = (_data: any, fields: string[] | string) => {
};
};

export const obfuscateDocument = (document: SignedDocument, fields: string[] | string): SignedDocument => {
// TODO shouldn't document be any and shouldn't we validate first it's a valid document ?
Copy link
Contributor

Choose a reason for hiding this comment

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

We should validate the schema first, but document shouldnt be any (so we can make use of it's type later), unless you want to cast it after it has been validated.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it depends if we want to trust consumer or not

  • if people use typescript then no problems
  • if they dont they can pass any value, it may fail without explanation

We should at least leave document as SignedDocument for people using TS, but extra validation would be nice also IMHO)

Copy link
Contributor

Choose a reason for hiding this comment

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

My concern is that whether people use TS or not, there might be type issue during runtime. Especially when we download and upload document all the time now.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes that's what I mean by extra validation would be nice

=> #59

In the meantime, keep the type correct, better for TS :)

@Nebulis Nebulis force-pushed the feat/add-open-attestation-schema branch 2 times, most recently from 145c32b to 04e697b Compare December 6, 2019 10:23
src/__generated__/,gitignore Outdated Show resolved Hide resolved
src/schema/2.0/w3c.test.ts Outdated Show resolved Hide resolved
src/schema/2.0/w3c.ts Outdated Show resolved Hide resolved
src/schema/schema.ts Show resolved Hide resolved
src/index.ts Outdated Show resolved Hide resolved
src/index.ts Outdated Show resolved Hide resolved
src/schema/2.0/w3c.ts Outdated Show resolved Hide resolved
@yehjxraymond
Copy link
Contributor

yehjxraymond commented Dec 13, 2019

Few other comments:

  1. Should we also export keccek256 from js-sha3? Will be nice as a utils.
  2. Rj suggested syncing the version with OC, so that this will start with v2 and then the new one will be v3. If not we will always need to make a mental note change the version number when referring to either OA or OC.
  3. I see an issue when not allowing people to issue using the OC v2 (in this case v1) schema. Once upgraded, there will be no support to that version at all. Like those using OCv2 can no longer generate a valid document from the CLI.
  4. Should OA also perform validation on external schema. So I can pass in my own json schema and it checks that it conforms to the base OA schema and the one I provide, instead of just the name of the external schema. This way, I dont have to implement AJV again when using this package.

On W3C compatibility:

  1. The transformer should map validFrom to issuanceDate.
  2. The transformer should map credentialSubject.reference to credentialSubject.id
  3. Separately should we contribute to W3C a test suite that test a given document against their test suite.
  4. Separately should we add in a generator that takes in a unsigned W3C VC document (Example) and then generate a valid OA document (adding the salt and shit) and the corresponding VC document? This way we can submit to W3C as one of the valid implementation (https://w3c.github.io/vc-test-suite/implementations/)

@yehjxraymond
Copy link
Contributor

Also, since this is a breaking change, am wondering if we want to fix the naming of the function for sign? It's acting more like envelop or wrap.

@Nebulis
Copy link
Contributor Author

Nebulis commented Dec 13, 2019

SignedDocument => Envelope
sign => wrap

@Nebulis
Copy link
Contributor Author

Nebulis commented Dec 13, 2019

Few other comments:

  1. Should we also export keccek257 from js-sha3? Will be nice as a utils.

Can export if you want

  1. Rj suggested syncing the version with OC, so that this will start with v2 and then the new one will be v3. If not we will always need to make a mental note change the version number when referring to either OA or OC.

skip v1 and start with v2 and upgraded to v3

  1. I see an issue when not allowing people to issue using the OC v2 (in this case v1) schema. Once upgraded, there will be no support to that version at all. Like those using OCv2 can no longer generate a valid document from the CLI.

Yes, I thought about that, but it will be my problem soon so I might come back on this later without breaking change on the lib

  1. Should OA also perform validation on external schema. So I can pass in my own json schema and it checks that it conforms to the base OA schema and the one I provide, instead of just the name of the external schema. This way, I dont have to implement AJV again when using this package.

I dont fully agree on this, I would rather than oa do something an do it well, than doing too many things, adding in the enhancement suggestion => #60

On W3C compatibility:

  1. The transformer should map validFrom to issuanceDate.

According to w3c doc, issuanceDate might be renamed to validFrom (same for validUntil). Keeping it like this for the moment

  1. The transformer should map credentialSubject.reference to credentialSubject.id

Will fix

  1. Separately should we contribute to W3C a test suite that test a given document against their test suite.

#61

  1. Separately should we add in a generator that takes in a unsigned W3C VC document (Example ) and then generate a valid OA document (adding the salt and shit) and the corresponding VC document? This way we can submit to W3C as one of the valid implementation (w3c.github.io/vc-test-suite/implementations)

#62

@Nebulis
Copy link
Contributor Author

Nebulis commented Dec 13, 2019

Changed $template => template

src/index.ts Outdated
};

export const validateSchema = (document: WrappedDocument): boolean => {
return validate(document, getSchema(`open-attestation/${document?.version || "2.0"}`)).length === 0;
Copy link
Contributor

Choose a reason for hiding this comment

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

actually I'm thinking instead of storing 3.0 in the version, does it make sense to store the version as open-attestation/3.0 so that it is unambiguous which version (OA schema or their subschema)? Dont hit me.

src/index.ts Outdated
export { utils, isSchemaValidationError };
export * from "./@types/document";
export * from "./schema/3.0/w3c";
export { keccak256 } from "js-sha3";
Copy link
Contributor

Choose a reason for hiding this comment

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

was thinking of it more from utils. if not the api looks a bit clunky.

recipient: {
name: "Recipient Name"
},
reference: "SERIAL_NUMBER_123",
Copy link
Contributor

Choose a reason for hiding this comment

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

would you like to fix this in this pr or can do in next one.

@yehjxraymond
Copy link
Contributor

Remember to release this as a breaking change in the commit message. Might want to do a manual squash?

https://github.com/semantic-release/semantic-release#commit-message-format

Copy link
Contributor

@yehjxraymond yehjxraymond left a comment

Choose a reason for hiding this comment

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

lgtm

@Nebulis Nebulis force-pushed the feat/add-open-attestation-schema branch 4 times, most recently from c390b38 to 327f04e Compare December 19, 2019 03:48
BREAKING CHANGE: New schema to be w3c compliant

Changes are:
- issuers is not an array anymore. Consequence, proof.value is not an array anymore
- issuer.id is a new mandatory field which must be an URI
- issuer.identityProof.subject has been remove because it's not needed anymore
- @context has been added for w3c compliance to help consumer provide terminology. It's an optional field
- id has been updated to follow w3c compliance. It's an optional uri about the subject of the VC
- reference has been added and is equal to the previous id
- type has been added as the type of the VC (w3c compliance)
- validFrom has been added (w3c compliance)
- validUntil has been added (w3c compliance). It's an optional field
- attachments.type has been updated to follow w3c compliance. It refers to a valid evidence type.
- attachments.mimeType has been added and is equal to the previous attachments.type
- rename $template to template

Adding v2 schema which is a simple version of schema used in tradetrust
and opencerts

Generate types from schema. Types are exported

Update wording:
- sign => wrap
- SignedDocument => WrappedDocument
@Nebulis Nebulis force-pushed the feat/add-open-attestation-schema branch from 327f04e to b9d307d Compare December 19, 2019 04:12
@Nebulis Nebulis merged commit f553343 into master Dec 19, 2019
@john-dot-oa
Copy link

🎉 This PR is included in version 3.0.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants