Mostly typed interface/trait for BaseLedger

[gmulhearn]

Similar to #814

Context

In aries_vcx_core/src/ledger/base_ledger.rs a trait BaseLedger is defined. When BaseLedger 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 BaseLedger is well integrated, we should go back and properly add types to the BaseLedger interface.

Requirements

• Most* (*see scope below) JSON strings inputs and return types in BaseLedger should be replaced with rust structs.
• implementations of BaseLedger (currently justIndySdkLedger and IndyVdrLedger) should be updated to use the new types and translate them appropriately
• consumers of BaseLedger in the code base should be updated to use the new types rather than the JSON strings
• methods with json in the name signature can be renamed to remove the json. e.g.: get_rev_reg_def_json -> get_rev_reg_def

Methods in Scope

Some methods that return raw request JSONs are too complex to be typed just yet. As such, this issue is only focused on typing the following methods:

• get_schema
• get_cred_def
• get_rev_reg_def_json
• get_rev_reg_delta_json
• get_rev_reg

Examples:

Example 1:

take this method;

    async fn get_schema(&self, schema_id: &str, submitter_did: Option<&str>) -> VcxCoreResult<String>;

this returns a JSON string of the following format:

    // {
    //     id: identifier of schema
    //     attrNames: array of attribute name strings
    //     name: Schema's name string
    //     version: Schema's version string
    //     ver: Version of the Schema json
    // }

a new type should be defined; pub struct Schema and given a structure which represents this JSON (e.g. see derived from libvdrtools):

#[derive(Debug, Serialize, Deserialize, Clone)]
#[serde(rename_all = "camelCase")]
pub struct Schema {
    pub id: String,
    pub name: String,
    pub version: String,
    #[serde(rename = "attrNames")]
    pub attr_names: AttributeNames,
    pub ver: String // "1"
}

Tips

Since IndySdkLedger is the primary implementation of BaseLedger currently, it's easiest to drill into the libvdrtools implementations for information about what the underlying JSON type should be. The information is typically contained in the string doc above the vdrtools method. Some methods are implemented by IndyVdrLedger too, which can be used to extract type information.

[gmulhearn]

@Patrik-Stas unsure if this would be a good first issue or if the types are too complex?? any thoughts? It might be fine

[Patrik-Stas]

I think it's alright, you wrote really nice description, but before anyone dives into this, one thing we should clarify:

• should new types be defined? I feel like we should reuse indy-data-types crate - like in case of the example you provided, in the indy-vdr-ledger implementation of get_schema, you are already constructing

        let schema = SchemaV1 {
            id: schema_id,
            name: name.to_string(),
            version: version.to_string(),
            attr_names,
            seq_no,
        };

So perhaps SchemaV1 is what get_schema should return, therefore I'd expect the refactoring to be trivial for modular libs components, and slightly more challenging for vdrtools implementations.

What do you think?

[gmulhearn-anonyome]

hmmm, using indy-data-types crate would force consumers to always depend on indy-data-types, I was thinking one of the main goals of aries_vcx_core is to not force any implementation details on the consumer.

Redefining your own types is a pattern I've seen in AFJ; E.g. this interface is equivalent to some parts of our BaseLedger:
https://github.com/hyperledger/aries-framework-javascript/tree/main/packages/anoncreds/src/services/registry

[gmulhearn-anonyome]

A part that I'm struggling a bit with is how complicated the structures really need to be. indy-data-types complicates things by putting "Schema" in an enum and having an enum variant of SchemaV1 so that they can majorly change the Schema shape in the future if they want... But I'm not sure if we necessarily need to do the same.

AFJ keeps their schema structure simple;

export interface AnonCredsSchema {
  issuerId: string
  name: string
  version: string
  attrNames: string[]
}

And the more recent anoncreds-rs defines it's own data type for schema too, which simplifies the structure as well:

#[derive(Debug, Clone, Serialize, Deserialize)]
#[serde(rename_all = "camelCase")]
pub struct Schema {
    pub name: String,
    pub version: String,
    pub attr_names: AttributeNames,
    pub issuer_id: IssuerId,
}

#[derive(Debug, Clone, Serialize, Deserialize, Default)]
pub struct AttributeNames(pub HashSet<String>);

[gmulhearn-anonyome]

btw fair enough blocking #814 for a resolution on this typing re-using idea ^. But I'm unsure if anyone exports types for wallet structures.

There isn't many types in BaseWallet, and they aren't anyway near the complexity of ledger and anoncreds types, so I think it would make sense if aries_vcx_core owned the BaseWallet types, even if reusing types is a possibility.

[Patrik-Stas]

@gmulhearn

I was thinking one of the main goals of aries_vcx_core is to not force any implementation details on the consumer.

I wouldn't consider this "forcing implementation details", it's just data models. Currently we only deal with indy, so if you were to create our own returns types for these interfaces, we could literally just copy-paste content of indy-data-types, which seems redundant.

in in AFJ; E.g. this interface is equivalent to some parts of our BaseLedger:
https://github.com/hyperledger/aries-framework-javascript/tree/main/packages/anoncreds/src/services/registry

Not quite, I believe what's happening there is they are a step ahead of us terms of the interface for ledger interaction. The code you linked seem to return more general structures returned by "anoncreds resolver", similar to "did resolver" - have a look at https://hyperledger.github.io/anoncreds-methods-registry/. It's same idea like DID Methods, but for schemas/cred-defs/revocation-registries. You can have revocation registry on Hedera blockchain for example.
But since for now we have yet just indy blockchain specific interface, I think reusing indy-data-types code is reasonable - unless there'd be some unexpected complications arising

[Patrik-Stas]

@gmulhearn but eventually we would end up following similar approach like AFJ you linked, something like

// probably anoncreds-rs will be reasonable place to have this structure contained in the future (if already isn't there)
// This must match anoncreds spec https://hyperledger.github.io/anoncreds-methods-registry/
struct AnoncredsSchema {
	...
}

// renamed BaseLedger to AnoncredsLedger
pub trait AnoncredsLedger: std::fmt::Debug + Send + Sync {
   async fn get_schema(&self, schema_id: &str, submitter_did: Option<&str>) -> VcxCoreResult<AnoncredsSchema> { 
}


impl AnoncredsLedger for IndyVdrLedger {
	async fn get_schema(&self, schema_id: &str) -> VcxCoreResult<AnoncredsSchema> {
       // connect to ledger and make sure to return the structure as per spec
       // IDK, but possibly Indy ledger returned schema might look slightly different than AnoncredsSchema per its spec
	}

}

impl AnoncredsLedger for VdrToolsLedger {
	async fn get_schema(&self, schema_id: &str) -> VcxCoreResult<AnoncredsSchema> {
}

impl AnoncredsLedger for HederaHashgrapLedger {
	async fn get_schema(&self, schema_id: &str) -> VcxCoreResult<AnoncredsSchema> {
}

That would be important step toward https://github.com/hyperledger/aries-vcx/blob/main/docs/ROADMAP_2023.md#ledger-agnosticity
and I think we can even pull that of in 2023 but I wouldn't do that right away now, simply typing the Indy specific interfaces is reasonable scope for good first issue

[gmulhearn]

Oh nice, yeah I think I agree. Currently thinking I'd prefer copy pasting indy-data-types structs, rather than having them exported/used from the real indy-data-types crate.

Let me think about it a little more, but it would be ideal if aries_vcx_core has a very small dependency tree (when no features are enabled).

[gmulhearn]

I spoke with @Patrik-Stas further about BaseWallet types. Given that they are quite simple types and that there possibly isn't any types we could re-use/export anyway, the BaseWallet typing work can go ahead without concern for the discussion happening here

[bobozaur]

I'd actually like to pick this up next in the upcoming days. The idea would be to have a look at all these traits and use stronger typing for their arguments/return values.

I think the way to go about it would be through associated types which would be declared when one of the traits is implemented on a specific type. This way, the trait itself is not tied to any implementation, while something like IndyLedger will be tied to the indy types, and so on.

I'll be drafting something soon enough so I can paint a clearer picture.

[gmulhearn]

Kk, awesome. I would be keen to hear what the approach will be.

Sounds like it could be way over-complicating things.. but I'll hear you out! Haha

[Patrik-Stas]

This has been implemented recently during anoncreds-rs integration. Example of the trait fn signatures now:

    async fn get_schema(
        &self,
        schema_id: &SchemaId,
        submitter_did: Option<&Did>,
    ) -> VcxCoreResult<Schema>;
    async fn get_cred_def(
        &self,
        cred_def_id: &CredentialDefinitionId,
        submitter_did: Option<&Did>,
    ) -> VcxCoreResult<CredentialDefinition>;