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

Handle messages with an empty ~thread decorator #875

Open
gmulhearn-anonyome opened this issue Jun 8, 2023 · 6 comments
Open

Handle messages with an empty ~thread decorator #875

gmulhearn-anonyome opened this issue Jun 8, 2023 · 6 comments

Comments

@gmulhearn-anonyome
Copy link
Contributor

I've found testing against ACA-py 0.8.1 that some ACA-py messages (particularly a new cred offer) will include a ~thread decorator with no content: "~thread": {}:

{
    "@type": "did:sov:BzCbsNYhMrjHiqZDTUASHg;spec/issue-credential/1.0/offer-credential",
    "@id": "3fb15d06-28ff-438d-b6d9-f8771f08e844",
    "~thread": {},
    "offers~attach": [
      {
        "@id": "libindy-cred-offer-0",
        "mime-type": "application/json",
...

Aries-vcx message create will fail to deserialize this message, as if OfferCredentialDecorators has a thread, then it expects there to be a thid.

So I'm wondering; is this an ACA-py bug, or an aries_vcx bug?... The language of the threading RFC makes it difficult to know whether an ~thread with nothing inside it is "valid" (despite being meaningless/useless); https://github.com/hyperledger/aries-rfcs/tree/main/concepts/0008-message-id-and-threading.

And further question; if it is an ACA-py bug, should aries_vcx build some tolerance for the bug anyway?

@swcurran
Copy link
Member

swcurran commented Jun 8, 2023

FYI @usingtechnology @Jsyro @andrewwhitehead @TelegramSam @dbluhm

Given the RFCs, is this a bug in ACA-Py? It should be easy to fix — adding a thid and setting it to the same value as @id, right?

@usingtechnology
Copy link

The RFC is ambiguous:

If the Thread object is defined and a thid is given, the Thread ID is the value given there. But if the Thread object is not defined in a message, the Thread ID is implicitly defined as the Message ID (@id) of the given message and that message is the first message of a new thread.

So we have the case where Thread is defined with and explicit not for what to do when thid is provided which would imply that there is another case where thid is not provided... But that is different than what to do if Thread is not provided. So... does that mean a Thread with no thid is treated the same as no Thread: @id is the implicit Thread ID?

If I look at aries-protocol-test-suite, it assumes that thid is @id and deals accordingly. It does not assume that ~thread has a thid. Reading that would mean an thread object without a thid is fine, the controller uses @id.

We can certainly populate thread.thid with @id but no guarantee other ARIES implementations will do the same. Probably a safer fix would be if thread is empty, then do not decorate.

@swcurran
Copy link
Member

swcurran commented Jun 8, 2023

Thanks, @usingtechnology — let’s get an issue into ACA-Py to do as you say and remove the empty ~thid. That said, being defensive, Aries might want to accept that an empty thid could happen.

We can do a quick PR to aries-rfc as a clarification — don’t put it in, but understand that implementations might.

@usingtechnology
Copy link

Added issue 2259 to ACA-Py.

We will not send an empty ~thread object. However, still best if aries-vcx acts defensively and handles missing thid.

@usingtechnology
Copy link

Just a follow up PR 2261 is ready for review. The PR sets the thid value for credential offer. It appears this was always intended but set with an uninitialized value.

Looking over the existing ACA-Py code, it is very defensive and always checks for a ~thread and a thid - expects that a thread may not have a thid - so highly recommended that aries-vcx do the same.

@swcurran
Copy link
Member

Just a follow up: ACA-Py PR 2261 is merged, on the main branch and will be in Release 0.8.2.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants