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 <jakub.koci@gmail.com>

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

jakubkoci force-pushed the out-of-band branch 3 times, most recently from 9677049 to cc60cb2 Compare November 18, 2021 07:59

jakubkoci force-pushed the out-of-band branch 2 times, most recently from c49b971 to 7fa4006 Compare November 23, 2021 08:35

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 force-pushed the out-of-band branch 2 times, most recently from 7464bf3 to 1bc158d Compare November 24, 2021 14:30

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 force-pushed the out-of-band branch from 26a44f4 to f457261 Compare December 2, 2021 13:45

jakubkoci force-pushed the out-of-band branch from f457261 to f9f7f55 Compare December 28, 2021 15:15

jakubkoci mentioned this pull request Dec 28, 2021

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

Merged

jakubkoci force-pushed the out-of-band branch 2 times, most recently from 7dd2460 to e3d2a74 Compare December 29, 2021 18:24

jakubkoci force-pushed the out-of-band branch from 17afca8 to c90ce26 Compare January 4, 2022 10:41

TimoGlastra reviewed Jan 5, 2022

View reviewed changes

jakubkoci force-pushed the out-of-band branch 4 times, most recently from 41e47db to 0eefa6b Compare January 12, 2022 15:33

jakubkoci force-pushed the out-of-band branch from 3f9b861 to f607f01 Compare January 18, 2022 08:22

jakubkoci added 2 commits January 19, 2022 14:59

Create OOB invitation

74f3ec7

Signed-off-by: Jakub Koci <jakub.koci@gmail.com>

Receive OOB connection invitaion

e31cafb

Signed-off-by: Jakub Koci <jakub.koci@gmail.com>

jakubkoci added 26 commits January 19, 2022 14:59

Update public API and reorganize folder structure

dcf5668

Signed-off-by: Jakub Koci <jakub.koci@gmail.com>

Add oob invitation encoding and decoding

3a7c38f

Signed-off-by: Jakub Koci <jakub.koci@gmail.com>

Use filter method for handshake protocols from dispatcher

d7be64e

Signed-off-by: Jakub Koci <jakub.koci@gmail.com>

Create a new connection instead of reusing an existing one

dc6d85c

Signed-off-by: Jakub Koci <jakub.koci@gmail.com>

Update according to main branch API changes

01fd06b

Signed-off-by: Jakub Koci <jakub.koci@gmail.com>

Use connections module instead of service

a12a0e8

Signed-off-by: Jakub Koci <jakub.koci@gmail.com>

Add support for old url encoded connection invitation

7ec4938

Signed-off-by: Jakub Koci <jakub.koci@gmail.com>

Iterate over all services and reciepient keys to find a connection

8514b8a

Signed-off-by: Jakub Koci <jakub.koci@gmail.com>

Replace custom event handler with async method provided by framework

ab38518

Signed-off-by: Jakub Koci <jakub.koci@gmail.com>

Update types

0c5e807

Signed-off-by: Jakub Koci <jakub.koci@gmail.com>

Pass connection label to old invitation message

95af882

Signed-off-by: Jakub Koci <jakub.koci@gmail.com>

Rename unpack to plaintext

e47d834

Signed-off-by: Jakub Koci <jakub.koci@gmail.com>

Add thread to handshake reuse message

97acfcf

Signed-off-by: Jakub Koci <jakub.koci@gmail.com>

Code review updates mainly about API and docs

654be67

Signed-off-by: Jakub Koci <jakub.koci@gmail.com>

Allow dids in services attribute of oob message

9895bd4

Signed-off-by: Jakub Koci <jakub.koci@gmail.com>

Process only first supported requests message

b8fa0c2

Signed-off-by: Jakub Koci <jakub.koci@gmail.com>

Throw error when there are no services in connection record

0769c48

Signed-off-by: Jakub Koci <jakub.koci@gmail.com>

Refactor get requests method

13ad50d

Signed-off-by: Jakub Koci <jakub.koci@gmail.com>

Log error insted of commented code

27578b9

Signed-off-by: Jakub Koci <jakub.koci@gmail.com>

Update api after rebase

2f11716

Signed-off-by: Jakub Koci <jakub.koci@gmail.com>

Throw error instead of resolving did from services attribute

697b6cb

Signed-off-by: Jakub Koci <jakub.koci@gmail.com>

Update structure of oob tests

ae94713

Signed-off-by: Jakub Koci <jakub.koci@gmail.com>

Add connection record to received message event

1e5cbf1

Signed-off-by: Jakub Koci <jakub.koci@gmail.com>

Update test of oob with requests witg parsing of encoded invitation

c939b01

Signed-off-by: Jakub Koci <jakub.koci@gmail.com>

Make label mandatory

ecc3830

Signed-off-by: Jakub Koci <jakub.koci@gmail.com>

Update receive message method signature

af2e53f

Signed-off-by: Jakub Koci <jakub.koci@gmail.com>

jakubkoci force-pushed the out-of-band branch from f607f01 to af2e53f Compare January 19, 2022 15:11

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

TimoGlastra left a comment

TimoGlastra Nov 8, 2021

jakubkoci Nov 24, 2021 •

edited

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

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

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

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

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

jakubkoci Nov 24, 2021 •

edited

jakubkoci commented Dec 30, 2021 •

edited