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.

Sign up for GitHub

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

Jump to bottom

VC DI proof request #2960

Merged
merged 29 commits into from
Jul 12, 2024
Merged

VC DI proof request #2960

merged 29 commits into from
Jul 12, 2024

Conversation

sarthakvijayvergiya
Copy link
Contributor

@sarthakvijayvergiya sarthakvijayvergiya commented May 21, 2024 •
edited by ianco
Loading

Presentation request (DIF) for VC_DI credentials

@sonarcloud SonarCloud
Copy link

sonarcloud bot commented May 21, 2024

Quality Gate Passed Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
No data about Duplication

See analysis details on SonarCloud

@ianco
Copy link
Member

ianco commented Jun 4, 2024

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 ...

@sarthakvijayvergiya sarthakvijayvergiya marked this pull request as draft June 5, 2024 06:58
@sarthakvijayvergiya sarthakvijayvergiya changed the title feat: VC DI proof request [WIP] feat: VC DI proof request Jun 5, 2024
@sarthakvijayvergiya sarthakvijayvergiya force-pushed the whatscookin/feat/vc-di-proof branch 2 times, most recently from 4dee13e to e344a1d Compare June 5, 2024 10:22
@ianco
Copy link
Member

ianco commented Jun 16, 2024

Also just noticed the commit is missing the DCO signoff

@sarthakvijayvergiya sarthakvijayvergiya force-pushed the whatscookin/feat/vc-di-proof branch 5 times, most recently from e3a364d to 4fee8fa Compare June 20, 2024 13:36
@sarthakvijayvergiya
WIP: vc di proof request - authored by ianco(hyperledger#3043)
23ad5bd 
Signed-off-by: Sarthak Vijayvergiya <sarthakvijayvergiya@gmail.com>
@sarthakvijayvergiya
fixed lint checks, cleanup
742a7df 
Signed-off-by: Sarthak Vijayvergiya <sarthakvijayvergiya@gmail.com>
EmadAnwer and others added 7 commits June 28, 2024 22:20
@EmadAnwer
fix: verify_pres, get_sign_key_credential_subject_id
92db3ea 
Signed-off-by: EmadAnwer <emadanwer.official@gmail.com>
@sarthakvijayvergiya
WIP: debugging revocation & fixes
dd800b0 
Signed-off-by: Sarthak Vijayvergiya <sarthakvijayvergiya@gmail.com>
@sarthakvijayvergiya
WIP: fix ununsed import
c7077dd 
Signed-off-by: Sarthak Vijayvergiya <sarthakvijayvergiya@gmail.com>
@EmadAnwer
refactor: create_signed_anoncreds_presentation, faber vcdi proof_requ…
147fbb9 
…est_web_request object for revocation

Signed-off-by: EmadAnwer <emadanwer.official@gmail.com>
@EmadAnwer
Refactor:Add W3cCredential loading for VCDI format handler
72f2d08 
Signed-off-by: EmadAnwer <emadanwer.official@gmail.com>
@EmadAnwer
fix: tests
6ad8125 
Signed-off-by: EmadAnwer <emadanwer.official@gmail.com>
@EmadAnwer
WPA: using static data to test the revocation validation
c13a3df 
Signed-off-by: EmadAnwer <emadanwer.official@gmail.com>
@EmadAnwer EmadAnwer force-pushed the whatscookin/feat/vc-di-proof branch from 6f53dc0 to c13a3df Compare July 4, 2024 19:56
@EmadAnwer
feat: Add revocation support to VCDI
2626b19 
- getting the correct timestamp
- create  rev_states
- remove static code

Signed-off-by: EmadAnwer <emadanwer.official@gmail.com>
@EmadAnwer
Remove unused code for credential definition and revocation
1034c80 
Signed-off-by: EmadAnwer <emadanwer.official@gmail.com>
@EmadAnwer
Merge branch 'main' into whatscookin/feat/vc-di-proof
d02441e 
Signed-off-by: Emad <emadanwer.official@gmail.com>
EmadAnwer and others added 6 commits July 8, 2024 21:49
@EmadAnwer
refactor: remove unused comments and TODO's
432923b 
 - implement _extract_cred_idx
- add try catch to some expected fail code

Signed-off-by: EmadAnwer <emadanwer.official@gmail.com>
@EmadAnwer
refactor: split create_signed_anoncreds_presentation
e58f1aa 
to
 - create_rev_states
- prepare_data_for_presentation

Signed-off-by: EmadAnwer <emadanwer.official@gmail.com>
@EmadAnwer
refactor: create_signed_anoncreds_presentation
a037382 
add
- prepare_data_for_presentation
- _load_w3c_credentials functions

remove holder flag

Signed-off-by: EmadAnwer <emadanwer.official@gmail.com>
@sarthakvijayvergiya
Merge pull request #3 from ianco/w3c-proof-req-2
dc351a4 
Additional integration tests for vc_di and revocation
@sarthakvijayvergiya
Merge branch 'main' into whatscookin/feat/vc-di-proof
852fa6a
@EmadAnwer
add: tests
c10c928 
- 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 <emadanwer.official@gmail.com>
ianco
ianco requested changes Jul 11, 2024
View reviewed changes
Copy link
Member

@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.

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!

demo/runners/agent_container.py Outdated Show resolved Hide resolved
aries_cloudagent/protocols/present_proof/v2_0/formats/dif/handler.py Outdated
@@ -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
Copy link
Member

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

@EmadAnwer
add: tests, remove todos
494ab81 
- test_create_presentation_w3c
- test_create_presentation_w3c_create_error

Signed-off-by: EmadAnwer <emadanwer.official@gmail.com>
@EmadAnwer
add: tests
b623dae 
- test_create_signed_anoncreds_presentation

Signed-off-by: EmadAnwer <emadanwer.official@gmail.com>
@EmadAnwer
Merge branch 'main' into whatscookin/feat/vc-di-proof
6365822 
Signed-off-by: Emad <emadanwer.official@gmail.com>
@EmadAnwer
fix: linter
b30d88d 
Signed-off-by: EmadAnwer <emadanwer.official@gmail.com>
@EmadAnwer
fix: remove unused imports
d13136c 
Signed-off-by: EmadAnwer <emadanwer.official@gmail.com>
@EmadAnwer EmadAnwer force-pushed the whatscookin/feat/vc-di-proof branch from ac32d5e to d13136c Compare July 11, 2024 19:37
@EmadAnwer
add: tests
0170055 
- test_store_credential_w3c
- test_get_type_manager_options

Signed-off-by: EmadAnwer <emadanwer.official@gmail.com>
@ianco ianco requested a review from jamshale July 11, 2024 22:27
@jamshale jamshale changed the title [WIP] feat: VC DI proof request VC DI proof request Jul 11, 2024
@jamshale
Copy link
Contributor

jamshale commented Jul 11, 2024 •
edited
Loading

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.

https://sonarcloud.io/project/issues?id=hyperledger_aries-cloudagent-python&pullRequest=2960&resolved=false&sinceLeakPeriod=true

Maybe have a go over and see if we can make some improvements. I think overall the code looks good and well tested.

@ianco
Copy link
Member

ianco commented Jul 11, 2024

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.

https://sonarcloud.io/project/issues?id=hyperledger_aries-cloudagent-python&pullRequest=2960&resolved=false&sinceLeakPeriod=true

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.

ianco
ianco requested changes Jul 11, 2024
View reviewed changes
Copy link
Member

@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.

SonarCloud as noted by Jamie

@EmadAnwer
Copy link
Contributor

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.
https://sonarcloud.io/project/issues?id=hyperledger_aries-cloudagent-python&pullRequest=2960&resolved=false&sinceLeakPeriod=true
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.

yes sure

@EmadAnwer
refactor: remove extra variables and comments
116a992 
Signed-off-by: EmadAnwer <emadanwer.official@gmail.com>
@sonarcloud SonarCloud
Copy link

sonarcloud bot commented Jul 12, 2024

Quality Gate Passed Quality Gate passed

Issues
12 New issues
0 Accepted issues

Measures
0 Security Hotspots
86.4% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud

@EmadAnwer
Copy link
Contributor

Quality Gate Passed Quality Gate passed

Issues 12 New issues 0 Accepted issues

Measures 0 Security Hotspots 86.4% Coverage on New Code 0.0% Duplication on New Code

See analysis details on SonarCloud

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.
https://sonarcloud.io/project/issues?id=hyperledger_aries-cloudagent-python&pullRequest=2960&resolved=false&sinceLeakPeriod=true
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.

yes sure

@jamshale @ianco I have fixed most of it, check it please, let me know if there are any suggestions

@jamshale jamshale merged commit a1a5afc into hyperledger:main Jul 12, 2024
8 checks passed
@jamshale
Copy link
Contributor

I'm going to merge this. Would be nice to have in the 1.0.0 release and don't predict any regressions.

@swcurran
Copy link
Member

w00t!!! Awesome.

@jamshale jamshale mentioned this pull request Jul 15, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ianco ianco ianco approved these changes

@jamshale jamshale jamshale approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@sarthakvijayvergiya @ianco @jamshale @EmadAnwer @swcurran