anoncreds-rs integration #1110

mirgee · 2024-01-22T10:15:44Z

A first working implementation of BaseAnoncreds trait using anoncreds-rs. Not final. Will be improved significantly in the forthcoming PRs.

codecov-commenter · 2024-01-24T11:04:43Z

Codecov Report

Attention: 46 lines in your changes are missing coverage. Please review.

Comparison is base (b837fcb) 0.05% compared to head (7061837) 0.05%.

Files	Patch %	Lines
aries/aries_vcx/src/handlers/issuance/issuer.rs	0.00%	10 Missing ⚠️
...s_vcx/src/common/primitives/revocation_registry.rs	0.00%	7 Missing ⚠️
aries/aries_vcx/tests/test_anoncreds.rs	0.00%	6 Missing ⚠️
...vcx/src/protocols/issuance/holder/state_machine.rs	0.00%	5 Missing ⚠️
aries/aries_vcx/tests/test_credentials.rs	0.00%	5 Missing ⚠️
aries/aries_vcx_core/src/errors/mapping_others.rs	0.00%	5 Missing ⚠️
...es_vcx/src/common/proofs/prover/prover_internal.rs	0.00%	3 Missing ⚠️
...vcx/src/common/primitives/credential_definition.rs	0.00%	2 Missing ⚠️
aries/aries_vcx/tests/test_primitives.rs	0.00%	2 Missing ⚠️
...es_vcx/tests/utils/scenarios/proof_presentation.rs	0.00%	1 Missing ⚠️

Additional details and impacted files

@@           Coverage Diff            @@
##            main   #1110      +/-   ##
========================================
- Coverage   0.05%   0.05%   -0.01%     
========================================
  Files        479     479              
  Lines      24338   24366      +28     
  Branches    4367    4374       +7     
========================================
  Hits          13      13              
- Misses     24324   24352      +28     
  Partials       1       1

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

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

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Signed-off-by: Miroslav Kovar <miroslav.kovar@absa.africa>

aries/aries_vcx/src/handlers/issuance/issuer.rs

aries/aries_vcx_core/Cargo.toml

aries/aries_vcx_core/src/anoncreds/mod.rs

aries/aries_vcx_core/src/errors/mapping_anoncreds.rs

Patrik-Stas · 2024-01-30T12:19:00Z

aries/misc/test_utils/src/devsetup.rs

@@ -192,8 +192,14 @@ pub async fn dev_build_featured_anoncreds() -> impl BaseAnonCreds {
        return IndyCredxAnonCreds;


If I compile with both credx and anoncreds flags enabled, by mistake, the build will go through and this function and return credx implementation (while I perhaps only intended to run with anoncreds).

Something like

#[cfg(all(feature = "credx", feature = "anoncreds"))] compile_error!("You can not enable 'credx' and 'anoncreds' features simultaneously. Choose one.");

Then the cfg conditions bellow can maybe also can be simplified by dropping the not part.

We are using --all-features flag in CI, and I am using it myself in my current LSP config. Although this should be avoided if at all possible, I can still change both (and switch feature flags manually), but I think for now, following a priority order of credx, anoncreds-rs, mock in case of "conflicting" feature flags as suggested by the Cargo Book is fine.

Signed-off-by: Miroslav Kovar <miroslav.kovar@absa.africa>

Patrik-Stas

good job 👍 left some comments, nothing big though

Haven't yet covered everything in the anoncreds.rs file

Patrik-Stas · 2024-01-31T14:17:02Z

aries/aries_vcx_core/src/anoncreds/anoncreds.rs

+}
+
+impl Anoncreds {
+    async fn get_wallet_record_value<T>(


Note: Wouldn't mind to make this part of wallet API, not in this PR though.

aries/aries_vcx_core/src/anoncreds/anoncreds.rs

Patrik-Stas · 2024-01-31T14:33:50Z

aries/aries_vcx_core/src/anoncreds/anoncreds.rs

+            None
+        };
+
+        // WTF, anoncreds-rs expects the whole status list just to use the accumulator :|


I understand the sometimes emotional nature of the job, but lets keep the comments assertive in the source code ;-)

How about you create tracking issue in anoncreds-rs repo to improve this?

As discussed, it may be fine to keep the API as thin as possible type-wise. It needs to be carefully considered what changes to anoncreds-rs are necessary. Not my priority right now.

Okay, can we track it? If the API was annoying enough to leave that comment, it's probably worthy of tracking

As I said, at this point, I only know some changes will be necessary (which is obviated by the fact we are using custom fork - hence no need to track IMO), but I don't know which ones. This question will get deserved attention later.

Patrik-Stas · 2024-01-31T17:43:52Z

aries/aries_vcx_core/src/anoncreds/anoncreds.rs

+            .add_wallet_record(
+                CATEGORY_CRED_MAP_SCHEMA_ID,
+                &cred_def_id.0,
+                &json!({"schemaId": schema_id}).to_string(),


I think the record category states explicitly enough what the value is. Not strongly against it, not fan either.

But also: I observe that you are willing to give up backwards compatibility (knowingly or unknowingly). If that would be the case, I wonder if there's other breaking changes we can/want to do?

Also I wonder what backwards non-compatibility implies for migration from credx -> anoncreds. Bit late now so I'll save that thinking for later ^_^

It's not about explicitness, it needs to be a json (again, this code should not exist)

Honest question - how come? Doesn't seem to be json in credx implementation

wallet .add_wallet_record( CATEGORY_CRED_MAP_SCHEMA_ID, &cred_def_id.0, &schema.id().0, None, )

I see, you may be right, I will look into it

aries/aries_vcx_core/src/anoncreds/anoncreds.rs

Patrik-Stas · 2024-01-31T17:51:40Z

aries/aries_vcx_core/src/anoncreds/anoncreds.rs

I think we should keep somewhere comment that we currently only support ISSUANCE_ON_DEMAND IssuanceType, and it's the default

aries/aries_vcx_core/src/anoncreds/anoncreds.rs

Patrik-Stas · 2024-01-31T18:47:25Z

aries/aries_vcx_core/src/anoncreds/anoncreds.rs

+        let rev_reg = cred.rev_reg.as_ref();
+
+        let cred_rev_id =
+            if let (Some(rev_reg_id), Some(rev_reg), Some((_, _, _, _, rev_reg_info, _))) =


Again, I know this is mostly copy-paste, but I'll take opportunity to comment this. We should only be testing one (preferably the input rev_reg_id, as that the initial source of truth as whether we issue revocable or irrevocable credential), not all tree for Some, and throw an error else.
If first of the tuple is Some, but other are None, something must had gone unexpectedly wrong, likely due a bug - so we shouldn't instead silently treat that same way as we treat irrevocable credentials.

Patrik-Stas

✅ Approved from my side, I trust my comments are acknowledged whether they would be addressed here or in the next rounds.

Feel free to merge if you are ready.

Patrik-Stas · 2024-02-01T11:36:45Z

aries/aries_vcx_core/src/anoncreds/anoncreds.rs

+            None
+        };
+
+        // WTF, anoncreds-rs expects the whole status list just to use the accumulator :|


Okay, can we track it? If the API was annoying enough to leave that comment, it's probably worthy of tracking

Patrik-Stas · 2024-02-01T11:40:30Z

aries/aries_vcx_core/src/anoncreds/anoncreds.rs

+        // Otherwise, create cred def
+        let (cred_def, cred_def_priv, cred_key_correctness_proof) =
+            anoncreds::issuer::create_credential_definition(
+                // Schema ID must be just the schema seq no for some reason


Okay, please let's track this somewhere. Can be just single comprehensive issue with a brief list of issues we run into with anoncreds-rs and needs to be looked into.

Patrik-Stas · 2024-02-01T11:42:03Z

aries/aries_vcx_core/src/anoncreds/anoncreds.rs

+            .add_wallet_record(
+                CATEGORY_CRED_MAP_SCHEMA_ID,
+                &cred_def_id.0,
+                &json!({"schemaId": schema_id}).to_string(),


Honest question - how come? Doesn't seem to be json in credx implementation

wallet .add_wallet_record( CATEGORY_CRED_MAP_SCHEMA_ID, &cred_def_id.0, &schema.id().0, None, )

aries/aries_vcx_core/src/anoncreds/anoncreds.rs

Patrik-Stas · 2024-02-01T13:06:06Z

aries/aries_vcx_core/src/anoncreds/anoncreds.rs

+            None,
+            Some(prover_did.did()),


Nitpick: it would be preferable to swap these. Provide first argument entropy of value Some(prover_did), and provide None for the 2nd arg.
I don't know why this became 2 arguments in anoncreds-rs, originally it was prover_did later renamed to entropy to reflect actual purpose of the parameter. But there was a shift from dealing with "prover_did" to more generally "some entropy".

Patrik-Stas · 2024-02-01T13:28:24Z

aries/aries_vcx_core/src/anoncreds/anoncreds.rs

+        });
+        let registry = CryptoRevocationRegistry { accum };
+
+        // let rev_status_list = create_revocation_status_list(


This reverts commit 433ba3c.

mirgee added the skip-ci label Jan 22, 2024

mirgee mentioned this pull request Jan 22, 2024

draft: Feature/anoncreds rs #902

Closed

mirgee force-pushed the feature/anoncreds-rs-ctd branch 5 times, most recently from 3a79dbc to 570f8e8 Compare January 24, 2024 10:41

mirgee removed the skip-ci label Jan 24, 2024

mirgee force-pushed the feature/anoncreds-rs-ctd branch 5 times, most recently from e424f28 to b7f2a91 Compare January 24, 2024 11:04

mirgee force-pushed the feature/anoncreds-rs-ctd branch 2 times, most recently from 5af7353 to 5a3c359 Compare January 24, 2024 11:38

mirgee requested a review from Patrik-Stas January 24, 2024 15:42

WIP

53d3ebc

Signed-off-by: Miroslav Kovar <miroslav.kovar@absa.africa>

Patrik-Stas reviewed Jan 30, 2024

View reviewed changes

mirgee force-pushed the feature/anoncreds-rs-ctd branch from 5a3c359 to 53d3ebc Compare January 31, 2024 07:33

mirgee marked this pull request as ready for review January 31, 2024 12:41

Remove special handling of input error

7061837

Signed-off-by: Miroslav Kovar <miroslav.kovar@absa.africa>

Patrik-Stas requested changes Jan 31, 2024

View reviewed changes

Patrik-Stas approved these changes Feb 1, 2024

View reviewed changes

mirgee merged commit 433ba3c into main Feb 2, 2024
29 checks passed

mirgee deleted the feature/anoncreds-rs-ctd branch February 2, 2024 07:03

mirgee restored the feature/anoncreds-rs-ctd branch February 2, 2024 07:30

mirgee added a commit that referenced this pull request Feb 2, 2024

Revert "anoncreds-rs integration (#1110)"

02cf998

This reverts commit 433ba3c.

mirgee added a commit that referenced this pull request Feb 2, 2024

Revert "anoncreds-rs integration (#1110)"

e177471

This reverts commit 433ba3c.

mirgee deleted the feature/anoncreds-rs-ctd branch February 2, 2024 07:41

This was referenced Feb 2, 2024

anoncreds-rs integration #1119

Merged

Retroactively address code review #1121

Closed

Patrik-Stas mentioned this pull request Feb 9, 2024

draft: refactored primitives interfaces #1042

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

anoncreds-rs integration #1110

anoncreds-rs integration #1110

mirgee commented Jan 22, 2024 •

edited

Loading

codecov-commenter commented Jan 24, 2024 •

edited

Loading

Patrik-Stas Jan 30, 2024

mirgee Jan 31, 2024 •

edited

Loading

Patrik-Stas left a comment

Patrik-Stas Jan 31, 2024

Patrik-Stas Jan 31, 2024

Patrik-Stas Jan 31, 2024

mirgee Feb 1, 2024

Patrik-Stas Feb 1, 2024 •

edited

Loading

mirgee Feb 1, 2024

Patrik-Stas Jan 31, 2024

mirgee Feb 1, 2024 •

edited

Loading

Patrik-Stas Feb 1, 2024

mirgee Feb 1, 2024

Patrik-Stas Jan 31, 2024

Patrik-Stas Jan 31, 2024

Patrik-Stas left a comment •

edited

Loading

Patrik-Stas Feb 1, 2024 •

edited

Loading

Patrik-Stas Feb 1, 2024

Patrik-Stas Feb 1, 2024

Patrik-Stas Feb 1, 2024

Patrik-Stas Feb 1, 2024

		@@ -192,8 +192,14 @@ pub async fn dev_build_featured_anoncreds() -> impl BaseAnonCreds {
		return IndyCredxAnonCreds;

anoncreds-rs integration #1110

anoncreds-rs integration #1110

Conversation

mirgee commented Jan 22, 2024 • edited Loading

codecov-commenter commented Jan 24, 2024 • edited Loading

Codecov Report

Choose a reason for hiding this comment

mirgee Jan 31, 2024 • edited Loading

Choose a reason for hiding this comment

Patrik-Stas left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Patrik-Stas Feb 1, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mirgee Feb 1, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Patrik-Stas left a comment • edited Loading

Choose a reason for hiding this comment

Patrik-Stas Feb 1, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mirgee commented Jan 22, 2024 •

edited

Loading

codecov-commenter commented Jan 24, 2024 •

edited

Loading

mirgee Jan 31, 2024 •

edited

Loading

Patrik-Stas Feb 1, 2024 •

edited

Loading

mirgee Feb 1, 2024 •

edited

Loading

Patrik-Stas left a comment •

edited

Loading

Patrik-Stas Feb 1, 2024 •

edited

Loading