-
Notifications
You must be signed in to change notification settings - Fork 204
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: allow sending problem report when declining a proof request #1408
Changes from 1 commit
4b973ed
1546215
01b5330
6adba72
7e4af38
582c64b
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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> { | ||
const proofRecord = await this.getById(proofRecordId) | ||
proofRecord.assertState(ProofState.RequestReceived) | ||
|
||
const protocol = this.getProtocol(proofRecord.protocolVersion) | ||
await protocol.updateState(this.agentContext, proofRecord, ProofState.Declined) | ||
|
||
if (sendProblemReport) { | ||
await this.sendProblemReport({ proofRecordId, description: 'Request declined' }) | ||
} | ||
|
||
return proofRecord | ||
} | ||
|
||
|
@@ -555,29 +559,59 @@ export class ProofsApi<PPs extends ProofProtocol[]> implements ProofsApi<PPs> { | |
*/ | ||
public async sendProblemReport(options: SendProofProblemReportOptions): Promise<ProofExchangeRecord> { | ||
const proofRecord = await this.getById(options.proofRecordId) | ||
if (!proofRecord.connectionId) { | ||
throw new AriesFrameworkError(`No connectionId found for proof record '${proofRecord.id}'.`) | ||
} | ||
|
||
const protocol = this.getProtocol(proofRecord.protocolVersion) | ||
const connectionRecord = await this.connectionService.getById(this.agentContext, proofRecord.connectionId) | ||
|
||
// Assert | ||
connectionRecord.assertReady() | ||
const requestMessage = await protocol.findRequestMessage(this.agentContext, proofRecord.id) | ||
|
||
const { message: problemReport } = await protocol.createProblemReport(this.agentContext, { | ||
proofRecord, | ||
description: options.description, | ||
}) | ||
|
||
const outboundMessageContext = new OutboundMessageContext(problemReport, { | ||
agentContext: this.agentContext, | ||
connection: connectionRecord, | ||
associatedRecord: proofRecord, | ||
}) | ||
if (proofRecord.connectionId) { | ||
const connectionRecord = await this.connectionService.getById(this.agentContext, proofRecord.connectionId) | ||
|
||
await this.messageSender.sendMessage(outboundMessageContext) | ||
return proofRecord | ||
// Assert | ||
connectionRecord.assertReady() | ||
|
||
const outboundMessageContext = new OutboundMessageContext(problemReport, { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The logical changes for There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 |
||
agentContext: this.agentContext, | ||
connection: connectionRecord, | ||
associatedRecord: proofRecord, | ||
}) | ||
|
||
await this.messageSender.sendMessage(outboundMessageContext) | ||
return proofRecord | ||
} else if (requestMessage?.service) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Added check that state is |
||
// Create ~service decorator | ||
const routing = await this.routingService.getRouting(this.agentContext) | ||
const ourService = new ServiceDecorator({ | ||
serviceEndpoint: routing.endpoints[0], | ||
recipientKeys: [routing.recipientKey.publicKeyBase58], | ||
routingKeys: routing.routingKeys.map((key) => key.publicKeyBase58), | ||
}) | ||
const recipientService = requestMessage.service | ||
|
||
await this.messageSender.sendMessageToService( | ||
new OutboundMessageContext(problemReport, { | ||
agentContext: this.agentContext, | ||
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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Removed setting of |
||
}, | ||
}) | ||
) | ||
|
||
return proofRecord | ||
} | ||
// Cannot send message without connectionId or ~service decorator | ||
else { | ||
throw new AriesFrameworkError( | ||
`Cannot send problem report without connectionId or ~service decorator on presentation request.` | ||
) | ||
} | ||
} | ||
|
||
public async getFormatData(proofRecordId: string): Promise<GetProofFormatDataReturn<ProofFormatsFromProtocols<PPs>>> { | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree with you that accepting single parameter is better but I afraid to make breaking change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I updated the method parameters to accept single
DeclineProofRequestOptions
object