feat: add new Wallet trait, implement for indy wallet #1084

xprazak2 · 2023-12-06T15:00:27Z

Obsoletes #1071 and takes an approach outlined in #1071 (review), meaning:

no associated types
new trait does not expose any implementation-specific types
initial implementation for vdrtools wallet

codecov-commenter · 2023-12-06T15:06:58Z

Codecov Report

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

Comparison is base (41ea29f) 0.05% compared to head (55cbf0a) 0.05%.

Files	Patch %	Lines
...core/src/wallet2/indy_wallet/indy_record_wallet.rs	0.00%	80 Missing ⚠️
...cx_core/src/wallet2/indy_wallet/indy_did_wallet.rs	0.00%	76 Missing ⚠️
aries/aries_vcx_core/src/wallet2/entry_tag.rs	0.00%	26 Missing ⚠️
...ries/aries_vcx_core/src/wallet2/indy_wallet/mod.rs	0.00%	24 Missing ⚠️
aries/aries_vcx_core/src/wallet2/mod.rs	0.00%	14 Missing ⚠️
aries/aries_vcx_core/src/wallet2/utils.rs	0.00%	3 Missing ⚠️
aries/aries_vcx/src/errors/mapping_others.rs	0.00%	1 Missing ⚠️

Additional details and impacted files

@@           Coverage Diff            @@
##            main   #1084      +/-   ##
========================================
- Coverage   0.05%   0.05%   -0.01%     
========================================
  Files        472     478       +6     
  Lines      24109   24334     +225     
  Branches    4336    4370      +34     
========================================
  Hits          13      13              
- Misses     24095   24320     +225     
  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.

aries/aries_vcx_core/src/wallet2/mod.rs

aries/aries_vcx_core/src/wallet2/indy_wallet/mod.rs

aries/aries_vcx_core/src/wallet2/mod.rs

Patrik-Stas · 2023-12-21T15:06:07Z

aries/aries_vcx_core/src/wallet2/indy_wallet/indy_did_wallet.rs

+impl DidWallet for crate::wallet::indy::IndySdkWallet {
+    async fn create_and_store_my_did(
+        &self,
+        seed: &str,


The existing BaseWallet interface has this as seed: Option<&str>. When None is specified, a random seed is generated. In fact, most of the code where create_and_store_my_did is being called passed None.
Is there a reason why, seemingly, you've decided to strip this feature?

I reverted it back, it is Option<&str> now.

I see it as a simpler and safer API design force the client to explicitly choose the RNG and randomness source instead of allowing multiple code paths by hiding an implementation which may unknowing change beneath their feet.

aries/aries_vcx_core/src/wallet2/indy_wallet/indy_did_wallet.rs

aries/aries_vcx_core/src/wallet2/indy_wallet/indy_record_wallet.rs

Patrik-Stas

Left some comments.

However, either way; before we merge this, I would like to see this #1093 passing, proving that vdrtools wallet implementation of the new traits in fact satisfy the contract the rest of the codebase expects.

I suggest to follow order:

get 1093 passing
merge this
merge 1093
merge the askar related implementations

aries/aries_vcx_core/src/wallet2/mod.rs

Patrik-Stas · 2024-01-03T14:29:32Z

aries/aries_vcx_core/src/wallet2/mod.rs

+pub struct UnpackedMessage {
+    pub message: String,
+    pub recipient_verkey: String,
+    pub sender_verkey: Option<String>,
+}
+
+#[cfg(feature = "vdrtools_wallet")]
+impl From<UnpackMessageOutput> for UnpackedMessage {
+    fn from(value: UnpackMessageOutput) -> Self {
+        Self {
+            message: value.message,
+            recipient_verkey: value.recipient_verkey,
+            sender_verkey: value.sender_verkey,
+        }
+    }
+}


why not to reuse UnpackMessageOutput?

Patrik-Stas · 2024-01-03T14:47:20Z

aries/aries_vcx_core/src/wallet2/indy_wallet/indy_did_wallet.rs

Probably the most important comment within this review:

You placed the tests into indy implementations directly, but the tests themselves are not necessarily indy specific - they rely on the DidWallet, RecordWallet traits in general, regardless of the underlying implementations. Am I perhaps missing something?

The current approach would indicate that given you have test test_indy_should_sign_and_verify, you will likewise have test_askar_should_sign_and_verify where the test case would only differ by its setup.
Also, you are using create_indy_test_wallet_handle for all of the tests you have added, which sets up mysql wallet - that's somewhat unexpected, most of the test for sake of simplicity are using the default sqlite version of vdrtools wallet. There's only 1 particular test which specifically aims to verify that mysql works (test_mysql_wallet.rs).

So in my view, solving that boils down to:

Make tests agnostic of underlying implementations - test against interfaces, not specific implementation

For that you can use dev_build_featured_wallet(seed).await; (all existing tests, except for the mysql one, are using this)

Because dev_build_featured_wallet is choosing wallet implementation based on feature flags, it will be easy to use the same test cases for testing askar implementation too

Consequently you shouldn't need create_indy_test_wallet_handle, which sets up mysql wallet, anymore. If it for some reason should stay, it would be nice to deduplicate it with test_mysql_init_issuer_with_mysql_wallet

Sounds good, there is one small complication though. dev_build_featured_wallet cannot really be used in aries_vcx_core, because that means adding test_utils crate as a dependency for aries_vcs_core thus creating a dependency cycle. While cargo initially does not object, having cycles in deps is generally not a good idea and problems start to surface quite rapidly with failing type checks.

I created a custom setup for now, any thoughts on this are appreciated.

As agreed to, we will look into removing aries_vcx_core as test_utils dependency and use the setup code in aries_vcx_core tests directly.

Tracking issue #1101 .

mirgee · 2024-01-05T09:28:04Z

aries/aries_vcx_core/src/wallet2/utils.rs

@@ -0,0 +1,9 @@
+use rand::{distributions::Alphanumeric, Rng};
+
+pub fn random_seed() -> String {


There already is a similar function in test_utils.

Perhaps, but using it means adding test_utils as a dependency for aries_vcx_core and creating dependency cycle aries_vcx_core -> test_utils -> aries_vcx_core.

aries/misc/test_utils/src/devsetup.rs

aries/aries_vcx/src/errors/error.rs

aries/aries_vcx_core/src/wallet2/mod.rs

mirgee · 2024-01-05T10:50:37Z

aries/aries_vcx_core/src/wallet2/indy_wallet/mod.rs

+const WALLET_OPTIONS: &str =
+    r#"{"retrieveType": true, "retrieveValue": true, "retrieveTags": true}"#;
+
+const SEARCH_OPTIONS: &str = r#"{"retrieveType": true, "retrieveValue": true, "retrieveTags": true, "retrieveRecords": true}"#;


I assume these can be associated constants. Not sure but maybe we might want to optimize access by allowing the client specify these.

I see any such optimisations as out of scope for this. These are also pretty implementation-specific options.

mirgee · 2024-01-05T10:53:43Z

aries/aries_vcx_core/src/wallet2/mod.rs

+}
+
+pub enum SearchFilter {
+    JsonFilter(String),


This is a pattern we wanted to avoid by the refactor. The clinet serializing a json just for the implementation to deserialize it, incurring overhead while losing type information.

I agree that using json is unfortunate, but these changes are about introducing a new backward-compatible trait that would allow us a transition to different wallet implementation(s). These changes are not about improving the existing wallet implementation.

Even though I could come up with a new type to use here, it would still need to be turned into json at some point because that is what vdrtools wallet requires and that would just result in more overhead.

mirgee · 2024-01-05T10:55:15Z

aries/aries_vcx_core/src/wallet2/mod.rs

+        seed: Option<&str>,
+        method_name: Option<&str>,


I wonder whether this should be an optional. Every DID creation requires a seed and method name. There should be no arbitrary defaults IMO.

See previous comments

aries/aries_vcx_core/src/wallet2/mod.rs

mirgee · 2024-01-05T11:06:09Z

aries/aries_vcx_core/src/wallet2/entry_tag.rs

+}
+
+#[cfg(feature = "vdrtools_wallet")]
+impl From<HashMap<String, String>> for EntryTags {


Is this conversion (i.e. the order of items in the resulting vec) deterministic?

Apparently, iterator over hash map visits elements in arbitrary order and the iteration is stable if the map is not changed. Are there any requirements on tag ordering?

I am not sure, but superficially seems potentially problematic.

xprazak2 · 2024-01-08T09:11:32Z

@mirgee thank you for a detailed review. I see a lot of suggestions for improvements that would be valuable but they would mean significant differences between BaseWallet and BaseWallet2 which would mean introducing impactful changes to the vdrtools wallet in order to move to the new trait.

While that was initially the plan, it was suggested to keep the new trait as close to the existing one so it would be easier to swap them and then gradually improve the new trait. That is also a reason this PR no longer contains create_did and create_key methods that we initially wanted as an improvement over create_and_store_my_did.

The approach here is to introduce a new, slightly better trait that would maintain a high degree of backward compatibility so it would be relatively easy to replace the old one and new trait not being tailored to a specific wallet implementation. Would you prefer we took a different way?

xprazak2 · 2024-01-08T09:13:36Z

Also, regarding multiple keys for a single DID: Looking at the BaseWallet methods, I do not think the trait supports that behaviour. Is that something that BaseWallet2 should address?

Signed-off-by: Ondrej Prazak <ondrej.prazak@absa.africa>

xprazak2 mentioned this pull request Dec 6, 2023

feat: Add new Wallet trait #1071

Closed

xprazak2 force-pushed the indy-record-wallet branch 6 times, most recently from 7c399a9 to f4f7b5e Compare December 20, 2023 09:12

xprazak2 marked this pull request as ready for review December 20, 2023 13:34

Patrik-Stas reviewed Dec 20, 2023

View reviewed changes

xprazak2 force-pushed the indy-record-wallet branch 2 times, most recently from c572e24 to 550ae5d Compare December 21, 2023 11:10

Patrik-Stas reviewed Dec 21, 2023

View reviewed changes

aries/aries_vcx_core/src/wallet2/indy_wallet/indy_did_wallet.rs Outdated Show resolved Hide resolved

Patrik-Stas reviewed Dec 21, 2023

View reviewed changes

aries/aries_vcx_core/src/wallet2/indy_wallet/indy_did_wallet.rs Outdated Show resolved Hide resolved

Patrik-Stas reviewed Dec 21, 2023

View reviewed changes

aries/aries_vcx_core/src/wallet2/indy_wallet/indy_did_wallet.rs Outdated Show resolved Hide resolved

Patrik-Stas reviewed Dec 21, 2023

View reviewed changes

aries/aries_vcx_core/src/wallet2/indy_wallet/indy_record_wallet.rs Show resolved Hide resolved

Patrik-Stas requested changes Dec 21, 2023

View reviewed changes

xprazak2 force-pushed the indy-record-wallet branch 10 times, most recently from d7199da to 748ab8a Compare January 3, 2024 06:58

Patrik-Stas requested changes Jan 3, 2024

View reviewed changes

Patrik-Stas added the review:level-2 At least 2 approvals required for merge label Jan 3, 2024

xprazak2 force-pushed the indy-record-wallet branch 6 times, most recently from bc3a5c6 to 91c0004 Compare January 4, 2024 15:16

Patrik-Stas previously approved these changes Jan 5, 2024

View reviewed changes

mirgee suggested changes Jan 5, 2024

View reviewed changes

xprazak2 dismissed Patrik-Stas’s stale review via 8cccc79 January 8, 2024 09:09

xprazak2 force-pushed the indy-record-wallet branch from 8cccc79 to 0105b1b Compare January 8, 2024 09:25

This was referenced Jan 9, 2024

Remove aries_vcx_core as dependency of test_utils #1101

Closed

Investigate and potentially remove #![allow(unused_imports)] in devsetup #1102

Closed

feat: add new Wallet trait, implement for indy wallet

c2d7e0e

Signed-off-by: Ondrej Prazak <ondrej.prazak@absa.africa>

xprazak2 force-pushed the indy-record-wallet branch 2 times, most recently from 1cf5beb to f85aedc Compare January 12, 2024 08:48

add minor improvements for entry tags

55cbf0a

Signed-off-by: Ondrej Prazak <ondrej.prazak@absa.africa>

xprazak2 force-pushed the indy-record-wallet branch from f85aedc to 55cbf0a Compare January 12, 2024 08:51

mirgee approved these changes Jan 12, 2024

View reviewed changes

Patrik-Stas approved these changes Jan 12, 2024

View reviewed changes

Patrik-Stas merged commit 88541f2 into main Jan 12, 2024
28 checks passed

Patrik-Stas deleted the indy-record-wallet branch January 12, 2024 10:48

mirgee mentioned this pull request Jan 29, 2024

feat: replace BaseWallet with BaseWallet2 #1093

Merged

Patrik-Stas mentioned this pull request Feb 9, 2024

draft: refactored primitives interfaces #1042

Closed

Patrik-Stas mentioned this pull request Mar 14, 2024

Typed interfaces/traits for BaseWallet #814

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add new Wallet trait, implement for indy wallet #1084

feat: add new Wallet trait, implement for indy wallet #1084

xprazak2 commented Dec 6, 2023

codecov-commenter commented Dec 6, 2023 •

edited

Loading

Patrik-Stas Dec 21, 2023

xprazak2 Dec 28, 2023

mirgee Jan 9, 2024

Patrik-Stas left a comment •

edited

Loading

Patrik-Stas Jan 3, 2024

Patrik-Stas Jan 3, 2024 •

edited

Loading

xprazak2 Jan 4, 2024

mirgee Jan 5, 2024

mirgee Jan 9, 2024

mirgee Jan 5, 2024

xprazak2 Jan 8, 2024

mirgee Jan 5, 2024

xprazak2 Jan 8, 2024

mirgee Jan 5, 2024

xprazak2 Jan 8, 2024

mirgee Jan 5, 2024

xprazak2 Jan 8, 2024

mirgee Jan 5, 2024

xprazak2 Jan 8, 2024

mirgee Jan 9, 2024

xprazak2 commented Jan 8, 2024

xprazak2 commented Jan 8, 2024

		@@ -0,0 +1,9 @@
		use rand::{distributions::Alphanumeric, Rng};

		pub fn random_seed() -> String {

feat: add new Wallet trait, implement for indy wallet #1084

feat: add new Wallet trait, implement for indy wallet #1084

Conversation

xprazak2 commented Dec 6, 2023

codecov-commenter commented Dec 6, 2023 • edited Loading

Codecov Report

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

Choose a reason for hiding this comment

Patrik-Stas Jan 3, 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

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

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

xprazak2 commented Jan 8, 2024

xprazak2 commented Jan 8, 2024

codecov-commenter commented Dec 6, 2023 •

edited

Loading

Patrik-Stas left a comment •

edited

Loading

Patrik-Stas Jan 3, 2024 •

edited

Loading