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

Allow public invites for alice/faber demo #1574

Merged
merged 6 commits into from
Jan 7, 2022

Conversation

ianco
Copy link
Contributor

@ianco ianco commented Dec 21, 2021

Signed-off-by: Ian Costanzo [email protected]

Addresses #1571

Note - added a --reuse-connections parameter to faber - this will trigger faber to use a public did in the didexchange connection (and alice will always "reuse connection" if possible) and the connection indeed gets reused, however right now there are no webhooks for the "reuse" messages so the alice/faber controllers are getting hung ...

@codecov-commenter
Copy link

codecov-commenter commented Dec 22, 2021

Codecov Report

Merging #1574 (9d003a0) into main (ca6adc6) will not change coverage.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##             main    #1574   +/-   ##
=======================================
  Coverage   95.78%   95.78%           
=======================================
  Files         528      528           
  Lines       32491    32491           
=======================================
  Hits        31120    31120           
  Misses       1371     1371           

@swcurran
Copy link
Contributor

Hmm...I ran this:

./run_demo faber --public-invites
Preparing agent image...
sha256:94c44484e255f32387e52e2b69f8f7e767625c3a36235cd1239b5481dbb67cc9
Trying to detect ngrok service endpoint
ngrok not detected for agent endpoint
172.17.0.1
Starting [faber] agent with args [--port 8020 --public-invites]
usage: faber.py [-h] [--no-auto] [-p <port>] [--did-exchange] [--revocation]
                [--tails-server-base-url <tails-server-base-url>]
                [--cred-type <cred-type>] [--aip <api>] [--timing]
                [--multitenant] [--mediation] [--multi-ledger]
                [--wallet-type <wallet-type>]
                [--endorser-role <endorser-role>] [--arg-file <arg-file>]
faber.py: error: unrecognized arguments: --public-invites

@ianco
Copy link
Contributor Author

ianco commented Dec 22, 2021

Hmm...I ran this:

./run_demo faber --public-invites
...
faber.py: error: unrecognized arguments: --public-invites

You don't need to add a new parameter to faber, the --public-invites param is added to the aca-py agent that faber (and alice) start up under the hood

@swcurran
Copy link
Contributor

So it works, but each time I do a new invitation, I get a new connection on both sides. That means that reuse is not being used, which is the goal. Alice should be responding with a "reuse" message, and her "new" connection should be removed. Likewise on Faber -- once she responds with the reuse, the hanging connection should be removed. This (pretty likely :-) ) works because there was a follow up tweak to DID Exchange to do the clean up after someone tried this out.

Thoughts?

@swcurran
Copy link
Contributor

I think the reuse should be automatic, handled by ACA-Py, but am not certain how it works.

@swcurran
Copy link
Contributor

Also, I noticed that the "their_public_did" item on the connection object was not set, which is a bit odd. Not sure why it wouldn't be, but I think that is a pre-requisite for "reuse" on a connection.

@ianco
Copy link
Contributor Author

ianco commented Dec 22, 2021

I believe the auto-reuse only works when you use public DIDs for connections, which the demo is not doing

@ianco
Copy link
Contributor Author

ianco commented Dec 22, 2021

I believe the auto-reuse only works when you use public DIDs for connections, which the demo is not doing

I guess it depends on the intent, I though we were just trying to unblock Peter (issue #1571 where he is using the API).

If we want to configure alice/faber to use connection reuse for the demo then there is more work required. Let me know ...

@swcurran
Copy link
Contributor

Let's leave it for now. Peter's goal is to enable connection reuse, so that's really what we are after.

I think it would be nice to have the demo optional support connection reuse, so it would have an optional parameter, making it easy to see what the controller has to do to support reuse. How about we leave this as a draft PR and when you get a chance, you can update it? I'm hoping it is a pretty small change.

@ianco ianco marked this pull request as draft December 22, 2021 17:56
@ianco
Copy link
Contributor Author

ianco commented Dec 22, 2021

Let's leave it for now. Peter's goal is to enable connection reuse, so that's really what we are after.

I think it would be nice to have the demo optional support connection reuse, so it would have an optional parameter, making it easy to see what the controller has to do to support reuse. How about we leave this as a draft PR and when you get a chance, you can update it? I'm hoping it is a pretty small change.

Sure that works, I've converted the PR to draft.

@ianco ianco force-pushed the demo_public-invites branch from 0049c4b to 7fcdfaf Compare January 7, 2022 15:47
Signed-off-by: Ian Costanzo <[email protected]>
@ianco ianco marked this pull request as ready for review January 7, 2022 15:52
@ianco ianco requested a review from shaangill025 January 7, 2022 15:53
@ianco
Copy link
Contributor Author

ianco commented Jan 7, 2022

Fixed demo - run with ./run_demo faber --reuse-connections - this will use faber's public DID for the connection and allow connection reuse.

(Alice will always try to reuse connections if possible.)

Copy link
Contributor

@shaangill025 shaangill025 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Alice reuses the existing connection as intended.

@swcurran swcurran merged commit 2c9b3c7 into openwallet-foundation:main Jan 7, 2022
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 this pull request may close these issues.

4 participants