feat(core): connection-less issuance and verification #359

TimoGlastra · 2021-07-03T15:15:12Z

Adds connection-less issuance and verification.

Agent.credentials.createOutOfBandOffer - to create credential offer not bound to connection
Agent.proofs.createOutOfBandRequest - to create presentation request not bound to connection
Add ~service decorator

Required more changes than initially thought because I had to remove the required connection parameter everywhere.

Fixes #347
Fixes #346

Signed-off-by: Timo Glastra <[email protected]>

codecov-commenter · 2021-07-03T15:23:05Z

Codecov Report

Merging #359 (707034e) into main (d84acc7) will increase coverage by 0.10%.
The diff coverage is 83.12%.

@@            Coverage Diff             @@
##             main     #359      +/-   ##
==========================================
+ Coverage   86.52%   86.62%   +0.10%     
==========================================
  Files         246      248       +2     
  Lines        5010     5198     +188     
  Branches      788      827      +39     
==========================================
+ Hits         4335     4503     +168     
- Misses        675      695      +20

Impacted Files	Coverage Δ
packages/core/src/agent/TransportService.ts	`92.30% <ø> (ø)`
packages/core/src/index.ts	`100.00% <ø> (ø)`
...modules/credentials/repository/CredentialRecord.ts	`91.22% <0.00%> (-3.32%)`	⬇️
.../core/src/modules/proofs/repository/ProofRecord.ts	`85.36% <0.00%> (-4.38%)`	⬇️
...ages/core/src/transport/HttpOutboundTransporter.ts	`12.50% <0.00%> (ø)`
...ckages/core/src/transport/WsOutboundTransporter.ts	`5.26% <0.00%> (-0.10%)`	⬇️
packages/core/src/types.ts	`100.00% <ø> (ø)`
...les/credentials/handlers/OfferCredentialHandler.ts	`65.51% <50.00%> (-23.96%)`	⬇️
packages/core/src/agent/MessageSender.ts	`74.22% <65.21%> (-13.62%)`	⬇️
...les/credentials/handlers/IssueCredentialHandler.ts	`95.45% <90.00%> (+5.98%)`	⬆️
... and 28 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 d84acc7...707034e. Read the comment docs.

berendsliedrecht

I think connectionless instead of connection-less might be better.

TimoGlastra · 2021-07-14T12:56:12Z

I think connectionless instead of connection-less might be better.

I used the notation as used in the Aries RFCs: https://github.com/hyperledger/aries-rfcs/search?q=connection-less

TimoGlastra · 2021-07-14T12:56:48Z

Ah but seems google like connectionless better

berendsliedrecht · 2021-07-14T13:49:05Z

I think connectionless instead of connection-less might be better.

I used the notation as used in the Aries RFCs: https://github.com/hyperledger/aries-rfcs/search?q=connection-less

Ah yeah that makes sense then.

JamesKEbert · 2021-07-14T19:36:24Z

This is awesome, I have not given this a thoroooough review yet, but I wanted to gauge/express concern over the naming on createOutOfBandOffer, since it could cause confusion/collision with the Out Of Band (OOB) invitations? What are your thoughts here @TimoGlastra?

TimoGlastra · 2021-07-14T22:55:27Z

This is awesome, I have not given this a thoroooough review yet, but I wanted to gauge/express concern over the naming on createOutOfBandOffer, since it could cause confusion/collision with the Out Of Band (OOB) invitations? What are your thoughts here @TimoGlastra?

I actually think it matches perfectly with the OOB protocol. Out-Of-Band is a protocol to facilitate out-of-band exchange of messages. ACA-Py has it as a separate out-of-band topic but IMO it makes more sense to house them under the different modules. If we adopt OOB and want to issue a connectionless credentials, we can call the credentials module, which will actually return an OOB message with a credential offer inside of it.

I think we can abstract the OOB protocol mostly away unless you have very specific requirements besides connection invitations, connectionless issuance/verification. Does my thinking make sense?

JamesKEbert · 2021-07-17T06:48:29Z

I tend to agree with you here actually. I think having a separate module/service here for Out-Of-Band messages would be helpful, since it can do the bulk of the formatting and any needed storage (for non-connectionless). However I think other modules being able to use Out-Of-Band messages as you've outlined makes sense (essentially initiating the offer, but then collaborating with the OOB module to create the message).
So, in short I agree with your thought process here @TimoGlastra.

JamesKEbert

This looks good to me--I would like to take another look after your updates here, specifically given some transport and mediation changes that I think are likely relevant.

Signed-off-by: Timo Glastra <[email protected]>

TimoGlastra · 2021-07-17T14:53:28Z

This looks good to me--I would like to take another look after your updates here, specifically given some transport and mediation changes that I think are likely relevant.

@JamesKEbert Updated

TimoGlastra · 2021-07-17T14:57:52Z

It was quite a hassle to merge. I think some pieces could be handled better, but I'd like to handle that in separate PRs and have some discussions about it first. Mainly:

With auto accept merged, I had to duplicate quite some code between handler and module because they do a lot of the same stuff.
Message sender is becoming quite complex. I think it's unavoidable, but would be good to streamline it as much as possible. We can now send unpacked messages, packed messages, and also connection-less messages. All have overlap but all have differences.
Connection less requires quite some code in the module / handlers. I would like to look at a way to reduce this and just let it be handled by the router / messaging / packing logic. However I think we can do this best in combination with out-of-band.

So I'd like to merge in this state and make additional improvements in the future

to prevent race conditions in the server being shutdown Signed-off-by: Timo Glastra <[email protected]>

Signed-off-by: Timo Glastra <[email protected]>

jakubkoci · 2021-07-17T15:13:42Z

packages/core/tests/helpers.ts

+    filter((e) => state === undefined || e.payload.proofRecord.state === state),
+    timeout(timeoutMs),
+    catchError(() => {
+      throw new Error(`ProofStateChanged event not emitted within specified timeout: {


This error message string is a little bit harder to read. Wouldn't be better to pass such info as an object into error.

Error doesn't take additional arguments

packages/core/src/agent/MessageSender.ts

jakubkoci · 2021-07-18T13:32:07Z

packages/core/src/agent/__tests__/MessageSender.test.ts

      messageSender.setOutboundTransporter(outboundTransporter)
      transportServiceFindServicesMock.mockReturnValue([])

      await expect(messageSender.sendMessage(outboundMessage)).rejects.toThrow(
-        `Connection with id test-123 has no service!`
+        `Message is undeliverable to connection test-123 (Test 123)`


What if theirLabel contains some malicious code? Are we protected against that? I'm not sure if it's safe to use theirLabel anywhere in the code 🤔

I don't think that could cause any harm? thread id is also external. You mean like eval() harm?

I'm not sure what makes this property different from the others. We're not doing any SQL / HTTP rendering

Ok, sorry I was probably confused remembering some call where there was a discussion about the security of invitations.

Wouldn't it be better to use connection.label in such cases instead? I'm also a little bit confused what's the case for label and theirLabel 🤔 😄

packages/core/src/agent/models/InboundMessageContext.ts

packages/core/src/agent/__tests__/MessageSender.test.ts

packages/core/src/modules/credentials/CredentialsModule.ts

jakubkoci · 2021-07-20T11:40:44Z

packages/core/src/modules/credentials/CredentialsModule.ts

+      await this.messageSender.sendMessage(outboundMessage)
+    }
+    // Use ~service decorator otherwise
+    else if (credentialRecord.credentialMessage?.service && credentialRecord.requestMessage?.service) {


I'm not sure if it's good to write if statements in this way. It seems like a convention from C# but not common in JS language. https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Statements/if...else#using_else_if

I'm not sure I fully understand. I've never heard that else if is an anti-pattern in JS.

However I'm that a big fan of this code. The if/else shouldn't be there in the first place. But until out-of-band I'd like to keep it there

I mean this

} // Use ~service decorator otherwise else if

instead of

} else if { // Use ~service decorator otherwise }

jakubkoci · 2021-07-20T12:19:44Z

packages/core/src/modules/proofs/ProofsModule.ts

+    const nonce = proofRequestOptions.nonce ?? (await this.proofService.generateProofRequestNonce())
+
+    const proofRequest = new ProofRequest({
+      name: proofRequestOptions.name ?? 'proof-request',
+      version: proofRequestOptions.name ?? '1.0',
+      nonce,
+      requestedAttributes: proofRequestOptions.requestedAttributes,
+      requestedPredicates: proofRequestOptions.requestedPredicates,
+    })


Shouldn't this be part of proofService.createRequest service? It seems like duplication from another method calling proofService.createRequest

I didn't want to make opinions about default values in the service. I'm not even sure we should set default values...

So for now I'd like to keep it in the module (and have a discussion about the duplication during the AFJ call)

jakubkoci · 2021-07-20T12:20:12Z

packages/core/src/modules/proofs/ProofsModule.ts

+
+    return { proofRecord, requestMessage: message }
+  }
+
  /**
   * Accept a presentation request as prover (by sending a presentation message) to the connection
   * associated with the proof record.


I built JS doc blindness so I usually don't read it so much :) but I'm not sure if it's updated according to the changes.

jakubkoci · 2021-07-20T12:30:18Z

packages/core/tests/helpers.ts

+  const credentialRecordPromise = waitForCredentialRecord(holderAgent, {
+    threadId: issuerCredentialRecord.threadId,
+    state: CredentialState.OfferReceived,
+  })


Is there any benefit of making a Promise variable rather than awating it later at line 400?

I'm in the progress of refactoring the method to work with observables. We need to start listening before calling the method that will emit the event. Otherwise we start listening potentially after the event has already been emitted. So we need to start the listener, make the function call and then await.

I've partly refactored some other call to use ReplaySubject so we can replay all events that came in.

jakubkoci · 2021-07-20T12:49:53Z

I added a few notes, but it seems good overall 👍 I believe we're able to improve MessageSender. There is a lot of small duplication all over the class. It was hard for me to get oriented.

Signed-off-by: Timo Glastra <[email protected]>

TimoGlastra · 2021-07-20T17:11:50Z

There is a lot of small duplication all over the class. It was hard for me to get oriented.

Definitely would like to improve the duplication in message sender

Signed-off-by: Timo Glastra <[email protected]>

JamesKEbert · 2021-07-21T17:18:52Z

I tend to agree on the complexity of the message sender, but I think I'm comfortable improving on that in subsequent PRs.

Connection less requires quite some code in the module / handlers. I would like to look at a way to reduce this and just let it be handled by the router / messaging / packing logic. However I think we can do this best in combination with out-of-band.

I think this is an important item/consideration ^

jakubkoci

It would be nice to merge it before adding support for mulitransports. It seems there is quite some logic in the same area.

Signed-off-by: Timo Glastra <[email protected]>

TimoGlastra added 7 commits June 30, 2021 22:48

feat: add ~service decorator

dd0adb4

Signed-off-by: Timo Glastra <[email protected]>

feat: support connection-less issuance

1ecaa73

Signed-off-by: Timo Glastra <[email protected]>

test: add connection-less credential exchange test

6a860d1

Signed-off-by: Timo Glastra <[email protected]>

feat: support connection-less verification

17a46d1

Signed-off-by: Timo Glastra <[email protected]>

style: eslint warnings

0e8fc5a

Signed-off-by: Timo Glastra <[email protected]>

test: add tests for assertConnectionOrServiceDecorator

ae00e7a

Signed-off-by: Timo Glastra <[email protected]>

docs: update features in readme

858ed99

Signed-off-by: Timo Glastra <[email protected]>

TimoGlastra requested a review from a team as a code owner July 3, 2021 15:15

berendsliedrecht reviewed Jul 14, 2021

View reviewed changes

JamesKEbert approved these changes Jul 17, 2021

View reviewed changes

TimoGlastra added 2 commits July 17, 2021 15:09

Merge remote-tracking branch 'upstream/main' into feat/conectionless

17031e5

style: eslint warnings

e6820a3

Signed-off-by: Timo Glastra <[email protected]>

TimoGlastra changed the title ~~feat: support connection-less issuance and verification~~ feat(core): connection-less issuance and verification Jul 17, 2021

TimoGlastra added 2 commits July 17, 2021 16:48

fix: updates after merge

1730586

Signed-off-by: Timo Glastra <[email protected]>

test: only log on error

38244db

Signed-off-by: Timo Glastra <[email protected]>

TimoGlastra added 3 commits July 17, 2021 17:28

test: add timeout before shutdown of mediator

7ff329a

to prevent race conditions in the server being shutdown Signed-off-by: Timo Glastra <[email protected]>

Merge remote-tracking branch 'upstream/main' into feat/conectionless

869d4d1

chore(core): update to work with auto-accept proofs

6fc2589

Signed-off-by: Timo Glastra <[email protected]>

jakubkoci reviewed Jul 20, 2021

View reviewed changes

Merge remote-tracking branch 'upstream/main' into feat/conectionless

b91dcd4

test(core): add connectionles auto accept proof case

75d9bb9

Signed-off-by: Timo Glastra <[email protected]>

TimoGlastra added 3 commits July 21, 2021 10:19

test(core): fix tests

a6206b8

Signed-off-by: Timo Glastra <[email protected]>

test(core): longer wait for shutdown

08c6d69

Signed-off-by: Timo Glastra <[email protected]>

fix(core): start with $stop event

330b4f5

Signed-off-by: Timo Glastra <[email protected]>

jakubkoci approved these changes Jul 22, 2021

View reviewed changes

TimoGlastra added 3 commits July 22, 2021 23:43

Merge remote-tracking branch 'upstream/main' into feat/conectionless

8d2f1cc

test: break up e2e test to prevent flakyness

a504079

Signed-off-by: Timo Glastra <[email protected]>

build: tsconfig updates

707034e

Signed-off-by: Timo Glastra <[email protected]>

TimoGlastra linked an issue Jul 22, 2021 that may be closed by this pull request

Split up the e2e tests #377

Closed

TimoGlastra enabled auto-merge (squash) July 22, 2021 22:07

TimoGlastra disabled auto-merge July 22, 2021 22:07

TimoGlastra enabled auto-merge (squash) July 22, 2021 22:08

TimoGlastra disabled auto-merge July 22, 2021 22:08

TimoGlastra enabled auto-merge (squash) July 22, 2021 22:08

TimoGlastra merged commit fb46ade into openwallet-foundation:main Jul 22, 2021

TimoGlastra deleted the feat/conectionless branch October 28, 2021 10:10

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(core): connection-less issuance and verification #359

feat(core): connection-less issuance and verification #359

TimoGlastra commented Jul 3, 2021 •

edited

Loading

codecov-commenter commented Jul 3, 2021 •

edited

Loading

berendsliedrecht left a comment

TimoGlastra commented Jul 14, 2021

TimoGlastra commented Jul 14, 2021

berendsliedrecht commented Jul 14, 2021

JamesKEbert commented Jul 14, 2021

TimoGlastra commented Jul 14, 2021 •

edited

Loading

JamesKEbert commented Jul 17, 2021

JamesKEbert left a comment

TimoGlastra commented Jul 17, 2021

TimoGlastra commented Jul 17, 2021

jakubkoci Jul 17, 2021

TimoGlastra Jul 20, 2021

jakubkoci Jul 18, 2021

TimoGlastra Jul 20, 2021 •

edited

Loading

jakubkoci Jul 22, 2021

jakubkoci Jul 22, 2021

jakubkoci Jul 20, 2021

TimoGlastra Jul 20, 2021

jakubkoci Jul 21, 2021

jakubkoci Jul 20, 2021

TimoGlastra Jul 20, 2021

TimoGlastra Jul 20, 2021

jakubkoci Jul 21, 2021

jakubkoci Jul 20, 2021

jakubkoci Jul 20, 2021

TimoGlastra Jul 20, 2021

jakubkoci commented Jul 20, 2021 •

edited

Loading

TimoGlastra commented Jul 20, 2021

JamesKEbert commented Jul 21, 2021

jakubkoci left a comment

feat(core): connection-less issuance and verification #359

feat(core): connection-less issuance and verification #359

Conversation

TimoGlastra commented Jul 3, 2021 • edited Loading

codecov-commenter commented Jul 3, 2021 • edited Loading

Codecov Report

berendsliedrecht left a comment

Choose a reason for hiding this comment

TimoGlastra commented Jul 14, 2021

TimoGlastra commented Jul 14, 2021

berendsliedrecht commented Jul 14, 2021

JamesKEbert commented Jul 14, 2021

TimoGlastra commented Jul 14, 2021 • edited Loading

JamesKEbert commented Jul 17, 2021

JamesKEbert left a comment

Choose a reason for hiding this comment

TimoGlastra commented Jul 17, 2021

TimoGlastra commented Jul 17, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TimoGlastra Jul 20, 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

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 Jul 20, 2021 • edited Loading

TimoGlastra commented Jul 20, 2021

JamesKEbert commented Jul 21, 2021

jakubkoci left a comment

Choose a reason for hiding this comment

TimoGlastra commented Jul 3, 2021 •

edited

Loading

codecov-commenter commented Jul 3, 2021 •

edited

Loading

TimoGlastra commented Jul 14, 2021 •

edited

Loading

TimoGlastra Jul 20, 2021 •

edited

Loading

jakubkoci commented Jul 20, 2021 •

edited

Loading