feat(routing): pickup v2 mediator role basic implementation #975

genaris · 2022-08-07T22:56:47Z

Basic implementation of the missing Mediator role for Pick Up V2 protocol (Aries RFC 0685). In order to make this PR smaller and non-breaking, not all features from protocol are supported, but only those used currently by Recipient implementation in AFJ 0.2.x.

There are some refactoring here and there to start doing the split up suggested by #800, but attempting to maintain compatibility with current modules API.

Note that current implementation of client side assumes that the mediator will use a WebSocket connection, which is not actually mandatory. This behaviour is not yet updated, but might require to update WsInboundTransporter to manage long-term WebSockets connections (e.g. use a heartbeat interval mechanism) in order to be really useful.

Solves #956 and hopefully #475 and #727.

Signed-off-by: Ariel Gentile gentilester@gmail.com

Signed-off-by: Ariel Gentile <gentilester@gmail.com>

codecov-commenter · 2022-08-07T23:19:14Z

Codecov Report

Merging #975 (6c23381) into main (6468a93) will increase coverage by 0.41%.
The diff coverage is 97.25%.

@@            Coverage Diff             @@
##             main     #975      +/-   ##
==========================================
+ Coverage   87.76%   88.18%   +0.41%     
==========================================
  Files         475      487      +12     
  Lines       11417    11515      +98     
  Branches     1804     1824      +20     
==========================================
+ Hits        10020    10154     +134     
+ Misses       1393     1357      -36     
  Partials        4        4

Impacted Files	Coverage Δ
...ackages/core/src/modules/routing/messages/index.ts	`100.00% <ø> (ø)`
...ackages/core/src/modules/routing/services/index.ts	`100.00% <ø> (ø)`
...routing/protocol/pickup/v1/MessagePickupService.ts	`93.10% <93.10%> (ø)`
...uting/protocol/pickup/v2/V2MessagePickupService.ts	`94.11% <94.11%> (ø)`
...ackages/core/src/modules/routing/MediatorModule.ts	`80.85% <100.00%> (ø)`
...ckages/core/src/modules/routing/RecipientModule.ts	`61.61% <100.00%> (-0.39%)`	⬇️
...ackages/core/src/modules/routing/handlers/index.ts	`100.00% <100.00%> (ø)`
packages/core/src/modules/routing/index.ts	`100.00% <100.00%> (ø)`
...ackages/core/src/modules/routing/protocol/index.ts	`100.00% <100.00%> (ø)`
.../core/src/modules/routing/protocol/pickup/index.ts	`100.00% <100.00%> (ø)`
... and 31 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

TimoGlastra

Nice work @genaris

TimoGlastra · 2022-08-08T14:11:25Z

packages/core/src/modules/routing/protocol/pickup/v1/MessagePickupService.ts

+    this.registerHandlers()
+  }
+
+  public async batch(messageContext: InboundMessageContext<BatchPickupMessage>) {


Suggested change

public async batch(messageContext: InboundMessageContext<BatchPickupMessage>) {

public async createBatch(messageContext: InboundMessageContext<BatchPickupMessage>) {

Actually, this is mostly a relocation of the original MessagePickUpService, as I wanted to keep current services interfaces as most as possible in this PR. But if this doesn't count as a 'breaking change', for sure I'll be happy to change it as suggested!

Well, moving it to another service already is 'breaking' if we see the service as public API, or is this just a moving of the file? It's not really clear what is public / private API. I've added it to the agenda for the AFJ WG call.

Yes, actually I meant that from outside of the framework you'd see the same MessagePickupService (i.e. when importing it from @aries-framework/core).

I agree that we should better clarify the rules for breaking changes. I don't know if I'll be able to attend today's call but my 2 cents from practical perspective is that we should care mostly on Module API and also items that are explicitly intended to be injected, such as Wallet, MessageRepository and the inbound/outbound transportes. It's desirable to do the same for services (as some custom plug-ins can make use of them), but IMHO it's acceptable if it's properly documented in the release and it is possible to workaround any change without impacting the behaviour. For instance if we add V1 prefix to the service, compilation of the plugin using it will fail but it will not have an unpredictable behaviour.

TimoGlastra · 2022-08-08T14:12:11Z

packages/core/src/modules/routing/protocol/pickup/v1/MessagePickupService.ts

+import { BatchMessage, BatchMessageMessage } from './messages'
+
+@injectable()
+export class MessagePickupService {


Should this have a prefix to avoid clashes? We've taken this approach for issue credential / present proof. This would mean we also need to update the messages, handlers, etc...

Suggested change

export class MessagePickupService {

export class V1MessagePickupService {

I think it should be prefixed. And in this case, protocol messages are named differently so I guess there will be no need to prefix all classes.

Hmm probably it's good to start doing it everywhere as it is leading to issues over time? Not sure

Yes, you are right. I'll prefix all V2 message classes. Also I've noticed that some messages did exist in V1 but we just don't support them :-O

packages/core/src/storage/MessageRepository.ts

packages/core/src/modules/routing/protocol/pickup/v1/handlers/BatchPickupHandler.ts

TimoGlastra · 2022-08-08T14:19:29Z

packages/core/src/modules/routing/protocol/pickup/v2/V2MessagePickupService.ts

+    // TODO: Add Queued Message ID
+    await this.messageRepository.takeFromQueue(
+      connection.id,
+      message.messageIdList ? message.messageIdList.length : undefined
+    )


We shouldn't take from the queue in this case I think?

I'm assuming this is what you're talking about in the pr description. We should somehow be able to get messages from the queue (without removing them) and also be able to remove messages from the queue. Currently the acknowledgement doesn't matter and it will always just remove them right?

Exactly. That's not yet handled properly. I was thinking on adding an optional parameter to takeFromQueue like keepMessages that defaults to current behaviour, so it does not break current V1 handler and limits changes to minimum for 0.2.x.

But if we are free to change MessageRepository/InMemoryMessageRepository and V1 service/handlers a bit (as long as MediationModule and MediationRecipient API does not break), probably it would be better to have a method to slice and another to splice the internal messages array.

The message repo can already be registered with a custom version at the moment (I'm using it in a project) so we should be careful with changing it.

I think for now this is fine and then we can go wild in the 0.3.0 branch. WDYT?

Absolutely!

Co-authored-by: Timo Glastra <timo@animo.id> Signed-off-by: Ariel Gentile <gentilester@gmail.com>

Signed-off-by: Ariel Gentile <gentilester@gmail.com>

…Service tests Signed-off-by: Ariel Gentile <gentilester@gmail.com>

…0-io/aries-framework-javascript into feat/pickup-v2-mediator-role

Signed-off-by: Ariel Gentile <gentilester@gmail.com>

genaris · 2022-08-09T19:42:42Z

TBH I'm not so happy with the resulting support of Pick Up V2 protocol, as the limitations to avoid breaking changes were more significant than I thought.

Still, I think it will work fine for the moment for E2E use cases in a pure-AFJ environment, much in a similar way than using Pick Up V1, as client side is not using Live Mode and also handles the pick-up automatically (i.e. sends Delivery Request message as soon as it receives a count > 0 in Status message).

So with these modifications we are covering these scenarios:

Edge Agent that wants to retrieve queued messages from another one (e.g. a connection between a mobile and a cloud agent without mediation)
Mobile Agent that retrieves all messages (regardless their recipient key) from its mediator. This will need some external help though, as mediationRecipient module does not send Status Request periodically (it seems that it relies on a sort of mediator's implicit live mode after sending a Trust Ping):
- Framework controller can manually execute mediationRecipient.pickupMessages(connectionId, MediatorPickupStrategy.PickUpV2) if they wish
- Mediator uses a customized WsInboundTransporter that keeps long-lived sessions as ACA-Py does

I'm not sure if we can add anything else without introducing breaking changes in both client/server sides. However in another PR (to merge in 0.3.x branch) we can immediately think about all missing features, such as:

Filtering messages by recipient key
Clearly identifying messages in delivery and its reception acknowledge to safely remove them from the queue
Supporting Live Mode
Allowing connection to mediator using "Explicit Pick Up V2" (i.e. periodic Status Requests)
Allowing Pick Up V2 with other transports than WebSocket (as it is actually only required for Live Mode)
Refactoring Mediation module, which probably would result in decoupling Pick Up from Mediation protocols, as it can be used to query any queuing-capable agent (not only the one that forwards messages to another one)

Please let me know what you think so we can find the best approach to go-on with this.

TimoGlastra · 2022-08-11T11:54:42Z

That sounds great @genaris. I think this is already a huge improvement, and then making the other changes in 0.3.0-pre will be great!

TimoGlastra · 2022-08-11T12:59:23Z

As we're going to make the other changes in 0.3.0-pre this PR is ready to merge then right?

genaris · 2022-08-11T13:07:47Z

As we're going to make the other changes in 0.3.0-pre this PR is ready to merge then right?

Yeah, I'll do the message classes renaming in that further PR.

TimoGlastra · 2022-08-11T13:31:11Z

Awesome, thanks @genaris. This will help a lot. I currently made a hacky implementation that uses AFJ with both pickup v1 and the implicit pickup method combined, but with this I can now switch to pickup v2 🎉

feat: pickup v2 mediator role initial structure

a06c001

Signed-off-by: Ariel Gentile <gentilester@gmail.com>

TimoGlastra reviewed Aug 8, 2022

View reviewed changes

genaris and others added 9 commits August 8, 2022 11:51

Update packages/core/src/storage/MessageRepository.ts

7d9516b

Co-authored-by: Timo Glastra <timo@animo.id> Signed-off-by: Ariel Gentile <gentilester@gmail.com>

fix: keep messages in queue when processing delivery request

000184e

Signed-off-by: Ariel Gentile <gentilester@gmail.com>

test: add e2e test using pickup v2 (websocket only)

5a9d5a6

Signed-off-by: Ariel Gentile <gentilester@gmail.com>

fix: available -> getAvailableMessageCount

6ef63b6

Signed-off-by: Ariel Gentile <gentilester@gmail.com>

fix: add threadId to response messages, reply with status

3f3ae90

Signed-off-by: Ariel Gentile <gentilester@gmail.com>

test: add V2MessagePickupService + fix threadId in MediationRecipient…

0efd359

…Service tests Signed-off-by: Ariel Gentile <gentilester@gmail.com>

Merge branch 'feat/pickup-v2-mediator-role' of https://github.com/206…

26a8223

…0-io/aries-framework-javascript into feat/pickup-v2-mediator-role

fix: do not require mediation to be established to pick up messages

5f52f8a

Signed-off-by: Ariel Gentile <gentilester@gmail.com>

feat: manual pickup messages using V2 protocol

6c23381

Signed-off-by: Ariel Gentile <gentilester@gmail.com>

genaris marked this pull request as ready for review August 9, 2022 19:42

genaris requested a review from a team as a code owner August 9, 2022 19:42

Merge branch 'main' into feat/pickup-v2-mediator-role

621b9e7

TimoGlastra approved these changes Aug 11, 2022

View reviewed changes

TimoGlastra merged commit a989556 into openwallet-foundation:main Aug 11, 2022

genaris deleted the feat/pickup-v2-mediator-role branch August 24, 2022 17:41

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(routing): pickup v2 mediator role basic implementation #975

feat(routing): pickup v2 mediator role basic implementation #975

genaris commented Aug 7, 2022

codecov-commenter commented Aug 7, 2022 •

edited

TimoGlastra left a comment

TimoGlastra Aug 8, 2022

genaris Aug 8, 2022

TimoGlastra Aug 11, 2022

genaris Aug 11, 2022

TimoGlastra Aug 8, 2022

genaris Aug 8, 2022

TimoGlastra Aug 11, 2022

genaris Aug 11, 2022

TimoGlastra Aug 8, 2022

genaris Aug 8, 2022

TimoGlastra Aug 11, 2022

genaris Aug 11, 2022

genaris commented Aug 9, 2022

TimoGlastra commented Aug 11, 2022

TimoGlastra commented Aug 11, 2022

genaris commented Aug 11, 2022

TimoGlastra commented Aug 11, 2022

	public async batch(messageContext: InboundMessageContext<BatchPickupMessage>) {
	public async createBatch(messageContext: InboundMessageContext<BatchPickupMessage>) {

	export class MessagePickupService {
	export class V1MessagePickupService {

feat(routing): pickup v2 mediator role basic implementation #975

feat(routing): pickup v2 mediator role basic implementation #975

Conversation

genaris commented Aug 7, 2022

codecov-commenter commented Aug 7, 2022 • edited

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

genaris commented Aug 9, 2022

TimoGlastra commented Aug 11, 2022

TimoGlastra commented Aug 11, 2022

genaris commented Aug 11, 2022

TimoGlastra commented Aug 11, 2022

codecov-commenter commented Aug 7, 2022 •

edited