Replace sephereon/PEX presentation definitions with web5 js VCs implementation #112

kirahsapong · 2023-12-05T16:01:50Z

This PR addresses #25

changeset-bot · 2023-12-05T16:01:54Z

🦋 Changeset detected

Latest commit: b96056a

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 3 packages

Name	Type
@tbdex/protocol	Patch
@tbdex/http-client	Patch
@tbdex/http-server	Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

kirahsapong · 2023-12-05T16:09:44Z

One note is this PR now explicitly types our offering's get requiredClaims in response to this error. TS threw this after replacing sphereon's PresentationDefinitionV2 type import with that from Web5

The inferred type of 'requiredClaims' cannot be named without a reference to '.pnpm/@[email protected]/node_modules/@sphereon/pex-models'. This is likely not portable. A type annotation is necessary.

It seems like this might be due to conflicting package deps but we've removed the dep from our protocols package. Haven't run into this before so any thoughts from others v welcome!

For added info: Here's where Web5 import/exports sphereon's type PresentationDefinitionV2

KendallWeihe · 2023-12-05T17:11:42Z

explicitly types our offering's get requiredClaims

I believe the issue is because PresentationDefinitionV2 is a type alias in web5-js; see here.

So I'm guessing the TypeScript compiler is replacing the type reference w/ @sphereon/pex-models, but that's not what is defined as the OfferingData.requiredClaims type from here

So yeah... it's an odd ball out, in that it's the only get() method with an explicit return type, but I guess that's okay.

The other option is, we could change web5-js's presentation-exchange.ts to not do a type alias, and just directly export the sphereon type, so change this (how it currently is)...

import type { PresentationDefinitionV2 as PexPresDefV2 } from '@sphereon/pex-models';

//...

export type PresentationDefinitionV2 = PexPresDefV2

to this...

import type { PresentationDefinitionV2 } from '@sphereon/pex-models';
export type { PresentationDefinitionV2 } from '@sphereon/pex-models';

If we keep it as-is, then I would classify this as technical debt, albeit it's an extremely small amount of debt. The other option is, I think we should include a comment above the get requiredClaims() accessor explaining what's going on here (maybe with a link to this comment).

KendallWeihe · 2023-12-05T17:15:38Z

packages/protocol/src/message-kinds/rfq.ts

@@ -77,9 +74,9 @@ export class Rfq extends Message<'rfq'> {
   * @throws if rfq's claims do not fulfill the offering's requirements
   */
  async verifyClaims(offering: Offering | ResourceModel<'offering'>) {
-    const { areRequiredCredentialsPresent } = pex.evaluateCredentials(offering.data.requiredClaims, this.claims)
+    const requiredCredentialsPresent = PresentationExchange.selectCredentials(this.claims, offering.data.requiredClaims)


This is a nit-pick, but variable name is slightly misleading IMO, could do either one of these...

Suggested change

const requiredCredentialsPresent = PresentationExchange.selectCredentials(this.claims, offering.data.requiredClaims)

const requiredCredentialsPresent = PresentationExchange.selectCredentials(this.claims, offering.data.requiredClaims).length > 0

And then update the if () condition below like this

if (!requiredCredentialsPresent) { throw new Error(`claims do not fulfill the offering's requirements`) }

Or, could do...

Suggested change

const requiredCredentialsPresent = PresentationExchange.selectCredentials(this.claims, offering.data.requiredClaims)

const credentials = PresentationExchange.selectCredentials(this.claims, offering.data.requiredClaims)

and then keep the if () condition as-is (I think I prefer this way)

yeah good callout, totes agree.

jiyoonie9 · 2023-12-06T00:55:52Z

packages/protocol/src/message-kinds/rfq.ts

@@ -77,9 +74,9 @@ export class Rfq extends Message<'rfq'> {
   * @throws if rfq's claims do not fulfill the offering's requirements
   */
  async verifyClaims(offering: Offering | ResourceModel<'offering'>) {
-    const { areRequiredCredentialsPresent } = pex.evaluateCredentials(offering.data.requiredClaims, this.claims)
+    const credentials = PresentationExchange.selectCredentials(this.claims, offering.data.requiredClaims)


does this method return credentials or error encapsulated in an object?

/** * Selects credentials that satisfy a given presentation definition. * * @param {string[]} vcJwts The list of Verifiable Credentials to select from. * @param {PresentationDefinitionV2} presentationDefinition The Presentation Definition to match against. * @return {string[]} selectedVcJwts A list of Verifiable Credentials that satisfy the Presentation Definition. */ public static selectCredentials( vcJwts: string[], presentationDefinition: PresentationDefinitionV2 ): string[] { this.resetPex(); const selectResults: SelectResults = this.pex.selectFrom(presentationDefinition, vcJwts); return selectResults.verifiableCredential as string[] ?? []; }

it retuns the list of Verifiable Credentials that can satisfy the Presentation Definition.

actually might be helpful to return the error. this way we can surface it back up to the caller

I propose we use exceptions for error cases. Two benefits I can think of:

The return type is scoped to the semantic use case, which makes naming more cohesive and self-evident (ex. List<Credentials> as opposed to SelectCredentialsResponse (which would include a credentials and errors arrays))

By decoupling the error from the type, we have flexibility to introduce deeper call stacks abstracted behind the selectCredentials down the road, which could bubble-up exceptions, rather than having to then concern ourselves with passing errors up the callstack in return types

The counter argument is it makes the consumer have to use try/catch which can become verbose, but it isn't much more verbose than the subsequent if (selectCredentialsResponse.errors?.length) { ...

jiyoonie9 · 2023-12-06T01:05:15Z

packages/protocol/package.json

@@ -50,8 +50,6 @@
  },


i think this PR necessitates a version bump - would you be able to do that? the readme has a section on how to add a changeset and automatically bump the version.

automagic nice!

nitro-neal · 2023-12-06T18:02:29Z

This is merged in now - decentralized-identity/web5-js#336

kirahsapong · 2023-12-06T21:28:40Z

Took another look at this and wondered why we verify all claims submitted including those that aren't needed to satisfy our PD? Would it make sense to only verify those claims that satisfy the PD?

Current:

  async verifyClaims(offering: Offering | ResourceModel<'offering'>) {
    const credentials = PresentationExchange.selectCredentials(this.claims, offering.data.requiredClaims)

    if (!credentials.length) {
      throw new Error(`claims do not fulfill the offering's requirements`)
    }

    for (let claim of this.claims) {
      await VerifiableCredential.verify(claim)
    }
  }

Proposed:

  async verifyClaims(offering: Offering | ResourceModel<'offering'>) {
    const credentials = PresentationExchange.selectCredentials(this.claims, offering.data.requiredClaims)

    if (!credentials.length) {
      throw new Error(`claims do not fulfill the offering's requirements`)
    }

    for (let credential of credentials) {
      await VerifiableCredential.verify(credential)
    }
  }

@mistermoe @jiyoontbd @KendallWeihe

KendallWeihe · 2023-12-06T22:20:38Z

Would it make sense to only verify those claims that satisfy the PD?

Yeah, I think so! @jiyoontbd can you think of an instance where we need to verify claims which are not defined in the Offering? That is, the Offering has N-number of claim requirements, and the RFQ has (N+M)-number of requirements, and we need to verify all N+M of them?

jiyoonie9 · 2023-12-07T00:24:34Z

@KendallWeihe @kirahsapong that makes sense to me! The API is a bit different now that we're using web5/credentials, which allows us to get back the credentials that meet the requirements, so i don't see why we should verify all incoming claims from the RFQ.

As long as the credential we get back from PresentationExchange.selectCredentials(..) is the credential(s) from the RFQ.claims that happen to meet the requirements, I'm good with this new approach!

KendallWeihe · 2023-12-07T15:09:28Z

packages/protocol/src/resource-kinds/offering.ts

@@ -64,7 +65,9 @@ export class Offering extends Resource<'offering'> {
  }

  /** Articulates the claim(s) required when submitting an RFQ for this offering. */
-  get requiredClaims() {
+  // TODO: Remove type annotation once type alias replaced with direct export in @web5/credentials


I think this is good to go now, right? Do we have to wait on a release to be cut? If so, and that may be some time, then I'm cool with leaving this comment, opening and issue, and going ahead with a merge here

KendallWeihe

this lgtm! barring if we can go ahead and use the latest @web5/credentials change

kirahsapong added 3 commits December 4, 2023 18:14

Replace rfq verifyClaims credentials check with web5 impl

752ee43

Swap sphereon PresentationDefinitionV2 type import with web5 import

e6d7784

Remove sphereon pex package deps

2a6b134

kirahsapong marked this pull request as ready for review December 5, 2023 16:15

kirahsapong requested review from michaelneale, mistermoe, jiyoonie9, phoebe-lew and KendallWeihe as code owners December 5, 2023 16:15

KendallWeihe reviewed Dec 5, 2023

View reviewed changes

kirahsapong mentioned this pull request Dec 6, 2023

Export PEX type directly decentralized-identity/web5-js#336

Merged

kirahsapong added 2 commits December 5, 2023 16:47

Comment tech debt

f8f5765

Rename variable in verifyClaims

3bd5cfd

kirahsapong requested a review from KendallWeihe December 6, 2023 00:47

jiyoonie9 reviewed Dec 6, 2023

View reviewed changes

jiyoonie9 approved these changes Dec 6, 2023

View reviewed changes

mistermoe approved these changes Dec 6, 2023

View reviewed changes

Only verify claims that satisfy PD

de22b4f

kirahsapong requested review from jiyoonie9, nitro-neal and mistermoe December 7, 2023 03:29

KendallWeihe reviewed Dec 7, 2023

View reviewed changes

KendallWeihe approved these changes Dec 7, 2023

View reviewed changes

Add changeset

b96056a

kirahsapong merged commit 5631d32 into main Dec 7, 2023
7 checks passed

kirahsapong deleted the ksap/replace-pex-impl branch December 7, 2023 17:07

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Replace sephereon/PEX presentation definitions with web5 js VCs implementation #112

Replace sephereon/PEX presentation definitions with web5 js VCs implementation #112

kirahsapong commented Dec 5, 2023

changeset-bot bot commented Dec 5, 2023 •

edited

Loading

kirahsapong commented Dec 5, 2023 •

edited

Loading

KendallWeihe commented Dec 5, 2023

KendallWeihe Dec 5, 2023

kirahsapong Dec 6, 2023

jiyoonie9 Dec 6, 2023

nitro-neal Dec 6, 2023 •

edited by mistermoe

Loading

mistermoe Dec 6, 2023

KendallWeihe Dec 6, 2023

jiyoonie9 Dec 6, 2023

kirahsapong Dec 6, 2023

nitro-neal commented Dec 6, 2023

kirahsapong commented Dec 6, 2023 •

edited

Loading

KendallWeihe commented Dec 6, 2023

jiyoonie9 commented Dec 7, 2023

KendallWeihe Dec 7, 2023

KendallWeihe left a comment

	const requiredCredentialsPresent = PresentationExchange.selectCredentials(this.claims, offering.data.requiredClaims)
	const requiredCredentialsPresent = PresentationExchange.selectCredentials(this.claims, offering.data.requiredClaims).length > 0

	const requiredCredentialsPresent = PresentationExchange.selectCredentials(this.claims, offering.data.requiredClaims)
	const credentials = PresentationExchange.selectCredentials(this.claims, offering.data.requiredClaims)

Replace sephereon/PEX presentation definitions with web5 js VCs implementation #112

Replace sephereon/PEX presentation definitions with web5 js VCs implementation #112

Conversation

kirahsapong commented Dec 5, 2023

changeset-bot bot commented Dec 5, 2023 • edited Loading

🦋 Changeset detected

kirahsapong commented Dec 5, 2023 • edited Loading

KendallWeihe commented Dec 5, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nitro-neal Dec 6, 2023 • edited by mistermoe Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nitro-neal commented Dec 6, 2023

kirahsapong commented Dec 6, 2023 • edited Loading

KendallWeihe commented Dec 6, 2023

jiyoonie9 commented Dec 7, 2023

Choose a reason for hiding this comment

KendallWeihe left a comment

Choose a reason for hiding this comment

changeset-bot bot commented Dec 5, 2023 •

edited

Loading

kirahsapong commented Dec 5, 2023 •

edited

Loading

nitro-neal Dec 6, 2023 •

edited by mistermoe

Loading

kirahsapong commented Dec 6, 2023 •

edited

Loading