-
Notifications
You must be signed in to change notification settings - Fork 204
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(routing): pickup v2 mediator role basic implementation #975
feat(routing): pickup v2 mediator role basic implementation #975
Conversation
Signed-off-by: Ariel Gentile <[email protected]>
Codecov Report
@@ 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
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.
Nice work @genaris
this.registerHandlers() | ||
} | ||
|
||
public async batch(messageContext: InboundMessageContext<BatchPickupMessage>) { |
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.
public async batch(messageContext: InboundMessageContext<BatchPickupMessage>) { | |
public async createBatch(messageContext: InboundMessageContext<BatchPickupMessage>) { |
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.
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!
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.
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.
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.
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.
import { BatchMessage, BatchMessageMessage } from './messages' | ||
|
||
@injectable() | ||
export class MessagePickupService { |
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.
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...
export class MessagePickupService { | |
export class V1MessagePickupService { |
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.
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.
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.
Hmm probably it's good to start doing it everywhere as it is leading to issues over time? Not sure
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.
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/modules/routing/protocol/pickup/v1/handlers/BatchPickupHandler.ts
Outdated
Show resolved
Hide resolved
// TODO: Add Queued Message ID | ||
await this.messageRepository.takeFromQueue( | ||
connection.id, | ||
message.messageIdList ? message.messageIdList.length : undefined | ||
) |
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 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?
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.
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.
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.
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?
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.
Absolutely!
Co-authored-by: Timo Glastra <[email protected]> Signed-off-by: Ariel Gentile <[email protected]>
Signed-off-by: Ariel Gentile <[email protected]>
Signed-off-by: Ariel Gentile <[email protected]>
Signed-off-by: Ariel Gentile <[email protected]>
Signed-off-by: Ariel Gentile <[email protected]>
…Service tests Signed-off-by: Ariel Gentile <[email protected]>
…0-io/aries-framework-javascript into feat/pickup-v2-mediator-role
Signed-off-by: Ariel Gentile <[email protected]>
Signed-off-by: Ariel Gentile <[email protected]>
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:
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:
Please let me know what you think so we can find the best approach to go-on with this. |
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! |
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. |
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 🎉 |
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 [email protected]