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

Peer DID Interop tests - Peer DID Transition #760

Merged

Conversation

nodlesh
Copy link
Contributor

@nodlesh nodlesh commented Jan 12, 2024

This PR aims to partially complete #752. This is a draft PR until all ACA-Py tasks are completed below. AFJ tasks and other backchannel updates will be broken out into separate tickets if a ticket doesn't exist for it yet.

  • Create Feature File and test definitions for Peer DIDs.
  • In this PR, there is also some cleanup of the format of the json output in the ACA-Py Backchannel for readability.
  • Add handling to restart an agent with different arguments if the Given "n" agents step contains startup args.
  • Get start API call to accept any startup options, including --emit-peer-did-#
  • In the ACA-Py Backchannel, make sure args passed in through the start API take precedence over the default args in the backchannel.
  • After the feature is finished, change the reset of agents to original configuration to only attempt to restart changed agents instead of all of them. Some of them may not even be running.
  • Add handling of what peer DID method is expected throughout the test and check for those in responses.
  • Add tests that do not set the peer did on the responder and make sure it can respond to the peer DID of the requester.

AFJ Backchannel Updates

VCX Backchannel Updates

  • Apply the modifications above to the VCX Backchannel
    • Research is needed to determine what is exactly required.
  • Create runsets for AFJ, ACA-Py, and VCX to fully exercise qualified Peer DIDs between these agents.

Findy Backchannel Updates?

  • Apply the modifications above to the findy Backchannel?

AFGO Backchannel Updates

  • Apply the modifications above to the AFGO Backchannel

@nodlesh nodlesh mentioned this pull request Jan 18, 2024
@nodlesh
Copy link
Contributor Author

nodlesh commented Jan 26, 2024

This PR is ready for review and merge. We may have some help on the AFJ side so it would be good if these enhancements were in main.

These Qualified DID Tests will run in the regular interop pipeline test execution cycle for ACA-py. It will run in the full acapy runset. It will not run for any other runset until the backchannels have be brought up to scope for these tests.

Though these tests work with ACA-Py, we expect some of the test specification like the start parameters, etc to change to be more generalized across frameworks once we start working with AFJ and other frameworks in this test context.

@nodlesh nodlesh marked this pull request as ready for review January 26, 2024 16:50
dbluhm
dbluhm previously approved these changes Jan 26, 2024
Copy link
Contributor

@dbluhm dbluhm left a comment

Choose a reason for hiding this comment

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

LGTM!

Comment on lines 92 to 95
| did:peer:1 | {"wallet-type":"askar-anoncreds", "flags":["emit-did-peer-1"]} | {"wallet-type":"askar-anoncreds", "flags":["emit-did-peer-3"]} |
| did:peer:2 | {"wallet-type":"askar-anoncreds", "flags":["emit-did-peer-2"]} | {"wallet-type":"askar-anoncreds", "flags":["emit-did-peer-4"]} |
| did:peer:3 | {"wallet-type":"askar-anoncreds", "flags":["emit-did-peer-3"]} | {"wallet-type":"askar-anoncreds", "flags":["emit-did-peer-1"]} |
| did:peer:4 | {"wallet-type":"askar-anoncreds", "flags":["emit-did-peer-4"]} | {"wallet-type":"askar-anoncreds", "flags":["emit-did-peer-2"]} |
Copy link
Contributor

@dbluhm dbluhm Jan 26, 2024

Choose a reason for hiding this comment

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

Actually, comments: I'm not sure it makes sense for us to include tests for did:peer:3. Also, ACA-Py has not implemented an emit-did-peer-1 mode.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've added them in for when they are implemented these tests should then automatically pass. However, if we prefer, we can just comment them out until or if we support those modes.
I like a test first approach, where we have failing tests then the software catches up to the tests to make them pass. I can understand that we may not want that here, showing a higher fail rate for unimplemented features.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@swcurran Would you prefer currently unsupported DID Peer Methods to be commented out or leave them failing? I'm inclined to think you would want them commented out.

Copy link
Contributor

Choose a reason for hiding this comment

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

We are unlikely to implement support for emitting a did:peer:1 or 3

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will any other Aries Framework implement those? The RFC mentions them so that is why I put them in.

Copy link
Contributor

Choose a reason for hiding this comment

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

AFJ implements 1 but it's actually not possible to exchange just did:peer:3s. You emit 2 and then derive a 3.

@nodlesh nodlesh self-assigned this Jan 29, 2024
@nodlesh
Copy link
Contributor Author

nodlesh commented Jan 29, 2024

The last commit here comments out the DID Peer Methods that ACA-Py doesn't support. Will add them back if and when we support them or another Aries framework does.
Also loosened the check for the DID Peer Method to pass of fail a test scenario. The responders DID is just checked to make sure it is qualified, not that it has the same DID Peer Method as the requester.

@nodlesh
Copy link
Contributor Author

nodlesh commented Jan 31, 2024

@dbluhm or @swcurran Could I get a quick final approval of this? Would like to merge at this point. Thanks.

@nodlesh nodlesh merged commit 6c9a7c8 into openwallet-foundation:main Jan 31, 2024
1 check passed
@swcurran
Copy link
Contributor

Awesome work! Nice!

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.

3 participants