fix: mediator transports #419

burdettadam · 2021-08-05T14:31:22Z

This PR has code that corrects issues related to mediation.
Corrections

multiple restarts with the same mediation invitation
second sorting of connection transport services based on inbound services
multiple outbound transport support
- add transport is now register transport, which allows using multiple types of transport.

codecov-commenter · 2021-08-06T20:32:57Z

Codecov Report

Merging #419 (f3dcc2d) into main (0e2338f) will decrease coverage by 0.64%.
The diff coverage is 51.30%.

@@            Coverage Diff             @@
##             main     #419      +/-   ##
==========================================
- Coverage   86.24%   85.60%   -0.65%     
==========================================
  Files         249      249              
  Lines        5264     5342      +78     
  Branches      784      798      +14     
==========================================
+ Hits         4540     4573      +33     
- Misses        724      769      +45

Impacted Files	Coverage Δ
.../core/src/modules/connections/ConnectionsModule.ts	`72.15% <0.00%> (-1.88%)`	⬇️
...ckages/core/src/transport/WsOutboundTransporter.ts	`3.94% <0.00%> (-1.32%)`	⬇️
...ckages/core/src/modules/routing/RecipientModule.ts	`63.04% <60.52%> (-0.96%)`	⬇️
packages/core/src/agent/MessageSender.ts	`71.92% <62.16%> (-2.30%)`	⬇️
packages/core/src/agent/Agent.ts	`94.68% <77.77%> (-2.20%)`	⬇️
.../modules/connections/models/did/service/Service.ts	`100.00% <100.00%> (ø)`
.../modules/connections/services/ConnectionService.ts	`98.83% <100.00%> (+0.01%)`	⬆️
...c/modules/routing/messages/KeylistUpdateMessage.ts	`100.00% <100.00%> (ø)`
...s/routing/messages/KeylistUpdateResponseMessage.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 0e2338f...f3dcc2d. Read the comment docs.

TimoGlastra

Thanks @burdettadam!

I think this PR packs some good changes with some other changes that need some ironing out. I think it would be ideal to split up the PR, as the fixes for the mediator should be resolved ASAP I think

TimoGlastra · 2021-08-09T13:15:14Z

packages/core/src/agent/Agent.ts

-    if (this.outboundTransporter) {
-      await this.outboundTransporter.start(this)
+    for (const transport of this.messageSender.outboundTransporters) {
+      transport?.start(this)


Suggested change

transport?.start(this)

transport.start(this)

TimoGlastra · 2021-08-09T13:15:49Z

packages/core/src/agent/Agent.ts

@@ -192,7 +220,9 @@ export class Agent {
    this.agentConfig.stop$.next(true)

    // Stop transports
-    await this.outboundTransporter?.stop()
+    for (const transport of this.messageSender.outboundTransporters) {
+      transport?.stop()


Suggested change

transport?.stop()

transport.stop()

TimoGlastra · 2021-08-10T08:56:44Z

packages/core/src/agent/Agent.ts

-      await this.mediationRecipient.setDefaultMediator(mediationRecord)
+      const invitation = await ConnectionInvitationMessage.fromUrl(mediatorConnectionsInvite)
+      // Check if invitation has been used already
+      const connections = await this.connections.getAll()


As mentioned previously, we can query by invitation key here to prevent retrieving all connections

Great point! +1

invitation key is the recipient key of the invitation? what if there is more than one recipient key?

TimoGlastra · 2021-08-10T08:57:42Z

packages/core/src/agent/Agent.ts

+        if (connection.invitation?.id === invitation.id) {
+          this.logger.warn(
+            `Mediator Invitation in configuration has already been used to ${
+              connection.isReady ? 'make' : 'initialize'


I don't think the difference between 'make' and 'initialize' adds a lot of value here. Maybe just 'create'?

I tend to agree--I'd potentially rather have two logs that call out whether the connection has been established and mediation granted.

TimoGlastra · 2021-08-10T09:00:20Z

packages/core/src/agent/Agent.ts

+            } a connection`
+          )
+          if (connection.isReady) {
+            const mediator = await this.mediationRecipient.findByConnectionId(connection.id)


Maybe we should throw an error or something if the connection is not ready yet? It seems like there's a gap in processing when the connection is created, but not finished yet. How would a user recover from such a situation?

this code is to report information about mediator connection invitations that have already been used. To recover from a connection from a mediator invitation that is not ready the consumer of AFJ would have to remove the invitation from the config, restart AFJ, and remove the connection or get it in a ready state. I will throw an error.

TimoGlastra · 2021-08-10T15:08:30Z

packages/redux-store/src/slices/agent.ts

@@ -1,6 +1,8 @@
 import type { AgentThunkApiConfig } from '../utils'
+// eslint-disable-next-line import/no-unresolved


I'm not having issues with no-unresolved. Maybe a yarn install will do the trick :)

TimoGlastra · 2021-08-10T15:10:01Z

tests/e2e.test.ts

@@ -0,0 +1,218 @@
+import type { SubjectMessage } from './transport/SubjectInboundTransport'


e2e.test.ts is replaced by more specific transport tests: e2e-ws.test.ts, e2e-subject.test.ts, e2e-http.test.ts

TimoGlastra · 2021-08-10T15:10:36Z

tests/transport/SubjectOutboundTransport.ts

@@ -11,7 +11,7 @@ export class SubjectOutboundTransporter implements OutboundTransporter {
  private ourSubject: Subject<SubjectMessage>
  private subjectMap: { [key: string]: Subject<SubjectMessage> | undefined }

-  public supportedSchemes = []
+  public supportedSchemes = ['subject', 'http', 'https', 'rxjs']


Would be good if we can use either subject or rxjs everywhere and remove all other ones

I was going to ask that. I will update this to use rxjs.

TimoGlastra · 2021-08-10T20:15:54Z

packages/core/src/agent/MessageSender.ts

@@ -93,26 +115,31 @@ export class MessageSender {
    }

    // Retrieve DIDComm services
-    const allServices = this.transportService.findDidCommServices(connection)
+    const allServices = this.transportService.findDidCommServices(connection, this.supportedTransports)


As I understand the code now, it seems like didcomm:transport/queue is not taken into account here. Maybe I'm missing something, but that would mean all didcomm:tranport/queue services are ignored.

As I'm thinking of it right now, maybe we can do something like the following:

findDidCommServices returns a diddoc based priority sorted list of all services (already did that)

We loop through the didoc services and for each service we check if we can send it using one of the registered transports (transport.suportedSchemes)

If none of the inbound or outbound transports can deliver the message, we check if a queue service is included. If this is the case, we queue the message

Addition could be made between 1. and 2. where we sort the return services from findDidCommServices, based on a preferred protocol scheme for that message. This would be useful in the mediation recipient class where we do message pickup from the mediator.

if I am not mistaken, sending a message on a connection with their did_doc containing only services with didcomm:transport/queue would error without explicit details, no queueing supported. The code ignores them. This will need to be updated with the check for a queue service described in suggestion 3.

Moving the preferred protocol scheme sort into the mediation recipient class would work for message pickup from mediators, but would not catch any other message sending. A nice feature of having the sort in the message sender is speed. While establishing a connection between an edge agent and an agent that supports WebSockets, the edge agent may initialize the connection protocol using HTTP, once the edge agent receives the other agent's did_doc the edge agent will finish establishing the connection using WebSockets. Why have multiple services in a did_doc if we are restricted to use the service a connection invitation is created with?

We could try and build a preferred protocol scheme by using findDidCommServices on senders did_doc. This will return the sender's inbound services, not the senders outbound services. I assumed we would need to update the did_doc services to have services that define sending services alongside the receiving services. To avoid changing/learning the definition of a service in a did_doc and supported code changes we opted to do list sorting and filtering based on a list of supported schemas returned from a list of registered transporters.
If you could provide an example of a did_doc with services that represent supported outbound transports we can update the message sender to use this. Another issue arises if we use the sender's did_doc services to build a preferred protocol list. connections and the did_docs associated are persisted, while transporters are not. We would need to check the did_doc services at the start of AFJ and update them along with the agent on the other end of the connection.

I have updated the code. didcomm:transport/queue is being handled correctly, I didn't notice that after the merge, no need to fix it.
The proposed flow is the intended behavior of this code except for priority sort. instead of moving priority sorting into mediation messages I have added an optional parameter priority with a default in send message and send package. sendMessage(outboundMessage: OutboundMessage, priority = false).
If priority is true we pass in a secondary list to findDidCommServices, otherwise an empty list.

// Retrieve DIDComm services let allServices: Array<DidCommService | IndyAgentService> if (priority) { allServices = this.transportService.findDidCommServices(connection, this.supportedTransportSchemes) } else { allServices = this.transportService.findDidCommServices(connection, []) }

Now any messaging were a consumer of AFJ wishes to prioritize supported transports can be sent by setting the priority to true. Enabling desired features for mediated messages while keeping the current functionality of the message sender.

TimoGlastra · 2021-08-10T20:21:58Z

packages/core/src/agent/MessageSender.ts

@@ -250,7 +275,14 @@ export class MessageSender {
    }

    const outboundPackage = await this.packMessage({ message, keys, endpoint: service.serviceEndpoint })
-    await this.outboundTransporter.sendMessage(outboundPackage)
+    outboundPackage.endpoint = service.serviceEndpoint
+    const protocol = outboundPackage.endpoint.split(':')[0]


Maybe we can add protocolScheme getter to the service class?

so we can do

transport.supportedSchemes.includes(service.protocolScheme)

I would use a protocolScheme getter if the service class had it. We should add it.

Awesome! Could you add it as part of this PR?

JamesKEbert · 2021-08-11T02:25:21Z

Awesome work here @burdettadam! I'd agree here with @TimoGlastra that splitting this into multiple PRs is likely helpful to work through the various aspects.
Also @TimoGlastra happy to talk with you guys on a call or at the AFJ call on Thursday if helpful.

jakubkoci

Just added a few comments for the case you discussed that during the call today.

packages/core/src/agent/Agent.ts

jakubkoci · 2021-08-12T14:06:53Z

packages/core/src/agent/TransportService.ts

+  public findDidCommServices(
+    connection: ConnectionRecord,
+    supportedProtocols: string[]
+  ): Array<DidCommService | IndyAgentService> {
    if (connection.theirDidDoc) {
-      return connection.theirDidDoc.didCommServices
+      // map for efficient lookup of sortIndex
+      const supportedProtocolsIndexTable = new Map(supportedProtocols.map((v, i) => [v, i]))
+      const services = connection.theirDidDoc.didCommServices
+      // filter out any un-supported
+      const filteredServices = services.filter((service) =>
+        supportedProtocols.includes(service.serviceEndpoint.split(':')[0])
+      )
+      // sort by protocol, if same protocol, sort by priority
+      filteredServices.sort(function (
+        serviceA: { serviceEndpoint: string; priority: number },
+        serviceB: { serviceEndpoint: string; priority: number }
+      ) {
+        const protocolA = serviceA.serviceEndpoint.split(':')[0] || ''
+        const protocolB = serviceB.serviceEndpoint.split(':')[0] || ''
+        const preferred =
+          (supportedProtocolsIndexTable.get(protocolA) || 0) - (supportedProtocolsIndexTable.get(protocolB) || 0)
+        const priority = serviceA.priority - serviceB.priority
+        return preferred || priority
+      })
+      return filteredServices


I agree with Timo here. We should always prefer didComm service priorities and do not filter them according currently supported transports. Filtering can happen after we have more services without any specific priority.

burdettadam · 2021-08-13T20:10:05Z

packages/core/src/modules/routing/RecipientModule.ts

+            )
+            if (mediator.isReady) {
+              defaultMediatorBootstrapped = true
+            }


if the mediator is not ready, throw new Errror('')

TimoGlastra

I left another round of comments. I think we're almost there. Lots of great improvements! :)

packages/core/src/agent/MessageSender.ts

TimoGlastra · 2021-08-16T14:03:40Z

packages/core/src/agent/MessageSender.ts

+        const protocol = service.serviceEndpoint.split(':')[0]
+        for (const transport of this.outboundTransporters) {
+          if (transport.supportedSchemes.includes(protocol)) {
+            await transport.sendMessage({
+              payload: packedMessage,
+              endpoint: service.serviceEndpoint,
+            })
+            break
+          }
+        }


I think we should extract this to a separate method as it is duplicated a few times:

Suggested change

const protocol = service.serviceEndpoint.split(':')[0]

for (const transport of this.outboundTransporters) {

if (transport.supportedSchemes.includes(protocol)) {

await transport.sendMessage({

payload: packedMessage,

endpoint: service.serviceEndpoint,

})

break

}

}

const transports = this.findTransportsForService(service)

for (const transport of transports) {

await transport.sendMessage({

payload: packedMessage,

endpoint: service.serviceEndpoint,

})

break

}

TimoGlastra · 2021-08-16T14:04:35Z

packages/core/src/agent/MessageSender.ts

@@ -250,7 +275,14 @@ export class MessageSender {
    }

    const outboundPackage = await this.packMessage({ message, keys, endpoint: service.serviceEndpoint })
-    await this.outboundTransporter.sendMessage(outboundPackage)
+    outboundPackage.endpoint = service.serviceEndpoint
+    const protocol = outboundPackage.endpoint.split(':')[0]


Awesome! Could you add it as part of this PR?

packages/core/src/agent/MessageSender.ts

TimoGlastra · 2021-08-16T14:13:18Z

packages/core/src/modules/connections/services/ConnectionService.ts

+    // Check if already done
+    const connection = await this.connectionRepository.findById(connectionId)
+    if (connection && isConnected(connection)) return connection //TODO: check if this leaves trailing listeners behind?


I think this could introduce a realllllly small gap where we retrieve the record, and before the listener is started, the connection state is updated to done.

By starting the listener before doing findById, this could never happen. Is there a specific reason you changed the order?

I think we could clean this up a bit in the future, but for now I think how it was implemented before works fine

packages/core/src/modules/routing/RecipientModule.ts

TimoGlastra · 2021-08-16T14:18:38Z

packages/core/src/modules/routing/RecipientModule.ts

+        routing: routing,
+      })
+      this.logger.debug('Processed invitation')
+      const { message, connectionRecord: connectionRecord } = await this.connectionService.createRequest(


Suggested change

const { message, connectionRecord: connectionRecord } = await this.connectionService.createRequest(

const { message, connectionRecord } = await this.connectionService.createRequest(

packages/core/src/modules/routing/RecipientModule.ts

TimoGlastra · 2021-08-16T14:20:31Z

packages/core/src/modules/routing/RecipientModule.ts

+      const { message, connectionRecord: connectionRecord } = await this.connectionService.createRequest(
+        invitationConnectionRecord.id
+      )
+      const outbound = createOutboundMessage(connectionRecord, message)


We should find a way around this as we're exactly duplicating the connection module code here (not for this PR, just something to think about)

Do we want to be able to import the module into a module? I don't think right now the modules are set up to be used via dependency injection IIRC.

No as we've set it up, modules don't use modules. But it has introduced some limitations...

TimoGlastra · 2021-08-16T14:22:08Z

packages/core/src/modules/routing/RecipientModule.ts

+      this.logger.debug('Default mediator set')
+      return
+    } else if (connection && !connection.isReady) {
+      const connectionRecord = await this.connectionService.returnWhenIsConnected(connection.id)


This will probably never resolve. If we need to wait for an action from the other party we're fine. However if we shutdown the agent mid connection and never sent the message, we must manually trigger the next step in the protocol. Not sure what the best approach is here.

commit 41646d8d93cf1e16cad2033aeace7949864246ac Author: Adam Burdett <[email protected]> Date: Mon Aug 16 11:07:24 2021 -0600 Update packages/core/src/agent/MessageSender.ts Co-authored-by: Timo Glastra <[email protected]> Signed-off-by: Adam Burdett <[email protected]> commit de9d5d3 Author: Adam Burdett <[email protected]> Date: Mon Aug 16 11:06:28 2021 -0600 Update packages/core/src/agent/MessageSender.ts Co-authored-by: Timo Glastra <[email protected]> commit d9b9164 Author: James Ebert <[email protected]> Date: Fri Aug 13 18:07:37 2021 -0600 chore: formatted Signed-off-by: James Ebert <[email protected]> commit 335286a Author: James Ebert <[email protected]> Date: Fri Aug 13 18:03:17 2021 -0600 chore: formatted changes Signed-off-by: James Ebert <[email protected]> commit a639d1d Author: James Ebert <[email protected]> Date: Fri Aug 13 17:59:42 2021 -0600 fix: added connection request sending and added logs Signed-off-by: James Ebert <[email protected]> commit 2800d92 Author: Adam Burdett <[email protected]> Date: Fri Aug 13 16:28:26 2021 -0600 even better provision code Signed-off-by: Adam Burdett <[email protected]> commit 2a153e0 Author: Adam Burdett <[email protected]> Date: Fri Aug 13 16:11:30 2021 -0600 throw errer Signed-off-by: Adam Burdett <[email protected]> commit 8d62fcb Author: Adam Burdett <[email protected]> Date: Fri Aug 13 16:07:58 2021 -0600 better provision code Signed-off-by: Adam Burdett <[email protected]> commit 47db23e Author: Adam Burdett <[email protected]> Date: Fri Aug 13 13:35:37 2021 -0600 sort instead of filter cat Signed-off-by: Adam Burdett <[email protected]> commit 343ff78 Author: Adam Burdett <[email protected]> Date: Fri Aug 13 12:45:49 2021 -0600 update test for error message Signed-off-by: Adam Burdett <[email protected]> commit d7dff12 Author: Adam Burdett <[email protected]> Date: Fri Aug 13 12:29:36 2021 -0600 remove duplicate services Signed-off-by: Adam Burdett <[email protected]> commit 1f204d8 Author: Adam Burdett <[email protected]> Date: Fri Aug 13 12:15:45 2021 -0600 removed dead code Signed-off-by: Adam Burdett <[email protected]> commit 3961197 Author: Adam Burdett <[email protected]> Date: Fri Aug 13 12:09:08 2021 -0600 less flexible code Signed-off-by: Adam Burdett <[email protected]> commit 81e3477 Author: Adam Burdett <[email protected]> Date: Fri Aug 13 11:30:16 2021 -0600 remove un-needed changes Signed-off-by: Adam Burdett <[email protected]> commit 361c28a Author: Adam Burdett <[email protected]> Date: Fri Aug 13 09:44:32 2021 -0600 remove throw new error Signed-off-by: Adam Burdett <[email protected]> commit 203824c Author: Adam Burdett <[email protected]> Date: Thu Aug 12 18:00:52 2021 -0600 update send message in RM Signed-off-by: Adam Burdett <[email protected]> commit abea05a Author: Adam Burdett <[email protected]> Date: Thu Aug 12 17:49:25 2021 -0600 handle empty supported Protocols in TS Signed-off-by: Adam Burdett <[email protected]> commit 9b3a78e Author: Adam Burdett <[email protected]> Date: Thu Aug 12 17:33:18 2021 -0600 default declared Signed-off-by: Adam Burdett <[email protected]> commit 9da1fd3 Author: Adam Burdett <[email protected]> Date: Thu Aug 12 17:25:13 2021 -0600 requested changes Signed-off-by: Adam Burdett <[email protected]> commit 356287d Author: Adam Burdett <[email protected]> Date: Tue Aug 10 00:04:18 2021 -0600 formatting Signed-off-by: Adam Burdett <[email protected]> commit 9cefd01 Author: Adam Burdett <[email protected]> Date: Mon Aug 9 23:59:08 2021 -0600 e2e updates Signed-off-by: Adam Burdett <[email protected]> commit 812c60f Author: Adam Burdett <[email protected]> Date: Mon Aug 9 12:44:53 2021 -0600 added schemes on subjectOutBoundTransport Signed-off-by: Adam Burdett <[email protected]> commit 87bdd8f Author: Adam Burdett <[email protected]> Date: Fri Aug 6 13:49:29 2021 -0600 formatting Signed-off-by: Adam Burdett <[email protected]> commit 5e44efd Merge: dd1f1f9 f0cf209 Author: Adam Burdett <[email protected]> Date: Fri Aug 6 13:28:08 2021 -0600 Merge branch 'main' of github.com:hyperledger/aries-framework-javascript into fix/mediator-transports commit dd1f1f9 Author: Adam Burdett <[email protected]> Date: Tue Aug 3 18:23:28 2021 -0600 ignore unresolved reduxjs Signed-off-by: Adam Burdett <[email protected]> commit 77fe594 Author: Adam Burdett <[email protected]> Date: Tue Aug 3 17:39:47 2021 -0600 fix for mediator Signed-off-by: Adam Burdett <[email protected]> commit 27b4818 Author: Adam Burdett <[email protected]> Date: Wed Jul 28 15:55:36 2021 -0600 multiple socket timeout support Signed-off-by: Adam Burdett <[email protected]> commit 3e6640f Author: Adam Burdett <[email protected]> Date: Wed Jul 28 15:40:00 2021 -0600 propper recursive backoff Signed-off-by: Adam Burdett <[email protected]> commit 6814600 Author: Adam Burdett <[email protected]> Date: Wed Jul 28 14:57:36 2021 -0600 logger Signed-off-by: Adam Burdett <[email protected]> commit 8bcd966 Author: Adam Burdett <[email protected]> Date: Wed Jul 28 14:55:27 2021 -0600 timeout time Signed-off-by: Adam Burdett <[email protected]> commit 12de34e Author: Adam Burdett <[email protected]> Date: Wed Jul 28 14:54:32 2021 -0600 timeout Signed-off-by: Adam Burdett <[email protected]> commit dccb596 Author: Adam Burdett <[email protected]> Date: Wed Jul 28 14:50:19 2021 -0600 recursive backoff Signed-off-by: Adam Burdett <[email protected]> commit bc4762d Author: Adam Burdett <[email protected]> Date: Wed Jul 28 13:42:02 2021 -0600 keep sockets with mediators open Signed-off-by: Adam Burdett <[email protected]> commit d67710c Author: Adam Burdett <[email protected]> Date: Wed Jul 28 12:50:35 2021 -0600 id check Signed-off-by: Adam Burdett <[email protected]> commit 23fe187 Author: Adam Burdett <[email protected]> Date: Wed Jul 28 10:58:18 2021 -0600 check invite during init Signed-off-by: Adam Burdett <[email protected]> commit cfd7ece Author: Adam Burdett <[email protected]> Date: Wed Jul 28 09:59:14 2021 -0600 correct protocal type Signed-off-by: Adam Burdett <[email protected]> commit 178bd6c Author: Adam Burdett <[email protected]> Date: Wed Jul 28 09:55:35 2021 -0600 indy wallet friendly bits Signed-off-by: Adam Burdett <[email protected]> commit 4db4da6 Author: Adam Burdett <[email protected]> Date: Tue Jul 27 18:31:42 2021 -0600 utf-8 decode ws event.data Signed-off-by: Adam Burdett <[email protected]> commit 9d732c8 Author: Adam Burdett <[email protected]> Date: Tue Jul 27 18:09:47 2021 -0600 if(0) does not work. Signed-off-by: Adam Burdett <[email protected]> commit a8a3fb3 Author: Adam Burdett <[email protected]> Date: Mon Jul 26 16:39:24 2021 -0600 lint import ordering Signed-off-by: Adam Burdett <[email protected]> commit af6431d Author: Adam Burdett <[email protected]> Date: Mon Jul 26 14:22:42 2021 -0600 structure fix Signed-off-by: Adam Burdett <[email protected]> commit 20ba447 Author: Adam Burdett <[email protected]> Date: Fri Jul 23 18:40:35 2021 -0600 broken message sender tests Signed-off-by: Adam Burdett <[email protected]> commit 3316e0d Author: Adam Burdett <[email protected]> Date: Wed Jul 21 18:34:58 2021 -0600 sorted transports Signed-off-by: Adam Burdett <[email protected]> Signed-off-by: Adam Burdett <[email protected]>

Signed-off-by: Adam Burdett <[email protected]>

Signed-off-by: Adam Burdett <[email protected]> Co-authored-by: Timo Glastra <[email protected]>

Signed-off-by: James Ebert <[email protected]>

Signed-off-by: Adam Burdett <[email protected]>

packages/core/src/agent/MessageSender.ts

TimoGlastra · 2021-08-17T11:24:08Z

packages/core/src/agent/MessageSender.ts

+
+    this.logger.debug(
+      `Retrieved ${services.length} services for message to connection '${connection.id}'${
+        ' (' && connection.theirLabel && ')'


I think this will still just render the value of theirLabel so (undefined). So we can just do this I think:

'(${connection.theirLabel})'

TimoGlastra

Still some improvements to be made, but I think it is good enough to merge now.

One thing that would be a good addition is having an e2e test that creates a connection with mediator using mediator connections invite, shuts down the agent and then restarts the agent and creates a new connection with another agent. But this can also be added in a separate PR.

TimoGlastra · 2021-08-18T10:44:10Z

@jakubkoci @JamesKEbert @burdettadam Can I go ahead and merge this PR?

JamesKEbert

The changes look good to me--we aren't actually making use of the priority options for message sending from the RecipientModule for implicit mediation, and I've also been working on a revamp for the message sender. However, I'd prefer to merge this (in the better state it's in), and do followup PR / work, especially since it fixes the agent mediation restart issues. I think this is a big topic (especially around transports), so I think there's more to do, but I'd rather merge this now.

Signed-off-by: Adam Burdett <[email protected]> Co-authored-by: Timo Glastra <[email protected]>

Signed-off-by: Adam Burdett <[email protected]>

jakubkoci

I'm good with merging.

jakubkoci · 2021-08-19T13:44:48Z

packages/core/src/transport/WsOutboundTransporter.ts

+        if (this.continue) {
+          const mediators = await this.agent.mediationRecipient.getMediators()
+          const mediatorConnIds = mediators.map((mediator) => mediator.connectionId)
+          if (mediatorConnIds.includes(socketId)) {
+            this.logger.debug(`WebSocket attempting to reconnect to ${endpoint}`)
+            // send trustPing to mediator to open socket
+            let interval = 100
+            if (this.recursiveBackOff[socketId as string]) {
+              interval = this.recursiveBackOff[socketId]
+            }
+            setTimeout(
+              () => {
+                this.agent.connections.acceptResponse(socketId)
+              },
+              interval < 1000 ? interval : 1000
+            )
+            this.recursiveBackOff[socketId] = interval * 2
+          }
+        }


Shouldn't be it setInterval instead of setTimout then? But it's highly probable I didn't understand the logic here 😄

jakubkoci · 2021-08-19T13:45:40Z

packages/core/src/transport/WsOutboundTransporter.ts

+        if (this.continue) {
+          const mediators = await this.agent.mediationRecipient.getMediators()
+          const mediatorConnIds = mediators.map((mediator) => mediator.connectionId)
+          if (mediatorConnIds.includes(socketId)) {
+            this.logger.debug(`WebSocket attempting to reconnect to ${endpoint}`)
+            // send trustPing to mediator to open socket
+            let interval = 100
+            if (this.recursiveBackOff[socketId as string]) {
+              interval = this.recursiveBackOff[socketId]
+            }
+            setTimeout(
+              () => {
+                this.agent.connections.acceptResponse(socketId)
+              },
+              interval < 1000 ? interval : 1000
+            )
+            this.recursiveBackOff[socketId] = interval * 2
+          }
+        }


Do I understand it correctly that socketId == connectionId here?

burdettadam marked this pull request as ready for review August 9, 2021 15:07

burdettadam requested a review from a team as a code owner August 9, 2021 15:07

TimoGlastra requested changes Aug 10, 2021

View reviewed changes

jakubkoci reviewed Aug 12, 2021

View reviewed changes

burdettadam commented Aug 13, 2021

View reviewed changes

TimoGlastra reviewed Aug 16, 2021

View reviewed changes

burdettadam added 2 commits August 16, 2021 11:34

formatting

791b740

Signed-off-by: Adam Burdett <[email protected]>

burdettadam force-pushed the fix/mediator-transports branch from a70c054 to 791b740 Compare August 16, 2021 17:47

burdettadam and others added 5 commits August 16, 2021 12:10

Update packages/core/src/modules/routing/RecipientModule.ts

c468e33

Signed-off-by: Adam Burdett <[email protected]> Co-authored-by: Timo Glastra <[email protected]>

Update packages/core/src/modules/routing/RecipientModule.ts

15d517e

Signed-off-by: Adam Burdett <[email protected]> Co-authored-by: Timo Glastra <[email protected]>

Update packages/core/src/modules/routing/RecipientModule.ts

3e6da2f

Signed-off-by: Adam Burdett <[email protected]> Co-authored-by: Timo Glastra <[email protected]>

feat: created service retrieval with priority sorting

b17d85a

Signed-off-by: James Ebert <[email protected]>

add get protocal scheme on service class

4f1da15

Signed-off-by: Adam Burdett <[email protected]>

TimoGlastra reviewed Aug 17, 2021

View reviewed changes

packages/core/src/agent/MessageSender.ts Outdated Show resolved Hide resolved

TimoGlastra reviewed Aug 17, 2021

View reviewed changes

TimoGlastra approved these changes Aug 17, 2021

View reviewed changes

TimoGlastra changed the title ~~Fix/mediator transports~~ fix: mediator transports Aug 17, 2021

JamesKEbert approved these changes Aug 18, 2021

View reviewed changes

burdettadam mentioned this pull request Aug 18, 2021

failed recipient mediation provision call, recovery. #427

Open

burdettadam and others added 2 commits August 18, 2021 15:45

Update packages/core/src/agent/MessageSender.ts

2e43117

Signed-off-by: Adam Burdett <[email protected]> Co-authored-by: Timo Glastra <[email protected]>

formatting

f3dcc2d

Signed-off-by: Adam Burdett <[email protected]>

jakubkoci approved these changes Aug 19, 2021

View reviewed changes

TimoGlastra merged commit 87bc589 into openwallet-foundation:main Aug 19, 2021

This was referenced Aug 20, 2021

passing mediatorConnectionsInvite does not provide best DX #381

Closed

serviceEndpoint not set when not providing mediatorConnectionsInvite to InitConfig #401

Closed

support multiple Service transports in a didDocs. #383

Closed

		@@ -1,6 +1,8 @@
		import type { AgentThunkApiConfig } from '../utils'
		// eslint-disable-next-line import/no-unresolved

		@@ -0,0 +1,218 @@
		import type { SubjectMessage } from './transport/SubjectInboundTransport'

	const { message, connectionRecord: connectionRecord } = await this.connectionService.createRequest(
	const { message, connectionRecord } = await this.connectionService.createRequest(

fix: mediator transports #419

fix: mediator transports #419

Conversation

burdettadam commented Aug 5, 2021

codecov-commenter commented Aug 6, 2021 • edited Loading

Codecov Report

TimoGlastra left a 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

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

JamesKEbert commented Aug 11, 2021

jakubkoci left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TimoGlastra left a 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 Aug 16, 2021 • edited Loading

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 left a comment

Choose a reason for hiding this comment

TimoGlastra commented Aug 18, 2021

JamesKEbert left a comment

Choose a reason for hiding this comment

jakubkoci left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-commenter commented Aug 6, 2021 •

edited

Loading

TimoGlastra Aug 16, 2021 •

edited

Loading