fix: issue #2434: Change DIDExchange States to Match rfc160

[anwalker293]

Includes fixes to #2434. Changes all ConnRecord.States to match that of rfc160.

For additional information, here's the identical PR with some feedback from @dbluhm ~
Indicio-tech#151

[dbluhm]

@swcurran @usingtechnology @Jsyro Alex is on my team so, in the interest of transparency, a review from one of you guys would be appreciated. To be explicit, this technically constitutes a breaking change if your controller was dependent on the incorrect state showing up on the final webhook for DID exchange. If your implementation was using the rfc23_state, there should be no change.

[dbluhm]

Relevant comment from the original issue: #2434 (comment)

@anwalker293 spotted the issue; in the DID Exchange protocol manager, the conn_record.state is getting set to ConnRecord.State.COMPLETED.rfc23 which is equal to the string completed. This follows the pattern used elsewhere in the manager but since the states happen to align (i.e. request for rfc160 is also request for rfc23), it wasn't obvious that this was incorrect.

conn_record.state should ALWAYS be set to the string for rfc160, whether it's getting set in the connections or did exchange protocols. I think we probably should have handled the state management for these protocols differently but we're here so this is the right fix for now.

[swcurran]

An interesting change. Looking through “blame” — this goes back to the initial implementation of DID Exchange and then was updated later. I’m slightly concerned with the long history, but leave that to those that know more about the impact. The change itself seems right.

[usingtechnology]

can we confirm if there are updates needed for abandoned?

ConnRecord.abandoned

and

routes.connections_sort_key

[dbluhm]

@usingtechnology good question and good catch -- I believe the abandoned method ought to use the rfc160 value as well (@anwalker293, one more spot to change here). I believe the routes.connections_sort_key is fine though; it's turning the string into the Enum and then checks against that, which is a good methodology.

[anwalker293]

I think this is the only place to update! But please let me know if you spot any others! :)

[usingtechnology]

Great catch, diagnosis and work by all on this issue!

[usingtechnology]

I think this is the only place to update! But please let me know if you spot any others! :)

Those were the only 2 places that I found, and the sorting one makes sense to leave alone.

[dbluhm]

Before we merge, I'm going to check in with some other members of our team at Indicio about the impact of this change on controller work. I'll report back with their input.

[dbluhm]

Heard back from them: as it happens, because of this inconsistency between the connections and DID Exchange webhooks, our controller work has treated active and completed the same way. So this change will not be breaking for us and it seems likely that those who have walked a similar path as us may have a similar equivalency in their controllers. Do we have any other controller authors on hand that we can ping for input? @swcurran

[swcurran]

As I understand this, the impact on Controllers will be that the state of a connection as reported in webhooks will be different than previously. We have one controller in the BC Gov world (@marcos-carretero) that I suspect might break. Most controllers use —auto-accept and so will not see it, but that one is not using —auto… so is explicitly watching for state changes.

Questions:

• From what I read, the only difference is completed will become active. Will any other state changes be seen?
• What release/PR did this regression occur?

Thanks!

[dbluhm]

Questions:

• From what I read, the only difference is completed will become active. Will any other state changes be seen?
• What release/PR did this regression occur?

Correct, no other state changes, just completed -> active and only when doing DID Exchange. The connections protocol webhooks are correct.

This was introduced at the same time DID Exchange was introduced. At least according to your previous message investigating the blame 😄 Given that the webhooks existed previous to that and had the correct states associated with it for the connections protocol, I'm still inclined to call this a regression.

[sonarqubecloud]

Kudos, SonarCloud Quality Gate passed!   

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

No Coverage information
No Duplication information