-
Notifications
You must be signed in to change notification settings - Fork 200
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!: message pickup live mode support #1638
Changes from all commits
7e6970d
2f40c6a
664c891
e5cb74e
ff54595
55ba318
8e7c1f8
c198993
5afcd33
ea27b62
dfbb1b9
1816dbb
8af1d33
b6f952d
f39fe5c
4cf3aa2
b41e818
180695c
b22dd3e
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -5,21 +5,21 @@ import type { TransportSession } from './TransportService' | |
import type { AgentContext } from './context' | ||
import type { ConnectionRecord } from '../modules/connections' | ||
import type { ResolvedDidCommService } from '../modules/didcomm' | ||
import type { DidDocument } from '../modules/dids' | ||
import type { OutOfBandRecord } from '../modules/oob/repository' | ||
import type { OutboundTransport } from '../transport/OutboundTransport' | ||
import type { OutboundPackage, EncryptedMessage } from '../types' | ||
import type { EncryptedMessage, OutboundPackage } from '../types' | ||
|
||
import { DID_COMM_TRANSPORT_QUEUE, InjectionSymbols } from '../constants' | ||
import { ReturnRouteTypes } from '../decorators/transport/TransportDecorator' | ||
import { AriesFrameworkError, MessageSendingError } from '../error' | ||
import { Logger } from '../logger' | ||
import { DidCommDocumentService } from '../modules/didcomm' | ||
import { DidKey, type DidDocument } from '../modules/dids' | ||
import { getKeyFromVerificationMethod } from '../modules/dids/domain/key-type' | ||
import { didKeyToInstanceOfKey } from '../modules/dids/helpers' | ||
import { didKeyToInstanceOfKey, verkeyToDidKey } from '../modules/dids/helpers' | ||
import { DidResolverService } from '../modules/dids/services/DidResolverService' | ||
import { MessagePickupRepository } from '../modules/message-pickup/storage' | ||
import { inject, injectable } from '../plugins' | ||
import { MessageRepository } from '../storage/MessageRepository' | ||
import { MessageValidator } from '../utils/MessageValidator' | ||
import { getProtocolScheme } from '../utils/uri' | ||
|
||
|
@@ -38,7 +38,7 @@ export interface TransportPriorityOptions { | |
export class MessageSender { | ||
private envelopeService: EnvelopeService | ||
private transportService: TransportService | ||
private messageRepository: MessageRepository | ||
private messagePickupRepository: MessagePickupRepository | ||
private logger: Logger | ||
private didResolverService: DidResolverService | ||
private didCommDocumentService: DidCommDocumentService | ||
|
@@ -48,15 +48,15 @@ export class MessageSender { | |
public constructor( | ||
envelopeService: EnvelopeService, | ||
transportService: TransportService, | ||
@inject(InjectionSymbols.MessageRepository) messageRepository: MessageRepository, | ||
@inject(InjectionSymbols.MessagePickupRepository) messagePickupRepository: MessagePickupRepository, | ||
@inject(InjectionSymbols.Logger) logger: Logger, | ||
didResolverService: DidResolverService, | ||
didCommDocumentService: DidCommDocumentService, | ||
eventEmitter: EventEmitter | ||
) { | ||
this.envelopeService = envelopeService | ||
this.transportService = transportService | ||
this.messageRepository = messageRepository | ||
this.messagePickupRepository = messagePickupRepository | ||
this.logger = logger | ||
this.didResolverService = didResolverService | ||
this.didCommDocumentService = didCommDocumentService | ||
|
@@ -113,9 +113,11 @@ export class MessageSender { | |
{ | ||
connection, | ||
encryptedMessage, | ||
recipientKey, | ||
options, | ||
}: { | ||
connection: ConnectionRecord | ||
recipientKey: string | ||
encryptedMessage: EncryptedMessage | ||
options?: { transportPriority?: TransportPriorityOptions } | ||
} | ||
|
@@ -176,7 +178,11 @@ export class MessageSender { | |
// If the other party shared a queue service endpoint in their did doc we queue the message | ||
if (queueService) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should we at some point change this to only queue when it is allowed? So a connection must have queueing enabled? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ah yes, I remember this from #1199. I think it will be important to find a way to configure this. I'm trying to find the best place to update the code to support this, as I don't want to make |
||
this.logger.debug(`Queue packed message for connection ${connection.id} (${connection.theirLabel})`) | ||
await this.messageRepository.add(connection.id, encryptedMessage) | ||
await this.messagePickupRepository.addMessage({ | ||
connectionId: connection.id, | ||
recipientDids: [verkeyToDidKey(recipientKey)], | ||
payload: encryptedMessage, | ||
}) | ||
return | ||
} | ||
|
||
|
@@ -318,7 +324,7 @@ export class MessageSender { | |
|
||
// We didn't succeed to send the message over open session, or directly to serviceEndpoint | ||
// If the other party shared a queue service endpoint in their did doc we queue the message | ||
if (queueService) { | ||
if (queueService && message.allowQueueTransport) { | ||
this.logger.debug(`Queue message for connection ${connection.id} (${connection.theirLabel})`) | ||
|
||
const keys = { | ||
|
@@ -328,7 +334,11 @@ export class MessageSender { | |
} | ||
|
||
const encryptedMessage = await this.envelopeService.packMessage(agentContext, message, keys) | ||
await this.messageRepository.add(connection.id, encryptedMessage) | ||
await this.messagePickupRepository.addMessage({ | ||
connectionId: connection.id, | ||
recipientDids: keys.recipientKeys.map((item) => new DidKey(item).did), | ||
payload: encryptedMessage, | ||
}) | ||
|
||
this.emitMessageSentEvent(outboundMessageContext, OutboundMessageSendStatus.QueuedForPickup) | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,5 +1,5 @@ | ||
export const InjectionSymbols = { | ||
MessageRepository: Symbol('MessageRepository'), | ||
MessagePickupRepository: Symbol('MessagePickupRepository'), | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If we make this a module config option, and expose the config on the new API, we don't need the injection symbol anymore (as you can inject the api and then do api.config.messagePickupRepository) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I agree. However I think I'll need to update a bit the logic in order to avoid dependency cycles between |
||
StorageService: Symbol('StorageService'), | ||
Logger: Symbol('Logger'), | ||
AgentContextProvider: Symbol('AgentContextProvider'), | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should rename repository, as it conflicts with all the other repositories that are based on the storage service.
Maybe we can use messagePickupQueue? Or messagePickupStorage?
Or maybe I should change my thinking on what a repository can be. As repository does make sense
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
At the beginning I named it
MessagePickupQueue
but discussing about it with my colleagues we noticed that we are not using it as a queue, in the sense that we are enqueuing data to it but not exactly dequeuing data when retrieving it (as we must store until it is actually acknowledged by the mediatee). Also, the data on it is somewhat organized (i.e. by connectionId, recipientKey, state, etc.) so the term 'Repository' feels to me more appropriate than 'Storage', which is usually used to generically save data.Having said this, I'm not happy because the unfortunate name clash with
Repository
interface, so I'm more than open to opinions about how to improve the naming!There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think
MessageRepository
is best i.e. it's the place where you store and fetch messages. I thinkxxxQueue
would tend to incorrectly enforce the underlying technology, when any persistent storage may actually work.Pickup
is just a fragment of the interface (we're not 'picking up' messages received that cannot be delivered, rather we're queuing them for later pickup), so I would just drop it to avoid narrowing the exhibited responsibility.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The problem with
MessageRepository
is that it collides withRepository
classes used within the framework, most notablyDidCommMessageRepository
, so leaving such generic name would be more confusing. The 'Pickup' word tries to specify that it is used for message pickup protocol.It seems it is still not clear, as this
MessagePickupRepository
is an interface rather than an implementation, like the otherRepository
.