Skip to content
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

refactor: resolve feedback for problem report #584

Merged
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 2 additions & 6 deletions packages/core/src/agent/MessageReceiver.ts
Original file line number Diff line number Diff line change
Expand Up @@ -9,12 +9,11 @@ import { Lifecycle, scoped } from 'tsyringe'

import { AriesFrameworkError } from '../error'
import { ConnectionService } from '../modules/connections/services/ConnectionService'
import { ProblemReportError, ProblemReportMessage } from '../modules/problem-reports'
import { ProblemReportError, ProblemReportMessage, ProblemReportReason } from '../modules/problem-reports'
import { JsonTransformer } from '../utils/JsonTransformer'
import { MessageValidator } from '../utils/MessageValidator'
import { replaceLegacyDidSovPrefixOnMessage } from '../utils/messageType'

import { CommonMessageType } from './../modules/common/messages/CommonMessageType'
import { AgentConfig } from './AgentConfig'
import { Dispatcher } from './Dispatcher'
import { EnvelopeService } from './EnvelopeService'
Expand All @@ -23,9 +22,6 @@ import { TransportService } from './TransportService'
import { createOutboundMessage } from './helpers'
import { InboundMessageContext } from './models/InboundMessageContext'

export enum ProblemReportReason {
MessageParseFailure = 'message-parse-failure',
}
@scoped(Lifecycle.ContainerScoped)
export class MessageReceiver {
private config: AgentConfig
Expand Down Expand Up @@ -226,7 +222,7 @@ export class MessageReceiver {
connection: ConnectionRecord,
plaintextMessage: PlaintextMessage
) {
if (plaintextMessage['@type'] === CommonMessageType.ProblemReport) {
if (plaintextMessage['@type'] === ProblemReportMessage.type) {
throw new AriesFrameworkError(message)
}
const problemReportMessage = new ProblemReportMessage({
Expand Down
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
import type { Wallet } from '../../wallet/Wallet'

import { ConnectionProblemReportError, ConnectionProblemReportReason } from '../../modules/connections/errors'
import { AriesFrameworkError } from '../../error'
import { BufferEncoder } from '../../utils/BufferEncoder'
import { JsonEncoder } from '../../utils/JsonEncoder'
import { Buffer } from '../../utils/buffer'
Expand Down Expand Up @@ -29,9 +29,7 @@ export async function unpackAndVerifySignatureDecorator(
const isValid = await wallet.verify(signerVerkey, signedData, signature)

if (!isValid) {
throw new ConnectionProblemReportError('Signature is not valid', {
problemCode: ConnectionProblemReportReason.RequestProcessingError,
})
throw new AriesFrameworkError('Signature is not valid')
}

// TODO: return Connection instance instead of raw json
Expand Down
Original file line number Diff line number Diff line change
@@ -1,4 +1,3 @@
export enum CommonMessageType {
Ack = 'https://didcomm.org/notification/1.0/ack',
ProblemReport = 'https://didcomm.org/notification/1.0/problem-report',
}
Original file line number Diff line number Diff line change
Expand Up @@ -10,5 +10,4 @@ export enum ConnectionState {
Requested = 'requested',
Responded = 'responded',
Complete = 'complete',
None = 'none',
}
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ export interface ConnectionRecordProps {
imageUrl?: string
multiUseInvitation: boolean
mediatorId?: string
errorMsg?: string
errorMessage?: string
}

export type CustomConnectionTags = TagsBase
Expand Down Expand Up @@ -69,7 +69,7 @@ export class ConnectionRecord

public threadId?: string
public mediatorId?: string
public errorMsg?: string
public errorMessage?: string

public static readonly type = 'ConnectionRecord'
public readonly type = ConnectionRecord.type
Expand All @@ -96,7 +96,7 @@ export class ConnectionRecord
this.imageUrl = props.imageUrl
this.multiUseInvitation = props.multiUseInvitation
this.mediatorId = props.mediatorId
this.errorMsg = props.errorMsg
this.errorMessage = props.errorMessage
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -323,7 +323,16 @@ export class ConnectionService {
connectionRecord.assertState(ConnectionState.Requested)
connectionRecord.assertRole(ConnectionRole.Invitee)

const connectionJson = await unpackAndVerifySignatureDecorator(message.connectionSig, this.wallet)
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,
})
}
}

const connection = JsonTransformer.fromJSON(connectionJson, Connection)
await MessageValidator.validate(connection)
Expand Down Expand Up @@ -433,8 +442,8 @@ export class ConnectionService {
)
}

connectionRecord.errorMsg = `${connectionProblemReportMessage.description.code} : ${connectionProblemReportMessage.description.en}`
await this.updateState(connectionRecord, ConnectionState.None)
connectionRecord.errorMessage = `${connectionProblemReportMessage.description.code} : ${connectionProblemReportMessage.description.en}`
await this.updateState(connectionRecord, connectionRecord.state)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The state won't change, so I think we should just call the connection repository with update directly as this will emit a state changed event.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, with the None state being removed, there is no way to determine that you received a problem report, so we should probably look into adding some events related to problem reports in follow up PRs

return connectionRecord
}

Expand Down
1 change: 0 additions & 1 deletion packages/core/src/modules/credentials/CredentialState.ts
Original file line number Diff line number Diff line change
Expand Up @@ -14,5 +14,4 @@ export enum CredentialState {
CredentialIssued = 'credential-issued',
CredentialReceived = 'credential-received',
Done = 'done',
None = 'none',
}
Original file line number Diff line number Diff line change
Expand Up @@ -981,7 +981,7 @@ describe('CredentialService', () => {
})
})

test(`updates state to ${CredentialState.None} and returns credential record`, async () => {
test(`updates problem report error message and returns credential record`, async () => {
const repositoryUpdateSpy = jest.spyOn(credentialRepository, 'update')

// given
Expand All @@ -992,7 +992,7 @@ describe('CredentialService', () => {

// then
const expectedCredentialRecord = {
state: CredentialState.None,
errorMessage: 'issuance-abandoned: Indy error',
}
expect(credentialRepository.getSingleByQuery).toHaveBeenNthCalledWith(1, {
threadId: 'somethreadid',
Expand All @@ -1003,28 +1003,6 @@ describe('CredentialService', () => {
expect(updatedCredentialRecord).toMatchObject(expectedCredentialRecord)
expect(returnedCredentialRecord).toMatchObject(expectedCredentialRecord)
})

test(`emits stateChange event from ${CredentialState.OfferReceived} to ${CredentialState.None}`, async () => {
const eventListenerMock = jest.fn()
eventEmitter.on<CredentialStateChangedEvent>(CredentialEventTypes.CredentialStateChanged, eventListenerMock)

// given
mockFunction(credentialRepository.getSingleByQuery).mockReturnValue(Promise.resolve(credential))

// when
await credentialService.processProblemReport(messageContext)

// then
expect(eventListenerMock).toHaveBeenCalledWith({
type: 'CredentialStateChanged',
payload: {
previousState: CredentialState.OfferReceived,
credentialRecord: expect.objectContaining({
state: CredentialState.None,
}),
},
})
})
})

describe('repository methods', () => {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ export interface CredentialRecordProps {
credentialAttributes?: CredentialPreviewAttribute[]
autoAcceptCredential?: AutoAcceptCredential
linkedAttachments?: Attachment[]
errorMsg?: string
errorMessage?: string
}

export type CustomCredentialTags = TagsBase
Expand All @@ -50,7 +50,7 @@ export class CredentialRecord extends BaseRecord<DefaultCredentialTags, CustomCr
public credentialId?: string
public state!: CredentialState
public autoAcceptCredential?: AutoAcceptCredential
public errorMsg?: string
public errorMessage?: string

// message data
@Type(() => ProposeCredentialMessage)
Expand Down Expand Up @@ -90,7 +90,7 @@ export class CredentialRecord extends BaseRecord<DefaultCredentialTags, CustomCr
this.credentialAttributes = props.credentialAttributes
this.autoAcceptCredential = props.autoAcceptCredential
this.linkedAttachments = props.linkedAttachments
this.errorMsg = props.errorMsg
this.errorMessage = props.errorMessage
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -733,7 +733,9 @@ export class CredentialService {
public async processProblemReport(
messageContext: InboundMessageContext<CredentialProblemReportMessage>
): Promise<CredentialRecord> {
const { message: credentialProblemReportMessage, connection } = messageContext
const { message: credentialProblemReportMessage } = messageContext

const connection = messageContext.assertReadyConnection()

this.logger.debug(`Processing problem report with id ${credentialProblemReportMessage.id}`)

Expand All @@ -743,8 +745,8 @@ export class CredentialService {
)

// Update record
credentialRecord.errorMsg = `${credentialProblemReportMessage.description.code}: ${credentialProblemReportMessage.description.en}`
await this.updateState(credentialRecord, CredentialState.None)
credentialRecord.errorMessage = `${credentialProblemReportMessage.description.code}: ${credentialProblemReportMessage.description.en}`
await this.updateState(credentialRecord, credentialRecord.state)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @TimoGlastra, If we update the updateState with an update, it will be difficult to capture the event in the e2e test case?

https://github.com/hyperledger/aries-framework-javascript/blob/1041aa77bdc1c441d99e7852d4062766520d6597/packages/core/tests/credentials.test.ts#L483

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, see my comment above:

Also, with the None state being removed, there is no way to determine that you received a problem report, so we should probably look into adding some events related to problem reports in follow up PRs

We should probably add a new event (e.g. ConnectionProblemReportReceived, CredentialProblemReportReceived etc...) where the event looks something like:

{
   payload: {
     credentialRecord: { /* the record */ },
     problemReportMessage: { /* the problem report message */ }
}
}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can either add it to the current PR, or you can bypass it by listening for the AgentMessageProcessedEvent for now and filtering on the problem report @type

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@TimoGlastra , As of now I have removed the test case and I will add it into upcoming PR with the problem-report events

return credentialRecord
}

Expand Down
1 change: 1 addition & 0 deletions packages/core/src/modules/problem-reports/index.ts
Original file line number Diff line number Diff line change
@@ -1,2 +1,3 @@
export * from './errors'
export * from './messages'
export * from './models'
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,6 @@ import { Expose } from 'class-transformer'
import { Equals, IsEnum, IsOptional, IsString } from 'class-validator'

import { AgentMessage } from '../../../agent/AgentMessage'
import { CommonMessageType } from '../../common/messages/CommonMessageType'

export enum WhoRetriesStatus {
You = 'YOU',
Expand Down Expand Up @@ -80,7 +79,7 @@ export class ProblemReportMessage extends AgentMessage {

@Equals(ProblemReportMessage.type)
public readonly type: string = ProblemReportMessage.type
public static readonly type: string = CommonMessageType.ProblemReport
public static readonly type: string = 'https://didcomm.org/notification/1.0/problem-report'

public description!: DescriptionOptions

Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
export enum ProblemReportReason {
MessageParseFailure = 'message-parse-failure',
}
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
export * from './ProblemReportReason'
1 change: 0 additions & 1 deletion packages/core/src/modules/proofs/ProofState.ts
Original file line number Diff line number Diff line change
Expand Up @@ -12,5 +12,4 @@ export enum ProofState {
PresentationReceived = 'presentation-received',
Declined = 'declined',
Done = 'done',
None = 'none',
}
2 changes: 1 addition & 1 deletion packages/core/src/modules/proofs/ProofsModule.ts
Original file line number Diff line number Diff line change
Expand Up @@ -386,7 +386,7 @@ export class ProofsModule {
const presentationProblemReportMessage = new PresentationProblemReportMessage({
description: {
en: message,
code: PresentationProblemReportReason.abandoned,
code: PresentationProblemReportReason.Abandoned,
},
})
presentationProblemReportMessage.setThread({
Expand Down
30 changes: 4 additions & 26 deletions packages/core/src/modules/proofs/__tests__/ProofService.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -196,7 +196,7 @@ describe('ProofService', () => {
const presentationProblemReportMessage = await new PresentationProblemReportMessage({
description: {
en: 'Indy error',
code: PresentationProblemReportReason.abandoned,
code: PresentationProblemReportReason.Abandoned,
},
})

Expand Down Expand Up @@ -224,7 +224,7 @@ describe('ProofService', () => {
const presentationProblemReportMessage = new PresentationProblemReportMessage({
description: {
en: 'Indy error',
code: PresentationProblemReportReason.abandoned,
code: PresentationProblemReportReason.Abandoned,
},
})
presentationProblemReportMessage.setThread({ threadId: 'somethreadid' })
Expand All @@ -233,7 +233,7 @@ describe('ProofService', () => {
})
})

test(`updates state to ${ProofState.None} and returns proof record`, async () => {
test(`updates problem report error message and returns proof record`, async () => {
const repositoryUpdateSpy = jest.spyOn(proofRepository, 'update')

// given
Expand All @@ -244,7 +244,7 @@ describe('ProofService', () => {

// then
const expectedCredentialRecord = {
state: ProofState.None,
errorMessage: 'abandoned: Indy error',
}
expect(proofRepository.getSingleByQuery).toHaveBeenNthCalledWith(1, {
threadId: 'somethreadid',
Expand All @@ -255,27 +255,5 @@ describe('ProofService', () => {
expect(updatedCredentialRecord).toMatchObject(expectedCredentialRecord)
expect(returnedCredentialRecord).toMatchObject(expectedCredentialRecord)
})

test(`emits stateChange event from ${ProofState.RequestReceived} to ${ProofState.None}`, async () => {
const eventListenerMock = jest.fn()
eventEmitter.on<ProofStateChangedEvent>(ProofEventTypes.ProofStateChanged, eventListenerMock)

// given
mockFunction(proofRepository.getSingleByQuery).mockReturnValue(Promise.resolve(proof))

// when
await proofService.processProblemReport(messageContext)

// then
expect(eventListenerMock).toHaveBeenCalledWith({
type: 'ProofStateChanged',
payload: {
previousState: ProofState.RequestReceived,
proofRecord: expect.objectContaining({
state: ProofState.None,
}),
},
})
})
})
})
Original file line number Diff line number Diff line change
Expand Up @@ -4,5 +4,5 @@
* @see https://github.com/hyperledger/aries-rfcs/blob/main/features/0037-present-proof/README.md
*/
export enum PresentationProblemReportReason {
abandoned = 'abandoned',
Abandoned = 'abandoned',
}
6 changes: 3 additions & 3 deletions packages/core/src/modules/proofs/repository/ProofRecord.ts
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ export interface ProofRecordProps {
presentationId?: string
tags?: CustomProofTags
autoAcceptProof?: AutoAcceptProof
errorMsg?: string
errorMessage?: string

// message data
proposalMessage?: ProposePresentationMessage
Expand All @@ -42,7 +42,7 @@ export class ProofRecord extends BaseRecord<DefaultProofTags, CustomProofTags> {
public presentationId?: string
public state!: ProofState
public autoAcceptProof?: AutoAcceptProof
public errorMsg?: string
public errorMessage?: string

// message data
@Type(() => ProposePresentationMessage)
Expand Down Expand Up @@ -71,7 +71,7 @@ export class ProofRecord extends BaseRecord<DefaultProofTags, CustomProofTags> {
this.presentationId = props.presentationId
this.autoAcceptProof = props.autoAcceptProof
this._tags = props.tags ?? {}
this.errorMsg = props.errorMsg
this.errorMessage = props.errorMessage
}
}

Expand Down
Loading