Misc Connection Protocol Improvements (Message IDs, Thread IDs, response building)

[gmulhearn]

There are a few inconsistencies in the Connection protocol impl that I've noticed and wanted to call out for some discussion.

Apologies for stuffing them all into 1 issue, but they are all mostly related. Let me know what you think!

1. Message IDs

1. When the invitee creates the Request message, a unique @id for the message is not set, which results in the default of MessageId being used - which is "testid". Ideally this should be set as a unique uuid, similar to how the inviter does when creating the invitation.
2. Similar, when the inviter creates the Response message an @id is not set. resulting in the same default id value.

2. Thread IDs

Current Implementation in VCX:

Currently, when an Invitee is in the Invited state (i.e. ready to send a request out), they perceive their "thread_id" as the ID of the invitation they processed:

impl ThreadId for Invited {
    fn thread_id(&self) -> &str {
        self.invitation.get_id()
    }
}

Then, when the Invitee calls the send_request, the following occurs:

1. if pub invite or OOB: the request pthid is set to thread_id() (i.e. the oob invite ID or the pub invite ID) - which is good IMO
2. if pub invite or OOB: the request thid is set to match the ID of the request msg itself - which is good IMO, but not so good considering the default ID of "testid".
3. if pairwise invite: the request thid is set to thread_id() (i.e. the pairwise invite ID).

What the RFCs say:

In general, if the conn protocol or did exchange was initiated from an OOB invitation, the parent thread ID of the request message should be set as the @id of that OOB invitation.

In general, for both Connection Protocol & DID Exchange via OOB, it is the Invitee who is in control of the Thread ID. And they are in control of that thread ID when they send their request message. i.e. from DID Exchange:

The ~thread decorator MUST be included:

• It MUST include the ID of the parent thread (pthid) such that the request can be correlated to the corresponding (implicit or explicit) invitation. More on correlation below.
• It MAY include the thid property. This works according to the thid property in the thread decorator, meaning that if thid is not defined it is implicitly defined as the @id on the same request message.

In both connection protocol & did exchange, the protocol really begins with the request message, and so it's the requester that defines the thread ID for the protocol - nothing to do with the invitation ID.

Further, in both protocols, the requester does not even need to set the thread ID explicitly, if the invitee does not set the thid in their request msg, then the protocol thread id is implicitly the @id of the request message. In fact connection protocol 0160 does not even mention the option of the request message containing thread information, it even states that the response messages thid should be set to the request messages ID:

The ~thread block contains a thid reference to the @id of the request message.

Thread ID suggestions:

I'd propose we update the behavior in the following ways:

Invitee

• In the Invited state, there should not be a thread_id() method, as the thread_id is not set until the request message is sent
• send_request for Public & OOB should set the pthid to the invitation ID (already does this.. but it does this via the thread_id() method). And it should either not set the thid at all, or it should set it to match the request messages ID (already does this.. but due to the issue with IDs above, this results in it being set as "testid").
• send_request for pairwise invitation (i.e. 0160 invites) should not set the pthid (already does this) and it should either not set the thid at all, or it should set it to match the request messages ID.
• When transitioning to Requested, the states thread_id should be set as the request messages ID, as this is now the expected thid for the protocol.

Inviter

• In the Invited state, there should not be a thread_id() method, as the thread_id is not set until the request message has been received. Instead perhaps a parent_thread_id() would be better fit?
• The first thing handle_request does is verify_thread_id, which I believe asserts that the request message's thread_id OR parent_thread_id is equal to the inviter's invitation ID.. If the invitation was OOB, it makes sense to check the pthid, however as mentioned above, in pure 0160 conn protocol, there is not a requirement for the request message to set these fields. I'd suggest that this verification step should be replaced with a method like verify_parent_thread_id if the invitation was OOB/public?, and no verification should be done if the invitation was pairwise (pure 0160 protocol).
• The response should be build with the thid set to the request messages thid or the request @id if thid is not present (which it already does I believe)
• I'm unsure if acknowledge_connection needs to verify the thread_id at all. I believe in conn proto 0160, any message along that connection is enough for it to be validated..

3. Inviter building response messages early

In the invitee flow, the request message is not built until send_request is called.

Unlike this, in the inviter flow, the response message is built early when handle_request is called, and not when send_response is called.

IMO it would be more logical if send_response is responsible for building the response message. - there is also a docco in the code stating this:

    // This should ideally belong in the Connection<Inviter, RequestedState>
    // but was placed here to retro-fit the previous API.

[gmulhearn]

P.S. i'm happy to take on these issues in a single PR or in separate PRs (if we agree they are issues). would be good to fix while the new connection handler/impl is somewhat 'fresh'

[gmulhearn]

On My suggestion for request messages to not set the thread ID at all;

I say this because connection protocol does not mention that the request message should/can set the thread id explicitly. The conn protocol even says that the response message's thread ID should be set to the ID of the request.

We know that from general didcomm rules, the request IS able to add a thread id decorator which should then be used instead of the request ID. However it's possible that other agents do not know this rule, and their response message will just always use the ID of the request as the thread ID.

So IMO it would be playing it safer if the request message just doesn't explicitly set the thread ID decorator. And instead it is implicitly set as the request message ID.

[bobozaur]

@gmulhearn The threading issue goes beyond just the connection. FYI, I actually got to the point where I'm addressing this in #754 . The goal is to implement message builders that would have consistent behavior. This also implies decorator builders.

The idea is that we should provide a consistent and correct way to reply to a message or branch in a subthread. For us and for our users as well. Having to manually set some fields to some practically arbitrary values is error prone and cumbersome.

I'm still thinking about a consistent way of handling this since we have to accommodate implicit threads and stuff like that, but I'll update the PR when I figure something out. Feel free to also experiment in the meantime and maybe we can all discuss our findings/approaches.