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 question answer protocol #557

Merged

Conversation

sabejensen
Copy link
Contributor

Summary of Changes

Added a question answer module (https://github.com/hyperledger/aries-rfcs/blob/main/features/0113-question-answer/README.md) to send and receive question messages and to send and receive answer messages with a valid response.

Pull Request Checklist

  • All commits contain a DCO Signed-off-by line (we use the DCO GitHub app to enforce this).
  • Updated LICENSE-3RD-PARTY.md for any added dependencies or vendored components.
  • Added tests for changed code (run scripts/preflight to run tests and check code style).
  • Prefixed code comments with GitHub nick and an appropriate prefix.
  • Coding style is consistent with the rest of the framework.
  • Updated documentation for changed code and new or modified features.

Signed-off-by: seajensen <seajensen@gmail.com>
Signed-off-by: seajensen <seajensen@gmail.com>
Signed-off-by: seajensen <seajensen@gmail.com>
Signed-off-by: seajensen <seajensen@gmail.com>
Signed-off-by: seajensen <seajensen@gmail.com>
Signed-off-by: seajensen <seajensen@gmail.com>
Signed-off-by: seajensen <seajensen@gmail.com>
Signed-off-by: seajensen <seajensen@gmail.com>
@codecov-commenter
Copy link

codecov-commenter commented Dec 1, 2021

Codecov Report

Merging #557 (6f1fae4) into main (a717a58) will decrease coverage by 0.27%.
The diff coverage is 74.33%.

@@            Coverage Diff             @@
##             main     #557      +/-   ##
==========================================
- Coverage   87.76%   87.49%   -0.28%     
==========================================
  Files         439      457      +18     
  Lines       10854    11080     +226     
  Branches     1837     1864      +27     
==========================================
+ Hits         9526     9694     +168     
- Misses       1267     1324      +57     
- Partials       61       62       +1     
Impacted Files Coverage Δ
...rc/modules/question-answer/models/ValidResponse.ts 25.00% <25.00%> (ø)
...rc/modules/question-answer/QuestionAnswerModule.ts 52.77% <52.77%> (ø)
.../question-answer/services/QuestionAnswerService.ts 57.14% <57.14%> (ø)
...s/question-answer/handlers/AnswerMessageHandler.ts 71.42% <71.42%> (ø)
...question-answer/handlers/QuestionMessageHandler.ts 71.42% <71.42%> (ø)
...question-answer/repository/QuestionAnswerRecord.ts 89.28% <89.28%> (ø)
...odules/question-answer/messages/QuestionMessage.ts 95.65% <95.65%> (ø)
packages/core/src/agent/Agent.ts 95.89% <100.00%> (+0.05%) ⬆️
packages/core/src/index.ts 100.00% <100.00%> (ø)
...rc/modules/question-answer/QuestionAnswerEvents.ts 100.00% <100.00%> (ø)
... and 10 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a717a58...6f1fae4. Read the comment docs.

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 work @seajensen.

Only thing I'm thinking is whether we want to put this in core or ext? E.g push notifications lives in ext, QA is also not part of AIP so should it also live in ext? Would be good to have some consistency in what goed where, but I do see the potential of this getting added to the core package.

@TimoGlastra TimoGlastra changed the title Feature/question answer feat: add question answer protocol Dec 1, 2021
@genaris
Copy link
Contributor

genaris commented Dec 2, 2021

Nice work @seajensen.

Only thing I'm thinking is whether we want to put this in core or ext? E.g push notifications lives in ext, QA is also not part of AIP so should it also live in ext? Would be good to have some consistency in what goed where, but I do see the potential of this getting added to the core package.

First of all, good work @seajensen! It's great to see how easy it becomes to create modules implementing new protocols in AFJ.

This is a good point @TimoGlastra. I think the choice could depend on the current RFC lifecycle status, which is eventually related to AIPs (i.e. most ACCEPTED RFCs become part of an AIP version). Push Notification Native RFC is currently PROPOSED, so it is not likely to be widely used right now and it's fine in ext repository. In this particular case, Question/Answer is DEMONSTRATED, which is not so different.

If we choose to put it in aries-javascript-framework-ext repository, I think there are a few classes/methods to be exposed (for instance BaseRecord and createOutboundMessage). This will be useful also for anyone who wants to use custom protocols in AFJ. I can create a PR including these exports shortly.

Signed-off-by: seajensen <seajensen@gmail.com>
@TimoGlastra
Copy link
Contributor

We had a discussion about this during the WG call, but I forgot the outcome. Do we add this to the core of the framework, or make it an extension?

@JamesKEbert @seajensen do you know what the outcome was?

@JamesKEbert
Copy link
Contributor

I unfortunately I was out sick during that WG call, but I recall @seajensen communicating to me that we were going to merge it in.

Signed-off-by: seajensen <seajensen@gmail.com>
…ethod, added error throw for invalid response

Signed-off-by: seajensen <seajensen@gmail.com>
Signed-off-by: seajensen <seajensen@gmail.com>
Signed-off-by: seajensen <seajensen@gmail.com>
Signed-off-by: seajensen <seajensen@gmail.com>
@TimoGlastra
Copy link
Contributor

@seajensen if you can resolve the conflicts we can get this merged!

Signed-off-by: seajensen <seajensen@gmail.com>
Signed-off-by: seajensen <seajensen@gmail.com>
@TimoGlastra TimoGlastra merged commit b5a2536 into openwallet-foundation:main Jun 5, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants