Added 'names' attribute field parsing in credx #955

bobozaur · 2023-08-24T09:47:51Z

Addresses #948 .

codecov-commenter · 2023-08-24T10:16:24Z

Codecov Report

Merging #955 (ce34c7c) into main (091529f) will decrease coverage by 0.01%.
The diff coverage is 30.23%.

@@            Coverage Diff             @@
##             main     #955      +/-   ##
==========================================
- Coverage   44.47%   44.47%   -0.01%     
==========================================
  Files         417      417              
  Lines       28964    28995      +31     
  Branches     6178     6184       +6     
==========================================
+ Hits        12882    12895      +13     
- Misses      12284    12298      +14     
- Partials     3798     3802       +4

Flag	Coverage Δ
unittests-aries-vcx	`44.47% <30.23%> (-0.01%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Changed	Coverage Δ
aries_vcx_core/src/anoncreds/credx_anoncreds.rs	`0.00% <0.00%> (ø)`
aries_vcx/src/common/anoncreds.rs	`79.51% <76.47%> (-0.79%)`	⬇️

Patrik-Stas

Left some comments, but happy to see this
#927

now fully passing on top of this PR!

Patrik-Stas · 2023-08-24T22:06:26Z

aries_vcx_core/src/anoncreds/credx_anoncreds.rs

-            let _attr_name = requested_val.try_get("name")?;
-            let _attr_name = _attr_name.try_as_str()?;
-            let attr_name = _normalize_attr_name(_attr_name);
+            if let Some(_attr_names) = requested_val.get("names").and_then(|v| v.as_array()) {


I'd suggest to also make sanity check on name/names first - if none, or both are specific, it's invalid proof request (according to spec). Currently if:

both are specified, we don't error but only prioritize names

if none is specified, we fail on line 765 with some error, but I imagine we could do a nicer one (signalling an invalid proof request for reason X).

Patrik-Stas · 2023-08-24T22:10:15Z

aries_vcx_core/src/anoncreds/credx_anoncreds.rs


-            let mut credentials_json = vec![];
+                    let non_revoked = requested_val.get("non_revoked"); // note that aca-py askar fetches from proof_req json


Right, it should use the top level non_revoked value by default, but get override if specified on specific level. And it seems like right now it's only looking on the level, is that right?

aries_vcx/src/common/anoncreds.rs

Patrik-Stas · 2023-08-24T22:20:44Z

aries_vcx_core/src/anoncreds/credx_anoncreds.rs

-            let _attr_name = _attr_name.try_as_str()?;
-            let attr_name = _normalize_attr_name(_attr_name);
+            if let Some(_attr_names) = requested_val.get("names").and_then(|v| v.as_array()) {
+                let _attr_names = requested_val.try_get("names")?.as_array().ok_or_else(|| {


It feels like a code smell because the ok_or_else err handling should be never reached. Perhaps would be cleaner to do a match on tuple of (requested_val.get("names"), requested_val.get("name")) and that would also sort out the invalid cases referred to in the comment above.

aries_vcx_core/src/anoncreds/credx_anoncreds.rs

Patrik-Stas · 2023-08-24T22:28:49Z

aries_vcx_core/src/anoncreds/credx_anoncreds.rs

+                for _attr_name in _attr_names {
+                    let _attr_name = _attr_name.try_as_str()?;
+                    let attr_name = _normalize_attr_name(_attr_name);

-            let mut credentials_json = vec![];
+                    let non_revoked = requested_val.get("non_revoked"); // note that aca-py askar fetches from proof_req json
+                    let restrictions = requested_val.get("restrictions");

-            for (cred_id, credx_cred) in credx_creds {
-                credentials_json.push(json!({
-                    "cred_info": _make_cred_info(&cred_id, &credx_cred)?,
-                    "interval": non_revoked
-                }))
-            }
+                    let credx_creds = self
+                        ._get_credentials_for_proof_req_for_attr_name(restrictions, &attr_name)
+                        .await?;

-            cred_by_attr[ATTRS][reft] = Value::Array(credentials_json);
+                    for (cred_id, credx_cred) in credx_creds {
+                        credentials_json.push(json!({
+                            "cred_info": _make_cred_info(&cred_id, &credx_cred)?,
+                            "interval": non_revoked
+                        }))
+                    }
+                }


Hmm, I haven't checked how this is done in other implementations, but it seems like something might be missing. If verifier specified names: ["name", "surname"] it means that fields from which these fields are selected from are originating from a single credential https://hyperledger.github.io/anoncreds-spec/#presentation-request-example
eg. not mixing "name" from one credential and "surname" from another.

While this code could coincidentally (and in practice currently even likely) work if there's not more alternative credentials in the wallet, it seems like prover is not trying to assure that property.

We don't necessarily have to address this right now, but lets at least create issue to track this.

ye IMO this is the whole point of names; requiring that multiple attributes are presented from the same credential. credentials_json should only be populated with credentials which have ALL the attributes (names) present. would be good to address now rather than letting a new bug in

Aight, tweak done. Can you check if it is sufficient?

Hmm no that doesn't fix it. Consider a request for names: ["a", "b", "c"], and a wallet with cred1 { a, d }, cred2 { b, e }, and cred3 { c, f }.

The tweaked implementation with all_found would result in cred1, cred2 and cred3 being added to credentials_json and included. Where really, none of the credentials satisfy the requirement of having all 3 attribute names (a, b, and c), so the credentials for that referent should be empty.

A credential should only be added to the list if it has ALL the name/names as attributes on the credential, AND it satisfies any restrictions applied to it.

The ideal way to do that IMO is to rewrite _get_credentials_for_proof_req_for_attr_name a bit, such that it only needs to be called once per referent, and it takes in an array of attr_names, and the WQL query will search for credentials which have ALL those attributes (currently it just checks for 1 attr_name passed in).

Anyway, will leave it up to you guys whether that is scope for this PR or for sometime else; as you've stated, the impl is far from perfect and a rewrite is probs best... the original impl was meant to act as a 'base' for the sake of getting it in there, so it's great that you guys are readdress the impl and improving

Okay I had another attempt at fixing this. Your insights were very valuable, thank you! I hope the query composition is fine, but I assume we'll see in the CI.

bobozaur · 2023-08-25T08:31:23Z

@Patrik-Stas Thanks for taking the time to review this. I want to highlight some things:

This was "hastily" done to fix the issue. There are far better ways to handle this.
Most of the implementation, apart from the issuer (which was done by me) was done by @gmulhearn . I'm not all that familiar with all the business logic.
There's a lot of code smell throughout the entire credx implementation. I would not tweak certain things but rather rewrite it all (it's my motto, after all 😆 ).

Given these things, my approach would be:

Get this into a suitable working state (if it's not already) and merge it.
Remove vdrtools anoncreds implementation in favor of credx anoncreds.
Rewrite ledger trait to add strong typing
Rewrite credx anoncreds so that:
- It uses strong typing introduced in the ledger trait refactor and brings its own
- It is readable and maintainable
- It is more performant, mainly due to stronger typing and not needing to serialize/deserialize stuff in every single function call.

For instance, the name or names situation could be handled in an enum type, thus enforcing that a single field of the two are provided. But I'd rather understand all the business logic, get all the prerequisites regarding strong typing done, and then rewrite the credx anoncreds impl rather than patching this single spot in the code pile we have now.

I'd like to hear what you, or the others, think about this roadmap.

gmulhearn · 2023-08-25T13:16:07Z

Roadmap makes sense to me; only passing note i have to make is that we should make sure the interfaces (ledger traits or anoncreds traits) aren't married to credx (e.g. interfaces that expose 3rd party types). It could very well be the case that credx dies and we all move to anoncreds-rs in the soon or distance future.

Patrik-Stas · 2023-08-26T15:40:49Z

@bobozaur sounds good, although I am not sure what would you rewrite about the ledger trait (other than adding typing (which would ideally be the case for all of the aries_vcx_core components) )

Anyway, in scope of this PR I am fine with merge

bobozaur · 2023-08-27T16:32:26Z

@bobozaur sounds good, although I am not sure what would you rewrite about the ledger trait (other than adding typing (which would ideally be the case for all of the aries_vcx_core components) )

Anyway, in scope of this PR I am fine with merge

We could still add the associated types. It looks like @gmulhearn has the same concern so that we don't end up with a very rigid interface that only works with credx.

Signed-off-by: Bogdan Mircea <mirceapetrebogdan@gmail.com>

gmulhearn · 2023-08-27T20:45:23Z

aries_vcx_core/src/anoncreds/credx_anoncreds.rs

+                _ => json!(attrs),
            }
        } else {
-            wql_attr_query
+            json!(attrs)


I think in these cases, we'd still want things to be ANDed together;

I.e. the query is for where 'attr1=="1" && attr2=="1" && attr3=="1" etc'. So I think it's still json({"$and": attrs}).

Either way, these branches are a bit of an edge case. I might make ticket about the wallet queries in this class, we really should be using the Indy-wql crate, or create our own utility for query construction

gmulhearn

lgtm assuming we make another ticket to address a revision to the wql structures

Patrik-Stas requested changes Aug 24, 2023

View reviewed changes

Patrik-Stas previously approved these changes Aug 26, 2023

View reviewed changes

bobozaur added 5 commits August 27, 2023 19:55

Added 'names' attribute field parsing in credx

34366b8

Signed-off-by: Bogdan Mircea <mirceapetrebogdan@gmail.com>

Adjusted 'names' handling in proof req and added test

2c3eeb0

Signed-off-by: Bogdan Mircea <mirceapetrebogdan@gmail.com>

Removed println

fb9cf28

Signed-off-by: Bogdan Mircea <mirceapetrebogdan@gmail.com>

Add cred only if all attrs from 'names' are found

929a12a

Signed-off-by: Bogdan Mircea <mirceapetrebogdan@gmail.com>

Fixed prover_get_credentials_for_proof_req

ce34c7c

Signed-off-by: Bogdan Mircea <mirceapetrebogdan@gmail.com>

bobozaur dismissed Patrik-Stas’s stale review via ce34c7c August 27, 2023 16:55

bobozaur force-pushed the feature/credx_attributes branch from bd34b29 to ce34c7c Compare August 27, 2023 16:55

gmulhearn reviewed Aug 27, 2023

View reviewed changes

Patrik-Stas approved these changes Aug 29, 2023

View reviewed changes

gmulhearn approved these changes Aug 29, 2023

View reviewed changes

mirgee approved these changes Aug 29, 2023

View reviewed changes

mirgee merged commit ce61866 into main Aug 29, 2023
30 checks passed

mirgee deleted the feature/credx_attributes branch August 29, 2023 15:08

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added 'names' attribute field parsing in credx #955

Added 'names' attribute field parsing in credx #955

bobozaur commented Aug 24, 2023

codecov-commenter commented Aug 24, 2023 •

edited

Loading

Patrik-Stas left a comment

Patrik-Stas Aug 24, 2023

Patrik-Stas Aug 24, 2023

Patrik-Stas Aug 24, 2023

Patrik-Stas Aug 24, 2023

gmulhearn Aug 25, 2023 •

edited

Loading

bobozaur Aug 25, 2023 •

edited

Loading

gmulhearn Aug 25, 2023

gmulhearn Aug 25, 2023

bobozaur Aug 27, 2023

bobozaur commented Aug 25, 2023

gmulhearn commented Aug 25, 2023

Patrik-Stas commented Aug 26, 2023 •

edited

Loading

bobozaur commented Aug 27, 2023

gmulhearn Aug 27, 2023 •

edited

Loading

gmulhearn left a comment


		let mut credentials_json = vec![];
		let non_revoked = requested_val.get("non_revoked"); // note that aca-py askar fetches from proof_req json

Added 'names' attribute field parsing in credx #955

Added 'names' attribute field parsing in credx #955

Conversation

bobozaur commented Aug 24, 2023

codecov-commenter commented Aug 24, 2023 • edited Loading

Codecov Report

Patrik-Stas left a comment

Choose a reason for hiding this comment

Patrik-Stas Aug 24, 2023

Choose a reason for hiding this comment

Patrik-Stas Aug 24, 2023

Choose a reason for hiding this comment

Patrik-Stas Aug 24, 2023

Choose a reason for hiding this comment

Patrik-Stas Aug 24, 2023

Choose a reason for hiding this comment

gmulhearn Aug 25, 2023 • edited Loading

Choose a reason for hiding this comment

bobozaur Aug 25, 2023 • edited Loading

Choose a reason for hiding this comment

gmulhearn Aug 25, 2023

Choose a reason for hiding this comment

gmulhearn Aug 25, 2023

Choose a reason for hiding this comment

bobozaur Aug 27, 2023

Choose a reason for hiding this comment

bobozaur commented Aug 25, 2023

gmulhearn commented Aug 25, 2023

Patrik-Stas commented Aug 26, 2023 • edited Loading

bobozaur commented Aug 27, 2023

gmulhearn Aug 27, 2023 • edited Loading

Choose a reason for hiding this comment

gmulhearn left a comment

Choose a reason for hiding this comment

codecov-commenter commented Aug 24, 2023 •

edited

Loading

gmulhearn Aug 25, 2023 •

edited

Loading

bobozaur Aug 25, 2023 •

edited

Loading

Patrik-Stas commented Aug 26, 2023 •

edited

Loading

gmulhearn Aug 27, 2023 •

edited

Loading