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

connection_protocol should be set when only one handshake_protocol specified #2879

Closed
dbluhm opened this issue Apr 10, 2024 · 2 comments · Fixed by #2880
Closed

connection_protocol should be set when only one handshake_protocol specified #2879

dbluhm opened this issue Apr 10, 2024 · 2 comments · Fixed by #2880
Assignees

Comments

@dbluhm
Copy link
Contributor

dbluhm commented Apr 10, 2024

@nodlesh reported that there are some failing tests in AATH as a result of a change I made in #2748:

The connection_protocol property is omitted from the webhook topic connections during the invitation state. This is a breaking change for controllers that depended on that value being set at the invitation phase.

Background to the change

OOB permits specifying one or more handshake_protocols. If more than one protocol is specified, at the invitation phase, we don't know what connection protocol will be used. We have to wait until we receive a request from either the connections/1.0, didexchange/1.0, or didexchage/1.1 protocol to know for sure. Before #2748, we did not have didexchange/1.1 but we still had the two other options. Despite this uncertainty during the invitation phase, ACA-Py was emitting didexchange/1.0, even though the receiver of the invitation could use connections/1.0 instead.

I "fixed" the bug by omitting the connection_protocol from the connection record until the request phase since that's the point at which we would know for sure what the protocol used actually was.

Solution

The proposed solution is to try to not lie about the protocol but still include the connection_protocol at the invitation phase if we can, i.e. when there is only one handshake protocol selected. Assuming controllers are likely using OOB for DID Exchange only, the connection_protocol can be set to didexchange/1.0 at the invitation phase and they can continue on their merry way.

When multiple handshake_protocols are specified, however, I believe we should omit the value from the webhook rather than "lie" to the controller about which protocol the connection is using.

This would still constitute a breaking change but would hopefully cover the most common case of using OOB for DID Exchange only (or even OOB for connections only, though I find this an unlikely combination).

cc: @swcurran @ianco

@dbluhm dbluhm self-assigned this Apr 10, 2024
@dbluhm
Copy link
Contributor Author

dbluhm commented Apr 10, 2024

Side note that I did a quick scan through a "real world" controller and found that we don't depend on the value of connection_protocol until the connection has completed, just to offer a data point.

@swcurran
Copy link
Contributor

I think that sounds right. Thanks for the analysis. In the changeling, let’s add a reference to this issue as the background to the change and what to do if upgrading and adding multiple handshake_protocols at the same time.

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 a pull request may close this issue.

2 participants