Skip to content
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

Merged
merged 7 commits into from
Apr 1, 2023

Conversation

genaris
Copy link
Contributor

@genaris genaris commented Mar 30, 2023

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 the MessagePickupModuleConfig, which would be the most revolutionary (?) addition from this PR from API perspective.

new Agent({
      config,
      dependencies: agentDependencies,
      modules: {
         /* ... */
         messagePickup: new MessagePickupModule({ messageRepository: new CustomMessageRepository() }),
      }
})

@genaris genaris requested a review from a team as a code owner March 30, 2023 21:14
@codecov-commenter
Copy link

codecov-commenter commented Mar 30, 2023

Codecov Report

Merging #1413 (7447f2c) into main (47636b4) will increase coverage by 11.76%.
The diff coverage is 92.04%.

@@             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     
Impacted Files Coverage Δ
.../core/src/modules/message-pìckup/protocol/index.ts 100.00% <ø> (ø)
...ackages/core/src/modules/routing/MediatorModule.ts 100.00% <ø> (ø)
packages/core/src/modules/routing/index.ts 100.00% <ø> (ø)
packages/core/src/modules/routing/MediatorApi.ts 77.27% <33.33%> (-5.42%) ⬇️
...p/protocol/v2/handlers/V2MessageDeliveryHandler.ts 54.54% <54.54%> (ø)
packages/core/src/modules/routing/RecipientApi.ts 67.85% <61.53%> (+10.46%) ⬆️
...p/protocol/v2/handlers/V2DeliveryRequestHandler.ts 62.50% <62.50%> (ø)
.../protocol/v2/handlers/V2MessagesReceivedHandler.ts 62.50% <62.50%> (ø)
...ore/src/modules/message-pìckup/MessagePickupApi.ts 83.87% <83.87%> (ø)
...age-pìckup/protocol/v2/handlers/V2StatusHandler.ts 90.90% <90.90%> (ø)
... and 25 more

... 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.

Copy link
Contributor

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

Comment on lines +29 to +33
/**
* Allows to specify a custom pickup message queue. It defaults to an in-memory repository
*
*/
messageRepository?: MessageRepository
Copy link
Contributor

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?!

Copy link
Contributor Author

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> {
Copy link
Contributor

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)

Copy link
Contributor Author

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.

packages/core/src/agent/AgentModules.ts Outdated Show resolved Hide resolved
@genaris
Copy link
Contributor Author

genaris commented Mar 31, 2023

now that the recipient will become much simpler, should we combine the mediator and mediation recipient modules?

Yes! I would like to do so. I think it could make it easier when adding more protocols like the ones for DIDComm V2

@genaris genaris merged commit a8439db into openwallet-foundation:main Apr 1, 2023
@genaris genaris deleted the pickup-module branch April 1, 2023 13:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants