decentralized-identity / didcomm.org

https://didcomm.org/
Apache License 2.0
36 stars 25 forks source link

[fix] clarify pickup protocol live mode delivery and acknowledge messages #105

Closed JamesKEbert closed 4 months ago

JamesKEbert commented 4 months ago

As discussed initially at the DIDComm UG 20240408.

This PR makes clarifications to the Message Pickup v3 protocol, clarifying that when in live mode, delivery and acknowledge messages are still required. This requirement is critical to prevent messages silently never being delivered. If for instance, the mediator thinks the message has been sent over a websocket, but the recipient is not listening to that socket any more, the mediator will think the message was delivered successfully, when in fact it was not (without delivery & acknowledgement).

This is also illustrated and discussed here: https://github.com/hyperledger/aries-rfcs/issues/760

A similar clarification should be made to the pickup v2 protocol in the Aries RFCs repo, including potentially this clarification (not exactly relevant to v3, as the line in question was removed in v3).

cc: @TelegramSam @genaris

dbluhm commented 4 months ago

This is a good clarification. Removing ambiguity both from the specification of the protocol and from the outcome of a message delivery is the right way to go, IMO. However, I think this would be considered a bit more than just a clarification, at least in terms of version of the specification. Existing v3 implementations would not necessarily be compatible, though I think this may be somewhat asymmetric. It will impact mediators more than their clients. For instance, the DIDComm Demo (https://demo.didcomm.org) would behave "correctly" whether the message was delivered as a delivery message or the raw message. The mediator it talks to, on the other hand, must be updated to send the live message wrapped in a delivery message.

In other words, I think before this clarification, "reasonable implementers" (i.e. me and my team lol) are likely to have interpreted it such that messages would be delivered without being wrapped in a delivery message. It seems like a version change should be necessary because of this.

dbluhm commented 4 months ago

It is also a bit unfortunate that this would result in a roughly 33% message bloat on each message delivered live to the client.

JamesKEbert commented 4 months ago

In other words, I think before this clarification, "reasonable implementers" (i.e. me and my team lol) are likely to have interpreted it such that messages would be delivered without being wrapped in a delivery message. It seems like a version change should be necessary because of this.

I could see that being a fair position @dbluhm--one of my worries would be creating a v4 version that would support both DIDComm v1 & v2 (as pickup v2 also needs to be clarified for DIDComm v1), or the creation of a v4 and v5. It feels a little bit clunky/confusing. But perhaps I'm just missing a clean way to do a version change.

It is also a bit unfortunate that this would result in a roughly 33% message bloat on each message delivered live to the client.

This is true--we had this debate (with you too IIRC), and we eventually decided the explicit nature was worth the additional cost. This line was even mentioned in V2 of the protocol (not sure why this line was removed in V3 though): This method of delivery does incur an encoding cost, but is much simpler to implement and a more robust interaction.

JamesKEbert commented 4 months ago

I'm open to additional discussion on the delivery wrapping vs grabbing an identifier from the message to be delivered (via a hash or other method). Either way though, we need to be able to explicitly acknowledge delivery of messages.

JamesKEbert commented 4 months ago

Discussed on the Aries WG call 20240424 (which reformatted is a palindrome - 42424 :laughing:)

Our discussion opted to make a new major release (4.0) for DIDComm v2, and a minor release (2.1) for DIDComm v1. We could have just made the clarification, but it will make it much clearer who has updated to the correct flow with explicit version changes. I'll make the relevant PRs for that approach soon.

Additionally, we discussed briefly the 33% message bloat issue, and Sam mentioned that this should be removed/adjusted/considered at the DIDComm layer in the next DIDComm iteration.