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: oob without handhsake improvements and routing #1511

Conversation

TimoGlastra
Copy link
Contributor

Several improvements to the OOB and connection-less exchange flows.

Fixes #1376
Fixes #1250

Mainly it introduces the following changes:

  • Integrate the legacy connection-less exchange with out of band, just like we do with the legacy connection invitations. This means that an out of band record will now be created when you call createLegacyConnectionlessInvitation. In addition, you can now also pass a legacy connectionless invitation url to agent.oob.recieveInvitationFromUrl. This will transform the legacy connectionless invitation to an out of band invitation. The method to receive the message using agent.receiveMessage works to not introduce breaking changes, but I've marked that method as deprecated and we can remove it in 0.5.0. This leaves multiple flows now that work, and we probably want to do some cleanup of the connectionless code once we remove the old flow in 0.5.0 (not using the oob api).
  • The receiveInvitation already supported a routing prop, but this was not used for connection-less exchanges. This means you could not control which routing config would be used for a connection-less exchange you received an invitation for. Now the routing is used when you respond to a connectionless out of band invitation.
  • Several bugs in the oob flow when using connectionless (this was broken).
  • Added a getOutboundMessageContext method (again ;), we just removed it lol). But this takes into account everything that is needed for responding in connection-full and connection-less exchanges and removes a lot of the duplicated complex logic for handling connection-less. Once we remove the old flow (not using oob api, but still can use legacy connection-less), we could even fully abstract connection-less vs connection, and thus any protocol that uses the DidCommMessageRepository could support connection-less out of the box.

Our main use case for these changes is DIDComm over Bluetooth, where we use a mediator for normal HTTP exchagnes, but if we respond to an invitation over BLE, we want to use custom routing and explicitly set useDefaultMediator to false, so we can directly communicate over bluetooth instead of through a mediator.

Signed-off-by: Timo Glastra <timo@animo.id>
@TimoGlastra TimoGlastra requested a review from a team as a code owner July 21, 2023 11:58
Signed-off-by: Timo Glastra <timo@animo.id>
Signed-off-by: Timo Glastra <timo@animo.id>
Signed-off-by: Timo Glastra <timo@animo.id>
@codecov-commenter
Copy link

Codecov Report

Merging #1511 (45200e5) into main (04a8058) will decrease coverage by 4.01%.
The diff coverage is 88.49%.

@@            Coverage Diff             @@
##             main    #1511      +/-   ##
==========================================
- Coverage   85.79%   81.79%   -4.01%     
==========================================
  Files         943      946       +3     
  Lines       22588    22591       +3     
  Branches     3915     3953      +38     
==========================================
- Hits        19380    18478     -902     
- Misses       3027     3850     +823     
- Partials      181      263      +82     
Impacted Files Coverage Δ
packages/core/src/agent/BaseAgent.ts 95.06% <ø> (ø)
...ore/src/modules/connections/DidExchangeProtocol.ts 84.93% <ø> (-0.07%) ⬇️
...ules/routing/services/MediationRecipientService.ts 82.35% <ø> (ø)
...s/core/src/storage/didcomm/DidCommMessageRecord.ts 96.77% <ø> (ø)
packages/core/tests/helpers.ts 88.23% <ø> (-0.07%) ⬇️
samples/extension-module/dummy/DummyApi.ts 93.54% <ø> (ø)
...nsion-module/dummy/handlers/DummyRequestHandler.ts 90.90% <66.66%> (ø)
...dentials/v1/handlers/V1RequestCredentialHandler.ts 94.44% <75.00%> (-1.56%) ⬇️
packages/core/src/modules/oob/OutOfBandService.ts 96.22% <76.92%> (-2.71%) ⬇️
...ckages/core/src/agent/getOutboundMessageContext.ts 82.19% <82.19%> (ø)
... and 33 more

... and 28 files with indirect coverage changes

Copy link
Contributor

@berendsliedrecht berendsliedrecht left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Woooooo! Awesome work :). I think an additional review would be good here but if no one has the capacity right now to review I think we can merge it when my minor comments are resolved.

packages/core/src/agent/getOutboundMessageContext.ts Outdated Show resolved Hide resolved
packages/core/src/agent/getOutboundMessageContext.ts Outdated Show resolved Hide resolved
packages/core/src/utils/parseInvitation.ts Show resolved Hide resolved
packages/core/src/utils/parseInvitation.ts Show resolved Hide resolved
packages/core/src/utils/parseInvitation.ts Show resolved Hide resolved
Signed-off-by: Timo Glastra <timo@animo.id>
Signed-off-by: Timo Glastra <timo@animo.id>
Signed-off-by: Timo Glastra <timo@animo.id>
@TimoGlastra TimoGlastra merged commit 9e69cf4 into openwallet-foundation:main Jul 25, 2023
7 checks passed
@TimoGlastra TimoGlastra deleted the feat/oob-no-default-mediator branch July 25, 2023 10:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants