Updates for python wrapper adding support for revocation status list changes #145

dbluhm · 2023-03-16T18:58:25Z

Opening this early. This PR updates the python wrapper to accommodate changes made at the rust layer for revocation status lists.

Feedback and corrections welcome. I'll continue working on this by updating the test.py file to bring it in line with the anoncreds_demo.rs.

Signed-off-by: Daniel Bluhm <[email protected]>

andrewwhitehead · 2023-03-16T19:02:04Z

You might want to integrate these changes as well? hyperledger/indy-shared-rs#26

dbluhm · 2023-03-16T19:03:06Z

You might want to integrate these changes as well? hyperledger/indy-shared-rs#26

That seems like a good idea! I'll check it out.

Signed-off-by: blu3beri <[email protected]>

berendsliedrecht · 2023-03-17T09:22:06Z

Awesome work! There was a slight misunderstanding on my side related to this work, I also made a draft for it as I was stuck on some other work. #146 is the related PR.

Maybe there are some things that you can take from my PR, but I will abandon mine so the work can continue here.

dbluhm · 2023-03-17T14:16:54Z

Shoot, apologies for the miscommunication -- duplicating work is less than ideal. It looks like you got further along than I did. I'll see what I can do to combine efforts here! Thanks for the heads up, @blu3beri

…thon-rev-status-lists

dbluhm · 2023-03-17T17:00:01Z

@blu3beri I merged in your commit from #146; I'll follow this up by integrating hyperledger/indy-shared-rs#26 as @andrewwhitehead suggested

And some other merge artifact fixes Signed-off-by: Daniel Bluhm <[email protected]>

Signed-off-by: Daniel Bluhm <[email protected]>

dbluhm · 2023-03-20T18:58:02Z

@blu3beri After combining everything, demo/test.py is currently failing with the following error:

Traceback (most recent call last):
  File "/home/dbluhm/dev/anoncreds/anoncreds-rs/wrappers/python/demo/test.py", line 192, in <module>
    verified = presentation.verify(
               ^^^^^^^^^^^^^^^^^^^^
  File "/home/dbluhm/dev/anoncreds/anoncreds-rs/wrappers/python/anoncreds/types.py", line 593, in verify
    return bindings.verify_presentation(
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/home/dbluhm/dev/anoncreds/anoncreds-rs/wrappers/python/anoncreds/bindings.py", line 803, in verify_presentation
    do_call(
  File "/home/dbluhm/dev/anoncreds/anoncreds-rs/wrappers/python/anoncreds/bindings.py", line 404, in do_call
    raise get_current_error(True)
anoncreds.error.AnoncredsError: Revocation Registry not provided for ID and timestamp: RevocationRegistryId("mock:uri:revregid"), 12

To see if this was caused by a change I introduced, I checked out 4a019cb from your branch and had the same issue. Is this expected?

Update: Nevermind, I've spotted the issue and it appears to just be a typo 🙂 I'll continue working on updating the test.py

Signed-off-by: Daniel Bluhm <[email protected]>

dbluhm · 2023-03-20T22:48:46Z

The python wrapper demo is now functioning as expected; I think this is ready for a review now. Thanks for all the feedback up to this point already 🙂

dbluhm · 2023-03-20T22:51:46Z

src/ffi/revocation.rs

-            Some(revoked.as_slice().iter().map(|r| *r as u32).collect())
-        } else {
            None
+        } else {
+            Some(revoked.as_slice().iter().map(|r| *r as u32).collect())
        };
        let issued: Option<BTreeSet<u32>> = if issued.is_empty() {
-            Some(issued.as_slice().iter().map(|r| *r as u32).collect())
-        } else {
            None
+        } else {
+            Some(issued.as_slice().iter().map(|r| *r as u32).collect())


I believe this condition was flipped around; if the revoked list was empty, return the list mapped to a BTreeSet, else, return None. So we were always getting back empty revoked lists.

@whalelephant I believe @dbluhm is correct here, but just to double check with you.

@dbluhm @blu3beri you are correct!

berendsliedrecht

LGTM! Might be good is someone from the ACA-Py team gives this a review? Maybe @andrewwhitehead ?

berendsliedrecht · 2023-03-21T10:00:38Z

src/ffi/revocation.rs

-            Some(revoked.as_slice().iter().map(|r| *r as u32).collect())
-        } else {
            None
+        } else {
+            Some(revoked.as_slice().iter().map(|r| *r as u32).collect())
        };
        let issued: Option<BTreeSet<u32>> = if issued.is_empty() {
-            Some(issued.as_slice().iter().map(|r| *r as u32).collect())
-        } else {
            None
+        } else {
+            Some(issued.as_slice().iter().map(|r| *r as u32).collect())


@whalelephant I believe @dbluhm is correct here, but just to double check with you.

berendsliedrecht · 2023-03-21T10:01:17Z

wrappers/python/demo/test.py

+    rev_status_lists,
+)
+
+print(verified)


This can be removed.

dbluhm added 2 commits March 16, 2023 14:58

fix: minor type hint correction in CredDef create

7b20b07

Signed-off-by: Daniel Bluhm <[email protected]>

feat: add bindings and types for rev status lists

0d584c7

Signed-off-by: Daniel Bluhm <[email protected]>

dbluhm force-pushed the feature/python-rev-status-lists branch from 225887e to 0d584c7 Compare March 16, 2023 18:58

feat(pyton): mostly updated

4a019cb

Signed-off-by: blu3beri <[email protected]>

Merge remote-tracking branch 'blu3beri/update-python' into feature/py…

25b8fb9

…thon-rev-status-lists

berendsliedrecht mentioned this pull request Mar 20, 2023

fix: python wrapper type hint tweaks #120

Closed

dbluhm added 3 commits March 20, 2023 14:40

feat: object finalizers and keepalives

6de565b

And some other merge artifact fixes Signed-off-by: Daniel Bluhm <[email protected]>

fix: imports and types

27b6acd

Signed-off-by: Daniel Bluhm <[email protected]>

Merge branch 'main' into feature/python-rev-status-lists

8cd9ee6

dbluhm added 2 commits March 20, 2023 15:05

fix: merge artifact

7f7ba61

Signed-off-by: Daniel Bluhm <[email protected]>

fix: update rev status list list conditions

39c51b3

Signed-off-by: Daniel Bluhm <[email protected]>

dbluhm marked this pull request as ready for review March 20, 2023 22:47

dbluhm commented Mar 20, 2023

View reviewed changes

berendsliedrecht mentioned this pull request Mar 21, 2023

feat(pyton): mostly updated #146

Closed

berendsliedrecht approved these changes Mar 21, 2023

View reviewed changes

berendsliedrecht merged commit 5ac823e into hyperledger:main Mar 23, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Updates for python wrapper adding support for revocation status list changes #145

Updates for python wrapper adding support for revocation status list changes #145

dbluhm commented Mar 16, 2023

andrewwhitehead commented Mar 16, 2023

dbluhm commented Mar 16, 2023

berendsliedrecht commented Mar 17, 2023

dbluhm commented Mar 17, 2023

dbluhm commented Mar 17, 2023

dbluhm commented Mar 20, 2023 •

edited

Loading

dbluhm commented Mar 20, 2023

dbluhm Mar 20, 2023

berendsliedrecht Mar 21, 2023

whalelephant Mar 21, 2023

berendsliedrecht left a comment

berendsliedrecht Mar 21, 2023

berendsliedrecht Mar 21, 2023

Updates for python wrapper adding support for revocation status list changes #145

Updates for python wrapper adding support for revocation status list changes #145

Conversation

dbluhm commented Mar 16, 2023

andrewwhitehead commented Mar 16, 2023

dbluhm commented Mar 16, 2023

berendsliedrecht commented Mar 17, 2023

dbluhm commented Mar 17, 2023

dbluhm commented Mar 17, 2023

dbluhm commented Mar 20, 2023 • edited Loading

dbluhm commented Mar 20, 2023

dbluhm Mar 20, 2023

Choose a reason for hiding this comment

berendsliedrecht Mar 21, 2023

Choose a reason for hiding this comment

whalelephant Mar 21, 2023

Choose a reason for hiding this comment

berendsliedrecht left a comment

Choose a reason for hiding this comment

berendsliedrecht Mar 21, 2023

Choose a reason for hiding this comment

berendsliedrecht Mar 21, 2023

Choose a reason for hiding this comment

dbluhm commented Mar 20, 2023 •

edited

Loading