feat: Add support for WebSocket transports #256

jakubkoci · 2021-05-02T10:17:18Z

This is how it works now:

A mediator takes endpoint for invitation or DidDoc from config.
An edge agent takes endpoint for invitation or DidDoc from inboundConnection if it has any.
The edge agent gets out of band invitation via http request to mediator (http://mediator1.com/invitation). The invitation contains ws endpoint because we want to make a connection via WebSocket.
The edge agent takes an endpoint for outbound communication from invitation or DidDoc. For a connection with another edge agent, it takes an endpoint from indbound connection. Therefore it’s the same ws endpoint as it is contained in the mediator invitation.

To integrate ws and http servers together and support multiple transports we would need to enable following:

A mediator sends a different endpoint for mediation to edge agent.
The edge agent sets the mediation endpoint somewhere else and do not take it from inbound connection invitation or DidDoc.

Other notes:

I set ws endpoint directly in mediator server by editing config.endpoint to value ws://localhost:${PORT}
I skip the ws tests. To run them it's needed to change the mediator server to mediator-ws.ts (or integrate ws and HTTP together as I mentioned)

Related to #250

codecov-commenter · 2021-05-02T10:25:29Z

Codecov Report

Merging #256 (e9a983f) into main (33c85ba) will increase coverage by 0.15%.
The diff coverage is 98.63%.

@@            Coverage Diff             @@
##             main     #256      +/-   ##
==========================================
+ Coverage   89.47%   89.62%   +0.15%     
==========================================
  Files         195      196       +1     
  Lines        3581     3645      +64     
  Branches      393      403      +10     
==========================================
+ Hits         3204     3267      +63     
- Misses        377      378       +1

Impacted Files	Coverage Δ
...modules/connections/repository/ConnectionRecord.ts	`96.22% <ø> (-0.14%)`	⬇️
src/types.ts	`100.00% <ø> (ø)`
src/agent/MessageReceiver.ts	`86.20% <80.00%> (-0.84%)`	⬇️
src/agent/Agent.ts	`98.57% <100.00%> (+0.04%)`	⬆️
src/agent/AgentConfig.ts	`95.34% <100.00%> (+0.11%)`	⬆️
src/agent/Dispatcher.ts	`96.96% <100.00%> (+0.19%)`	⬆️
src/agent/MessageSender.ts	`95.00% <100.00%> (+2.14%)`	⬆️
src/agent/TransportService.ts	`100.00% <100.00%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 33c85ba...e9a983f. Read the comment docs.

samples/mediator-ws.ts

TimoGlastra · 2021-05-07T07:37:09Z

samples/mediator-ws.ts

@@ -0,0 +1,140 @@
+import express from 'express'


Seems like we could get away with a bit less duplication in the http/ws mediator files

TimoGlastra · 2021-05-07T07:38:09Z

src/agent/Agent.ts

@@ -159,8 +162,8 @@ export class Agent {
    return this.agentConfig.mediatorUrl
  }

-  public async receiveMessage(inboundPackedMessage: unknown) {
-    return await this.messageReceiver.receiveMessage(inboundPackedMessage)
+  public async receiveMessage(inboundPackedMessage: unknown, transport?: Transport) {


I would rather create a single InboundMessage or InboundSession that contains both the packed message, the transport and any other context that may arise in the future.

What about creating the InboundMessageContext here and then enhanced it with other attributes inside MessageReciever?

I would rather not introduce yet another type because we already have perhaps too much (XxxMessage, XxxMessageContext, XxxPackage, ...) Then we could do the same for the outbound messages as OutboundMessageContext.

Ah we already have InboundMessageContext of course. Yes that sounds perfect!

It seems like a much larger change than I anticipated. I suggest doing it in separate PR.

We would need to add wireMessage into InboundMessageContext and make message optional but then validate everywhere that it's present. Or we can figure out a different solution.

src/agent/EnvelopeService.ts

src/agent/TransportService.ts

TimoGlastra · 2021-05-07T07:52:24Z

src/agent/TransportService.ts

+
+  private findEndpoint(connection: ConnectionRecord) {
+    if (connection.theirDidDoc) {
+      const endpoint = connection.theirDidDoc.service[0].serviceEndpoint


didCommServices returns only the didcomm services (diddoc may include other service types) and also automatically orders the transports by priority.

We should probably also update this method to pass in supported outbound transport /schemes so we don't return the first transport scheme that may be my_custom_scheme:// while the second service is http:// which we do support

Suggested change

const endpoint = connection.theirDidDoc.service[0].serviceEndpoint

const endpoint = connection.theirDidDoc.didCommServices[0].serviceEndpoint

Also, this could potentially throw an error if there are no service endpoints. Dunno if we want to check that here

TimoGlastra · 2021-05-07T08:00:16Z

src/agent/TransportService.ts

+    this.transportTable[connectionId] = transport
+  }
+
+  public resolveTransport(connection: ConnectionRecord): Transport {


Would be nice if we can make this more dynamic. this implementation requires to support the transport in core. Ideally adding an outbound/inbound transport class is enough

Agree. I think we would need to move this logic to outbound transporters and just provide some utility functions to get the endpoint from DidDoc instead of having it in the core. That would also solve your other suggestions like "update this method to pass in supported outbound transport /schemes".

src/agent/TransportService.ts

Signed-off-by: Jakub Koci <jakub.koci@gmail.com>

jakubkoci · 2021-05-11T14:40:17Z

@TimoGlastra Updated the PR according to your suggestions.

I think we can update and refactor the transport resolve mechanism when we add multi-transport. It will be more obvious how to structure the code then.

jakubkoci · 2021-05-11T14:41:45Z

I added another two mediators to test WebSocket transports. So now, we run tests for both HTTP and websocket.

TimoGlastra

LGTM.

This introduces some technical debt with how transports are managed that we can hopefully fix in the near future, but I think it is good to get this merged now.

TimoGlastra · 2021-05-12T07:40:49Z

The release pipeline is failing. I'll take a look at it today

TimoGlastra reviewed May 7, 2021

View reviewed changes

TimoGlastra mentioned this pull request May 7, 2021

Websockets #218

Closed

TimoGlastra linked an issue May 7, 2021 that may be closed by this pull request

Add transport into message context #81

Closed

jakubkoci added 12 commits May 10, 2021 15:12

WIP: Add websockets transports

89b67ab

Signed-off-by: Jakub Koci <jakub.koci@gmail.com>

Refactor transport resolver from sender to transport service

d209a7e

Signed-off-by: Jakub Koci <jakub.koci@gmail.com>

Use e2e test with http and skip ws tests

b807788

Signed-off-by: Jakub Koci <jakub.koci@gmail.com>

Small updates based on code review

0213b2b

Signed-off-by: Jakub Koci <jakub.koci@gmail.com>

Run ws mediator as separate service along http mediator

72dc77d

Signed-off-by: Jakub Koci <jakub.koci@gmail.com>

Update ci to run ws mediator along http

216e891

Signed-off-by: Jakub Koci <jakub.koci@gmail.com>

Remove listeners and close socket after tests

7c92839

Signed-off-by: Jakub Koci <jakub.koci@gmail.com>

Run jest with --detectOpenHandles

37f0acd

Signed-off-by: Jakub Koci <jakub.koci@gmail.com>

Move closing websockets before message flush await

7d89cbc

Signed-off-by: Jakub Koci <jakub.koci@gmail.com>

Fix stop method to close sockets

3a3069d

Signed-off-by: Jakub Koci <jakub.koci@gmail.com>

Use test logger in e2e ws test

d6b1c26

Signed-off-by: Jakub Koci <jakub.koci@gmail.com>

Re-use queue transport endpoint constant

8934710

Signed-off-by: Jakub Koci <jakub.koci@gmail.com>

jakubkoci marked this pull request as ready for review May 11, 2021 14:36

jakubkoci requested a review from a team as a code owner May 11, 2021 14:36

Update websocket transport type

e9a983f

Signed-off-by: Jakub Koci <jakub.koci@gmail.com>

TimoGlastra approved these changes May 11, 2021

View reviewed changes

jakubkoci merged commit 07b479f into openwallet-foundation:main May 12, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Add support for WebSocket transports #256

feat: Add support for WebSocket transports #256

jakubkoci commented May 2, 2021 •

edited

Loading

codecov-commenter commented May 2, 2021 •

edited

Loading

TimoGlastra May 7, 2021

TimoGlastra May 7, 2021

jakubkoci May 10, 2021

TimoGlastra May 10, 2021

jakubkoci May 11, 2021 •

edited

Loading

TimoGlastra May 7, 2021 •

edited

Loading

TimoGlastra May 7, 2021

TimoGlastra May 7, 2021 •

edited

Loading

jakubkoci May 10, 2021

jakubkoci commented May 11, 2021

jakubkoci commented May 11, 2021

TimoGlastra left a comment

TimoGlastra commented May 12, 2021

	const endpoint = connection.theirDidDoc.service[0].serviceEndpoint
	const endpoint = connection.theirDidDoc.didCommServices[0].serviceEndpoint

feat: Add support for WebSocket transports #256

feat: Add support for WebSocket transports #256

Conversation

jakubkoci commented May 2, 2021 • edited Loading

codecov-commenter commented May 2, 2021 • edited Loading

Codecov Report

TimoGlastra May 7, 2021

Choose a reason for hiding this comment

TimoGlastra May 7, 2021

Choose a reason for hiding this comment

jakubkoci May 10, 2021

Choose a reason for hiding this comment

TimoGlastra May 10, 2021

Choose a reason for hiding this comment

jakubkoci May 11, 2021 • edited Loading

Choose a reason for hiding this comment

TimoGlastra May 7, 2021 • edited Loading

Choose a reason for hiding this comment

TimoGlastra May 7, 2021

Choose a reason for hiding this comment

TimoGlastra May 7, 2021 • edited Loading

Choose a reason for hiding this comment

jakubkoci May 10, 2021

Choose a reason for hiding this comment

jakubkoci commented May 11, 2021

jakubkoci commented May 11, 2021

TimoGlastra left a comment

Choose a reason for hiding this comment

TimoGlastra commented May 12, 2021

jakubkoci commented May 2, 2021 •

edited

Loading

codecov-commenter commented May 2, 2021 •

edited

Loading

jakubkoci May 11, 2021 •

edited

Loading

TimoGlastra May 7, 2021 •

edited

Loading

TimoGlastra May 7, 2021 •

edited

Loading