openwallet-foundation / credo-ts

Typescript framework for building decentralized identity and verifiable credential solutions
https://credo.js.org
Apache License 2.0
258 stars 196 forks source link

Thread id when Issue Credential protocol is started by Offer Credential #588

Closed genaris closed 1 year ago

genaris commented 2 years ago

I've seen that when processing Offer Credential messages, CredentialService creates a CredentialRecord whose threadId is set to Offer Credential message's id. This is usually fine because the flow starts with that message, so threadId = id. However, that's not true when Issue Credential is started as a sub-protocol and therefore all messages include the ~thread decorator.

So I think all Credential Records should be created with originating message's threadId. That is actually the case when creating a CredentialRecord when processing Credential Proposal message.

TimoGlastra commented 2 years ago

If I understand correctly, when starting issue credential as a sub-protocol, the threadId of the super protocol should be set as the phtid. The thid is reserved for the issue credential flow, which if starting with the offer as a sub-protocol, is the id of the offer message.

Do you have a different interpretation of this functionality? If so, could you point me to some of the docs?

genaris commented 2 years ago

I agree with you about the usage of pthid for sub-protocols. Maybe my issue was a bit 'nitpicking' because in other cases, such as createProposal method, we are indeed setting threadId to proposalMessage.threadId. Isn't conceptually the same case? In ACA-PY I see something similar.

I actually raised this because it would make easier for me (with some hacking around) to relate an Issue Credential flow with another before we fully support sub-protocols, which I think it would need some more effort. But anyway, probably it's worth to tackle it as it deserves and think the changes needed to start protocols with a parent thread and being able to filter out events by their parent thread as well.

andrewwhitehead commented 1 year ago

I believe AFJ should always use the thid. DIDComm specifies that thid = @id when not provided.

The flow where I ran into this:

TimoGlastra commented 1 year ago

You were right all along @genaris, I couldn't see it at the moment. Should be fixed in #1311

Found the issue here: https://github.com/hyperledger/aries-framework-javascript/pull/1311/files#r1110228962