-
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: add question answer protocol #557
feat: add question answer protocol #557
Conversation
Signed-off-by: seajensen <[email protected]>
Signed-off-by: seajensen <[email protected]>
Signed-off-by: seajensen <[email protected]>
Signed-off-by: seajensen <[email protected]>
Signed-off-by: seajensen <[email protected]>
Signed-off-by: seajensen <[email protected]>
Signed-off-by: seajensen <[email protected]>
Signed-off-by: seajensen <[email protected]>
Codecov Report
@@ 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
Continue to review full report at Codecov.
|
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 @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.
packages/core/src/modules/question-answer/QuestionAnswerModule.ts
Outdated
Show resolved
Hide resolved
packages/core/src/modules/question-answer/QuestionAnswerModule.ts
Outdated
Show resolved
Hide resolved
packages/core/src/modules/question-answer/handlers/QuestionAnswerHandler.ts
Outdated
Show resolved
Hide resolved
packages/core/src/modules/question-answer/messages/AnswerMessage.ts
Outdated
Show resolved
Hide resolved
packages/core/src/modules/question-answer/messages/AnswerMessage.ts
Outdated
Show resolved
Hide resolved
packages/core/src/modules/question-answer/services/QuestionAnswerService.ts
Outdated
Show resolved
Hide resolved
packages/core/src/modules/question-answer/services/QuestionAnswerService.ts
Outdated
Show resolved
Hide resolved
packages/core/src/modules/question-answer/QuestionAnswerModule.ts
Outdated
Show resolved
Hide resolved
packages/core/src/modules/question-answer/services/QuestionAnswerService.ts
Outdated
Show resolved
Hide resolved
packages/core/src/modules/question-answer/services/QuestionAnswerService.ts
Outdated
Show resolved
Hide resolved
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 |
Signed-off-by: seajensen <[email protected]>
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? |
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 <[email protected]>
…ethod, added error throw for invalid response Signed-off-by: seajensen <[email protected]>
Signed-off-by: seajensen <[email protected]>
packages/core/src/modules/question-answer/services/QuestionAnswerService.ts
Outdated
Show resolved
Hide resolved
packages/core/src/modules/question-answer/services/QuestionAnswerService.ts
Show resolved
Hide resolved
packages/core/src/modules/question-answer/services/QuestionAnswerService.ts
Show resolved
Hide resolved
packages/core/src/modules/question-answer/services/QuestionAnswerService.ts
Show resolved
Hide resolved
Signed-off-by: seajensen <[email protected]>
Signed-off-by: seajensen <[email protected]>
@seajensen if you can resolve the conflicts we can get this merged! |
Signed-off-by: seajensen <[email protected]>
Signed-off-by: seajensen <[email protected]>
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
Signed-off-by
line (we use the DCO GitHub app to enforce this).scripts/preflight
to run tests and check code style).