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: Action Menu protocol (Aries RFC 0509) implementation #974

Merged
merged 17 commits into from
Sep 1, 2022

Conversation

genaris
Copy link
Contributor

@genaris genaris commented Aug 6, 2022

Implementation of module fully supporting Aries RFC 0509 for both requester and responder 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:

  • requestMenu: to ask for the root menu. If a menu navigation is ongoing, it will be reset
  • performAction: to select an option on the last received menu. Framework checks its validity (name exists) before sending
  • clearActiveMenu: to discard current flow. It will clear record contents and set protocol state to Null (as it is reset)
  • findActiveMenu: to query current menu record (the menu itself, our selection if any, protocol state, etc.)

As a responder:

  • sendMenu: to send an action menu. Depending on current state, agent controller will determine if it's the root menu or a submenu based on a selection
  • clearActiveMenu: to discard current flow. This is intended to be used when agent controller determines that a timeout has been occurred. Requester will not be informed immediately, but with a Problem Report if they want to perform an action (as suggested by the RFC)
  • findActiveMenu: to query current menu record (the menu itself, requester selection if any, protocol state, etc.)

Solves #471
Signed-off-by: Ariel Gentile gentilester@gmail.com

Signed-off-by: Ariel Gentile <gentilester@gmail.com>
@codecov-commenter
Copy link

codecov-commenter commented Aug 7, 2022

Codecov Report

Merging #974 (c8fafbd) into main (f487182) will increase coverage by 0.28%.
The diff coverage is 94.86%.

@@            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     
Impacted Files Coverage Δ
...modules/action-menu/models/ActionMenuOptionForm.ts 53.84% <53.84%> (ø)
...ction-menu/models/ActionMenuOptionFormParameter.ts 55.55% <55.55%> (ø)
...src/modules/action-menu/models/ActionMenuOption.ts 88.88% <88.88%> (ø)
...modules/action-menu/repository/ActionMenuRecord.ts 94.11% <94.11%> (ø)
...re/src/modules/action-menu/messages/MenuMessage.ts 95.65% <95.65%> (ø)
.../modules/action-menu/services/ActionMenuService.ts 97.98% <97.98%> (ø)
...s/core/src/modules/action-menu/ActionMenuModule.ts 98.07% <98.07%> (ø)
packages/core/src/agent/Agent.ts 95.40% <100.00%> (+0.05%) ⬆️
...s/core/src/modules/action-menu/ActionMenuEvents.ts 100.00% <100.00%> (ø)
...ges/core/src/modules/action-menu/ActionMenuRole.ts 100.00% <100.00%> (ø)
... and 20 more

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 <gentilester@gmail.com>
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.

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?

packages/core/src/modules/action-menu/index.ts Outdated Show resolved Hide resolved
Comment on lines 44 to 50
if (actionMenuRecord) {
const previousState = actionMenuRecord.state
actionMenuRecord.state = ActionMenuState.AwaitingRootMenu
actionMenuRecord.threadId = menuRequestMessage.id

await this.actionMenuRepository.update(actionMenuRecord)
this.emitStateChangedEvent(actionMenuRecord, previousState)
Copy link
Contributor

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?

Copy link
Contributor Author

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.

Comment on lines 79 to 86
if (actionMenuRecord) {
const previousState = actionMenuRecord.state
actionMenuRecord.state = ActionMenuState.PreparingRootMenu
actionMenuRecord.threadId = menuRequestMessage.id

await this.actionMenuRepository.update(actionMenuRecord)
this.emitStateChangedEvent(actionMenuRecord, previousState)
} else {
Copy link
Contributor

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?

packages/core/tests/action-menu.test.ts Outdated Show resolved Hide resolved
packages/core/tests/helpers.ts Outdated Show resolved Hide resolved
@genaris
Copy link
Contributor Author

genaris commented Aug 13, 2022

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?

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 <gentilester@gmail.com>
Signed-off-by: Ariel Gentile <gentilester@gmail.com>
Signed-off-by: Ariel Gentile <gentilester@gmail.com>
Signed-off-by: Ariel Gentile <gentilester@gmail.com>
Signed-off-by: Ariel Gentile <gentilester@gmail.com>
@genaris genaris marked this pull request as ready for review August 18, 2022 05:04
@genaris genaris requested a review from a team as a code owner August 18, 2022 05:04
@genaris
Copy link
Contributor Author

genaris commented Aug 18, 2022

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.

@sonarcloud
Copy link

sonarcloud bot commented Aug 19, 2022

SonarCloud Quality Gate failed.    Quality Gate failed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 71 Code Smells

No Coverage information No Coverage information
6.8% 6.8% Duplication

@TimoGlastra
Copy link
Contributor

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

@genaris
Copy link
Contributor Author

genaris commented Aug 26, 2022

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?

@TimoGlastra
Copy link
Contributor

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.

@TimoGlastra
Copy link
Contributor

Can you update once more? Then I'll merge this PR

@TimoGlastra TimoGlastra enabled auto-merge (squash) September 1, 2022 10:43
@TimoGlastra TimoGlastra merged commit 60a8091 into openwallet-foundation:main Sep 1, 2022
@genaris genaris deleted the feat/action-menu branch September 9, 2022 21:11
genaris added a commit to 2060-io/aries-framework-javascript that referenced this pull request Sep 16, 2022
TimoGlastra added a commit that referenced this pull request Oct 11, 2022
* feat: OOB public did (#930)

Signed-off-by: Pavel Zarecky <zarecky@procivis.ch>

* feat(routing): manual mediator pickup lifecycle management (#989)

Signed-off-by: Ariel Gentile <gentilester@gmail.com>

* docs(demo): faber creates invitation (#995)

Signed-off-by: conanoc <conanoc@gmail.com>

* 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 <gentilester@gmail.com>

* feat: Action Menu protocol (Aries RFC 0509) implementation (#974)

Signed-off-by: Ariel Gentile <gentilester@gmail.com>

* fix(ledger): remove poolConnected on pool close (#1011)

Signed-off-by: Niall Shaw <niall.shaw@absa.africa>

* fix(ledger): check taa version instad of aml version (#1013)

Signed-off-by: Jakub Koci <jakub.koci@gmail.com>

* chore: add @janrtvld to maintainers (#1016)

Signed-off-by: Timo Glastra <timo@animo.id>

* feat(routing): add settings to control back off strategy on mediator reconnection (#1017)

Signed-off-by: Sergi Garreta <sergi.garreta@entrust.com>

* fix: avoid crash when an unexpected message arrives (#1019)

Signed-off-by: Pavel Zarecky <zarecky@procivis.ch>

* 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 <gentilester@gmail.com>

* feat: use did:key flag (#1029)

Signed-off-by: Ariel Gentile <gentilester@gmail.com>

* ci: set default rust version (#1036)

Signed-off-by: Sai Ranjit Tummalapalli <sairanjit5@gmail.com>

* fix(oob): allow encoding in content type header (#1037)

Signed-off-by: Timo Glastra <timo@animo.id>

* feat: connection type (#994)

Signed-off-by: KolbyRKunz <KolbyKunz@yahoo.com>

* chore(module-tenants): match package versions

Signed-off-by: Ariel Gentile <gentilester@gmail.com>

* feat: improve sending error handling (#1045)

Signed-off-by: Ariel Gentile <gentilester@gmail.com>

* feat: expose findAllByQuery method in modules and services (#1044)

Signed-off-by: Jim Ezesinachi <jim@animo.id>

* feat: possibility to set masterSecretId inside of WalletConfig (#1043)

Signed-off-by: Andrii Uhryn <an.ugryn@gmail.com>

* fix(oob): set connection alias when creating invitation (#1047)

Signed-off-by: Jakub Koci <jakub.koci@gmail.com>

* build: fix missing parameter

Signed-off-by: Ariel Gentile <gentilester@gmail.com>

Signed-off-by: Pavel Zarecky <zarecky@procivis.ch>
Signed-off-by: Ariel Gentile <gentilester@gmail.com>
Signed-off-by: conanoc <conanoc@gmail.com>
Signed-off-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Signed-off-by: Niall Shaw <niall.shaw@absa.africa>
Signed-off-by: Jakub Koci <jakub.koci@gmail.com>
Signed-off-by: Timo Glastra <timo@animo.id>
Signed-off-by: Sergi Garreta <sergi.garreta@entrust.com>
Signed-off-by: Sai Ranjit Tummalapalli <sairanjit5@gmail.com>
Signed-off-by: KolbyRKunz <KolbyKunz@yahoo.com>
Signed-off-by: Jim Ezesinachi <jim@animo.id>
Signed-off-by: Andrii Uhryn <an.ugryn@gmail.com>
Co-authored-by: Iskander508 <pavel.zarecky@seznam.cz>
Co-authored-by: conanoc <conanoc@gmail.com>
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Co-authored-by: Niall Shaw <100220424+niallshaw-absa@users.noreply.github.com>
Co-authored-by: jakubkoci <jakub.koci@gmail.com>
Co-authored-by: Timo Glastra <timo@animo.id>
Co-authored-by: Sergi Garreta Serra <garretaserra@gmail.com>
Co-authored-by: Sai Ranjit Tummalapalli <34263716+sairanjit@users.noreply.github.com>
Co-authored-by: KolbyRKunz <KolbyKunz@yahoo.com>
Co-authored-by: Jim Ezesinachi <ezesinachijim@gmail.com>
Co-authored-by: an-uhryn <55444541+an-uhryn@users.noreply.github.com>
genaris added a commit to 2060-io/aries-framework-javascript that referenced this pull request Oct 13, 2022
* feat: OOB public did (openwallet-foundation#930)

Signed-off-by: Pavel Zarecky <zarecky@procivis.ch>

* feat(routing): manual mediator pickup lifecycle management (openwallet-foundation#989)

Signed-off-by: Ariel Gentile <gentilester@gmail.com>

* docs(demo): faber creates invitation (openwallet-foundation#995)

Signed-off-by: conanoc <conanoc@gmail.com>

* 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 <gentilester@gmail.com>

* feat: Action Menu protocol (Aries RFC 0509) implementation (openwallet-foundation#974)

Signed-off-by: Ariel Gentile <gentilester@gmail.com>

* fix(ledger): remove poolConnected on pool close (openwallet-foundation#1011)

Signed-off-by: Niall Shaw <niall.shaw@absa.africa>

* fix(ledger): check taa version instad of aml version (openwallet-foundation#1013)

Signed-off-by: Jakub Koci <jakub.koci@gmail.com>

* chore: add @janrtvld to maintainers (openwallet-foundation#1016)

Signed-off-by: Timo Glastra <timo@animo.id>

* feat(routing): add settings to control back off strategy on mediator reconnection (openwallet-foundation#1017)

Signed-off-by: Sergi Garreta <sergi.garreta@entrust.com>

* fix: avoid crash when an unexpected message arrives (openwallet-foundation#1019)

Signed-off-by: Pavel Zarecky <zarecky@procivis.ch>

* 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 <gentilester@gmail.com>

* feat: use did:key flag (openwallet-foundation#1029)

Signed-off-by: Ariel Gentile <gentilester@gmail.com>

* ci: set default rust version (openwallet-foundation#1036)

Signed-off-by: Sai Ranjit Tummalapalli <sairanjit5@gmail.com>

* fix(oob): allow encoding in content type header (openwallet-foundation#1037)

Signed-off-by: Timo Glastra <timo@animo.id>

* feat: connection type (openwallet-foundation#994)

Signed-off-by: KolbyRKunz <KolbyKunz@yahoo.com>

* chore(module-tenants): match package versions

Signed-off-by: Ariel Gentile <gentilester@gmail.com>

* feat: improve sending error handling (openwallet-foundation#1045)

Signed-off-by: Ariel Gentile <gentilester@gmail.com>

* feat: expose findAllByQuery method in modules and services (openwallet-foundation#1044)

Signed-off-by: Jim Ezesinachi <jim@animo.id>

* feat: possibility to set masterSecretId inside of WalletConfig (openwallet-foundation#1043)

Signed-off-by: Andrii Uhryn <an.ugryn@gmail.com>

* fix(oob): set connection alias when creating invitation (openwallet-foundation#1047)

Signed-off-by: Jakub Koci <jakub.koci@gmail.com>

* build: fix missing parameter

Signed-off-by: Ariel Gentile <gentilester@gmail.com>

Signed-off-by: Pavel Zarecky <zarecky@procivis.ch>
Signed-off-by: Ariel Gentile <gentilester@gmail.com>
Signed-off-by: conanoc <conanoc@gmail.com>
Signed-off-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Signed-off-by: Niall Shaw <niall.shaw@absa.africa>
Signed-off-by: Jakub Koci <jakub.koci@gmail.com>
Signed-off-by: Timo Glastra <timo@animo.id>
Signed-off-by: Sergi Garreta <sergi.garreta@entrust.com>
Signed-off-by: Sai Ranjit Tummalapalli <sairanjit5@gmail.com>
Signed-off-by: KolbyRKunz <KolbyKunz@yahoo.com>
Signed-off-by: Jim Ezesinachi <jim@animo.id>
Signed-off-by: Andrii Uhryn <an.ugryn@gmail.com>
Co-authored-by: Iskander508 <pavel.zarecky@seznam.cz>
Co-authored-by: conanoc <conanoc@gmail.com>
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Co-authored-by: Niall Shaw <100220424+niallshaw-absa@users.noreply.github.com>
Co-authored-by: jakubkoci <jakub.koci@gmail.com>
Co-authored-by: Timo Glastra <timo@animo.id>
Co-authored-by: Sergi Garreta Serra <garretaserra@gmail.com>
Co-authored-by: Sai Ranjit Tummalapalli <34263716+sairanjit@users.noreply.github.com>
Co-authored-by: KolbyRKunz <KolbyKunz@yahoo.com>
Co-authored-by: Jim Ezesinachi <ezesinachijim@gmail.com>
Co-authored-by: an-uhryn <55444541+an-uhryn@users.noreply.github.com>
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