fix: miscellaneous issue credential v2 fixes #769

NB-MikeRichardson · 2022-05-12T07:38:20Z

Changes from ICV2 PR review comments which were left to do after the merge

Signed-off-by: Mike Richardson <[email protected]>

…N object Signed-off-by: Mike Richardson <[email protected]>

Signed-off-by: Mike Richardson <[email protected]>

codecov-commenter · 2022-05-12T07:54:28Z

Codecov Report

Merging #769 (c2bf770) into main (0c3cc49) will decrease coverage by 0.04%.
The diff coverage is 78.57%.

@@            Coverage Diff             @@
##             main     #769      +/-   ##
==========================================
- Coverage   87.53%   87.48%   -0.05%     
==========================================
  Files         435      435              
  Lines       10666    10678      +12     
  Branches     1891     1897       +6     
==========================================
+ Hits         9336     9342       +6     
- Misses       1268     1274       +6     
  Partials       62       62

Impacted Files	Coverage Δ
...rc/modules/credentials/CredentialsModuleOptions.ts	`100.00% <ø> (ø)`
packages/core/src/modules/credentials/index.ts	`100.00% <ø> (ø)`
...redentials/protocol/v2/CredentialMessageBuilder.ts	`87.40% <ø> (+0.57%)`	⬆️
...les/credentials/protocol/v2/V2CredentialService.ts	`93.83% <ø> (-0.02%)`	⬇️
...s/protocol/v2/messages/V2IssueCredentialMessage.ts	`100.00% <ø> (ø)`
...s/protocol/v2/messages/V2OfferCredentialMessage.ts	`100.00% <ø> (ø)`
...protocol/v2/messages/V2RequestCredentialMessage.ts	`100.00% <ø> (ø)`
.../modules/credentials/services/CredentialService.ts	`100.00% <ø> (ø)`
...s/protocol/v1/handlers/V1OfferCredentialHandler.ts	`71.05% <50.00%> (ø)`
...s/protocol/v2/handlers/V2OfferCredentialHandler.ts	`71.79% <50.00%> (ø)`
... and 6 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0c3cc49...c2bf770. Read the comment docs.

TimoGlastra

Nice improvements @NB-MikeRichardson!

Left some notes

packages/core/src/modules/credentials/formats/indy/IndyCredentialFormatService.ts

TimoGlastra · 2022-05-12T09:48:26Z

packages/core/src/modules/credentials/formats/indy/IndyCredentialFormatService.ts

      attachId: this.generateId(),
-      format: 'hlindy/[email protected]',
-    }
+      format: 'hlindy/[email protected]',


This also doesn't seem resolved

packages/core/src/modules/credentials/repository/CredentialExchangeRecord.ts

…mework-javascript into fix/icv2comments

…on/aries-framework-javascript into fix/icv2comments

Signed-off-by: Mike Richardson <[email protected]>

packages/core/src/modules/credentials/repository/CredentialExchangeRecord.ts

Signed-off-by: Mike Richardson <[email protected]>

packages/core/src/modules/credentials/protocol/v2/V2CredentialService.ts

packages/core/src/modules/credentials/repository/CredentialExchangeRecord.ts

packages/core/src/utils/CredentialResponseCoordinator.ts

Signed-off-by: Mike Richardson <[email protected]>

TimoGlastra · 2022-05-19T10:37:27Z

packages/core/src/modules/credentials/CredentialsModuleOptions.ts

@@ -41,7 +39,7 @@ interface NegotiateProposalOptions extends BaseOptions {
 interface OfferCredentialOptions extends BaseOptions {
  credentialRecordId?: string
  connectionId?: string
-  protocolVersion: CredentialProtocolVersion
+  protocolVersion?: CredentialProtocolVersion


protocolVersion shouldn't be optional for offerCredential, as it's the start of the protocol in this case

That change breaks loads of stuff. I suggest leaving this for now

What is the reason for this? It is required in the current main branch.

Not keen on merging this change as it's now not clear you're required to pass the protocolVersion

I have reverted it back to being mandatory for now.

As stated I would like to allot a PR just for this one Protocol Version field as I find it bewilderingly confusing and I want to fully understand it and do it properly.

packages/core/src/modules/credentials/formats/indy/IndyCredentialFormatService.ts

TimoGlastra · 2022-05-19T10:38:42Z

packages/core/src/modules/credentials/formats/indy/IndyCredentialFormatService.ts

      attachId: this.generateId(),
-      format: 'hlindy/[email protected]',
-    }
+      format: 'hlindy/[email protected]',


This also doesn't seem resolved

TimoGlastra · 2022-05-19T10:40:24Z

packages/core/src/modules/credentials/repository/CredentialExchangeRecord.ts

    const metadata = this.metadata.get(CredentialMetadataKeys.IndyCredential)
-    let credentialIds: string[] = []
-
+    let ids: string[] = []
    if (this.credentials) {
-      credentialIds = this.credentials.map((c) => c.credentialRecordId)
+      ids = this.credentials.map((c) => c.credentialRecordId)
    }
-


With the change to || [] I think we can just do:

const credentialIds = this.credentials.map(c => c.credentialRecordId)

That change doesn't work
Test fails

Then we should look at why it is failing. The types indicate the array is always defined, so somewhere it's assigning an undefined value where it should be an empty array I think. Please investigate.

packages/core/src/utils/composeAutoAccept.ts

Signed-off-by: Mike Richardson <[email protected]>

…on/aries-framework-javascript into fix/icv2comments

Signed-off-by: Mike Richardson <[email protected]>

TimoGlastra · 2022-05-19T17:25:26Z

packages/core/src/modules/credentials/protocol/v2/V2CredentialService.ts

@@ -221,6 +221,7 @@ export class V2CredentialService extends CredentialService {
    const options: ServiceOfferCredentialOptions = {
      credentialFormats: proposal.credentialFormats,
      comment: proposal.comment,
+      protocolVersion: credentialRecord.protocolVersion,
    }
    const message = await this.createOfferAsResponse(credentialRecord, options)


When creating an offer as response we can take the protocol version from the credential record

This also seems unresolved @NB-MikeRichardson

TimoGlastra

I think only the issue with credentials being undefined is worth looking at, but otherwise looks good

NB-MikeRichardson added 5 commits May 12, 2022 09:44

Fix message validation error related to CredentialFormatSpec

fde70a0

Signed-off-by: Mike Richardson <[email protected]>

Move Holder DID into options object

91f33ac

Signed-off-by: Mike Richardson <[email protected]>

Tweak to processProposal to ensure message validation operates on JSO…

ef856d7

…N object Signed-off-by: Mike Richardson <[email protected]>

Various small tweaks

3a98889

Signed-off-by: Mike Richardson <[email protected]>

Various (further) small tweaks

9dfc359

Signed-off-by: Mike Richardson <[email protected]>

NB-MikeRichardson requested a review from a team as a code owner May 12, 2022 07:38

TimoGlastra changed the title ~~Fix: Miscellaneous Fixes from Review Comments~~ fix: credential fixes from review comments May 12, 2022

TimoGlastra reviewed May 12, 2022

View reviewed changes

NB-MikeRichardson changed the title ~~fix: credential fixes from review comments~~ fix: issue credential v2 - fixes from review comments May 12, 2022

NB-MikeRichardson and others added 4 commits May 13, 2022 11:04

Merge branch 'main' into fix/icv2comments

c1ff509

Merge branch 'main' of https://github.com/NB-MikeRichardson/aries-fra…

045209d

…mework-javascript into fix/icv2comments

Merge branch 'fix/icv2comments' of https://github.com/NB-MikeRichards…

a4dfeb0

…on/aries-framework-javascript into fix/icv2comments

Change to CredentialExchangeRecord to prevent tests failing

f7094ba

Signed-off-by: Mike Richardson <[email protected]>

TimoGlastra reviewed May 14, 2022

View reviewed changes

packages/core/src/modules/credentials/repository/CredentialExchangeRecord.ts Outdated Show resolved Hide resolved

TimoGlastra added the Waiting on Author label May 14, 2022

NB-MikeRichardson added 7 commits May 16, 2022 14:34

merge with AFJ main

fcfbd73

Signed-off-by: Mike Richardson <[email protected]>

Merge branch 'hyperledger-main3' into fix/icv2comments

31e7b3c

move CredentialResponseCoordinator into utils

adc0d1c

Signed-off-by: Mike Richardson <[email protected]>

Change to CredentialExchangeRecord

61f2b1a

Signed-off-by: Mike Richardson <[email protected]>

check whether indy key is present in v1 credential service

1263e51

Signed-off-by: Mike Richardson <[email protected]>

Remove protocol version from accept proposal options

2ceab37

Signed-off-by: Mike Richardson <[email protected]>

Remove protocol version from accept proposal options

7436770

Signed-off-by: Mike Richardson <[email protected]>

TimoGlastra reviewed May 18, 2022

View reviewed changes

NB-MikeRichardson added 4 commits May 19, 2022 11:00

More small changes from comments

e223d2e

Signed-off-by: Mike Richardson <[email protected]>

merge with main

0e5d444

Signed-off-by: Mike Richardson <[email protected]>

Merge branch 'hyperledger-main' into fix/icv2comments

14850c0

fix broken test

17b26c6

Signed-off-by: Mike Richardson <[email protected]>

TimoGlastra reviewed May 19, 2022

View reviewed changes

Merge branch 'main' into fix/icv2comments

0c987a5

NB-MikeRichardson added 4 commits May 19, 2022 14:42

Minor fixes

409428a

Signed-off-by: Mike Richardson <[email protected]>

Minor fixes

91742ee

Signed-off-by: Mike Richardson <[email protected]>

Merge branch 'fix/icv2comments' of https://github.com/NB-MikeRichards…

9ea1dc7

…on/aries-framework-javascript into fix/icv2comments

Restore mandatory protocol version in offer cred options (temporarily)

c2bf770

Signed-off-by: Mike Richardson <[email protected]>

TimoGlastra reviewed May 19, 2022

View reviewed changes

TimoGlastra added the ICv2 label May 19, 2022

TimoGlastra added this to the v0.2.0 milestone May 19, 2022

TimoGlastra reviewed May 24, 2022

View reviewed changes

TimoGlastra mentioned this pull request May 24, 2022

refactor: delete credentials by default when deleting exchange #767

Merged

Merge branch 'main' into fix/icv2comments

973a8a4

NB-MikeRichardson mentioned this pull request May 25, 2022

Investigate why credentials attribute in CredentialExchangeRecord is undefined #793

Closed

TimoGlastra approved these changes May 25, 2022

View reviewed changes

TimoGlastra changed the title ~~fix: issue credential v2 - fixes from review comments~~ fix: miscellaneous issue credential v2 fixes May 25, 2022

TimoGlastra merged commit 537b51e into openwallet-foundation:main May 25, 2022

NB-MikeRichardson deleted the fix/icv2comments branch May 27, 2022 10:00

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: miscellaneous issue credential v2 fixes #769

fix: miscellaneous issue credential v2 fixes #769

NB-MikeRichardson commented May 12, 2022

codecov-commenter commented May 12, 2022 •

edited

Loading

TimoGlastra left a comment

TimoGlastra May 12, 2022

TimoGlastra May 19, 2022

TimoGlastra May 19, 2022

NB-MikeRichardson May 19, 2022

TimoGlastra May 19, 2022

NB-MikeRichardson May 19, 2022

TimoGlastra May 19, 2022

TimoGlastra May 19, 2022

NB-MikeRichardson May 19, 2022

TimoGlastra May 24, 2022

TimoGlastra May 19, 2022

TimoGlastra May 24, 2022

TimoGlastra left a comment

fix: miscellaneous issue credential v2 fixes #769

fix: miscellaneous issue credential v2 fixes #769

Conversation

NB-MikeRichardson commented May 12, 2022

codecov-commenter commented May 12, 2022 • edited Loading

Codecov Report

TimoGlastra left a 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

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

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

TimoGlastra left a comment

Choose a reason for hiding this comment

codecov-commenter commented May 12, 2022 •

edited

Loading