-
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: Action Menu protocol (Aries RFC 0509) implementation #974
feat: Action Menu protocol (Aries RFC 0509) implementation #974
Conversation
Signed-off-by: Ariel Gentile <[email protected]>
Codecov Report
@@ Coverage Diff @@
## main #974 +/- ##
==========================================
+ Coverage 88.06% 88.35% +0.28%
==========================================
Files 492 520 +28
Lines 11623 12071 +448
Branches 1936 1992 +56
==========================================
+ Hits 10236 10665 +429
- Misses 1326 1343 +17
- Partials 61 63 +2
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
Signed-off-by: Ariel Gentile <[email protected]>
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 and tidy 👍 . My preference would go to making this an optional add-on module. However I'm fine with merging it in core in 0.2.0 and then extracting it from core into a separate package (like module-tenants) in 0.3.0. What do you think?
Edit: I see unit tests are missing, but that's probably still todo as you mention in the pr description?
if (actionMenuRecord) { | ||
const previousState = actionMenuRecord.state | ||
actionMenuRecord.state = ActionMenuState.AwaitingRootMenu | ||
actionMenuRecord.threadId = menuRequestMessage.id | ||
|
||
await this.actionMenuRepository.update(actionMenuRecord) | ||
this.emitStateChangedEvent(actionMenuRecord, previousState) |
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.
What if the action menu is currently in progress? Can I just start a new one?
I see you mentioned:
As this protocol currently defines a single active menu per connection, there will be at most 2 records per connection (as theoretically is possible to serve as requester and responder at the same time).
Is this a limitation imposed by the RFC?
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.
From the RFC I understood that this situation would discard current flow and request back the root menu:
At any time a requester can reset the protocol by requesting the root menu from a responder.
The RFC states also that:
both Agents may support both requester and responder roles simultaneously. Such cases would result in two instances of the action-menu protocol operating in parrallel.
So that's why we are supporting one instance per role and connection. At first I thought to create an ActionMenuRecord per interaction (for instance, every time the requester agent selects an option, it creates a new record, which would be set to Done once a new menu is requested and then another record is created). But it could become a bit too complicated considering the simplicity of current specification.
if (actionMenuRecord) { | ||
const previousState = actionMenuRecord.state | ||
actionMenuRecord.state = ActionMenuState.PreparingRootMenu | ||
actionMenuRecord.threadId = menuRequestMessage.id | ||
|
||
await this.actionMenuRepository.update(actionMenuRecord) | ||
this.emitStateChangedEvent(actionMenuRecord, previousState) | ||
} else { |
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.
Do we need to assert state?
Thanks! I think it would be a good idea as you say, we can merge it in 0.2.x and then move it to a separate package, as it is not a core protocol. I'm writing the unit tests and actually found a few issues, so I'll fix them and submit the PR for review shortly (hopefully 😄). |
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]>
Well now I've added some error handling and tests (with our new friend SonarCloud complaining) I think it's not as nice and tidy as before, but let's see how it goes 😄 . I updated PR description with some more details about the work done and what's left. |
Signed-off-by: Ariel Gentile <[email protected]>
…ther perform Signed-off-by: Ariel Gentile <[email protected]>
SonarCloud Quality Gate failed. 0 Bugs No Coverage information |
I'm a bit hesitant to merge this now as it will be quite a hassle to merge into 0.3.0-pre afterwards. When I get the time to immediately update the implementation to work with 0.3.0-pre I'll merge |
Do you mean to merge this module directly on 0.3.0-pre? Or do you want to first update 0.3.0-pre with current main branch status and then merge this one into main? |
No it's probably good to have it in 0.2.x if possible, but after it has been merged in main we need to rebase the 0.3.0-pre branch and integrate the changes from main. I'll do that soon, and then we can make an additional PR to extract the action menu into a separate package. |
Can you update once more? Then I'll merge this PR |
…t-foundation#974) Signed-off-by: Ariel Gentile <[email protected]>
* feat: OOB public did (#930) Signed-off-by: Pavel Zarecky <[email protected]> * feat(routing): manual mediator pickup lifecycle management (#989) Signed-off-by: Ariel Gentile <[email protected]> * docs(demo): faber creates invitation (#995) Signed-off-by: conanoc <[email protected]> * chore(release): v0.2.3 (#999) Signed-off-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> Signed-off-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> * fix(question-answer): question answer protocol state/role check (#1001) Signed-off-by: Ariel Gentile <[email protected]> * feat: Action Menu protocol (Aries RFC 0509) implementation (#974) Signed-off-by: Ariel Gentile <[email protected]> * fix(ledger): remove poolConnected on pool close (#1011) Signed-off-by: Niall Shaw <[email protected]> * fix(ledger): check taa version instad of aml version (#1013) Signed-off-by: Jakub Koci <[email protected]> * chore: add @janrtvld to maintainers (#1016) Signed-off-by: Timo Glastra <[email protected]> * feat(routing): add settings to control back off strategy on mediator reconnection (#1017) Signed-off-by: Sergi Garreta <[email protected]> * fix: avoid crash when an unexpected message arrives (#1019) Signed-off-by: Pavel Zarecky <[email protected]> * chore(release): v0.2.4 (#1024) Signed-off-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> Signed-off-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> * style: fix some lint errors Signed-off-by: Ariel Gentile <[email protected]> * feat: use did:key flag (#1029) Signed-off-by: Ariel Gentile <[email protected]> * ci: set default rust version (#1036) Signed-off-by: Sai Ranjit Tummalapalli <[email protected]> * fix(oob): allow encoding in content type header (#1037) Signed-off-by: Timo Glastra <[email protected]> * feat: connection type (#994) Signed-off-by: KolbyRKunz <[email protected]> * chore(module-tenants): match package versions Signed-off-by: Ariel Gentile <[email protected]> * feat: improve sending error handling (#1045) Signed-off-by: Ariel Gentile <[email protected]> * feat: expose findAllByQuery method in modules and services (#1044) Signed-off-by: Jim Ezesinachi <[email protected]> * feat: possibility to set masterSecretId inside of WalletConfig (#1043) Signed-off-by: Andrii Uhryn <[email protected]> * fix(oob): set connection alias when creating invitation (#1047) Signed-off-by: Jakub Koci <[email protected]> * build: fix missing parameter Signed-off-by: Ariel Gentile <[email protected]> Signed-off-by: Pavel Zarecky <[email protected]> Signed-off-by: Ariel Gentile <[email protected]> Signed-off-by: conanoc <[email protected]> Signed-off-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> Signed-off-by: Niall Shaw <[email protected]> Signed-off-by: Jakub Koci <[email protected]> Signed-off-by: Timo Glastra <[email protected]> Signed-off-by: Sergi Garreta <[email protected]> Signed-off-by: Sai Ranjit Tummalapalli <[email protected]> Signed-off-by: KolbyRKunz <[email protected]> Signed-off-by: Jim Ezesinachi <[email protected]> Signed-off-by: Andrii Uhryn <[email protected]> Co-authored-by: Iskander508 <[email protected]> Co-authored-by: conanoc <[email protected]> Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> Co-authored-by: Niall Shaw <[email protected]> Co-authored-by: jakubkoci <[email protected]> Co-authored-by: Timo Glastra <[email protected]> Co-authored-by: Sergi Garreta Serra <[email protected]> Co-authored-by: Sai Ranjit Tummalapalli <[email protected]> Co-authored-by: KolbyRKunz <[email protected]> Co-authored-by: Jim Ezesinachi <[email protected]> Co-authored-by: an-uhryn <[email protected]>
* feat: OOB public did (openwallet-foundation#930) Signed-off-by: Pavel Zarecky <[email protected]> * feat(routing): manual mediator pickup lifecycle management (openwallet-foundation#989) Signed-off-by: Ariel Gentile <[email protected]> * docs(demo): faber creates invitation (openwallet-foundation#995) Signed-off-by: conanoc <[email protected]> * chore(release): v0.2.3 (openwallet-foundation#999) Signed-off-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> Signed-off-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> * fix(question-answer): question answer protocol state/role check (openwallet-foundation#1001) Signed-off-by: Ariel Gentile <[email protected]> * feat: Action Menu protocol (Aries RFC 0509) implementation (openwallet-foundation#974) Signed-off-by: Ariel Gentile <[email protected]> * fix(ledger): remove poolConnected on pool close (openwallet-foundation#1011) Signed-off-by: Niall Shaw <[email protected]> * fix(ledger): check taa version instad of aml version (openwallet-foundation#1013) Signed-off-by: Jakub Koci <[email protected]> * chore: add @janrtvld to maintainers (openwallet-foundation#1016) Signed-off-by: Timo Glastra <[email protected]> * feat(routing): add settings to control back off strategy on mediator reconnection (openwallet-foundation#1017) Signed-off-by: Sergi Garreta <[email protected]> * fix: avoid crash when an unexpected message arrives (openwallet-foundation#1019) Signed-off-by: Pavel Zarecky <[email protected]> * chore(release): v0.2.4 (openwallet-foundation#1024) Signed-off-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> Signed-off-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> * style: fix some lint errors Signed-off-by: Ariel Gentile <[email protected]> * feat: use did:key flag (openwallet-foundation#1029) Signed-off-by: Ariel Gentile <[email protected]> * ci: set default rust version (openwallet-foundation#1036) Signed-off-by: Sai Ranjit Tummalapalli <[email protected]> * fix(oob): allow encoding in content type header (openwallet-foundation#1037) Signed-off-by: Timo Glastra <[email protected]> * feat: connection type (openwallet-foundation#994) Signed-off-by: KolbyRKunz <[email protected]> * chore(module-tenants): match package versions Signed-off-by: Ariel Gentile <[email protected]> * feat: improve sending error handling (openwallet-foundation#1045) Signed-off-by: Ariel Gentile <[email protected]> * feat: expose findAllByQuery method in modules and services (openwallet-foundation#1044) Signed-off-by: Jim Ezesinachi <[email protected]> * feat: possibility to set masterSecretId inside of WalletConfig (openwallet-foundation#1043) Signed-off-by: Andrii Uhryn <[email protected]> * fix(oob): set connection alias when creating invitation (openwallet-foundation#1047) Signed-off-by: Jakub Koci <[email protected]> * build: fix missing parameter Signed-off-by: Ariel Gentile <[email protected]> Signed-off-by: Pavel Zarecky <[email protected]> Signed-off-by: Ariel Gentile <[email protected]> Signed-off-by: conanoc <[email protected]> Signed-off-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> Signed-off-by: Niall Shaw <[email protected]> Signed-off-by: Jakub Koci <[email protected]> Signed-off-by: Timo Glastra <[email protected]> Signed-off-by: Sergi Garreta <[email protected]> Signed-off-by: Sai Ranjit Tummalapalli <[email protected]> Signed-off-by: KolbyRKunz <[email protected]> Signed-off-by: Jim Ezesinachi <[email protected]> Signed-off-by: Andrii Uhryn <[email protected]> Co-authored-by: Iskander508 <[email protected]> Co-authored-by: conanoc <[email protected]> Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> Co-authored-by: Niall Shaw <[email protected]> Co-authored-by: jakubkoci <[email protected]> Co-authored-by: Timo Glastra <[email protected]> Co-authored-by: Sergi Garreta Serra <[email protected]> Co-authored-by: Sai Ranjit Tummalapalli <[email protected]> Co-authored-by: KolbyRKunz <[email protected]> Co-authored-by: Jim Ezesinachi <[email protected]> Co-authored-by: an-uhryn <[email protected]>
Implementation of module fully supporting Aries RFC 0509 for both
requester
andresponder
roles.Protocol states and information are stored in records of type
ActionMenuRecord
. As this protocol currently defines a single active menu per connection, there will be at most 2 records per connection (as theoretically is possible to serve as requester and responder at the same time). While this information could be stored also in Connection Metadata, specific records were created thinking in a future version/extension of this protocol where multiple menus could be deployed (e.g. a menu per thread).There are still some tests to do (especially in regards of message processing and protocol state machine asserts) but it would be nice if you guys can do some review, as this protocol is not so explicit as others about state transitions and error management and I'm not sure if it's clear enough in terms of usage.
Module API is quite simple and hopefully understandable:
As a requester:
As a responder:
Solves #471
Signed-off-by: Ariel Gentile [email protected]