Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: Add new Wallet trait #1071

Closed
wants to merge 1 commit into from
Closed

feat: Add new Wallet trait #1071

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions aries/aries_vcx/src/errors/error.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -148,6 +148,9 @@ pub enum AriesVcxErrorKind {
#[error("Could not parse a value")]
ParsingError,

#[error("Unexpected wallet error")]
WalletUnexpected,

// A2A
#[error("Invalid HTTP response.")]
InvalidHttpResponse,
Expand Down
2 changes: 2 additions & 0 deletions aries/aries_vcx/src/errors/mapping_others.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -151,6 +151,7 @@ impl From<AriesVcxCoreError> for AriesVcxError {
AriesVcxErrorKind::DuplicationMasterSecret
}
AriesVcxCoreErrorKind::DuplicationDid => AriesVcxErrorKind::DuplicationDid,
AriesVcxCoreErrorKind::WalletUnexpected => AriesVcxErrorKind::WalletUnexpected,
AriesVcxCoreErrorKind::LoggingError => AriesVcxErrorKind::LoggingError,
AriesVcxCoreErrorKind::EncodeError => AriesVcxErrorKind::EncodeError,
AriesVcxCoreErrorKind::UnknownError => AriesVcxErrorKind::UnknownError,
Expand Down Expand Up @@ -247,6 +248,7 @@ impl From<AriesVcxError> for AriesVcxCoreError {
AriesVcxErrorKind::DuplicationMasterSecret => {
AriesVcxCoreErrorKind::DuplicationMasterSecret
}
AriesVcxErrorKind::WalletUnexpected => AriesVcxCoreErrorKind::WalletUnexpected,
AriesVcxErrorKind::DuplicationDid => AriesVcxCoreErrorKind::DuplicationDid,
AriesVcxErrorKind::LoggingError => AriesVcxCoreErrorKind::LoggingError,
AriesVcxErrorKind::EncodeError => AriesVcxCoreErrorKind::EncodeError,
Expand Down
3 changes: 3 additions & 0 deletions aries/aries_vcx_core/src/errors/error.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -124,6 +124,9 @@ pub enum AriesVcxCoreErrorKind {
#[error("Attempted to add a DID to wallet when that DID already exists in wallet")]
DuplicationDid,

#[error("Unexpected wallet error")]
WalletUnexpected,

// Logger
#[error("Logging Error")]
LoggingError,
Expand Down
1 change: 1 addition & 0 deletions aries/aries_vcx_core/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@ pub mod global;
pub mod ledger;
pub mod utils;
pub mod wallet;
pub mod wallet2;

pub use indy_ledger_response_parser::ResponseParser;
pub use indy_vdr::config::PoolConfig;
Expand Down
72 changes: 72 additions & 0 deletions aries/aries_vcx_core/src/wallet2/mod.rs
View file
Open in desktop
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

  1. Design a trait, taking askar needs in consideration, albeit not perfect interface, not exposing all of Askar capabilities right off the bat.
  2. Implement the trait for vdrtools wallet first
  3. Update all consuming code (such as aries_vcx) to use the new trait
  4. 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:

  1. Provide migration scripts to migrate vdrtools base wallet to askar implementation
  2. Deprecate/Delete vdrtools implementation
  3. 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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Original file line number Diff line number Diff line change
@@ -0,0 +1,72 @@
use async_trait::async_trait;

use crate::errors::error::VcxCoreResult;

pub enum SigType {
EdDSA,
ES256,
ES256K,
ES384,
}

impl From<SigType> for &str {
fn from(value: SigType) -> Self {
match value {
SigType::EdDSA => "eddsa",
SigType::ES256 => "es256",
SigType::ES256K => "es256k",
SigType::ES384 => "es384",
}
}
}

#[async_trait]
pub trait Wallet: RecordWallet + DidWallet {}

#[async_trait]
pub trait DidWallet {
type DidAttrs;
type CreatedDid;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will this type be different from Did and how?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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;

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't mean Did from libvdrtools

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Which one did you mean?

type DidKey;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will this type be different from PublicKey and how?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't mean PublicKey from libvdrtools

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mirgee what did you mean?

type KeyAttrs;
type FindDidKeyAttrs;

async fn create_key(&self, key_attrs: Self::KeyAttrs) -> VcxCoreResult<()>;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


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

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

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

async fn sign(&self, verkey: &str, msg: &[u8], sig_type: SigType) -> VcxCoreResult<Vec<u8>>;

async fn verify(
&self,
verkey: &str,
msg: &[u8],
signature: &[u8],
sig_type: SigType,
) -> VcxCoreResult<bool>;
}

#[async_trait]
pub trait RecordWallet {
type Record;
type RecordId;
type FoundRecord;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

type SearchFilter;

async fn add_record(&self, record: Self::Record) -> VcxCoreResult<()>;

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

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is the ID of the record being updated here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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<()>;


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

async fn search_record(
&self,
filter: Self::SearchFilter,
) -> VcxCoreResult<Vec<Self::FoundRecord>>;
}
3 changes: 3 additions & 0 deletions aries/misc/legacy/libvcx_core/src/errors/error.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -158,6 +158,9 @@ pub enum LibvcxErrorKind {
#[error("Attempted to add a DID to wallet when that DID already exists in wallet")]
DuplicationDid,

#[error("Unexpected wallet error")]
WalletUnexpected,

// Logger
#[error("Logging Error")]
LoggingError,
Expand Down
1 change: 1 addition & 0 deletions aries/misc/legacy/libvcx_core/src/errors/mapping_from_ariesvcx.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -73,6 +73,7 @@ impl From<AriesVcxErrorKind> for LibvcxErrorKind {
AriesVcxErrorKind::WalletAlreadyOpen => LibvcxErrorKind::WalletAlreadyOpen,
AriesVcxErrorKind::DuplicationMasterSecret => LibvcxErrorKind::DuplicationMasterSecret,
AriesVcxErrorKind::DuplicationDid => LibvcxErrorKind::DuplicationDid,
AriesVcxErrorKind::WalletUnexpected => LibvcxErrorKind::WalletUnexpected,
AriesVcxErrorKind::LoggingError => LibvcxErrorKind::LoggingError,
AriesVcxErrorKind::EncodeError => LibvcxErrorKind::EncodeError,
AriesVcxErrorKind::UnknownError => LibvcxErrorKind::UnknownError,
Expand Down
1 change: 1 addition & 0 deletions aries/misc/legacy/libvcx_core/src/errors/mapping_from_ariesvcxcore.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -85,6 +85,7 @@ impl From<AriesVcxCoreErrorKind> for LibvcxErrorKind {
LibvcxErrorKind::DuplicationMasterSecret
}
AriesVcxCoreErrorKind::DuplicationDid => LibvcxErrorKind::DuplicationDid,
AriesVcxCoreErrorKind::WalletUnexpected => LibvcxErrorKind::WalletUnexpected,
AriesVcxCoreErrorKind::LoggingError => LibvcxErrorKind::LoggingError,
AriesVcxCoreErrorKind::EncodeError => LibvcxErrorKind::EncodeError,
AriesVcxCoreErrorKind::UnknownError => LibvcxErrorKind::UnknownError,
Expand Down
Loading