-
Notifications
You must be signed in to change notification settings - Fork 516
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: support connectionless exchange #1710
feat: support connectionless exchange #1710
Conversation
Signed-off-by: Timo Glastra <[email protected]>
Signed-off-by: Timo Glastra <[email protected]>
Signed-off-by: Timo Glastra <[email protected]>
Signed-off-by: Timo Glastra <[email protected]>
Signed-off-by: Timo Glastra <[email protected]>
Signed-off-by: Timo Glastra <[email protected]>
Signed-off-by: Timo Glastra <[email protected]>
Signed-off-by: Timo Glastra <[email protected]>
Signed-off-by: Timo Glastra <[email protected]>
Signed-off-by: Timo Glastra <[email protected]>
Signed-off-by: Timo Glastra <[email protected]>
Signed-off-by: Timo Glastra <[email protected]>
Signed-off-by: Timo Glastra <[email protected]>
Signed-off-by: Timo Glastra <[email protected]>
Signed-off-by: Timo Glastra <[email protected]>
Signed-off-by: Timo Glastra <[email protected]>
Signed-off-by: Timo Glastra <[email protected]>
Signed-off-by: Timo Glastra <[email protected]>
Signed-off-by: Timo Glastra <[email protected]>
Signed-off-by: Timo Glastra <[email protected]>
Signed-off-by: Timo Glastra <[email protected]>
@cvarjao --- FYI about this PR. |
Codecov Report
@@ Coverage Diff @@
## main #1710 +/- ##
==========================================
- Coverage 95.21% 94.11% -1.10%
==========================================
Files 530 533 +3
Lines 33235 33639 +404
==========================================
+ Hits 31644 31660 +16
- Misses 1591 1979 +388 |
I just realized (when integrating with AATH) this PR does introduce a breaking change.... Would like some input on how to resolve. In the current main branch of ACA-Py the Now the endpoint returns an out of band record, which optionally contains a connection_id (if not using connectionless). So how should I go about handling this case? I see a few options:
My preference would go to 2, but that does mean it's not fit for a 0.7.4 release. However 1. is technically also a breaking change. |
Signed-off-by: Timo Glastra <[email protected]>
Any updates on this? :) |
@TimoGlastra -- can you address the merge conflicts? @shaangill025 and @ianco -- can the two of you take a look at this one? Perhaps at ACA-Pug tomorrow, y'all can discuss if this is an 0.7.4 PR. |
Discussed at today's aca-pug. The consensus is that this is not a "breaking change" if it only affects connectionless. I.e. if an implementation is not using connectionless, then there are no breaking changes introduced. If they are using connectionless, then there re changes required. I'm happy with this solution:
|
@TimoGlastra looks like there's a conflict |
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.
@TimoGlastra if you can cleanup the conflicts I think we can get this reviewed/merged
…-cloudagent-python into fix/oob-connectionless
Conflicts have been resolved! Sorry this one slipped my mind |
Some AATH tests are failing, for example (acapy-main built off of this branch)
There are no errors or stack traces showing up in the agent logs though ... |
@ianco did you try with the draft PR in AATH? openwallet-foundation/owl-agent-test-harness#474 I had to change some things in AATH to make it work, but it should work fine on that branch. If you have, then I'll take another look and see if some things broke in the meantime |
I'll try this today @TimoGlastra thanks |
I'm still getting one AATH failure with (including the draft PR mentioned above by @TimoGlastra )
|
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.
Overall looks good, except the AATH issue I mentioned. Do we need to synchronize merging the AATH PR before we can merge this one?
... actually I get this failure even running against the |
|
||
if context.message_receipt.thread_id not in allowed_thread_ids: | ||
LOGGER.debug( | ||
"Inbound message is for not allowed thread " |
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.
Possible typo, shouldn't it be something like Inbound message is not allowed for thread
Depends on AATH PR: openwallet-foundation/owl-agent-test-harness#474 |
Remember to close #1636 when this is merged. |
Adds support for connectionnless exchange using ICv1, ICv2, PPv1 and PPv2 protocols for both roles (so issuer, holder and verifier).
Can be initiated by creating or receiving an oob invitation without handshake protocols. If a connection already exists for the did in the oob invitation service, the connection will be used instead (as specified in OOB RFC).
Fixes #1636
Some notes on the implementation:
OobRecord
to keep track of the oob state. This is used for both connectionfull and connectionless exchanges and will be removed once the interaction is doneKeeping it in draft until I know for sure all CI tests have passed (can't run all tests on my M1 mac)