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

Add clarification to thread RFC about handling an empty thread decorator #789

Merged
merged 1 commit into from
Jun 28, 2023
Merged

Conversation

swcurran
Copy link
Member

Signed-off-by: Stephen Curran [email protected]

See issue #875 in Aries VCX. This PR clarifies that while it is not recommended, an Aries agent MAY put an empty ~thread: {} item in the first message of a protocol instance, and a recipient Aries agent MUST be able to handle that.

@usingtechnology
Copy link

That should help clear up the ambiguity.
The code in aries-cloudagent-python is very defensive and assumes ~thread may not exist and ~thread{} may exist without a thid. All applications should be coded with the same defensiveness.

Copy link

@usingtechnology usingtechnology left a comment

Choose a reason for hiding this comment

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

LGTM.

Copy link

@gmulhearn-anonyome gmulhearn-anonyome left a comment

Choose a reason for hiding this comment

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

LGTM. But curious; why go this route rather than a route of "~thread MUST have a thid"?

@swcurran
Copy link
Member Author

My thought is that making the thid required is a change in the protocol (and hence a minor version change), and simply being defensive in the implementation is sufficient. And, even if we make the change now, some implementations may not do it, and so it is best to be defensive anyway.

@gmulhearn-anonyome
Copy link

thanks, sounds good

@usingtechnology
Copy link

Not that ACA-Py's implementation should dictate the RFC, but whoever implemented it in ACA-Py understood it as expecting to encounter a thread without a thid. And unfortunately, in ACA-Py the thread logic is not 100% isolated. Ensuring that all threads had a thid with a value would be a fairly significant exercise, certainly at the regression testing level.

@TimoGlastra
Copy link
Member

I think it's valid to have a pthid, without a thid, so making this required would also be problematic in such cases

@TimoGlastra
Copy link
Member

AFJ has same behaviour, that is allows an empty object

@swcurran swcurran merged commit ac55e23 into hyperledger:main Jun 28, 2023
@swcurran
Copy link
Member Author

Discussed on the Aries Working Group call on 2023.06.28 and there was agreement to merge.

@swcurran swcurran deleted the emptythid branch March 13, 2024 15:13
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

Successfully merging this pull request may close these issues.

4 participants