feat: add support for new out-of-band protocol #531

jakubkoci · 2021-11-08T15:59:57Z

This is the first step in the implementation of #344.

It corresponds approximately to Step 1 defined in Aries RFC 0496: Transition to the Out of Band and DID Exchange Protocols.

As you can see I decided to create an Out-of-band module and keep the implementation there. I realized that putting it into the Connection module could cause more issues than benefits right now. It has only two drawbacks:

API users would need to decide what method to call for now. But it allows more gradual adoption of new out-of-band by users. When we have a full implementation, users will ideally just call one method for OOB messages.
I copy-pasted methods for connection creation. That can be solved by calling the whole Connections module from the OOB module.

codecov-commenter · 2021-11-08T16:23:18Z

Codecov Report

Merging #531 (7a6ea0e) into main (8e03f35) will increase coverage by 0.49%.
The diff coverage is 92.59%.

@@            Coverage Diff             @@
##             main     #531      +/-   ##
==========================================
+ Coverage   86.88%   87.38%   +0.49%     
==========================================
  Files         308      316       +8     
  Lines        6513     6755     +242     
  Branches      965     1067     +102     
==========================================
+ Hits         5659     5903     +244     
+ Misses        850      848       -2     
  Partials        4        4

Impacted Files	Coverage Δ
.../core/src/modules/connections/ConnectionsModule.ts	`83.13% <ø> (+9.63%)`	⬆️
.../modules/connections/services/ConnectionService.ts	`89.86% <ø> (+1.32%)`	⬆️
packages/core/src/modules/oob/OutOfBandModule.ts	`91.25% <91.25%> (ø)`
.../core/src/modules/oob/messages/OutOfBandMessage.ts	`92.72% <92.72%> (ø)`
packages/core/src/agent/Agent.ts	`99.03% <100.00%> (+0.99%)`	⬆️
packages/core/src/agent/MessageReceiver.ts	`83.16% <100.00%> (+1.72%)`	⬆️
packages/core/src/index.ts	`100.00% <100.00%> (ø)`
.../src/modules/oob/handlers/HandshakeReuseHandler.ts	`100.00% <100.00%> (ø)`
packages/core/src/modules/oob/handlers/index.ts	`100.00% <100.00%> (ø)`
.../src/modules/oob/messages/HandshakeReuseMessage.ts	`100.00% <100.00%> (ø)`
... and 8 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 8e03f35...7a6ea0e. Read the comment docs.

TimoGlastra

Nice work @jakubkoci! Glad to see the OOB protocol finally being added to AFJ.

I'm not so sure on splitting the createInvitation from createOobMessage. Both create an OOB invitation. And I think we're missing some of the nice combinations here. handshake_protocols and requests~attach are not exclusive, so I think it may be easier to just create a single createInvitation and a single receiveInvitation method that allows to specify what should be done with handshake protocols / attached messages.

Second point is that I'd rather not merge this PR as is. It feels like we are leaving a lot of TODOs out there, while I would rather just add OOB as a whole, where we have really thought about all implementation details. There are still a lot of unknowns with connection reuse, connectionless, did-exchange and an OOB record that I would rather keep it in a separate OOB branch for now. What do you think?

packages/core/src/modules/oob/OutOfBandMessage.ts

TimoGlastra · 2021-11-08T16:19:31Z

packages/core/src/modules/oob/OutOfBandMessage.ts

+  public readonly goal?: string
+
+  // TODO what type is it, is there any enum or should we create a new one
+  public readonly accept: string[] = []


No enum yet, need to create a new one

Not sure if the enum is the best solution. It could prevent the framework from being dynamically enhanced. I've been thinking about where the information about accepted mime types should reside. Maybe inside the EnvelopeService 🤔

TimoGlastra · 2021-11-08T16:21:15Z

packages/core/src/modules/oob/OutOfBandMessage.ts

+  @IsInstance(Attachment, { each: true })
+  private requests?: Attachment[]
+
+  public readonly services: DidCommService[] = []


Can also be plain dids (strings)

packages/core/src/modules/oob/OutOfBandModule.ts

TimoGlastra · 2021-11-08T16:31:01Z

packages/core/src/modules/oob/OutOfBandModule.ts

+    const invitation = new ConnectionInvitationMessage({ label: 'connection label', ...outOfBandMessage.services[0] })
+
+    let connectionRecord = await this.connectionService.processInvitation(invitation, { routing })
+    if (config.autoAccept) {


Do we want this to take into account agent config autoAcceptConnection?

TimoGlastra · 2021-11-08T16:33:26Z

packages/core/src/modules/oob/OutOfBandModule.ts

+    outOfBandMessage.services.push(message.service.toDidCommService())
+    message.service = undefined


I think the advantage of using OOB is that the AgentMessage itself doesn't need the ~service anymore.

This is just a workaround for now. It allows us to pass the AgentMessage into receiveMessage and still be able to process it with the current implementation.

TimoGlastra · 2021-11-08T16:34:56Z

packages/core/src/modules/oob/OutOfBandModule.ts

+   */
+  public async receiveOobMessage(outOfBandMessage: OutOfBandMessage): Promise<void> {
+    if (outOfBandMessage.handshakeProtocols) {
+      throw new AriesFrameworkError('OOB message contains unsupported `handshake_protocols` attribute.')


This feels a bit misleading, handshake_protocols is definitely supported, just not in this method.

Also, does this mean handshake_protocols and requests~attach can't be combined in AFJ?

I'll definitely look at the unification of those methods as you suggested in another comment.

But the combination of handshake_protocols and requests~attach is not allowed according to Step 1 in Aries RFC 0496: Transition to the Out of Band and DID Exchange Protocols

Updated. It supports both handshake_protocols and requests~attach.

packages/core/src/modules/oob/OutOfBandMessage.ts

TimoGlastra · 2021-11-08T16:40:32Z

packages/core/src/modules/oob/OutOfBandModule.ts

+    const mediationRecord = await this.mediationRecipientService.discoverMediation()
+    const routing = await this.mediationRecipientService.getRouting(mediationRecord)
+    const { connectionRecord: connectionRecord } = await this.connectionService.createInvitation({


Maybe it would be good to allow modules to call modules as it can prevent some duplication here

jakubkoci · 2021-11-10T21:45:19Z

Hi @TimoGlastra. Thanks for the suggestions and feedback. I unified it into just two methods createMessage and receiveMessage. It make sense for receiveMessage I don't have doubt there, but createMessage looks much worse now. I'm gonna look at oob record and other missing parts you mentioned, how they fit in and maybe it improves implementation by itself :)

I'm good with not merging it right now. I wasn't sure if we want to do it according to that transition spec or not. If we don't we can wait and merge as a whole, I guess.

jakubkoci · 2021-12-30T11:14:51Z

It's still not polished as I wished, but it's working according to Step 1 at least. It actually does more than Step 1. I wrote a brief overview of what's done and what's missing:

Done

create oob with connection invitation
create oob with connection invitation with message requests
create oob only with message requests
create a connection based on oob invitation
create a connection based on oob invitation and process message requests
process connection-less message requests
support for both old and new encoded URL

Todo

Handle handshake reuse message
Store all messages to prevent losing them when an agent goes down.
Replace old ConnectionInvitationMessage with the new OutOfBandMessage.
Use DIDs. Transform to/from Peer DID, DID key, DID peer (feat: add generic did resolver #554 helps a lot with it)
Allow creating oob for an existing connection.
~~Process only the first supported message~~
Validate services

TimoGlastra

Overall structure LGTM 👍 , but have left a lot of quibbles

packages/core/src/modules/oob/OutOfBandModule.ts

packages/core/src/modules/oob/__tests__/OutOfBandMessage.test.ts

packages/core/src/modules/oob/messages/OutOfBandMessage.ts

packages/core/src/modules/oob/OutOfBandModule.ts

Signed-off-by: Jakub Koci <[email protected]>

…ssage receiver Signed-off-by: Jakub Koci <[email protected]>

Signed-off-by: Jakub Koci <[email protected]>

jakubkoci · 2022-01-21T16:45:30Z

I consider the code in here approved. I moved it to the feat/out-of-band branch, where I'll continue with adding did exchange protocol.

jakubkoci requested a review from a team as a code owner November 8, 2021 15:59

jakubkoci changed the title ~~Out of band~~ feat: Add support for new out-of-band protocol Stage 1 Nov 8, 2021

TimoGlastra requested changes Nov 8, 2021

View reviewed changes

This was referenced Nov 23, 2021

refactor: move methods to get supported protocols to Dispatcher #543

Merged

Handle attachments in a generic way #548

Closed

jakubkoci changed the title ~~feat: Add support for new out-of-band protocol Stage 1~~ feat: add support for new out-of-band protocol Nov 25, 2021

jakubkoci mentioned this pull request Dec 28, 2021

feat(core): allow to set auto accept connetion exchange when accepting invitation #589

Merged

TimoGlastra reviewed Jan 5, 2022

View reviewed changes

jakubkoci added 19 commits January 19, 2022 14:59

Create OOB invitation

74f3ec7

Signed-off-by: Jakub Koci <[email protected]>

Receive OOB connection invitaion

e31cafb

Signed-off-by: Jakub Koci <[email protected]>

Send and recevie request via OOB invitation

103ebb3

Signed-off-by: Jakub Koci <[email protected]>

Separate OOB invitation and OOB message handling

6528488

Signed-off-by: Jakub Koci <[email protected]>

Refactor and get rid of unnecessary dependencies to dispatcher and me…

ab6ec4e

…ssage receiver Signed-off-by: Jakub Koci <[email protected]>

Allow undefined handshake protocols and request attach attributes

5cf076d

Signed-off-by: Jakub Koci <[email protected]>

Remove unused code and logs

90b41d6

Signed-off-by: Jakub Koci <[email protected]>

Rename oob tests wallets

c4ca249

Signed-off-by: Jakub Koci <[email protected]>

Add small changes based on review

5388c81

Signed-off-by: Jakub Koci <[email protected]>

Extract getting supported handshake protocols into discover service

07cf37c

Signed-off-by: Jakub Koci <[email protected]>

Pass all oob message props via constructor

8e2129e

Signed-off-by: Jakub Koci <[email protected]>

Unify create and receive oob message methods

f108330

Signed-off-by: Jakub Koci <[email protected]>

Create OOB message with both handshake and requests

114e45b

Signed-off-by: Jakub Koci <[email protected]>

Accept OOB message with both handshake and requests

7f637a2

Signed-off-by: Jakub Koci <[email protected]>

Check if handshake protocols are supported

ee7ea3a

Signed-off-by: Jakub Koci <[email protected]>

Wait until the connection is made before processing requests

eeb0b3d

Signed-off-by: Jakub Koci <[email protected]>

Reuse connection when it exists

d7edbf0

Signed-off-by: Jakub Koci <[email protected]>

Refactor logic when connection already exists

b7acb63

Signed-off-by: Jakub Koci <[email protected]>

Refactor finding of existing connection

ade0982

Signed-off-by: Jakub Koci <[email protected]>

jakubkoci added 27 commits January 19, 2022 14:59

Omit version from handshake protocol list

5c9b2a4

Signed-off-by: Jakub Koci <[email protected]>

Update public API and reorganize folder structure

dcf5668

Signed-off-by: Jakub Koci <[email protected]>

Add oob invitation encoding and decoding

3a7c38f

Signed-off-by: Jakub Koci <[email protected]>

Use filter method for handshake protocols from dispatcher

d7be64e

Signed-off-by: Jakub Koci <[email protected]>

Create a new connection instead of reusing an existing one

dc6d85c

Signed-off-by: Jakub Koci <[email protected]>

Update according to main branch API changes

01fd06b

Signed-off-by: Jakub Koci <[email protected]>

Use connections module instead of service

a12a0e8

Signed-off-by: Jakub Koci <[email protected]>

Add support for old url encoded connection invitation

7ec4938

Signed-off-by: Jakub Koci <[email protected]>

Iterate over all services and reciepient keys to find a connection

8514b8a

Signed-off-by: Jakub Koci <[email protected]>

Replace custom event handler with async method provided by framework

ab38518

Signed-off-by: Jakub Koci <[email protected]>

Update types

0c5e807

Signed-off-by: Jakub Koci <[email protected]>

Pass connection label to old invitation message

95af882

Signed-off-by: Jakub Koci <[email protected]>

Rename unpack to plaintext

e47d834

Signed-off-by: Jakub Koci <[email protected]>

Add thread to handshake reuse message

97acfcf

Signed-off-by: Jakub Koci <[email protected]>

Code review updates mainly about API and docs

654be67

Signed-off-by: Jakub Koci <[email protected]>

Allow dids in services attribute of oob message

9895bd4

Signed-off-by: Jakub Koci <[email protected]>

Process only first supported requests message

b8fa0c2

Signed-off-by: Jakub Koci <[email protected]>

Throw error when there are no services in connection record

0769c48

Signed-off-by: Jakub Koci <[email protected]>

Refactor get requests method

13ad50d

Signed-off-by: Jakub Koci <[email protected]>

Log error insted of commented code

27578b9

Signed-off-by: Jakub Koci <[email protected]>

Update api after rebase

2f11716

Signed-off-by: Jakub Koci <[email protected]>

Throw error instead of resolving did from services attribute

697b6cb

Signed-off-by: Jakub Koci <[email protected]>

Update structure of oob tests

ae94713

Signed-off-by: Jakub Koci <[email protected]>

Add connection record to received message event

1e5cbf1

Signed-off-by: Jakub Koci <[email protected]>

Update test of oob with requests witg parsing of encoded invitation

c939b01

Signed-off-by: Jakub Koci <[email protected]>

Make label mandatory

ecc3830

Signed-off-by: Jakub Koci <[email protected]>

Update receive message method signature

af2e53f

Signed-off-by: Jakub Koci <[email protected]>

jakubkoci closed this Jan 21, 2022

jakubkoci deleted the out-of-band branch January 21, 2022 11:25

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add support for new out-of-band protocol #531

feat: add support for new out-of-band protocol #531

jakubkoci commented Nov 8, 2021

codecov-commenter commented Nov 8, 2021 •

edited

Loading

TimoGlastra left a comment

TimoGlastra Nov 8, 2021

jakubkoci Nov 24, 2021 •

edited

Loading

TimoGlastra Nov 8, 2021

TimoGlastra Nov 8, 2021

TimoGlastra Nov 8, 2021

jakubkoci Nov 9, 2021

TimoGlastra Nov 8, 2021

jakubkoci Nov 9, 2021

jakubkoci Nov 24, 2021

TimoGlastra Nov 8, 2021

jakubkoci commented Nov 10, 2021

jakubkoci commented Dec 30, 2021 •

edited

Loading

TimoGlastra left a comment

jakubkoci commented Jan 21, 2022

		outOfBandMessage.services.push(message.service.toDidCommService())
		message.service = undefined

feat: add support for new out-of-band protocol #531

feat: add support for new out-of-band protocol #531

Conversation

jakubkoci commented Nov 8, 2021

codecov-commenter commented Nov 8, 2021 • edited Loading

Codecov Report

TimoGlastra left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jakubkoci Nov 24, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jakubkoci commented Nov 10, 2021

jakubkoci commented Dec 30, 2021 • edited Loading

Done

Todo

TimoGlastra left a comment

Choose a reason for hiding this comment

jakubkoci commented Jan 21, 2022

codecov-commenter commented Nov 8, 2021 •

edited

Loading

jakubkoci Nov 24, 2021 •

edited

Loading

jakubkoci commented Dec 30, 2021 •

edited

Loading