feat: pickup v2 protocol

[KolbyRKunz]

Implemented pickup v2 protocol along with unit testing for the implemented methods leaving room for future testing within the Mediation Recipient Module.

[codecov-commenter]

Codecov Report

Merging #711 (c9af7db) into main (e3e2ebf) will decrease coverage by 0.55%.
The diff coverage is 70.78%.

@@            Coverage Diff             @@
##             main     #711      +/-   ##
==========================================
- Coverage   86.84%   86.28%   -0.56%     
==========================================
  Files         358      367       +9     
  Lines        7814     8014     +200     
  Branches     1317     1356      +39     
==========================================
+ Hits         6786     6915     +129     
- Misses        974     1039      +65     
- Partials       54       60       +6     

Impacted Files	Coverage Δ
packages/core/src/types.ts	100.00% <ø> (ø)
...ckages/core/src/modules/routing/RecipientModule.ts	63.72% <36.95%> (-8.22%)	⬇️
...odules/discover-features/DiscoverFeaturesModule.ts	63.82% <50.00%> (-36.18%)	⬇️
...modules/routing/handlers/MessageDeliveryHandler.ts	54.54% <54.54%> (ø)
...core/src/modules/routing/handlers/StatusHandler.ts	54.54% <54.54%> (ø)
...c/modules/routing/messages/StatusRequestMessage.ts	58.33% <58.33%> (ø)
...ules/routing/services/MediationRecipientService.ts	74.25% <68.42%> (-2.09%)	⬇️
.../src/modules/routing/repository/MediationRecord.ts	80.76% <85.71%> (+1.22%)	⬆️
...core/src/modules/routing/messages/StatusMessage.ts	93.10% <93.10%> (ø)
packages/core/src/agent/AgentConfig.ts	94.91% <100.00%> (+0.17%)	⬆️
... and 18 more

---

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 e3e2ebf...c9af7db. Read the comment docs.

[TimoGlastra]

This is a really great addition, thanks!

Is it correct this doesn't include the live delivery?

Also, it seems this doesn't include the mediator side of processing, is that right? I'm a bit reluctant to this approach as it means we can't e2e test the protocol. We already have this with revocation, but I'd like to keep this to a minimum. In addition I think it's important to not only have unit tests as we now don't have any guarantee it will actually work or we don't somehow break something in the future.

[TimoGlastra]

Hmm does the pickup v2 protocol use raw keys? I thought we were moving away from raw keys as it doesn't reveal the key type. Wouldn't this be a good candidate for using did:key instead?

cc @JamesKEbert

[JamesKEbert]

Hmm good question--let me followup on this question and I'll get back to you on this.

[TimoGlastra]

This is fine for now, but maybe we should look into a WebSocket opened event and hook into that for v2

[JamesKEbert]

Is it correct this doesn't include the live delivery?

That's correct. We'd go ahead and implement it, but given ACA-Py's current logic for processing messages/queue/etc, this would be a more significant addition on that side of things. @dbluhm can probably communicate the specific issues better. So for as of right now this PR still just uses the trust ping response_requested: false mechanism within ACA-Py.

Also, it seems this doesn't include the mediator side of processing, is that right? I'm a bit reluctant to this approach as it means we can't e2e test the protocol. We already have this with revocation, but I'd like to keep this to a minimum. In addition I think it's important to not only have unit tests as we now don't have any guarantee it will actually work or we don't somehow break something in the future.

That's correct. From our side of things our needs plan on using ACA-Py as the mediator (with the acapy-pickup-plugin) so we don't have need for AFJ as a mediator--not to say we won't implement things out of our scope or that don't directly impact us but do benefit the project/community, but this does seem a little larger in scope to implement. Also, I know there's some things already implemented for being a mediator in AFJ that I'm not really familiar with that I think others likely would have better domain in.

So, I'd agree in that it'd be good to get the mediator portion of the protocol implemented for an E2E case. I know we'd be happy to help someone with that in testing/troubleshooting, etc. Does my hesitation to build both sides of the protocol make sense? Thoughts? (I'd like to know what you're thinking here) 🤔

Also, maybe it's worthwhile looking at some of our testing strategy to see if we can add an approach to better test the end to end case when we don't support part of a protocol?

[JamesKEbert]

In addition I think it's important to not only have unit tests as we now don't have any guarantee it will actually work or we don't somehow break something in the future.

Also, isn't this what the AATH is supposed to do? Or is the fact that it's separated enough mean it's insufficient for this problem?

[TimoGlastra]

This looks good to me. I think there's two outstanding points with 1. the naming of the status request method in the module, and 2. the usage of a raw key for the message. 2. is something that needs to be fixed in the RFC (if we want to address this) so I think we can fix it later. 1. would be nice to still change in this PR.

In addition (If I'm not the person merging) can we add the following BREAKING CHANGE clause to the bottom of the squashed commit message:

BREAKING CHANGE: The mediator pickup strategy enum value `MediatorPickupStrategy.Explicit` has been renamed to `MediatorPickupStrategy.PickUpV1` to better align with the naming of the new `MediatorPickupStrategy.PickUpV2`

[KolbyRKunz]

In regards to changing the name of the status request method within the module we were thinking something along the lines of requestStatus, we are planning on running with that unless you had something more specific in mind.

[TimoGlastra]

In regards to changing the name of the status request method within the module we were thinking something along the lines of requestStatus, we are planning on running with that unless you had something more specific in mind.

Hmm the problem I see with requestStatus is that it still not conveys what is done: sending a status request message, and if there are any pending messages also request the delivery of those pending messages, causing a loop of retrieving messages from the mediator (as The mediator SHOULD send an updated status message reflecting the changes to the queue which triggers a new delivery request. ) . And then we're basically back at intiateMessagePickup... I need to think about it a bit more, but hope you understand my thought process

[TimoGlastra]

Ok I've been thinking about it a bit more and maybe we should call it requestStatus, but not automatically send a message delivery request in the handler of the status message. This way an agent can choose to request the status of a message without being required to also pick up the messages.

We can then add an even listener (can be just the agent message processed event) that listens for a response message and triggers the pickup of messages -- but only if this is intended (so not when calling requestStatus, but do when initiating message pickup)

If we don't want to take this approach for now (understandable because more complex) I'd be in favour of not exposing this method in the public module until we actually allow to request the status without initiating message pickup

[JamesKEbert]

@TimoGlastra what's your thoughts on if we provide an agent config for whether or not to automatically followup with a delivery request message for status messages that default to true? This would require the initial message pickup to be manually triggered, but that may be a decent tradeoff for now?

We don't currently require the ability to send a status message from the public API, but we figured it'd be a good edition--so we're 100% good with keeping it private or using a config param (just figured I'd voice the option). Thoughts?

[KolbyRKunz]

What work, if any, needs to be done before we can get this finalized and pulled in? As far as the naming scheme goes on your previous comment I think something like openWebsocketAndPickup sounds appropriate, if that is the only thing holding us up at this point let me know and I can get that change in today. Thank you for your time.

[TimoGlastra]

@TimoGlastra what's your thoughts on if we provide an agent config for whether or not to automatically followup with a delivery request message for status messages that default to true? This would require the initial message pickup to be manually triggered, but that may be a decent tradeoff for now?

Maybe we can keep it private for now and automatically pick up the messages? Then if we later have more knowledge on the usage patterns of pickup v2 and the status messages we can update accordingly. I think a global parameter may not be the right cut-off for configuration as sometimes you do want to pick up afterwards (when you want to retrieve all messages), and sometimes you just want to have the status.

What work, if any, needs to be done before we can get this finalized and pulled in? As far as the naming scheme goes on your previous comment I think something like openWebsocketAndPickup sounds appropriate, if that is the only thing holding us up at this point let me know and I can get that change in today. Thank you for your time.

Changing the method name seems fine to me for now. We can look at reoganizing and splitting up the mediation module later on to make it all a bit easier to understand and follow.

[TimoGlastra]

LGTM

[KolbyRKunz]

Okay, fixed all the unit tests and updated to main, ready to merge.

[TimoGlastra]

@KolbyRKunz can you update the branch with main so we can merge? I can't do that when you create a PR from an organization.

[KolbyRKunz]

@TimoGlastra Got main merged back in and pushed up.

[TimoGlastra]

Awesome, thanks! Should automatically merge once CI passes