feat: Add new Wallet trait #1071

xprazak2 · 2023-11-27T14:22:29Z

A new trait for wallet heavily inspired by #1042. Associated types are leveraged to provide flexibility to various implementations.

codecov-commenter · 2023-11-27T14:32:54Z

Codecov Report

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

Comparison is base (37019db) 0.05% compared to head (a80bc06) 0.05%.
Report is 8 commits behind head on main.

Files	Patch %	Lines
aries/aries_vcx_core/src/wallet2/mod.rs	0.00%	7 Missing ⚠️
aries/aries_vcx/src/errors/mapping_others.rs	0.00%	2 Missing ⚠️

Additional details and impacted files

@@           Coverage Diff            @@
##            main   #1071      +/-   ##
========================================
- Coverage   0.05%   0.05%   -0.01%     
========================================
  Files        471     472       +1     
  Lines      24009   24024      +15     
  Branches    4306    4293      -13     
========================================
  Hits          13      13              
- Misses     23995   24010      +15     
  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.

mirgee

Just a couple of observations after a brief skim

mirgee · 2023-11-28T10:44:21Z

aries/aries_vcx_core/src/wallet2/mod.rs

+#[async_trait]
+pub trait DidWallet {
+    type DidAttrs;
+    type CreatedDid;


Will this type be different from Did and how?

Using a custom type is less restrictive, but using Did here would be ok:

async fn create_did(&self, attrs: Self::DidAttrs) -> VcxCoreResult<Did>;

I found out that Did struct actually comes from libvdrtools, so we would need vdrtools_wallet compile flag to use the trait if we make it a return type. Keeping for now as is, let me know if we really want to change it. In the meantime, any implementation can do type CreatedDid = Did;

I didn't mean Did from libvdrtools

Which one did you mean?

mirgee · 2023-11-28T10:44:36Z

aries/aries_vcx_core/src/wallet2/mod.rs

+pub trait DidWallet {
+    type DidAttrs;
+    type CreatedDid;
+    type DidKey;


Will this type be different from PublicKey and how?

It depends on how much restriction we want to place on implementations. Same as above, we could just omit this type and use PublicKey directly.

Similar as above, PublicKey comes from libvdrtools. It also seems that ed25519 is the only type supported.

I didn't mean PublicKey from libvdrtools

@mirgee what did you mean?

mirgee · 2023-11-28T11:00:32Z

aries/aries_vcx_core/src/wallet2/mod.rs

+    type DidKey;
+    type KeyAttrs;
+
+    async fn create_key(&self, key_attrs: Self::KeyAttrs) -> VcxCoreResult<()>;


What will happen if a key with name foo is created twice? What will happen if a key with name foo has an associated DID and a key with name foo is created again?

Not allowing multiple keys with the same name seems like the most sensible solution to me and Askar has a schema constraint that enforces unique names for keys.

But how to handle key creation is actually on the implementation.

mirgee · 2023-11-28T11:02:21Z

aries/aries_vcx_core/src/wallet2/mod.rs

+
+    async fn did_key(&self, did: &str) -> VcxCoreResult<Self::DidKey>;
+
+    async fn replace_did_key(&self, did: &str) -> VcxCoreResult<Self::DidKey>;


What is this supposed to do? If it creates new key, what KeyAttrs will be used to create it?

This should set a new key for did, the signature will need a change.

mirgee · 2023-11-28T11:10:13Z

aries/aries_vcx_core/src/wallet2/mod.rs

+
+    async fn sign(
+        &self,
+        verkey_name: &str,


Currently we just pass around the public key - verkey name is a new concept, introducing may be quite consequential...

Changed to just verkey.

mirgee · 2023-11-28T11:27:33Z

aries/aries_vcx_core/src/wallet2/mod.rs

+
+    async fn verify(
+        &self,
+        vk: &str,


Why not make PublicKey the argument here? I suppose the signature type can be inferred from the key type.

Public key only supports ed25519.

mirgee · 2023-11-28T11:29:02Z

aries/aries_vcx_core/src/wallet2/mod.rs

+pub trait RecordWallet {
+    type Record;
+    type RecordId;
+    type FoundRecord;


Not sure whether FoundRecord and Record will end up being different.

It would be rare, but they might be. Askar allows specifying expiry when creating/updating a record, but expiry is not returned when fetching the record.

mirgee · 2023-11-28T12:09:51Z

aries/aries_vcx_core/src/wallet2/mod.rs

+
+    async fn get_record(&self, id: &Self::RecordId) -> VcxCoreResult<Self::FoundRecord>;
+
+    async fn update_record(&self, update: Self::Record) -> VcxCoreResult<()>;


What is the ID of the record being updated here?

It is hidden behind the Self::Record type - there is no guarantee that different implementations will use the same id type, so we cannot have something like:

async fn update_record(&self, id: &str, update_attrs: Self::Record) -> VcxCoreResult<()>;

mirgee · 2023-11-28T12:16:32Z

aries/aries_vcx_core/src/wallet2/mod.rs

+    async fn search_record(
+        &self,
+        filter: Self::SearchFilter,
+    ) -> VcxCoreResult<BoxStream<VcxCoreResult<Self::FoundRecord>>>;


I am confused about the return type here.

The implementation's SearchFilter contains limit, option, but this function seems to return a single record? Can't multiple records match the filter and if so, what is the behavior if they do?

What is the reasoning behind using BoxStream (and thus hardcoding futures into the signature)?

What is the reasoning behind the Result "nesting"?

This method returns a stream in Result, streams can contain multiple items. Multiple records can match the filter, I would expect all matches to be included in result stream.

This method signature was "borrowed" so I think the idea was to have lazily evaluated search.

When fetching each record lazily, each fetch can fail.

The "eager" variant could simplify the method signature into something like -> VcxCoreResult<Vec<Self::FoundRecord>>

It now simplified.

Signed-off-by: Ondrej Prazak <[email protected]>

Patrik-Stas · 2023-12-01T12:54:28Z

aries/aries_vcx_core/src/wallet2/mod.rs

This is disruptive comment but come to think about it, I am not if it's good idea to use associated types at all after all. I think associated type bring level of generalization we don't need because ultimately we want to end up with Askar implementation, and we definitely want to have stable, singular wallet interface we consume from aries_vcx.

This is obivous statement, but let me still type it out, because it's important - using associated types means that ultimately the implemented interfaces are different. This will imply the code consuming such traits - DidWallet, RecordWallet can not be used interchangeable, without modifying the code using them.
So while we could have 2 implementation of DidWallet something like:

AskarDidWallet, (implementing trait DidWallet(--Askar types--) )

VdrtoolsDidWallet (implementing trait DidWallet(--Vdrtools types--)
Although the the name of the methods are the same, it's not the same interface. Again, I am saying obivous statements but I want to point out the impacts this will have on the upstream consumers (aries_vcx crate).
Because VdrtoolsDidWallet to use AskarDidWallet they are not interchangeable (different argument types, different return types). Crate aries_vcx would need to decide to either work with one or the other. Can't support both at once without either:

feature flags

additional wrapper layer funneling the differences into single interface

Interchangeability of implementations is great property which makes things a lot easier from organizational perspective, as it doesn't require binary switch of consuming code, and therefore:

everyone can switch from one impl to another whenever ready rather than being enforced,

no ultra-long-lived PRs,

less likelyhood of merge conflicts.

I would like to suggest different strategy in multiple phases and steps.

Phase 1 - interchangeability

Design a trait, taking askar needs in consideration, albeit not perfect interface, not exposing all of Askar capabilities right off the bat.

Implement the trait for vdrtools wallet first

Update all consuming code (such as aries_vcx) to use the new trait

Implement the trait for askar

At that point, we have one and only one interface for both askar and vdrtools implementations. The two implementations are interchangeable. It's possible to run integration tests with either wallet implementation.

Phase 2 - migration

This is further down the line so it might flesh out differently, but something along lines:

Provide migration scripts to migrate vdrtools base wallet to askar implementation

Deprecate/Delete vdrtools implementation

Further improve the new trait to expose more of askar capabilities

I am sorry for voicing this concern only now. I think the Bogdan's PR was somewhat overwhelming in different terms (the technicalities) which made me to miss the important implications I am voicing here. The traits with associated types in this simpler form made it easier to see bigger picture.

I would like to also hear @gmulhearn thoughts on the matter, as you've already have some experience with askar too.

I believe I agree with this comment. I'm having a hard time imagining how these associated types would be used by code within the aries_vcx crate. Or perhaps how it would be used by other aries_vcx_core components (such as by Anoncreds trait, which has functions that take in a Wallet impl).

For instance, when anoncreds stores a credential, it'll need to translate a Credential structure into a Wallet::Record. So I suppose Anoncreds trait would maybe need to specify that Wallet::Record satisfies From (or something like that). We might end up loading up traits with where constraints etc. I think this trail of trait constraints is how Bogdan's POC PR got to where it got to. Which is maybe fine depending on what Rust dev you ask.

Maybe I'm just unsure if we need it right now. The types in vdrtools wallet vs askar are fairly similar, they are both based on the same Aries RFC: https://github.com/hyperledger/aries-rfcs/blob/main/concepts/0050-wallets/README.md . An Aries "record" type is mostly already defined IMO. And the search filter is WQL (as specified in that spec).

IMO The biggest problem with the current trait is that we are using JSON strings everywhere 🤮, so this is a great step towards having good types. Anyway, happy to discuss more. Cheers

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

Closing in favour of #1084

xprazak2 force-pushed the wallet-trait-iface branch from f7dee38 to e0f2c25 Compare November 27, 2023 14:27

xprazak2 mentioned this pull request Nov 27, 2023

feat: support aries-askar #1063

Closed

mirgee reviewed Nov 28, 2023

View reviewed changes

Patrik-Stas changed the title ~~Add new Wallet trait~~ feat: Add new Wallet trait Nov 30, 2023

xprazak2 force-pushed the wallet-trait-iface branch 2 times, most recently from 49f542a to 542eac0 Compare November 30, 2023 13:05

feat: add new Wallet trait

a80bc06

Signed-off-by: Ondrej Prazak <[email protected]>

xprazak2 force-pushed the wallet-trait-iface branch from 542eac0 to a80bc06 Compare November 30, 2023 13:48

xprazak2 marked this pull request as ready for review November 30, 2023 13:50

xprazak2 mentioned this pull request Nov 30, 2023

feat: implement RecordWallet for askar #1078

Closed

Patrik-Stas requested changes Dec 1, 2023

View reviewed changes

xprazak2 mentioned this pull request Dec 6, 2023

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

Merged

xprazak2 closed this Dec 6, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Add new Wallet trait #1071

feat: Add new Wallet trait #1071

xprazak2 commented Nov 27, 2023

codecov-commenter commented Nov 27, 2023 •

edited

Loading

mirgee left a comment

mirgee Nov 28, 2023

xprazak2 Nov 29, 2023

xprazak2 Nov 30, 2023

mirgee Dec 1, 2023

xprazak2 Dec 1, 2023

mirgee Nov 28, 2023

xprazak2 Nov 29, 2023

xprazak2 Nov 30, 2023

mirgee Dec 1, 2023

Patrik-Stas Dec 1, 2023

mirgee Nov 28, 2023

xprazak2 Nov 29, 2023

mirgee Nov 28, 2023

xprazak2 Nov 29, 2023

mirgee Nov 28, 2023

xprazak2 Nov 30, 2023

mirgee Nov 28, 2023

xprazak2 Nov 30, 2023

mirgee Nov 28, 2023

xprazak2 Nov 30, 2023

mirgee Nov 28, 2023

xprazak2 Nov 29, 2023

mirgee Nov 28, 2023

xprazak2 Nov 29, 2023

xprazak2 Nov 30, 2023

Patrik-Stas Dec 1, 2023

gmulhearn Dec 2, 2023

xprazak2 commented Dec 6, 2023


		async fn did_key(&self, did: &str) -> VcxCoreResult<Self::DidKey>;

		async fn replace_did_key(&self, did: &str) -> VcxCoreResult<Self::DidKey>;


		async fn get_record(&self, id: &Self::RecordId) -> VcxCoreResult<Self::FoundRecord>;

		async fn update_record(&self, update: Self::Record) -> VcxCoreResult<()>;

feat: Add new Wallet trait #1071

feat: Add new Wallet trait #1071

Conversation

xprazak2 commented Nov 27, 2023

codecov-commenter commented Nov 27, 2023 • edited Loading

Codecov Report

mirgee 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

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

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

Phase 1 - interchangeability

Phase 2 - migration

Choose a reason for hiding this comment

xprazak2 commented Dec 6, 2023

codecov-commenter commented Nov 27, 2023 •

edited

Loading