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.

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

Typed interfaces/traits for BaseWallet #814

Closed
gmulhearn opened this issue Apr 27, 2023 · 12 comments · Fixed by #843
Closed

Typed interfaces/traits for BaseWallet #814

gmulhearn opened this issue Apr 27, 2023 · 12 comments · Fixed by #843
Assignees
Labels
good first issue Good for newcomers

Comments

@gmulhearn
Copy link
Contributor

gmulhearn commented Apr 27, 2023

Context

In aries_vcx_core/src/wallet/base_wallet.rs a trait BaseWallet is defined. When BaseWallet was originally defined, it kept the implementation 'simple' by taking and returning structures in the form of JSON Strings rather than typed rust structs. Now that BaseWallet is well integrated, we should go back and properly add types to the BaseWallet interface.

Requirements

  • All JSON strings inputs and return types in BaseWallet should be replaced with rust structs.
  • implementations of BaseWallet (currently mostly just IndySdkWallet) should be updated to use the new types and translate them appropriately
  • consumers of BaseWallet in the code base should be updated to use the new types rather than the JSON strings

Examples:

Example 1:

take this method;

    async fn add_wallet_record(&self, xtype: &str, id: &str, value: &str, tags_json: Option<&str>)
        -> VcxCoreResult<()>;

tags_json is an optional JSON string that represents a HashMap<String, String>.

so the method should be updated as:

    async fn add_wallet_record(&self, xtype: &str, id: &str, value: &str, tags: Option<HashMap<String,String>>)
        -> VcxCoreResult<()>;

Example 2

    async fn get_wallet_record(&self, xtype: &str, id: &str, options_json: &str) -> VcxCoreResult<String>;

This method takes a JSON string via options_json and returns a JSON string that represents a "wallet record"

they should both be replaced with struct types.

If we drill into the libvdrtools implementation of get_record (drilled into via the IndySdkWallet::get_wallet_record implementation), we get a clue for what these structures would look like:

    /// options_json: //TODO: FIXME: Think about replacing by bitmask
    ///  {
    ///    retrieveType: (optional, false by default) Retrieve record type,
    ///    retrieveValue: (optional, true by default) Retrieve record value,
    ///    retrieveTags: (optional, false by default) Retrieve record tags
    ///  }
    /// #Returns
    /// wallet record json:
    /// {
    ///   id: "Some id",
    ///   type: "Some type", // present only if retrieveType set to true
    ///   value: "Some value", // present only if retrieveValue set to true
    ///   tags: <tags json>, // present only if retrieveTags set to true
    /// }

Tips

Since IndySdkWallet is the primary implementation of BaseWallet currently, it's easiest to drill into the libvdrtools implementations for information about what the underlying JSON type should be.

@gmulhearn gmulhearn added the good first issue Good for newcomers label Apr 27, 2023
@gmulhearn
Copy link
Contributor Author

Unable to assign an assignee, but i believe @tech-bash is covering this ticket!

@tech-bash
Copy link
Contributor

Yes, I will try to cover it in the upcoming days. Thanks @gmulhearn

@tech-bash
Copy link
Contributor

tech-bash commented Apr 27, 2023

I guess @Patrik-Stas can solve the assignee's issue!

@Patrik-Stas
Copy link
Contributor

Hi @tech-bash, you are assigned. Apparently being member of organization is not requirement, but a person have to interact with the repo in certain way (star the repo / submit PR / etc.)

@tech-bash before you dive in, please wait for this #815 (comment) to be clarified.

@tech-bash
Copy link
Contributor

Alright.

@gmulhearn
Copy link
Contributor Author

@tech-bash this issue is not blocked by the other issue. For this issue you should go ahead with implementing your own types as structs where required (nothing has changed from the issue description above)

@tech-bash
Copy link
Contributor

OK

@tech-bash
Copy link
Contributor

tech-bash commented May 4, 2023

@gmulhearn in example 1 that you gave I guess there is nothing new in the updated expression ;), also just to make sure if we would write serde::json::from_str(&tags_json): Options<&str>, would this be valid in our case? Coz I suppose we only need to deserialize the json data for conversion into a rust struct! Or is there something that I am missing?

@gmulhearn
Copy link
Contributor Author

Whoops. I have fixed the example 1, thanks for mentioning

@tech-bash
Copy link
Contributor

Alright, got you!

@gmulhearn
Copy link
Contributor Author

Reopening, as #843 does not completely solve this issue. Still remaining is the typed struct for a WalletRecord item

@gmulhearn gmulhearn reopened this Jun 10, 2023
@Patrik-Stas
Copy link
Contributor

Done in
#1085
#1084

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Good for newcomers
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants