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

Adding builder for VerifiableCredential #125

Closed
wants to merge 5 commits into from
Closed

Conversation

nitro-neal
Copy link
Contributor

Added VerifiableCredential builder along with unit tests.

The default constructor sets only things that are not optional:

    this.verifiableCredential = {
      '@context'        : contexts as ['https://www.w3.org/2018/credentials/v1', ...string[]],
      credentialSubject : credentialSubject,
      issuer            : issuer,
      type              : types,
      issuanceDate      : getCurrentTimestamp(),
    };

Added the builder pattern for the rest of the fields with simple validation
Added unit tests
image

@codecov
Copy link

codecov bot commented Jun 13, 2023

Codecov Report

Merging #125 (4b94db3) into main (d1e2d10) will increase coverage by 1.47%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##             main     #125      +/-   ##
==========================================
+ Coverage   64.74%   66.21%   +1.47%     
==========================================
  Files          33       32       -1     
  Lines        3551     3472      -79     
  Branches      198      197       -1     
==========================================
  Hits         2299     2299              
+ Misses       1250     1171      -79     
  Partials        2        2              
Components Coverage Δ
credentials ∅ <ø> (∅)
crypto 0.00% <ø> (ø)
dids 48.61% <ø> (ø)
web5 81.51% <ø> (ø)
web5-agent 0.00% <ø> (ø)
web5-proxy-agent 0.00% <ø> (ø)
web5-user-agent 79.60% <ø> (ø)

Copy link
Contributor

@frankhinek frankhinek left a comment

Choose a reason for hiding this comment

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

Awesome to see this contribution @nitro-neal !

Looking forward to many more as you help us build out VC capabilities.

packages/credentials/src/utils.ts Outdated Show resolved Hide resolved
packages/credentials/src/utils.ts Outdated Show resolved Hide resolved
packages/credentials/src/verifiable-credential-builder.ts Outdated Show resolved Hide resolved
packages/credentials/src/utils.ts Outdated Show resolved Hide resolved
@frankhinek
Copy link
Contributor

@mistermoe When reviewing this PR, I spotted a few things or had a few questions that I should have probably raised in the initial PR. I didn't notice them until going through a more thorough review today:

1. VerifiableCredential type definition - @context

Including a static string in the type definition:

export type VerifiableCredential = {
/** see {@link https://www.w3.org/TR/vc-data-model/#contexts | Contexts} */
'@context': ['https://www.w3.org/2018/credentials/v1', ...string[]];

results in awkward type issues in the code, for example:

this.verifiableCredential['@context'] = [
...new Set([...this.verifiableCredential['@context'], ...contextArray])
] as ['https://www.w3.org/2018/credentials/v1', ...string[]];

Proposal is to define @context as its own type and then reference in the VerifiableCredential type:

/**
 * The data type for `@context` properties of credentials, presentations, etc.
 *
 * @see {@link https://www.w3.org/TR/vc-data-model/#contexts | Credential Contexts}
 */
export type CredentialContext = string | Record<string, any> | (string | Record<string, any>)[]

/**
 * @see {@link https://www.w3.org/TR/vc-data-model/#credentials | VC data model}
 */
export type VerifiableCredential = {
  /** @see {@link CredentialContext} */
  '@context': CredentialContext;
  credentialSubject: CredentialSubject | CredentialSubject[];
...

Neil is already including the mandatory 'https://www.w3.org/2018/credentials/v1' value when a new credential is created, and this seems like a better trade-off to the intellisense code completion resulting from including the string in the type def, since its not likely developers are writing credentials by hand in an IDE.

2. VerifiableCredential type definition - type

Based on my read of the spec: https://www.w3.org/TR/vc-data-model/#types

it appears that type is a required property for a VerifiableCredential:
Screenshot 2023-06-14 at 9 55 33 AM

and that it must be an array of strings for a VerifiableCredential.

If that is accurate, then we should change the type definition from:

/** @see {@link https://www.w3.org/TR/vc-data-model/#types | Types} */
type?: string[] | string;

to:

  type: string[];

This would make the code that modifies types easier to deal with by not having to contend with strings or arrays of strings and the potential to be undefined.

@nitro-neal
Copy link
Contributor Author

@frankhinek
image

It seems to me context needs to just be an array of objects according to the spec. In practicality though we use it as an array of strings. In the ssi-sdk we just have a new string array:

contexts := []string{VerifiableCredentialsLinkedDataContext}

@nitro-neal
Copy link
Contributor Author

image

Updating based on comments

@mistermoe
Copy link
Member

@nitro-neal should we close this in favor of #148 ?

@mistermoe mistermoe added the credentials SSI functionality label Aug 4, 2023
@nitro-neal nitro-neal closed this Aug 4, 2023
@nitro-neal nitro-neal deleted the vc-builder branch August 4, 2023 22:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
credentials SSI functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants