fix(oob): support oob with connection and messages

[TimoGlastra]

Fixes #1129

We now correctly check whether an oob exchange is associated with an exchange if there's no connectionId, and assign the connection if this is the case.

I think we now really support al flows and combinations of messages / handhsake / connectionless from the oob protocol.

[codecov-commenter]

Codecov Report

Merging #1558 (c015708) into main (11050af) will decrease coverage by 23.14%.
The diff coverage is 71.42%.

@@             Coverage Diff             @@
##             main    #1558       +/-   ##
===========================================
- Coverage   85.73%   62.59%   -23.14%     
===========================================
  Files         950      774      -176     
  Lines       22761    17843     -4918     
  Branches     3982     3069      -913     
===========================================
- Hits        19514    11169     -8345     
- Misses       3062     6136     +3074     
- Partials      185      538      +353     

Files Changed	Coverage Δ
...oncreds/src/protocols/proofs/v1/V1ProofProtocol.ts	2.42% <0.00%> (-86.27%)	⬇️
...ges/core/src/modules/credentials/CredentialsApi.ts	85.56% <ø> (-1.04%)	⬇️
packages/core/src/types.ts	100.00% <ø> (ø)
...c/protocols/credentials/v1/V1CredentialProtocol.ts	30.21% <45.45%> (-62.76%)	⬇️
.../src/modules/proofs/protocol/v2/V2ProofProtocol.ts	76.78% <60.00%> (-16.91%)	⬇️
.../modules/connections/services/ConnectionService.ts	85.16% <66.66%> (-3.18%)	⬇️
...es/credentials/protocol/v2/V2CredentialProtocol.ts	76.98% <66.66%> (-13.48%)	⬇️
packages/core/src/utils/parseInvitation.ts	46.15% <66.66%> (-31.63%)	⬇️
...entials/protocol/v2/CredentialFormatCoordinator.ts	89.05% <83.33%> (-0.73%)	⬇️
packages/core/src/modules/oob/OutOfBandApi.ts	83.80% <89.65%> (-5.38%)	⬇️
... and 6 more

... and 467 files with indirect coverage changes

[berendsliedrecht]

Couple of very minor points, otherwise LGTM.

[berendsliedrecht]

this file has an unused import from http which is a mistake, but not from your PR. Can you remove it?

GitHub does not allow me to add a comment there...

[berendsliedrecht]

Would it make more sense to have legacyInvitationType be an enum?

[TimoGlastra]

Yeah probably :)

[genaris]

In ConnectionRecord we have the protocol field where we define the handshake protocol for the DID Exchange, and in the case of DIDComm V2, where no handshake protocol is involved, we set it to None. If this legacyInvitationType refers to the message type, would it make sense to call it 'none'. in the case of a connection-less exchange?

[TimoGlastra]

would it make sense to call it 'none'. in the case of a connection-less exchange?

Well in that case we don't know explicitly if it's a connectionless message, and we have custom behaviour for that as you can see here. I'd still call a message with a ~service an invitation to do something.

So invitation type here refers less to connections, and more to an invitation to an exchange that is sent not over didcomm. (connectionless, connection v1, oob v1, etc..)

[berendsliedrecht]

Maybe it should be

Suggested change

	{ knownConnectionId }: { knownConnectionId?: string }
	{ expectedConnectionId }: { expectedConnectionId?: string }

[TimoGlastra]

Yeah that makes sense. I was thinking if it is undefined it doesn't make sense. But after thining about it again, if it's undefined we don't really expect a specific connection Id

[genaris]

In ConnectionRecord we have the protocol field where we define the handshake protocol for the DID Exchange, and in the case of DIDComm V2, where no handshake protocol is involved, we set it to None. If this legacyInvitationType refers to the message type, would it make sense to call it 'none'. in the case of a connection-less exchange?

[genaris]

Wow this is a huge method with a huge name!

[TimoGlastra]

Yeah, i like explicitness over being brief. But suggestions welcome! :)