feat: add problem report protocol

[nbAmit]

Summary of Changes

Implemented the problem report RFC-0035

Problem Report RFC effective for reporting of errors and warnings to other agents. Once this PR is merged agent will be able to report problems occurred in Connection, Issue Credential & Present Proof

Related Issue

#58

[codecov-commenter]

Codecov Report

Merging #560 (deff98f) into main (29baae7) will decrease coverage by 0.32%.
The diff coverage is 71.48%.

@@            Coverage Diff             @@
##             main     #560      +/-   ##
==========================================
- Coverage   86.15%   85.83%   -0.33%     
==========================================
  Files         267      286      +19     
  Lines        5845     6063     +218     
  Branches      876      889      +13     
==========================================
+ Hits         5036     5204     +168     
- Misses        806      856      +50     
  Partials        3        3              

Impacted Files	Coverage Δ
...s/credentials/handlers/RequestCredentialHandler.ts	96.00% <ø> (ø)
packages/core/src/types.ts	100.00% <ø> (ø)
packages/core/src/agent/Dispatcher.ts	84.90% <12.50%> (-6.77%)	⬇️
.../modules/connections/services/ConnectionService.ts	90.77% <14.28%> (-5.10%)	⬇️
.../core/src/modules/credentials/CredentialsModule.ts	82.45% <23.07%> (-4.89%)	⬇️
packages/core/src/modules/proofs/ProofsModule.ts	80.55% <23.07%> (-5.71%)	⬇️
...es/proofs/errors/PresentationProblemReportError.ts	50.00% <50.00%> (ø)
packages/core/src/agent/MessageReceiver.ts	81.25% <53.84%> (-7.94%)	⬇️
...s/core/src/modules/proofs/services/ProofService.ts	86.91% <61.53%> (+0.36%)	⬆️
.../modules/credentials/services/CredentialService.ts	95.96% <64.70%> (+0.15%)	⬆️
... and 32 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 29baae7...deff98f. Read the comment docs.

[TimoGlastra]

Hi @nbAmit this is really great!

I left some comments, but the main overarching points are:

• Try to avoid non-null assertions, as it bypasses the typescript checks
• If we try include the problem report message also in the error for connection/credential/proofs we can handle it generically in the dispatcher instead of the separate handler classes

Also I have some questions:

• Do you think we need to expose this as a seperate module / do we need to expose this on the issue / proofs / connections modules (like aca-py)?
• Have you tested this with connectionless?
• Maybe it would be good to also add some e2e tests using problem report messages

[TimoGlastra]

Maybe we can extract this to a separate method (like transformMessage) so we don't have to nest try/catch statements

[TimoGlastra]

Suggested change

	this.sendProblemReportMessage(
	await this.sendProblemReportMessage(

[TimoGlastra]

We should try to avoid non-null assertions. If we don't have a connection, we can't send the message so we shouldn't ignore this I think

[TimoGlastra]

Suggested change

// throw new AriesFrameworkError(`No message class found for message type "${messageType}"`)

[TimoGlastra]

Same here, avoid non-null assertions

[TimoGlastra]

Suggested change

	proofRecord.errorMsg = `${presentationProblemReportMessage.description.code} : ${presentationProblemReportMessage.description.en}`
	proofRecord.errorMsg = `${presentationProblemReportMessage.description.code}: ${presentationProblemReportMessage.description.en}`

[TimoGlastra]

If you add the errorMsg assignment to before the updateState call you don't need have to call update again. (the updateState will also store the record)

[TimoGlastra]

update not needed if you call updateState

[TimoGlastra]

I'm not sure if state none is a good representation of the state. Shouldn't this be more something like error?

@jakubkoci @JamesKEbert any insights?

[jakubkoci]

Hmm, that's a good point. It's a question if we want to update the state at all? If I look at list of states at Aries RFC 0023: DID Exchange Protocol 1.0 for example, there is no error/none state.

It can be convenient to keep the state as it is when an error happens and preserve the information about the error somewhere else (let's say non-empty errorMessage attribute). A developer can see there is an error and in what state the exchange ended.

[TimoGlastra]

I can't find the connection problem report handler.

[TimoGlastra]

.

[nbAmit]

Thanks @TimoGlastra , I will check and update the code base on your suggestions and also check the e2e testing part as well.

[TimoGlastra]

Great improvements @nbAmit!

I have a few additional comments, but once those are resolved this is good to go I think

[TimoGlastra]

If we check if the error is an instance of ProblemReportError we can generically handle it for all errors that extend from the ProblemReportError. this way we don't have to check for specific message type in the dispatcher

Suggested change

	const problemReportMessage = error.problemReport
	if (
	(problemReportMessage instanceof ConnectionProblemReportMessage ||
	problemReportMessage instanceof CredentialProblemReportMessage ||
	problemReportMessage instanceof PresentationProblemReportMessage) &&
	messageContext.connection
	) {
	if (problemReportMessage instanceof ProblemReportError && messageContext.connection) {
	const problemReportMessage = error.problemReport

[TimoGlastra]

Suggested change

	this.validateMessage(message)
	await this.validateMessage(message)

[TimoGlastra]

The validate method also logs an error, I think logging it once is good enough

[TimoGlastra]

The error messages is not updated I think?

[TimoGlastra]

If connection is provided (which is always the case in this method) this will never be true, as it used for connectionless exchanges.

you can call messageSender.sendMessage directly

[TimoGlastra]

I think we should create a generic ProblemReportError where the ConnectionProblemReportError can extend from. This way we can check just for instanceof ProblemReportError in the dispatcher. As the ConectionProblemReportMessage extends the ProblemReportMessage it is also possible to use the connection specific problem report message for the problemReport property

[TimoGlastra]

There is a good chance the connection won't be availalbe on the messageContext yet as the connection is not complete yet. If you look at the other methods in the conenction service you can see they often retrieve it from the repository. This is only the case in the connection implementation as before the protocol is finished there is no complete connection yet.

[TimoGlastra]

I think we should add an check here whether the received message is of @type == https://didcomm.org/notification/1.0/problem-reportand otherwise throw an error (not a problem report error) that we are not sending a problem report as response to a problem. to prevent accidential problem report loops from happening. We should probably never respond with a problem report to a problem report.

[TimoGlastra]

Same, should extend ProblemReportError

[TimoGlastra]

I think description is not optional

[TimoGlastra]

Great work @nbAmit! One comment left

[TimoGlastra]

This doesn't extend problem report error yet

[TimoGlastra]

LGTM

[jakubkoci]

Good job. Just added a few notes. Nothing serious.

[jakubkoci]

Isn't it too low-level area to care about a connection here? Throwing Signature is not valid! and handling it at the level above seems reasonable to me.

[nbAmit]

    let connectionJson = null
    try {
      connectionJson = await unpackAndVerifySignatureDecorator(message.connectionSig, this.wallet)
    } catch (error) {
      if(error instanceof AriesFrameworkError) {
        throw new ConnectionProblemReportError(error.message, {
          problemCode: ConnectionProblemReportReason.RequestProcessingError,
        })
      }
    }

Is it the right way to handle it in connection service? or any other suggestion @jakubkoci @TimoGlastra

[jakubkoci]

Hmm, that's a good point. It's a question if we want to update the state at all? If I look at list of states at Aries RFC 0023: DID Exchange Protocol 1.0 for example, there is no error/none state.

It can be convenient to keep the state as it is when an error happens and preserve the information about the error somewhere else (let's say non-empty errorMessage attribute). A developer can see there is an error and in what state the exchange ended.

[TimoGlastra]

Sorry @jakubkoci, just merged.....

[TimoGlastra]

@nbAmit could you take a look at Jakubs comments and address those in a follow up PR?

[TimoGlastra]

Also I think we should only allow problem reports to be received for exchanges with connections for now, as with the processProblemReport method now in every service, they are not tied to a specific connection and we also don't check whether this is a valid connectionless exchange (hard to do right now). This is quite a large security flaw, as everyone who nows a thread id can now send a problem report message to it.

Could you add a messageContext.assertReadyConnection to the credential and proof problem report message handlers? For the connection handler we will need to add checks based on where in the protocol we are

[TimoGlastra]

@nbAmit are you going to pick these up? The comments from @jakubkoci have some breaking changes to records so we should try to fix those ASAP before people update to this version. If you don't have the time let me know and I'll pick it up.

[nbAmit]

Hi @TimoGlastra This week I am on vacation so is it okay if I start working on Monday?