refactor: use BaseWallet instead of a specific wallet type #1122

xprazak2 · 2024-02-06T15:24:54Z

Replaces the direct usage of IndySdkWallet with BaseWallet trait.

codecov-commenter · 2024-02-06T16:03:48Z

Codecov Report

Attention: Patch coverage is 0% with 327 lines in your changes are missing coverage. Please review.

Project coverage is 0.05%. Comparing base (274f776) to head (3b94256).
Report is 3 commits behind head on main.

Files	Patch %	Lines
...ies_vcx_core/src/wallet/indy/indy_wallet_config.rs	0.00%	98 Missing ⚠️
...ies/misc/wallet_migrator/src/vdrtools2credx/mod.rs	0.00%	38 Missing ⚠️
aries/aries_vcx_core/src/wallet/indy/mod.rs	0.00%	28 Missing ⚠️
...ies_vcx_core/src/wallet/indy/indy_import_config.rs	0.00%	27 Missing ⚠️
aries/misc/test_utils/src/mock_wallet.rs	0.00%	18 Missing ⚠️
...es/aries_vcx_core/src/wallet/base_wallet/record.rs	0.00%	17 Missing ⚠️
...ies_vcx_core/src/wallet/indy/indy_wallet_record.rs	0.00%	15 Missing ⚠️
.../aries_vcx_core/src/wallet/agency_client_wallet.rs	0.00%	12 Missing ⚠️
...s/aries_vcx_core/src/wallet/indy/partial_record.rs	0.00%	11 Missing ⚠️
...aries_vcx_core/src/wallet/indy/all_indy_records.rs	0.00%	10 Missing ⚠️
... and 13 more

Additional details and impacted files

@@           Coverage Diff            @@
##            main   #1122      +/-   ##
========================================
- Coverage   0.05%   0.05%   -0.01%     
========================================
  Files        499     506       +7     
  Lines      25018   25477     +459     
  Branches    4520    4635     +115     
========================================
  Hits          13      13              
- Misses     25005   25464     +459

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/wallet/base_wallet/did_wallet.rs

mirgee · 2024-02-12T14:02:13Z

aries/aries_vcx_core/src/wallet/base_wallet/mod.rs


-    async fn unpack_message(&self, msg: &[u8]) -> VcxCoreResult<UnpackMessageOutput>;
+    async fn all(&self) -> VcxCoreResult<Box<dyn AllRecords + Send>>;


Why custom trait instead of simply implementing an iterator? Why on BaseWallet instead of RecordWallet?

Because I need a custom abstraction for how different wallet implementations fetch all records. vdrtools wallet provides WalletIterator which does not conform to std::iter::Iterator nor to std::async_iter::AsyncIterator.

Moved to RecordWallet.

I asked why it's implemented on BaseWallet instead of RecordWallet, I am not arguing for moving it (or not).

Also, I still don't understand why a new, wallet specific, trait was created instead of just implementing an iterator.

RecordWallet operates only on records, so it was implemented in BaseWallet because it fetches both records and keys. Even though keys are "key records" since everything is stored in the same table. Indy wallet fetches everything with a single method while askar has separate methods for keys and records.

Instead of just implementing an iterator, a wallet specific trait was created because
getting all records for indy gives me WalletIterator which is async and I do not see a way how to implement a sync trait using async function.

I could implement async iterator but that requires bringing in additional dependency.

aries/aries_vcx_core/src/wallet/base_wallet/record.rs

aries/aries_vcx_core/src/wallet/indy/mod.rs

aries/aries_vcx_core/src/wallet/indy/restore_wallet_configs.rs

aries/aries_vcx_core/src/wallet/indy/wallet_credentials.rs

aries/aries_vcx_core/src/wallet/indy/wallet_config.rs

mirgee · 2024-02-12T14:29:31Z

aries/aries_vcx_core/src/wallet/indy/indy_tags.rs

IndyTags don't need to be pub(crate)

@xprazak2 if this is still true, let's mininize the visibility.

If I make them private, how can they be used in indy_record_wallet.rs?

I am not sure, but I think what I meant when posting this comment 3 weeks ago was just that the (crate) modifier was unnecessary as the module is private anyways.

aries/aries_vcx_core/src/wallet/askar/key_method.rs

xprazak2 · 2024-02-27T15:14:53Z

Things that will need to be addressed in the future:

close/delete wallet
import/export wallet

Patrik-Stas

Left a review, I am mostly concerned about usage of trait objects

aries/agents/rust/mediator/src/aries_agent/mod.rs

aries/aries_vcx_core/src/wallet/base_wallet/mod.rs

aries/aries_vcx_core/src/wallet/base_wallet/did_wallet.rs

aries/aries_vcx_core/src/wallet/indy/wallet_config.rs

aries/aries_vcx_core/src/wallet/base_wallet/mod.rs

Patrik-Stas · 2024-03-01T04:49:14Z

aries/aries_vcx_core/src/wallet/indy/indy_tags.rs

@xprazak2 if this is still true, let's mininize the visibility.

aries/aries_vcx_core/src/wallet/indy/mod.rs

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

xprazak2 · 2024-03-06T15:44:52Z

The dynamic dispatch is no longer used.

Patrik-Stas

LGTM ✅ Great job @xprazak2 this is good improvement

xprazak2 force-pushed the gaw branch 10 times, most recently from b500b4d to 21162fc Compare February 7, 2024 12:20

xprazak2 requested review from Patrik-Stas and mirgee February 7, 2024 15:45

xprazak2 force-pushed the gaw branch 2 times, most recently from 2f5be37 to d54fdf7 Compare February 12, 2024 08:33

mirgee suggested changes Feb 12, 2024

View reviewed changes

xprazak2 force-pushed the gaw branch 2 times, most recently from 72ad926 to 4fc6053 Compare February 14, 2024 13:04

xprazak2 mentioned this pull request Feb 20, 2024

Remove the use of aries_vcx_core/wallet/indy/internal from api_global #1115

Closed

xprazak2 force-pushed the gaw branch from 4fc6053 to e52e87d Compare February 27, 2024 10:48

xprazak2 commented Feb 27, 2024

View reviewed changes

aries/aries_vcx_core/src/wallet/askar/key_method.rs Show resolved Hide resolved

xprazak2 force-pushed the gaw branch 8 times, most recently from 6c77fbb to e03f1f4 Compare February 28, 2024 08:36

xprazak2 force-pushed the gaw branch 2 times, most recently from 0d90aff to dd8ae2b Compare February 28, 2024 08:53

Patrik-Stas requested changes Mar 1, 2024

View reviewed changes

xprazak2 force-pushed the gaw branch 5 times, most recently from a54387a to c009c9a Compare March 6, 2024 12:56

refactor: use BaseWallet instead of a specific wallet type, closes #1117

3b94256

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

xprazak2 force-pushed the gaw branch from c009c9a to 3b94256 Compare March 6, 2024 13:17

Patrik-Stas approved these changes Mar 8, 2024

View reviewed changes

Patrik-Stas merged commit 5a72612 into main Mar 11, 2024
39 checks passed

Patrik-Stas deleted the gaw branch March 11, 2024 08:53

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: use BaseWallet instead of a specific wallet type #1122

refactor: use BaseWallet instead of a specific wallet type #1122

xprazak2 commented Feb 6, 2024

codecov-commenter commented Feb 6, 2024 •

edited

Loading

mirgee Feb 12, 2024

xprazak2 Feb 13, 2024

mirgee Feb 16, 2024 •

edited

Loading

xprazak2 Feb 27, 2024

mirgee Feb 12, 2024

Patrik-Stas Mar 1, 2024

xprazak2 Mar 1, 2024

mirgee Mar 1, 2024

xprazak2 Mar 6, 2024

xprazak2 commented Feb 27, 2024

Patrik-Stas left a comment

Patrik-Stas Mar 1, 2024

xprazak2 commented Mar 6, 2024

Patrik-Stas left a comment


		async fn unpack_message(&self, msg: &[u8]) -> VcxCoreResult<UnpackMessageOutput>;
		async fn all(&self) -> VcxCoreResult<Box<dyn AllRecords + Send>>;

refactor: use BaseWallet instead of a specific wallet type #1122

refactor: use BaseWallet instead of a specific wallet type #1122

Conversation

xprazak2 commented Feb 6, 2024

codecov-commenter commented Feb 6, 2024 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mirgee Feb 16, 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

xprazak2 commented Feb 27, 2024

Patrik-Stas left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xprazak2 commented Mar 6, 2024

Patrik-Stas left a comment

Choose a reason for hiding this comment

codecov-commenter commented Feb 6, 2024 •

edited

Loading

mirgee Feb 16, 2024 •

edited

Loading