feat: Discover Features V2 over DIDComm V2 #1576

genaris · 2023-09-16T18:39:26Z

Discover Features 2.0 is one of those protocols that are suitable for both DIDComm V1 and V2. However, this becomes a bit complicated because current AFJ model considers that a given message type is defined for either V1 or V2.

So after an interesting discussion in a previous AFJ WG Call, we found out that we can leave each protocol determine which versions it supports and register both models into the MessageHandlerRegistry, which will now allow to filter not only by message type (PIURI) but also by DIDComm version.

In this PR we have:

Different fixes and adaptations for DIDComm V2 (like attachment transformers, message handling, etc.) found during the work of other PRs (feat: Basic Messages V2 for DIDComm V2 #1546 and feat: credential protocols v3 #1560). This would make this PR to be the basis of those, which will hopefully only add new protocols and make their review easier
New handling of received messages in MessageReceiver and Dispatcher to figure out whether it is a DIDComm V1 or V2 message and find the right class and handler
Updates in Discover Features module to add DIDComm V2 modeling for Query and Disclosure messages, and register/process both in a single handler and service
There are some slight API changes, like the return types and events, due to messages can be now either be DIDComm V1 or V2. I think in the next major release (before merging DIDComm V2 to main) we can remove the complete message object from the event, as the relevant fields are sent as separate properties (and in case we want to go low level and get the complete message we can subscribe to AgentMessageProcessedEvent)
Discover Features V2 E2E test has been updated to work in both DIDComm V1 and V2. This makes it to require Askar. As we'll switch soon our tests to node 18 + shared components I guess there is no problem on that

Signed-off-by: Ariel Gentile <gentilester@gmail.com>

TimoGlastra · 2023-09-29T09:35:24Z

packages/core/src/agent/MessageReceiver.ts

@@ -300,7 +302,9 @@ export class MessageReceiver {
      throw new AriesFrameworkError(`No type found in the message: ${message}`)
    }

-    const MessageClass = this.messageHandlerRegistry.getMessageClassForMessageType(messageType)
+    const didcommVersion = isPlaintextMessageV1(message) ? DidCommMessageVersion.V1 : DidCommMessageVersion.V2


Should we make this future proof and have an if / else if / else throw error?

TimoGlastra · 2023-09-29T09:38:04Z

packages/core/src/agent/getOutboundMessageContext.ts

+    // Check that message DIDComm version matches connection
+    if (
+      (connectionRecord.isDidCommV1Connection && message.didCommVersion !== DidCommMessageVersion.V1) ||
+      (connectionRecord.isDidCommV2Connection && message.didCommVersion !== DidCommMessageVersion.V2)
+    ) {
+      throw new AriesFrameworkError(
+        `Message DIDComm version ${message.didCommVersion} does not match connection ${connectionRecord.id}`
+      )
+    }


Interesting. I think we may have to tweak this at some point as the services in a did document dictate which didcomm versions are supported. We only have did:peer connections now, but if we want to make it future proof already, we could also handle this later on, after we've resolved the services for a connection.

Thoughts?

TimoGlastra · 2023-09-29T09:39:32Z

packages/core/src/agent/getOutboundMessageContext.ts

+    // Attach 'from' and 'to' fields according to connection record (unless they are previously defined)
+    if (message instanceof DidCommV2Message) {
+      message.from = message.from ?? connectionRecord.did
+      const recipients = message.to ?? (connectionRecord.theirDid ? [connectionRecord.theirDid] : undefined)
+      if (!recipients) {
+        throw new AriesFrameworkError('Cannot find recipient did for message')
+      }
+      message.to = recipients
+    }


Can there be any danger in having different from/to than the connectionRecord? What would be the use case? I think if you want custom from/to you should probably not pass a connection record to the getOutboundMessageContext message.

TimoGlastra · 2023-09-29T09:41:01Z

packages/core/src/agent/getOutboundMessageContext.ts

@@ -67,6 +91,14 @@ export async function getOutboundMessageContext(
    )
  }

+  if (
+    !(message instanceof DidCommV1Message) ||


I think we can invert all these statements?

Suggested change

!(message instanceof DidCommV1Message) ||

message instanceof DidCommV2Message

TimoGlastra · 2023-09-29T09:43:56Z

packages/core/src/didcomm/transformers.ts

+  })
+}
+
+export function toV1Attachment(v2Attachment: V2Attachment): Attachment {


Can we rename Attachment to V1Attachment? We're not trying to avoid breaking changes anymore.

Also should we rename this to V1DidCommAttachment and V2DidCommAttachment to start the process of making didcomm not the default?

Oh and finally, do you thin we need a generic didcomm attachment class / interface that we can map to v1 / v2?

TimoGlastra · 2023-09-29T09:47:15Z

packages/core/src/modules/discover-features/protocol/v2/V2DiscoverFeaturesService.ts

+      ? new V2QueriesDidCommV2Message({ queries: options.queries })
+      : new V2QueriesMessage({ queries: options.queries })


TimoGlastra · 2023-09-29T09:47:36Z

packages/core/src/modules/discover-features/protocol/v2/V2DiscoverFeaturesService.ts

@@ -71,10 +75,11 @@ export class V2DiscoverFeaturesService extends DiscoverFeaturesService {
      },
    })

-    // Process query and send responde automatically if configured to do so, otherwise
+    // Process query and send respose automatically if configured to do so, otherwise


Suggested change

// Process query and send respose automatically if configured to do so, otherwise

// Process query and send response automatically if configured to do so, otherwise

TimoGlastra · 2023-09-29T09:49:40Z

packages/core/src/modules/discover-features/protocol/v2/V2DiscoverFeaturesService.ts

-      threadId: options.threadId,
-      features: matches,
-    })
+    const discloseMessage = options.connectionRecord?.isDidCommV2Connection


To extend on my proposal to infer which didomm version to use based on the services. Maybe there can be a toDidCommV1Message on the v2 base class and a toDidCommV2Message on the v1 base class and that this is optionally implemented when you support 2 didcomm versions for hte same protocol version. Then we could automatically transform it into the other type when sending the message if the method is available, and the didcomm version does not match.

But it doens't make fully sense, because we do need to know upfront what didcomm version a connection supports. And we should be able to update that in the connection record when we re-fetch a did and resolve their servides (so it's not something static). But that can be done later ofc.

TimoGlastra · 2023-09-29T09:53:50Z

packages/core/src/storage/didcomm/DidCommMessageRecord.ts

-    const messageId = this.message['@id'] as string
-    const messageType = this.message['@type'] as string
+    const messageId = (this.message['@id'] ?? this.message['id']) as string
+    const messageType = (this.message['@type'] ?? this.message['type']) as string

    const { protocolName, protocolMajorVersion, protocolMinorVersion, messageName } = parseMessageType(messageType)

    const thread = this.message['~thread']
-    let threadId = messageId
+    let threadId = (this.message['thid'] ?? messageId) as string


Should we split this in if (didcommv1) {} else if (didcommv2) {} ?

to not make it so intertwined. As it could lead to some issues if for some magic reason a didcomm v2 message has a ~thread header 😆

TimoGlastra · 2023-09-29T09:54:41Z

packages/core/tests/helpers.ts

@@ -217,7 +218,7 @@ const isCredentialStateChangedEvent = (e: BaseEvent): e is CredentialStateChange
 const isConnectionStateChangedEvent = (e: BaseEvent): e is ConnectionStateChangedEvent =>
  e.type === ConnectionEventTypes.ConnectionStateChanged
 const isTrustPingReceivedEvent = (e: BaseEvent): e is TrustPingReceivedEvent =>
-  e.type === TrustPingEventTypes.TrustPingReceivedEvent
+  e.type === TrustPingEventTypes.TrustPingReceivedEvent || e.type === TrustPingEventTypes.V2TrustPingReceivedEvent


I think we can unify these again as we're okay with introducing breaking changes?

So just one trust ping received event (as we have with all other multi-verion protocols)

TimoGlastra · 2023-09-29T09:55:14Z

Apologies for the long wait @genaris 😉

genaris added 8 commits September 15, 2023 19:36

feat: generalize getOutboundMessageContext

c447c32

Signed-off-by: Ariel Gentile <gentilester@gmail.com>

fix: OOB in outbound message context

76e8a5c

Signed-off-by: Ariel Gentile <gentilester@gmail.com>

feat: filter messages by didcomm version

0a4d4f2

Signed-off-by: Ariel Gentile <gentilester@gmail.com>

refactor: V2ProblemReport

7acfdac

Signed-off-by: Ariel Gentile <gentilester@gmail.com>

feat: add transformers and test helpers

af61ae7

Signed-off-by: Ariel Gentile <gentilester@gmail.com>

feat: do didcomm version checking according to connection

49ad374

Signed-off-by: Ariel Gentile <gentilester@gmail.com>

feat: support Discover Features V2 over DIDComm V2

ef0b934

Signed-off-by: Ariel Gentile <gentilester@gmail.com>

fix: check retourn routes

7d75254

Signed-off-by: Ariel Gentile <gentilester@gmail.com>

genaris requested a review from a team as a code owner September 16, 2023 18:39

genaris requested a review from Artemkaaas September 16, 2023 18:40

genaris mentioned this pull request Sep 16, 2023

feat: Basic Messages V2 for DIDComm V2 #1546

Draft

TimoGlastra approved these changes Sep 29, 2023

View reviewed changes

TimoGlastra added this to the 0.6 milestone Feb 14, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Discover Features V2 over DIDComm V2 #1576

feat: Discover Features V2 over DIDComm V2 #1576

genaris commented Sep 16, 2023

TimoGlastra Sep 29, 2023

TimoGlastra Sep 29, 2023

TimoGlastra Sep 29, 2023

TimoGlastra Sep 29, 2023

TimoGlastra Sep 29, 2023

TimoGlastra Sep 29, 2023

TimoGlastra Sep 29, 2023

TimoGlastra Sep 29, 2023

TimoGlastra Sep 29, 2023

TimoGlastra Sep 29, 2023

TimoGlastra Sep 29, 2023

TimoGlastra Sep 29, 2023

TimoGlastra commented Sep 29, 2023

	!(message instanceof DidCommV1Message) \|\|
	message instanceof DidCommV2Message

		? new V2QueriesDidCommV2Message({ queries: options.queries })
		: new V2QueriesMessage({ queries: options.queries })

	// Process query and send respose automatically if configured to do so, otherwise
	// Process query and send response automatically if configured to do so, otherwise

feat: Discover Features V2 over DIDComm V2 #1576

Are you sure you want to change the base?

feat: Discover Features V2 over DIDComm V2 #1576

Conversation

genaris commented Sep 16, 2023

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 commented Sep 29, 2023