-
Notifications
You must be signed in to change notification settings - Fork 516
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
VC DI proof request #2960
VC DI proof request #2960
Conversation
Quality Gate passedIssues Measures |
I know this is still WIP, but we need to have a description on the PR (is necessary in case anyone needs to look back on this PR in the future, and is also used for release notes). Also it's good - if the PR is in WIP state - to make a note of what is done and what's still outstanding ... |
4dee13e
to
e344a1d
Compare
Also just noticed the commit is missing the DCO signoff |
8208477
to
75f6c53
Compare
e3a364d
to
4fee8fa
Compare
Signed-off-by: Sarthak Vijayvergiya <[email protected]>
Signed-off-by: Sarthak Vijayvergiya <[email protected]>
4fee8fa
to
742a7df
Compare
Signed-off-by: EmadAnwer <[email protected]>
Signed-off-by: Sarthak Vijayvergiya <[email protected]>
Signed-off-by: Sarthak Vijayvergiya <[email protected]>
…est_web_request object for revocation Signed-off-by: EmadAnwer <[email protected]>
Signed-off-by: EmadAnwer <[email protected]>
Signed-off-by: EmadAnwer <[email protected]>
Signed-off-by: EmadAnwer <[email protected]>
6f53dc0
to
c13a3df
Compare
- getting the correct timestamp - create rev_states - remove static code Signed-off-by: EmadAnwer <[email protected]>
Signed-off-by: EmadAnwer <[email protected]>
Signed-off-by: Emad <[email protected]>
to - create_rev_states - prepare_data_for_presentation Signed-off-by: EmadAnwer <[email protected]>
add - prepare_data_for_presentation - _load_w3c_credentials functions remove holder flag Signed-off-by: EmadAnwer <[email protected]>
Additional integration tests for vc_di and revocation
- test_assert_no_callenge_error - test_assert_verify_presentation - test__extract_cred_idx - test__get_predicate_type_and_value - test__load_w3c_credentials Signed-off-by: EmadAnwer <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A couple of comments to be removed (minor issue) and unit test coverage (SonarCloud reports 27 new lines in each of aries_cloudagent/anoncreds/holder.py
and aries_cloudagent/vc/vc_di/prove.py
that are not covered - I think 2 simple "happy path" tests could cover most of this).
Also PR description ... (@EmadAnwer not sure if you mentioned you can't edit the description?)
Overall looks really good!
@@ -169,7 +171,7 @@ async def create_pres( | |||
domain = proof_request["options"].get("domain") | |||
if not challenge: | |||
challenge = str(uuid4()) | |||
|
|||
# TODO handle vc_di format in the future |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This comment can be removed
- test_create_presentation_w3c - test_create_presentation_w3c_create_error Signed-off-by: EmadAnwer <[email protected]>
- test_create_signed_anoncreds_presentation Signed-off-by: EmadAnwer <[email protected]>
Signed-off-by: Emad <[email protected]>
Signed-off-by: EmadAnwer <[email protected]>
Signed-off-by: EmadAnwer <[email protected]>
ac32d5e
to
d13136c
Compare
- test_store_credential_w3c - test_get_type_manager_options Signed-off-by: EmadAnwer <[email protected]>
There is quite a few things reported in sonarcloud. I don't think everything needs to be addressed but some definitely should like the unused variable and shadow variable names. Maybe have a go over and see if we can make some improvements. I think overall the code looks good and well tested. |
Good catch @jamshale @EmadAnwer can you review the SonarCloud report and do a bit of cleanup? As Jamie says we don't need to address everything but take a pass and see what you think. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
SonarCloud as noted by Jamie
yes sure |
Signed-off-by: EmadAnwer <[email protected]>
Quality Gate passedIssues Measures |
@jamshale @ianco I have fixed most of it, check it please, let me know if there are any suggestions |
I'm going to merge this. Would be nice to have in the 1.0.0 release and don't predict any regressions. |
w00t!!! Awesome. |
* WIP: vc di proof request - authored by ianco(openwallet-foundation#3043) Signed-off-by: Sarthak Vijayvergiya <[email protected]> * fixed lint checks, cleanup Signed-off-by: Sarthak Vijayvergiya <[email protected]> * fix: verify_pres, get_sign_key_credential_subject_id Signed-off-by: EmadAnwer <[email protected]> * WIP: debugging revocation & fixes Signed-off-by: Sarthak Vijayvergiya <[email protected]> * WIP: fix ununsed import Signed-off-by: Sarthak Vijayvergiya <[email protected]> * refactor: create_signed_anoncreds_presentation, faber vcdi proof_request_web_request object for revocation Signed-off-by: EmadAnwer <[email protected]> * Refactor:Add W3cCredential loading for VCDI format handler Signed-off-by: EmadAnwer <[email protected]> * fix: tests Signed-off-by: EmadAnwer <[email protected]> * WPA: using static data to test the revocation validation Signed-off-by: EmadAnwer <[email protected]> * feat: Add revocation support to VCDI - getting the correct timestamp - create rev_states - remove static code Signed-off-by: EmadAnwer <[email protected]> * Remove unused code for credential definition and revocation Signed-off-by: EmadAnwer <[email protected]> * WPA: fix lint Signed-off-by: EmadAnwer <[email protected]> * Fix cred search for vc_di proof Signed-off-by: EmadAnwer <[email protected]> * Additional integration tests for vc_di and revocation Signed-off-by: Ian Costanzo <[email protected]> * refactor: remove unused comments and TODO's - implement _extract_cred_idx - add try catch to some expected fail code Signed-off-by: EmadAnwer <[email protected]> * refactor: split create_signed_anoncreds_presentation to - create_rev_states - prepare_data_for_presentation Signed-off-by: EmadAnwer <[email protected]> * refactor: `create_signed_anoncreds_presentation` add - prepare_data_for_presentation - _load_w3c_credentials functions remove holder flag Signed-off-by: EmadAnwer <[email protected]> * add: tests - test_assert_no_callenge_error - test_assert_verify_presentation - test__extract_cred_idx - test__get_predicate_type_and_value - test__load_w3c_credentials Signed-off-by: EmadAnwer <[email protected]> * add: tests, remove todos - test_create_presentation_w3c - test_create_presentation_w3c_create_error Signed-off-by: EmadAnwer <[email protected]> * add: tests - test_create_signed_anoncreds_presentation Signed-off-by: EmadAnwer <[email protected]> * fix: linter Signed-off-by: EmadAnwer <[email protected]> * fix: remove unused imports Signed-off-by: EmadAnwer <[email protected]> * add: tests - test_store_credential_w3c - test_get_type_manager_options Signed-off-by: EmadAnwer <[email protected]> * refactor: remove extra variables and comments Signed-off-by: EmadAnwer <[email protected]> --------- Signed-off-by: Sarthak Vijayvergiya <[email protected]> Signed-off-by: EmadAnwer <[email protected]> Signed-off-by: Emad <[email protected]> Signed-off-by: Ian Costanzo <[email protected]> Co-authored-by: EmadAnwer <[email protected]> Co-authored-by: Ian Costanzo <[email protected]>
Presentation request (DIF) for VC_DI credentials