feat: allow sending problem report when declining a proof request #1408
Conversation
Codecov Report
@@ Coverage Diff @@
## main #1408 +/- ##
==========================================
+ Coverage 85.34% 85.37% +0.02%
==========================================
Files 788 788
Lines 19435 19446 +11
Branches 3159 3163 +4
==========================================
+ Hits 16587 16602 +15
+ Misses 2841 2837 -4
Partials 7 7
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
Signed-off-by: Artemkaaas <[email protected]>
Artemkaaas
force-pushed
the
feature/decline-connectionless-proof-request
branch
from
5d0ae3a to
4b973ed
Compare
| @@ -368,13 +368,17 @@ export class ProofsApi<PPs extends ProofProtocol[]> implements ProofsApi<PPs> { | |||
| } | |||
| } | |||
|
|
|||
| public async declineRequest(proofRecordId: string): Promise<ProofExchangeRecord> { | |||
| public async declineRequest(proofRecordId: string, sendProblemReport?: boolean): Promise<ProofExchangeRecord> { | |||
There was a problem hiding this comment.
In general we tend to create objects for functionality like named parameters. Here for example, calling declineProofRequest('abc-id', true) would be quite vague.
If we include this into 0.4.0 we can make the breaking change and the function would accept 1 parameter DeclineRequestOptions which is {proofRecordId: string, sendProblemRequest?: boolean}
There was a problem hiding this comment.
I agree with you that accepting single parameter is better but I afraid to make breaking change.
There was a problem hiding this comment.
We're just about to release 0.4.0 with a lot of breaking changes to the proofs api. Now is the time!
There was a problem hiding this comment.
I updated the method parameters to accept single DeclineProofRequestOptions object
| // Assert | ||
| connectionRecord.assertReady() | ||
|
|
||
| const outboundMessageContext = new OutboundMessageContext(problemReport, { |
There was a problem hiding this comment.
@TimoGlastra is this the correct place to do this logic? If possbile I think it would be good to defer this to another method (creating the service decorator and adding it as serviceParams).
There was a problem hiding this comment.
The logical changes for send Problem Report function (handle connectionless request) repeats logic from accept Request method.
There was a problem hiding this comment.
We need to refactor conecctionless in general. Once we integrate it with oob we can generalize the implementation and get rid of repeating the same code everywhere. For now I think it's fine to do it like this
| serviceParams: { | ||
| service: recipientService.resolvedDidCommService, | ||
| senderKey: ourService.resolvedDidCommService.recipientKeys[0], | ||
| returnRoute: options.useReturnRoute ?? true, // defaults to true if missing |
There was a problem hiding this comment.
I think it would be good to define the default value either inside the function signature or at the first line. This is a bit hidden.
There was a problem hiding this comment.
Nit sure if we need it. We don't expect a responsw to the problem report right? I'd just set it to false / leave it out
There was a problem hiding this comment.
Removed setting of returnRoute and useReturnRoute field in the parameters
Signed-off-by: Artemkaaas <[email protected]>
Artemkaaas
force-pushed
the
feature/decline-connectionless-proof-request
branch
from
d57f2da to
1546215
Compare
|
|
||
| await this.messageSender.sendMessage(outboundMessageContext) | ||
| return proofRecord | ||
| } else if (requestMessage?.service) { |
There was a problem hiding this comment.
What if we want to send a problem report as the verifier? We'd send the message to ourselves in that case (as we're the ones that created the request). I think we shouldn't assume the role here, and instead infer it based on the state
There was a problem hiding this comment.
Added check that state is ProofState.RequestReceived but I had to move sending of the problem report before updating the state to Declined
Signed-off-by: Artemkaaas <[email protected]>
Signed-off-by: Artemkaaas <[email protected]>
TimoGlastra
changed the title
Updated
declineRequestmethod to acces optionalsendProblemReportparameter indicating that Problem Report message also should be sent to Verifier.Also updated
sendProblemReportmethod to support connectionless proof request case.