feat: add oob proof proposal #1370

Zzocker · 2023-03-07T06:53:09Z

No description provided.

codecov-commenter · 2023-03-07T07:14:55Z

Codecov Report

Merging #1370 (a822161) into main (a4204ef) will decrease coverage by 1.85%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main    #1370      +/-   ##
==========================================
- Coverage   87.27%   85.43%   -1.85%     
==========================================
  Files         780      780              
  Lines       18706    18717      +11     
  Branches     3210     3216       +6     
==========================================
- Hits        16326    15991     -335     
- Misses       2373     2719     +346     
  Partials        7        7

Impacted Files	Coverage Δ
...oncreds/src/protocols/proofs/v1/V1ProofProtocol.ts	`88.82% <100.00%> (+0.13%)`	⬆️
packages/core/src/modules/proofs/ProofsApi.ts	`84.21% <100.00%> (+0.25%)`	⬆️
.../src/modules/proofs/protocol/v2/V2ProofProtocol.ts	`93.76% <100.00%> (+0.07%)`	⬆️

... and 9 files with indirect coverage changes

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

TimoGlastra

Thanks for this PR @Zzocker!

I'm okay with the changes, but we need to make sure to implement it for all protocol versions and make sure we can also receive connectionless proposals in addition to create them.

could you also add some new tests for this?

Zzocker · 2023-03-07T15:33:26Z

Yes, I wanted to add test for the changes but I wasn't sure whether to create a new test file or add in an existing test file.

When verifier receives the proposal message, the proposal handler function will be invoked and proposal will be proceeded, and if auto accept proof is true, verifier will send the proof request. Do we need to have any aditional logic for reciving the proposal message ?

Zzocker · 2023-03-09T14:30:54Z

I have added oob proof proposal for both (v1 and v2) versions of the proof request. Also added test cases. @TimoGlastra can you again review this PR?

TimoGlastra

Grest changes @Zzocker. I have some concerns on the connection between oob and proofs. This is something that needs to be fixes across the framework.

TimoGlastra · 2023-03-11T10:29:36Z

packages/anoncreds/src/protocols/proofs/v1/V1ProofProtocol.ts

@@ -416,6 +416,12 @@ export class V1ProofProtocol extends BaseProofProtocol implements ProofProtocol<

    let proofRecord = await this.findByThreadAndConnectionId(agentContext, proofRequestMessage.threadId, connection?.id)

+    if (!proofRecord) {
+      // Proof request not bound to any connection: requests_attach in OOB msg


This doesn't mean the proof record is not bound to any connection, and it means if i know the thread id i respond to this exchange even though not involved.

We should update this logic to be fully integrated with the oob module:

we look for the oob record

if oob is connectionless/now has a connection afterwards we allow the proof record to not have a connection

I'd like to solve this generically for both connectionless/non-connectionless exchanges with attached messages. Is this needed for this PR?

Sorry for the wrong code comment, connection-less proof requests will also not be linked to a connection.

In case of connection-less proof request both findByThreadAndConnectionId and findByThread will return null proofRecord.

In case of oob proof proposal findByThreadAndConnectionId will return null since the connection is not linked to any proofRecord, but findByThread will return proofRecord at ProposalSent state.

For now, can I replace the comment to?

if (!proofRecord) { // Proof request bound to a proofRecord by threadId: proof proposal in OOB msg // TODO integrate with oob module proofRecord = await this.findByThreadAndConnectionId(messageContext.agentContext, requestMessage.threadId) if (proofRecord) proofRecord.connectionId = connection?.id }

@TimoGlastra

TimoGlastra · 2023-03-11T10:30:50Z

packages/anoncreds/src/protocols/proofs/v1/__tests__/v1-indy-proof-proposal.e2e.test.ts

+    })
+    await faberAgent.oob.receiveInvitation(outOfBandInvitation)
+    testLogger.test('Faber waits for proof proposal message from Alice')
+    let faberProofExchangeRecord = await waitForProofExchangeRecord(faberAgent, {


Can you use waitForProofExchangeRecordSubject? This method can have timing issues

TimoGlastra · 2023-03-11T10:32:20Z

packages/core/src/modules/proofs/ProofsApiOptions.ts

@@ -38,6 +38,17 @@ export interface ProposeProofOptions<PPs extends ProofProtocol[] = ProofProtocol
  parentThreadId?: string
 }

+/**
+ * Interface for ProofsApi.createProofProposal. Will create an out of band request.


Suggested change

* Interface for ProofsApi.createProofProposal. Will create an out of band request.

* Interface for ProofsApi.createProofProposal. Will create an out of band proposal.

TimoGlastra · 2023-03-11T10:32:49Z

packages/core/src/modules/proofs/protocol/v2/V2ProofProtocol.ts

@@ -396,6 +396,12 @@ export class V2ProofProtocol<PFs extends ProofFormatService[] = ProofFormatServi
      connection?.id
    )

+    if (!proofRecord) {
+      // Proof request not bound to any connection: requests_attach in OOB msg
+      proofRecord = await this.findByThreadAndConnectionId(messageContext.agentContext, requestMessage.threadId)


Same comments here

TimoGlastra · 2023-03-11T10:35:12Z

packages/core/src/modules/proofs/protocol/v2/__tests__/v2-indy-oob-proof-proposal.e2e.test.ts

+    })
+  })
+
+  test('Alice start with oob proof proposal for Faber with auto-accept enabled and both agents having a mediator', async () => {


Specific reason you added this test? We have a lot of tests that do almost the same and it's making updates harder.

So if there's a specific reason for this test great, but otherwise we can maybe remove it?

Right, For the oob proof proposal, we want to test

After the proof exchange flow is completed,

The holder should have all three proof messages proposalMessage, requestMessage , and presentationMessage

proof exchange record should have nonNull connection id.

I will remove the first and last tests and add the above assertion in 2nd test.

Signed-off-by: Pritam Singh <[email protected]>

TimoGlastra · 2023-07-06T13:18:40Z

Discussed during WG call. @TimoGlastra will take another look

Zzocker requested a review from a team as a code owner March 7, 2023 06:53

TimoGlastra reviewed Mar 7, 2023

View reviewed changes

Zzocker force-pushed the oob-proof-proposal branch from d760d9a to cb4badf Compare March 9, 2023 13:45

Zzocker requested a review from TimoGlastra March 9, 2023 14:31

TimoGlastra requested changes Mar 11, 2023

View reviewed changes

Zzocker added 4 commits March 15, 2023 09:35

feat: add oob proof proposal

009a8ad

Signed-off-by: Pritam Singh <[email protected]>

fix: add v2 oob proof proposal test

0b044f0

Signed-off-by: Pritam Singh <[email protected]>

fix: add v1 oob proof proposal test

b501b18

Signed-off-by: Pritam Singh <[email protected]>

refactor!: remove duplicate tests from v2-indy-oob-proof-proposal

a822161

Signed-off-by: Pritam Singh <[email protected]>

Zzocker force-pushed the oob-proof-proposal branch from cb4badf to a822161 Compare March 15, 2023 04:05

Zzocker requested a review from TimoGlastra March 15, 2023 09:09

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add oob proof proposal #1370

feat: add oob proof proposal #1370

Zzocker commented Mar 7, 2023

codecov-commenter commented Mar 7, 2023 •

edited

Loading

TimoGlastra left a comment

Zzocker commented Mar 7, 2023 •

edited

Loading

Zzocker commented Mar 9, 2023

TimoGlastra left a comment

TimoGlastra Mar 11, 2023

Zzocker Mar 13, 2023 •

edited

Loading

TimoGlastra Mar 11, 2023

TimoGlastra Mar 11, 2023

TimoGlastra Mar 11, 2023

TimoGlastra Mar 11, 2023

Zzocker Mar 13, 2023

TimoGlastra commented Jul 6, 2023

	* Interface for ProofsApi.createProofProposal. Will create an out of band request.
	* Interface for ProofsApi.createProofProposal. Will create an out of band proposal.

feat: add oob proof proposal #1370

Are you sure you want to change the base?

feat: add oob proof proposal #1370

Conversation

Zzocker commented Mar 7, 2023

codecov-commenter commented Mar 7, 2023 • edited Loading

Codecov Report

TimoGlastra left a comment

Choose a reason for hiding this comment

Zzocker commented Mar 7, 2023 • edited Loading

Zzocker commented Mar 9, 2023

TimoGlastra left a comment

Choose a reason for hiding this comment

TimoGlastra Mar 11, 2023

Choose a reason for hiding this comment

Zzocker Mar 13, 2023 • edited Loading

Choose a reason for hiding this comment

TimoGlastra Mar 11, 2023

Choose a reason for hiding this comment

TimoGlastra Mar 11, 2023

Choose a reason for hiding this comment

TimoGlastra Mar 11, 2023

Choose a reason for hiding this comment

TimoGlastra Mar 11, 2023

Choose a reason for hiding this comment

Zzocker Mar 13, 2023

Choose a reason for hiding this comment

TimoGlastra commented Jul 6, 2023

codecov-commenter commented Mar 7, 2023 •

edited

Loading

Zzocker commented Mar 7, 2023 •

edited

Loading

Zzocker Mar 13, 2023 •

edited

Loading