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

Get integration tests running on the anoncreds-rs branch, including the new AnonCreds integration test #2297

Closed
swcurran opened this issue Jul 11, 2023 · 6 comments
Assignees
Labels
AnonCreds Ledger Agnostic AnonCreds

Comments

@swcurran
Copy link
Contributor

swcurran commented Jul 11, 2023

Scope of the effort:

  • Initial state -- all of the AnonCreds related tests are failing.
  • Comment out the Credential Exchange V1 tests for now.
  • For the existing endpoints that will be removed (per Remove/Adapt old endpoints #2383) -- comment out the tests. We'll decide on eliminating them or not.
  • For the existing endpoints that will be deprecated (per Remove/Adapt old endpoints #2383), rework the endpoint implementations to use the AnonCreds endpoint implementations. Adjust the inputs, invoke the AnonCreds endpoint implementation, adjust the outputs. Get the integration tests that use those endpoints working.
    • Create a PR per endpoint? Your call -- good to have in the changelog.
    • As appropriate, make breaking changes to the interface. Add a separate BREAKING PR for each breaking change to the existing endpoints.
  • Add the AnonCreds test, per the "Original Definition" of this ticket (below) and get it working.
  • Provide a list of tests that should be removed or reworked, and add a ticket for getting them reworked as we add new features.
    • The Credential Exchange V1 tasks definitely fall into this category.
    • Revocation testing may fall into this category. Revocation tests that require the Controller do the work definitely should be removed. As necessary, revocation tests where ACA-Py does the work may need to be added.
    • Endorser tests likely need to be commented out.

Expected deliverable:

  • A set of working integration tests, including the new AnonCreds endpoints test.
  • A list of the commented out tests with an assessment of how to deal with each:
    • Remove for good, with a recommendation of a replacement test, if any.
    • Get working when feature X is updated (e.g., Revocation, V1, etc.)

Original definition:

In creating #2276 , @dbluhm and team used this gist test to verify the work they were doing. This task is to add this test to the ACA-Py BDD integration tests so that it is run automatically on merging into (for now) the AnonCreds development branch, and ultimately into the main branch when the AnonCreds branch is merged into the main branch.

The task is to adapt the test for running as an integration testing, and working with all of the other integration tests we have for ACA-Py.

As needed, please consult with @dbluhm for guidance on challenges that come up in trying to merge the work.

@dbluhm dbluhm changed the title Add AnonCreds Rust integration into ACA-Py Add AnonCreds Rust integration test into ACA-Py Jul 11, 2023
@usingtechnology
Copy link
Contributor

usingtechnology commented Jul 13, 2023

Had to step off to concentrate on #2304.
Leaving a trail here for myself to pick up later. Had a lot of issues running BDD, but figure kind of fighting up hill without pulling in commits to remove 3.6. So pull in changes to get this branch closer to main, see where that leaves the original set of tests and add new ones.

Base command to use for BDD (matches github action/workflow):

LEDGER_URL=http://test.bcovrin.vonx.io PUBLIC_TAILS_URL=https://tails-test.vonx.io LOG_LEVEL=warning NO_TTY=1 ./run_bdd -t @GHA

Which currently leaves main at:

4 features passed, 0 failed, 1 skipped
26 scenarios passed, 0 failed, 38 skipped
202 steps passed, 0 failed, 538 skipped, 0 undefined

This is the anoncreds-rs branch:

Failing scenarios:
  features/0453-issue-credential.feature:17  Issue a credential with the Issuer beginning with an offer -- @1.1
  features/0453-issue-credential.feature:56  Issue a credential with revocation, with the Issuer beginning with an offer, and then revoking the credential -- @1.1
  features/0453-issue-credential.feature:57  Issue a credential with revocation, with the Issuer beginning with an offer, and then revoking the credential -- @1.2
  features/0453-issue-credential.feature:58  Issue a credential with revocation, with the Issuer beginning with an offer, and then revoking the credential -- @1.3
  features/0454-present-proof.feature:17  Present Proof where the prover does not propose a presentation of the proof and is acknowledged -- @1.1
  features/0454-present-proof.feature:18  Present Proof where the prover does not propose a presentation of the proof and is acknowledged -- @1.2
  features/0454-present-proof.feature:75  Present Proof where the issuer revokes the credential and the proof fails -- @1.1
  features/0454-present-proof.feature:76  Present Proof where the issuer revokes the credential and the proof fails -- @1.2
  features/0454-present-proof.feature:118  Present Proof for multiple credentials where the one is revocable and one isn't, neither credential is revoked -- @1.1
  features/0454-present-proof.feature:159  Present Proof for multiple credentials where the one is revocable and one isn't, and the revocable credential is revoked, and the proof checks for revocation and fails -- @1.1
  features/0454-present-proof.feature:160  Present Proof for multiple credentials where the one is revocable and one isn't, and the revocable credential is revoked, and the proof checks for revocation and fails -- @1.2
  features/0454-present-proof.feature:181  Present Proof for multiple credentials where the one is revocable and one isn't, and the revocable credential is revoked, and the proof doesn't check for revocation and passes -- @1.1
  features/0586-sign-transaction.feature:52  endorse a transaction and write to the ledger -- @1.1
  features/0586-sign-transaction.feature:139  endorse a schema and cred def transaction, write to the ledger, issue and revoke a credential, manually invoking each endorsement endpoint -- @1.1
  features/0586-sign-transaction.feature:196  endorse a schema and cred def transaction, write to the ledger, issue and revoke a credential, with auto endorsing workflow -- @1.1

1 feature passed, 3 failed, 1 skipped
11 scenarios passed, 15 failed, 38 skipped
106 steps passed, 15 failed, 619 skipped, 0 undefined

@dbluhm dbluhm added the AnonCreds Ledger Agnostic AnonCreds label Jul 31, 2023
@usingtechnology
Copy link
Contributor

For discussion at tomorrow's (2023-08-01) Maintainer's meeting.
@dbluhm - having trouble reconciling the changes in the anoncreds-rs branch and main. Mostly having trouble getting anoncreds-rs branch to work with the test script (success) and existing BDD tests (not successful). Will need to discuss what the plan was/is...

ledger/base.py send_schema method "replaces" create_and_send_schema? But the create_and_send_schema is still used in messaging/schema/routes.py, similarly with create_and_send_credential_definition and credential_definitions. I had to add IndyIssuer binding back in to askar profile also.
So, I am not sure if the plan is to continue retrofitting the existing schema/cred_def routes to use new functions in ledger and issuer classes? I had attempted using the issuer methods (issuer.create_schema andissuer.create_and_store_credential_definition) before calling the new ledger methods (ledger.send_schema and ledger.send_credential_definition) but they don't quite play nicely with each other (ledger method signatures do not accept the returns from the issuer methods, other logic in place that don't work with payloads/responses (like parsing seqNo from schema response and deserializing CredentialDefinition payloads...)

So all that needs to be addressed before we address the changes @andrewwhitehead has made to ledger base with updates to indy credx.

We are probably not too far away, I am just unclear on the thoughts that got us here (anoncreds-rs) and what the plan was moving forward.

@dbluhm
Copy link
Contributor

dbluhm commented Aug 1, 2023

The idea was that the messaging/schema/routes.py would be removed altogether, its endpoints having been replaced by those in anoncreds/routes.py.

@usingtechnology
Copy link
Contributor

Thanks @dbluhm - that goes a long way to explaining why I couldn't quite get the pieces together!

@swcurran
Copy link
Contributor Author

swcurran commented Aug 3, 2023

Added what I think we agreed would be the scope of this work. @dbluhm @cjhowland @burdettadam @usingtechnology @esune

@swcurran swcurran changed the title Add AnonCreds Rust integration test into ACA-Py Get integration tests running on the anoucreds-rs branch, including the new AnonCreds integration test Aug 3, 2023
@swcurran swcurran changed the title Get integration tests running on the anoucreds-rs branch, including the new AnonCreds integration test Get integration tests running on the anoncreds-rs branch, including the new AnonCreds integration test Aug 3, 2023
@swcurran swcurran moved this from Todo to In Progress in Ledger Agnostic AnonCreds in ACA-Py Aug 8, 2023
@usingtechnology
Copy link
Contributor

Closing this ticket now that an initial cleanup has been done (see PR #2401).
Will add more tickets/issues related to this work.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
AnonCreds Ledger Agnostic AnonCreds
Projects
None yet
Development

No branches or pull requests

3 participants