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

Endorser support for updating DID endpoints on ledger #1696

Conversation

TheTechmage
Copy link
Contributor

As stated in #1447, the changes by Harsh fix an issue where ACA-Py could not publish ATTRIB records because they were not signed by the endorser. We need those changes in for 0.7.4 if possible, but PR #1616 has not seen any updates and has merge conflicts with the latest changes in main. I have attempted to fix those merge conflicts and make the requested changes from @ianco in that PR, as well as fix all the unit test failures.

This closes #1616. I am not sure if more work is needed for #1447.

HarshMultani and others added 3 commits March 25, 2022 12:04
…get updated on ledger after endorsement from an endorser.

Signed-off-by: Harsh Multani <harsh1multani@gmail.com>
Signed-off-by: Colton Wolkins (Indicio work address) <colton@indicio.tech>
Signed-off-by: Colton Wolkins (Indicio work address) <colton@indicio.tech>
Signed-off-by: Colton Wolkins (Indicio work address) <colton@indicio.tech>
@codecov-commenter
Copy link

codecov-commenter commented Mar 28, 2022

Codecov Report

Merging #1696 (459b72f) into main (6b1066a) will decrease coverage by 0.20%.
The diff coverage is 42.06%.

@@            Coverage Diff             @@
##             main    #1696      +/-   ##
==========================================
- Coverage   95.50%   95.29%   -0.21%     
==========================================
  Files         528      528              
  Lines       32766    32878     +112     
==========================================
+ Hits        31293    31331      +38     
- Misses       1473     1547      +74     

Copy link
Contributor

@ianco ianco left a comment

Choose a reason for hiding this comment

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

Some black formatting issues in aries_cloudagent/wallet/routes.py

Signed-off-by: Daniel Bluhm <dbluhm@pm.me>
@dbluhm dbluhm force-pushed the fix/author-endorser-attrib-txn-2 branch from e79a2e4 to 8674f8d Compare March 28, 2022 23:19
@dbluhm
Copy link
Contributor

dbluhm commented Mar 28, 2022

Evidently new versions of black behave slightly differently. I've updated the pre-commit hook to point to the most recent version that is now being used by the format checker.

@TheTechmage
Copy link
Contributor Author

I took a peek at that integration test failure. Looks like the error is from this code which is part of these test cases. I'm not quite sure how it's supposed to work, but I'll do my best to figure it out and provide a fix.

@ianco
Copy link
Contributor

ianco commented Mar 29, 2022

I think the error is in the order of the operations in the test:

      Given we have "2" agents
         | name  | role     | capabilities        |
         | Acme  | endorser | <Acme_capabilities> |
         | Bob   | author   | <Bob_capabilities>  |
      And "Acme" and "Bob" have an existing connection
      When "Acme" has a DID with role "ENDORSER"
      And "Bob" has a DID with role "AUTHOR"
      And "Acme" connection has job role "TRANSACTION_ENDORSER"
      And "Bob" connection has job role "TRANSACTION_AUTHOR"
      And "Bob" connection sets endorser info
      And "Bob" authors a schema transaction with <Schema_name>

The "Bob has a DID ..." line should happen after the connection endorser settings.

@ianco
Copy link
Contributor

ianco commented Mar 29, 2022

Like this:

      Given we have "2" agents
         | name  | role     | capabilities        |
         | Acme  | endorser | <Acme_capabilities> |
         | Bob   | author   | <Bob_capabilities>  |
      And "Acme" and "Bob" have an existing connection
      When "Acme" has a DID with role "ENDORSER"
      And "Acme" connection has job role "TRANSACTION_ENDORSER"
      And "Bob" connection has job role "TRANSACTION_AUTHOR"
      And "Bob" connection sets endorser info
      And "Bob" has a DID with role "AUTHOR"
      And "Bob" authors a schema transaction with <Schema_name>
...

... now that the DID creation requires an endorser.

@TheTechmage
Copy link
Contributor Author

That's what I'm thinking as well. Running the tests locally after making the changes A) to see if I can run the integration tests and B) to see if it fixes the problem. I really like how these tests are structured in that they're more "human readable" than actual code.

@TheTechmage
Copy link
Contributor Author

I'm seeing tests that don't exist in the failed CI, so I'm assuming that the answer is yes to both of those! Pushing the changes now. Thank you @ianco for the help. :)

Signed-off-by: Colton Wolkins (Indicio work address) <colton@indicio.tech>
@TheTechmage TheTechmage force-pushed the fix/author-endorser-attrib-txn-2 branch from 2045c1f to f87522c Compare March 29, 2022 02:20
@TheTechmage TheTechmage requested a review from ianco March 29, 2022 02:41
Copy link
Contributor

@ianco ianco left a comment

Choose a reason for hiding this comment

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

./run_bdd -t @T003.1-RFC0586 works

However BDD_EXTRA_AGENT_ARGS="{\"wallet-type\":\"askar\"}" ./run_bdd -t @T003.1-RFC0586 fils with an exception:

Shutting down agent ...
Aries      |   File "/home/indy/aries_cloudagent/wallet/routes.py", line 559, in promote_wallet_public_did
Aries      |     endorser_did=endorser_did,
Aries      |   File "/home/indy/aries_cloudagent/wallet/askar.py", line 488, in set_did_endpoint
Aries      |     endorser_did=endorser_did,
Aries      |   File "/home/indy/aries_cloudagent/ledger/indy_vdr.py", line 946, in update_endpoint_for_did
Aries      |     attrib_req = await ledger.append_request_endorser(
Aries      | AttributeError: module 'indy_vdr.ledger' has no attribute 'append_request_endorser'
Aries      | 2022-03-29 15:27:37,388 aiohttp.server ERROR Error handling request
Aries      | Traceback (most recent call last):
Aries      |   File "/home/indy/.pyenv/versions/3.6.13/lib/python3.6/site-packages/aiohttp/web_protocol.py", line 435, in _handle_request
Aries      |     resp = await request_handler(request)
Aries      |   File "/home/indy/.pyenv/versions/3.6.13/lib/python3.6/site-packages/aiohttp/web_app.py", line 504, in _handle
Aries      |     resp = await handler(request)
Aries      |   File "/home/indy/.pyenv/versions/3.6.13/lib/python3.6/site-packages/aiohttp/web_middlewares.py", line 117, in impl
Aries      |     return await handler(request)
Aries      |   File "/home/indy/aries_cloudagent/admin/server.py", line 159, in ready_middleware
Aries      |     return await handler(request)
Aries      |   File "/home/indy/aries_cloudagent/admin/server.py", line 196, in debug_middleware
Aries      |     return await handler(request)
Aries      |   File "/home/indy/.pyenv/versions/3.6.13/lib/python3.6/site-packages/aiohttp_apispec/middlewares.py", line 45, in validation_middleware
Aries      |     return await handler(request)
Aries      |   File "/home/indy/aries_cloudagent/admin/server.py", line 383, in setup_context
Aries      |     return await task
Aries      |   File "/home/indy/aries_cloudagent/wallet/routes.py", line 437, in wallet_set_public_did
Aries      |     context.profile, context, context.session, did, write_ledger=write_ledger
Aries      |   File "/home/indy/aries_cloudagent/wallet/routes.py", line 559, in promote_wallet_public_did
Aries      |     endorser_did=endorser_did,
Aries      |   File "/home/indy/aries_cloudagent/wallet/askar.py", line 488, in set_did_endpoint
Aries      |     endorser_did=endorser_did,
Aries      |   File "/home/indy/aries_cloudagent/ledger/indy_vdr.py", line 946, in update_endpoint_for_did
Aries      |     attrib_req = await ledger.append_request_endorser(
Aries      | AttributeError: module 'indy_vdr.ledger' has no attribute 'append_request_endorser'

Signed-off-by: Colton Wolkins (Indicio work address) <colton@indicio.tech>
@TheTechmage
Copy link
Contributor Author

Currently running into an issue where a couple of tests in @T003-RFC0586 are complaining about not having an endorser. Specifically the two multi-tenant ones (last two examples). Currently trying to figure out why those would error out.

@TheTechmage
Copy link
Contributor Author

Looking at that test, it appears to be skipped during GitHub actions anyways. I'm not sure how critical it is to try and fix it at this time.

@dbluhm dbluhm added the 0.7.4 label Mar 29, 2022
@ianco
Copy link
Contributor

ianco commented Mar 29, 2022

Looking at that test, it appears to be skipped during GitHub actions anyways. I'm not sure how critical it is to try and fix it at this time.

Github actions runs a subset of tests because if you try to run too many (i.e. all of them) it seems to blow up.
I'm just taking a look at this failing test now ...

@ianco
Copy link
Contributor

ianco commented Mar 29, 2022

Weird that something is failing with multi-tenancy, I suggest to log a separate issue. We can merge this PR and then deal with the multi-tenant issue separately.

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.

FWIW 🙂

@swcurran swcurran merged commit 845af96 into openwallet-foundation:main Mar 29, 2022
@TheTechmage TheTechmage deleted the fix/author-endorser-attrib-txn-2 branch March 29, 2022 20:18
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.

6 participants