feat(vs-agent): add the receipts module to the v2 didcomm api - #685
Conversation
| // the module is OPTIONAL, and it is only registered when the chat plugin is enabled | ||
| let receiptsService: DidCommReceiptsService | ||
| try { | ||
| receiptsService = agent.context.dependencyManager.resolve(DidCommReceiptsService) |
There was a problem hiding this comment.
This try/catch does not add any value here. The dependency manager's resolve method returns undefined when the service cannot be resolved, so there is no need to catch an exception in this case.
There was a problem hiding this comment.
Without the catch a deployment with no chat plugin gets a 500 instead of the 404 the module is meant to answer. But isRegistered is on the same interface and reads better, so I switched to that.
There was a problem hiding this comment.
Please review the listProtocols requirement, as it has not been implemented yet and is explicitly required by the issue.
|
|
||
| const connection = await agent.didcomm.connections.findById(body.connectionId) | ||
| if (!connection) { | ||
| throw new AdminApiError( |
There was a problem hiding this comment.
Why not use the existing unknownConnection function here?
There was a problem hiding this comment.
It was module-private in V2DidcommConnectionsController, so I could not import it. Promoted it to common/AdminApiError.ts and both controllers use it now.
8ddd856 to
07603d7
Compare
genaris
left a comment
There was a problem hiding this comment.
I'd like to avoid the direct dependency on credo-ts-ext modules. I know that updating receipts module can take some more time, but I think that's the right way to do it. I have no problem if we use a patch in the meantime, but try to not add this redundant code on vs-agent.
| 'this deployment does not serve the receipts module', | ||
| ) | ||
| } | ||
| const receiptsService = agent.context.dependencyManager.resolve(DidCommReceiptsService) |
There was a problem hiding this comment.
So this reimplementing the whole DidCommReceiptsApi.send() just because is not returning the id?
I think the correct fix for this is open a PR on https://github.com/openwallet-foundation/credo-ts-didcomm-ext and, once implemented, just call agent.didcomm.receipts.send(). If VsAgent is properly typed, we won't need the direct dependency on @2060.io/credo-ts-didcomm-receipts.
| public async sendReceipts(@Body() body: SendReceiptsBodyDto): Promise<SendReceiptsResponseDto> { | ||
| const agent = await this.vsAgentService.getAgent() | ||
|
|
||
| if (!agent.context.dependencyManager.isRegistered(DidCommReceiptsService)) { |
There was a problem hiding this comment.
We can do a direct check on the enabled chat modules rather than doing this indirectly (which also add a direct dependency on receipts credo-ts-ext module)
Signed-off-by: Tarun Vadde <vaddeofficial@gmail.com>
4a35b90 to
e50a101
Compare
Signed-off-by: Tarun Vadde <vaddeofficial@gmail.com>
Thanks and both fixed. The controller checks I raised openwallet-foundation/credo-ts-didcomm-ext#179 to make |
Signed-off-by: Tarun Vadde <vaddeofficial@gmail.com>
genaris
left a comment
There was a problem hiding this comment.
With the latest changes you've introduced, now the plugin stays an optionalDependency. The only plugin import is import type, and the endpoint degrades to a 404 when the module is absent.
One architectural note, to resolve in #692 rather than here: the receipts controller lives in the app, so the route is registered (and advertised in Swagger) even on deployments that do not serve the receipts module, and it needs the agent as unknown as VsAgent<ChatAgentModules> cast. In #692 we are converging on the pattern where plugin endpoints live in the plugin package and register through VsAgentNestPlugin.controllers (as the MRTD controller does there). Since #692 already touches this controller, let's move it into plugin-chat in that PR, together with the other chat-module controllers.
# Conflicts: # pnpm-lock.yaml
Closes #659
POST /v2/didcomm/receiptssends receipts on a connection and returns the sent message id.UNKNOWN_ID404 for an unknown connection, and for a deployment that does not serve the module, which is only registered with the chat plugin.DidCommReceiptsService, same path the module takes internally, just keeping the id.Two calls worth a look:
@2060.io/credo-ts-didcomm-receiptstoapps/vs-agent, which had kept the chat packages behindoptImport.VsAgentNestPluginalready takes acontrollersarray, so this could ship with the chat plugin instead and get the OPTIONAL 404 structurally. I followed the location the issue names.listProtocolsis left out, it does not exist yet. Raised as Protocol Discovery and Basic Messages are missing from the v2 DIDComm API #683.