hyperledger / aries-cloudagent-python

Hyperledger Aries Cloud Agent Python (ACA-Py) is a foundation for building decentralized identity applications and services running in non-mobile environments.
https://wiki.hyperledger.org/display/aries
Apache License 2.0
403 stars 510 forks source link

DID Exchange in ACA-Py 0.7.4 with explicit invitations and without auto-accept broken #1936

Closed dbluhm closed 1 year ago

dbluhm commented 1 year ago

While working with @mepeltier on testing out a few different scenarios on ACA-Py 0.7.4, we (re)discovered that the connection state after receiving a did exchange request was not being saved, making it impossible to accept the request. This was fixed in main (and included in 1.0.0-rc0) by #1881. At the time I reviewed that PR, I did not realize how big of an issue the PR was fixing. Looking at it again now, I think it warrants a patch release.

Main has seen some changes lately that likely wouldn't fit in a patch release. With that in mind, I think cherry-picking and producing a release on a separate branch is appropriate. I'd be happy to put in the effort to prepare a branch with this state.

Should we do this? Are there other fixes that should be included in a 0.7.5 patch release? @swcurran @ianco @andrewwhitehead

dbluhm commented 1 year ago

Also probably worth making sure we hit this scenario in the integration tests.

dbluhm commented 1 year ago

I'll review PRs merged since 0.7.4 and report back whether there are any I think should also be included in a 0.7.5 patch release.

ianco commented 1 year ago

Also probably worth making sure we hit this scenario in the integration tests.

The integration tests run with most of the "auto" flags on, whereas AATH runs with most of the "auto" flags off

ianco commented 1 year ago

I'll review PRs merged since 0.7.4 and report back whether there are any I think should also be included in a 0.7.5 patch release.

This one please: https://github.com/hyperledger/aries-cloudagent-python/pull/1913

dbluhm commented 1 year ago

The mobile team at Indicio is interested in seeing #1940 included in a patch.

swcurran commented 1 year ago

@ianco -- I tend to call #1913 a breaking change, so would prefer to leave it for 1.0.0. What is the rationale for putting it into 0.7.5?

ianco commented 1 year ago

@ianco -- I tend to call #1913 a breaking change, so would prefer to leave it for 1.0.0. What is the rationale for putting it into 0.7.5?

Northern Block has specifically asked for a release with this fix

swcurran commented 1 year ago

Could we do that as a 1.0.0-rc1 for that? I'm happy to do that.

ianco commented 1 year ago

Could we do that as a 1.0.0-rc1 for that? I'm happy to do that.

For sure I think that would be fine.

Durai46 commented 1 year ago

@swcurran Are you going to publish the docker image for the same?

swcurran commented 1 year ago

Yes. Completing the 0.7.5 release is still a in progress -- there is at least one more PR to go. Would it help you for me to produce a 1.0.0-rc1, or do you need the 0.7.5-rc0/0.7.5 image?

Sorry for the delay.

Durai46 commented 1 year ago

Thank you @swcurran I would go for 0.7.5 image.

swcurran commented 1 year ago

Tag and image 0.7.5-rc0 exists and seems to be fine. @Durai46 and @dbluhm -- can you confirm we are good to go with "0.7.5" final? I think it is an easy one, but just wanted to get this finished.

We've run AATH tests and things are working with one odd, but likely unrelated bit of weirdness. If you want the details I can provide, but it's convoluted! :-(

Once I get a thumbs up I'll do a final PR for the 0.7.5 release and we'll count it done.

dbluhm commented 1 year ago

I haven't had a chance to give it the full battery of tests we like to run (basically just plugging into our plugins/projects and running integration tests) but I have run a few and it's looking good so far. Critically, the original problem I opened this issue for I have tested and found working in 0.7.5-rc0.

swcurran commented 1 year ago

OK -- Glad to hear the main issue is addressed. I'm doing a bit more and we'll make a call that it is done Real Soon Now. Please let me know if you do more and the results are good/bad.

Thanks!

swcurran commented 1 year ago

FYI -- the "weirdness" with AATH has been solved. Weird answer, but now working... :-)

dbluhm commented 1 year ago

With 0.7.5 released, closing this issue :slightly_smiling_face: