-
Notifications
You must be signed in to change notification settings - Fork 202
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: add message pickup module #1413
Conversation
Signed-off-by: Ariel Gentile <gentilester@gmail.com>
Signed-off-by: Ariel Gentile <gentilester@gmail.com>
Signed-off-by: Ariel Gentile <gentilester@gmail.com>
Codecov Report
@@ Coverage Diff @@
## main #1413 +/- ##
===========================================
+ Coverage 73.60% 85.37% +11.76%
===========================================
Files 695 799 +104
Lines 17144 19660 +2516
Branches 2759 3167 +408
===========================================
+ Hits 12619 16784 +4165
+ Misses 4520 2869 -1651
- Partials 5 7 +2
... and 284 files with indirect coverage changes Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In general looks good, nice! Just some questions on what's the role of the mediation recipient vs message pickup module.
now that the recipient will become much simpler, should we combine the mediator and mediation recipient modules?
/** | ||
* Allows to specify a custom pickup message queue. It defaults to an in-memory repository | ||
* | ||
*/ | ||
messageRepository?: MessageRepository |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Curious what the line is between mediator vs message pickup. Is the mediator just about routing, and is message pickup module about queuing messages? As you mentioned before, allowing messages to be queued even without mediator (so just e.g. issuer that queues messages for the holder directly) should be possible, in which case this approach makes sense.
Does the message pickup module still support implicit
pickup? And if so, what's the protocolVersion in that case?!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We have to dig a little bit more about it but I initially I see "Message Pickup" mainly dedicated to the explicit pickup and setting up the pickup mode in general. Although the "live mode" can be also seen as an implicit mode, it can probably also handled by this module (maybe using a specific queue as suggested by @rodolfomiranda in hyperledger/aries-rfcs#760).
The implicit mode is tricky because to me it's like a particular case that works for ACA-Py but don't see it specified in an RFC. Maybe it can still be triggered from the Mediation Recipient module (as it is being done right now) or left it to be handled from outside of the framework: for instance, we can add MediatorWebSocketConnected
and MediatorWebSocketDisconnected
events so the consumer application can manage the reconnections as needed.
* | ||
* @param options connectionId, protocol version to use and batch size | ||
*/ | ||
public async pickupMessages(options: PickupMessagesOptions<MPPs>): Promise<PickupMessagesReturnType> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This doesn't actually wait for the pickup to be returned. Should we add this to function documentationion or something?
If i understand correctly, mediation recipient will call the pickup in an interval, and you still need to handle the lifecycle in the mediation recipient module? Shouldn't we move all pickup logic, to the message pickup module? So picking up messages, whether implicit, explicit, live module, on an interval is managed by this module. The mediation recipient module is just a way to request mediation, register new keys, etc... (so really about the routing part)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's true: this pickupMessages
will perform a single loop that can be called once or within an interval, and will return before the actual messages were pickup (e.g. Batch or Status/Delivery messages are received). Do you think it would be good to make it sync in order to block until the loop is completed or fail in the case of a timeout?
In this PR I left the lifecycle management in mediation module mostly to avoid introducing more breaking changes, but what I would like to do in a future is MessagePickupApi
to create pickup loops that can be initiated/queried/stopped either by MediationRecipientApi
or externally.
So I would say that MediationRecipientModule will be dedicated to the things you say and also holding the configuration (the MediatorPickupStrategy
) and call MessagePickupApi
to start/stop pickup loops as needed at the initialization/shutdown/mediator switching/etc.
Signed-off-by: Ariel Gentile <gentilester@gmail.com>
Signed-off-by: Ariel Gentile <gentilester@gmail.com>
Yes! I would like to do so. I think it could make it easier when adding more protocols like the ones for DIDComm V2 |
This PR moves Message Pickup specific features into a separate module, rather than putting all together into Mediator and Recipient modules.
The idea behind this (besides making mediator/routing module more lightweight) is to start adding more advanced features to Message Pickup, for both recipient (e.g. be able to manage multiple message pickup loops) and message holder roles (e.g. support message pickup sessions in order to determine what to do with forwarded messages depending on Live mode status).
For the moment, it attempts to not introduce any breaking change at API level (other than renaming some message and handler classes that were previously exposed so somebody could have been referencing them externally).
MessageRepository
API remains the same, although now it can be injected by using theMessagePickupModuleConfig
, which would be the most revolutionary (?) addition from this PR from API perspective.