Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: Discover Features V2 over DIDComm V2 #1576

Open
wants to merge 8 commits into
base: feat/didcomm-v2
Choose a base branch
from

Conversation

genaris
Copy link
Contributor

@genaris genaris commented Sep 16, 2023

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

@@ -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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Comment on lines +55 to +63
// 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}`
)
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Comment on lines +65 to +73
// 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
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

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

if (
!(message instanceof DidCommV1Message) ||
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we can invert all these statements?

Suggested change
!(message instanceof DidCommV1Message) ||
message instanceof DidCommV2Message

})
}

export function toV1Attachment(v2Attachment: V2Attachment): Attachment {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Comment on lines +54 to +55
? new V2QueriesDidCommV2Message({ queries: options.queries })
: new V2QueriesMessage({ queries: options.queries })
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤯

@@ -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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

threadId: options.threadId,
features: matches,
})
const discloseMessage = options.connectionRecord?.isDidCommV2Connection
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment on lines -61 to +67
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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 😆

@@ -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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Contributor

Apologies for the long wait @genaris 😉

@TimoGlastra TimoGlastra added this to the 0.6 milestone Feb 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants