OpenMined / PyDentity

A repository for leveraging Self-Sovereign Identity in applications
65 stars 25 forks source link

Bump aca-py version to 0.6 and change API and examples accordingly - issue 56 #68

Closed morrieinmaas closed 3 years ago

morrieinmaas commented 3 years ago

Description

closes #56

Affected Dependencies

aca-py

How has this been tested?

Checklist

review-notebook-app[bot] commented 3 years ago

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

lohanspies commented 3 years ago

@morrieinmaas we can extract the added AcaPy parameter from https://github.com/OpenMined/PyDentity/pull/68/files#diff-53a0dfff4019907a159c1ac9502ca216fadd8b3c2cfef9bb999e8f8fe56fdd02 into the yml file instead to keep it consistent with the rest of the repo.

lohanspies commented 3 years ago

@morrieinmaas we can extract the added AcaPy parameter from https://github.com/OpenMined/PyDentity/pull/68/files#diff-53a0dfff4019907a159c1ac9502ca216fadd8b3c2cfef9bb999e8f8fe56fdd02 into the yml file instead to keep it consistent with the rest of the repo.

See you already did this in https://github.com/OpenMined/PyDentity/pull/68/files#diff-232bac0e229b355a979026366e0a706bf87b48da75db720e309ed7fa1c6b8208 and https://github.com/OpenMined/PyDentity/pull/68/files#diff-540c821638d1899c5fb9b43372d6a108952362e99666e2514c333d4d66e54a61 i.e. Not sure why it is required in the Dockerfile as well as part of the AcaPy startup cmd

morrieinmaas commented 3 years ago

@morrieinmaas we can extract the added AcaPy parameter from https://github.com/OpenMined/PyDentity/pull/68/files#diff-53a0dfff4019907a159c1ac9502ca216fadd8b3c2cfef9bb999e8f8fe56fdd02 into the yml file instead to keep it consistent with the rest of the repo.

Yeah totally that makes sense. Thanks for pointing that out @lohanspies

morrieinmaas commented 3 years ago

@morrieinmaas we can extract the added AcaPy parameter from https://github.com/OpenMined/PyDentity/pull/68/files#diff-53a0dfff4019907a159c1ac9502ca216fadd8b3c2cfef9bb999e8f8fe56fdd02 into the yml file instead to keep it consistent with the rest of the repo.

See you already did this in https://github.com/OpenMined/PyDentity/pull/68/files#diff-232bac0e229b355a979026366e0a706bf87b48da75db720e309ed7fa1c6b8208 and https://github.com/OpenMined/PyDentity/pull/68/files#diff-540c821638d1899c5fb9b43372d6a108952362e99666e2514c333d4d66e54a61 i.e. Not sure why it is required in the Dockerfile as well as part of the AcaPy startup cmd

The one in the docker command I probs used when trying to make it run first and didn't remove it when i moved it over to the yaml file. Will do that now.

morrieinmaas commented 3 years ago

@lohanspies shuld be the right way now

lohanspies commented 3 years ago

@lohanspies shuld be the right way now

I am happy. Looked at the code changes and all make sense. @wip-abramson ?

wip-abramson commented 3 years ago

Nice!

wip-abramson commented 3 years ago

I think this is all fine, but struggling to see the exact changes in the proofs.py file.

Did you check these changes do not affect any use of this API in the notebooks

morrieinmaas commented 3 years ago

I think this is all fine, but struggling to see the exact changes in the proofs.py file.

Did you check these changes do not affect any use of this API in the notebooks

@wip-abramson I added send_problem_report and send_presentation. I re-ordered the functions in order they appear in the swagger API interface so it's easier to cross compare when working on it. I also checked whether any args have changed against swagger and that looks fine. I did have a quick run through the aries-basic-controller notebook and all worked (on my machine ;-P ). happy to have another run through though.

But I guess this is just another pointer that we should think of at least unit tests for this so we don't need to have this opint of discussion ...😅

wip-abramson commented 3 years ago

No all good!