fix(oob): support oob with connection and messages #1558

TimoGlastra · 2023-08-29T14:02:41Z

We now correctly check whether an oob exchange is associated with an exchange if there's no connectionId, and assign the connection if this is the case.

I think we now really support al flows and combinations of messages / handhsake / connectionless from the oob protocol.

Signed-off-by: Timo Glastra <timo@animo.id>

codecov-commenter · 2023-08-30T08:18:39Z

Codecov Report

Merging #1558 (c015708) into main (11050af) will decrease coverage by 23.14%.
The diff coverage is 71.42%.

@@             Coverage Diff             @@
##             main    #1558       +/-   ##
===========================================
- Coverage   85.73%   62.59%   -23.14%     
===========================================
  Files         950      774      -176     
  Lines       22761    17843     -4918     
  Branches     3982     3069      -913     
===========================================
- Hits        19514    11169     -8345     
- Misses       3062     6136     +3074     
- Partials      185      538      +353

Files Changed	Coverage Δ
...oncreds/src/protocols/proofs/v1/V1ProofProtocol.ts	`2.42% <0.00%> (-86.27%)`	⬇️
...ges/core/src/modules/credentials/CredentialsApi.ts	`85.56% <ø> (-1.04%)`	⬇️
packages/core/src/types.ts	`100.00% <ø> (ø)`
...c/protocols/credentials/v1/V1CredentialProtocol.ts	`30.21% <45.45%> (-62.76%)`	⬇️
.../src/modules/proofs/protocol/v2/V2ProofProtocol.ts	`76.78% <60.00%> (-16.91%)`	⬇️
.../modules/connections/services/ConnectionService.ts	`85.16% <66.66%> (-3.18%)`	⬇️
...es/credentials/protocol/v2/V2CredentialProtocol.ts	`76.98% <66.66%> (-13.48%)`	⬇️
packages/core/src/utils/parseInvitation.ts	`46.15% <66.66%> (-31.63%)`	⬇️
...entials/protocol/v2/CredentialFormatCoordinator.ts	`89.05% <83.33%> (-0.73%)`	⬇️
packages/core/src/modules/oob/OutOfBandApi.ts	`83.80% <89.65%> (-5.38%)`	⬇️
... and 6 more

... and 467 files with indirect coverage changes

berendsliedrecht

Couple of very minor points, otherwise LGTM.

packages/anoncreds/src/protocols/credentials/v1/V1CredentialProtocol.ts

berendsliedrecht · 2023-08-30T10:33:14Z

packages/core/src/agent/getOutboundMessageContext.ts

@@ -297,8 +297,11 @@ async function addExchangeDataToMessage(
    associatedRecord: BaseRecordAny
  }
 ) {
+  const legacyInvitationMetadata = outOfBandRecord?.metadata.get(OutOfBandRecordMetadataKeys.LegacyInvitation)


this file has an unused import from http which is a mistake, but not from your PR. Can you remove it?

GitHub does not allow me to add a comment there...

berendsliedrecht · 2023-08-30T10:33:58Z

packages/core/src/agent/getOutboundMessageContext.ts

  // Set the parentThreadId on the message from the oob invitation
-  if (outOfBandRecord) {
+  // If connectionless is used, we should not add the parentThreadId
+  if (outOfBandRecord && legacyInvitationMetadata?.legacyInvitationType !== 'connectionless') {


Would it make more sense to have legacyInvitationType be an enum?

Yeah probably :)

In ConnectionRecord we have the protocol field where we define the handshake protocol for the DID Exchange, and in the case of DIDComm V2, where no handshake protocol is involved, we set it to None. If this legacyInvitationType refers to the message type, would it make sense to call it 'none'. in the case of a connection-less exchange?

would it make sense to call it 'none'. in the case of a connection-less exchange?

Well in that case we don't know explicitly if it's a connectionless message, and we have custom behaviour for that as you can see here. I'd still call a message with a ~service an invitation to do something.

So invitation type here refers less to connections, and more to an invitation to an exchange that is sent not over didcomm. (connectionless, connection v1, oob v1, etc..)

berendsliedrecht · 2023-08-30T10:34:36Z

packages/core/src/modules/connections/services/ConnectionService.ts

+   */
+  public async matchIncomingMessageToRequestMessageInOutOfBandExchange(
+    messageContext: InboundMessageContext,
+    { knownConnectionId }: { knownConnectionId?: string }


Maybe it should be

Suggested change

{ knownConnectionId }: { knownConnectionId?: string }

{ expectedConnectionId }: { expectedConnectionId?: string }

Yeah that makes sense. I was thinking if it is undefined it doesn't make sense. But after thining about it again, if it's undefined we don't really expect a specific connection Id

packages/core/src/modules/oob/repository/outOfBandRecordMetadataTypes.ts

genaris · 2023-08-30T11:35:35Z

packages/core/src/agent/getOutboundMessageContext.ts

  // Set the parentThreadId on the message from the oob invitation
-  if (outOfBandRecord) {
+  // If connectionless is used, we should not add the parentThreadId
+  if (outOfBandRecord && legacyInvitationMetadata?.legacyInvitationType !== 'connectionless') {


In ConnectionRecord we have the protocol field where we define the handshake protocol for the DID Exchange, and in the case of DIDComm V2, where no handshake protocol is involved, we set it to None. If this legacyInvitationType refers to the message type, would it make sense to call it 'none'. in the case of a connection-less exchange?

genaris · 2023-08-30T11:36:45Z

packages/core/src/modules/connections/services/ConnectionService.ts

+   * If is the case, and the state of the out of band record is still await response, the state will be updated to done
+   *
+   */
+  public async matchIncomingMessageToRequestMessageInOutOfBandExchange(


Wow this is a huge method with a huge name!

Yeah, i like explicitness over being brief. But suggestions welcome! :)

Signed-off-by: Tom Lanser <tom@devv.nl>

Signed-off-by: Timo Glastra <timo@animo.id>

…tion#1558) Signed-off-by: Timo Glastra <timo@animo.id> Signed-off-by: Ariel Gentile <gentilester@gmail.com>

…tion#1558) Signed-off-by: Timo Glastra <timo@animo.id> Signed-off-by: Martin Auer <martin.auer97@gmail.com>

TimoGlastra added 2 commits August 29, 2023 14:51

fix(oob): support oob with connection and messages

3139f39

Signed-off-by: Timo Glastra <timo@animo.id>

fix: broken connectionless flow

d337453

Signed-off-by: Timo Glastra <timo@animo.id>

TimoGlastra requested a review from a team as a code owner August 29, 2023 14:02

TimoGlastra mentioned this pull request Aug 29, 2023

Cannot process RequestCredentialMessage for the OOB attached credential-offer #1129

Closed

Merge branch 'main' into fix/oob-connections-and-messages

093ac67

berendsliedrecht approved these changes Aug 30, 2023

View reviewed changes

genaris approved these changes Aug 30, 2023

View reviewed changes

TimoGlastra and others added 6 commits September 21, 2023 16:26

Merge branch 'main' into fix/oob-connections-and-messages

1924169

chore: Resolve open comments

943a669

Signed-off-by: Tom Lanser <tom@devv.nl>

fix: Also changed some values to the new enum

e9792c8

Signed-off-by: Tom Lanser <tom@devv.nl>

test: fix tests

5802818

Signed-off-by: Timo Glastra <timo@animo.id>

test: fix tests

bd92510

Signed-off-by: Timo Glastra <timo@animo.id>

test: update snapshots

c015708

Signed-off-by: Timo Glastra <timo@animo.id>

TimoGlastra merged commit 9732ce4 into openwallet-foundation:main Sep 22, 2023

TimoGlastra deleted the fix/oob-connections-and-messages branch September 22, 2023 14:41

genaris pushed a commit to genaris/credo-ts that referenced this pull request Sep 22, 2023

fix(oob): support oob with connection and messages (openwallet-founda…

0dbff6b

…tion#1558) Signed-off-by: Timo Glastra <timo@animo.id> Signed-off-by: Ariel Gentile <gentilester@gmail.com>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(oob): support oob with connection and messages #1558

fix(oob): support oob with connection and messages #1558

TimoGlastra commented Aug 29, 2023

codecov-commenter commented Aug 30, 2023 •

edited

Loading

berendsliedrecht left a comment

berendsliedrecht Aug 30, 2023

berendsliedrecht Aug 30, 2023

TimoGlastra Aug 30, 2023

genaris Aug 30, 2023

TimoGlastra Aug 30, 2023

berendsliedrecht Aug 30, 2023

TimoGlastra Aug 30, 2023

genaris Aug 30, 2023

genaris Aug 30, 2023

TimoGlastra Aug 30, 2023

	{ knownConnectionId }: { knownConnectionId?: string }
	{ expectedConnectionId }: { expectedConnectionId?: string }

fix(oob): support oob with connection and messages #1558

fix(oob): support oob with connection and messages #1558

Conversation

TimoGlastra commented Aug 29, 2023

codecov-commenter commented Aug 30, 2023 • edited Loading

Codecov Report

berendsliedrecht 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

codecov-commenter commented Aug 30, 2023 •

edited

Loading