refactor: services function signatures improvements

[bobozaur]

This PR is based on #226 .

Small improvements to the services module function signatures aiming to make it easier for native consumers to use the library.

Main goal was to get rid of TryInto<T, Error = ValidationError> like arguments because that prevented consumers from passing T as an argument (because TryFrom<T> for T has an Infallible error type).

There still are arguments like &HashMap<&SchemaId, &Schema> which aren't really straight-forward to fix due to the implications this has in the FFI layer (maps are constructed at runtime from FFI objects references).

Worth looking into would also be issuer::update_revocation_status_list() as it's fairly painful to deal with, but I believe that's really more due to the RevocationRegistryDelta - RevocationStatusList data structures being different and anoncreds-clsignatures-rs using the former.

[swcurran]

Thanks!! Good stuff. I’ve asked a few people to review this.

@bobozaur — can you checking the errors in the lint and tests?

I see you made the changes in the wrappers — thanks. Are there changes in the interface into the wrappers? E.g. is downstream code going to have to be updated for the change? I want to be sure what has to be documented.

[andrewwhitehead]

I think this could use a rebase against main to make the diff easier to read

[bobozaur]

Thanks!! Good stuff. I’ve asked a few people to review this.

@bobozaur — can you checking the errors in the lint and tests?

I see you made the changes in the wrappers — thanks. Are there changes in the interface into the wrappers? E.g. is downstream code going to have to be updated for the change? I want to be sure what has to be documented.

Errors are fixed now. Regarding the wrappers, no downstream changes should have to be made. I removed the issuer ID in some of the lower level function signatures as it was compared to the one in, say, credential definition, and an error would be thrown if they weren't equal.

I figured that we might as well just take it from there than rely on the user to provide the exact same value. The FFI still accepts the issuer_id but doesn't use it, so it might be wise to remove it in the future as it might lead to confusion.

[berendsliedrecht]

Could you squash some of the commits? We can merge it afterwards.

[andrewwhitehead]

Thanks, I think this looks reasonable. It's good to remove the generic parameters from the issuer methods so that they don't get duplicated when used with different input types.

The issue with the HashMap inputs could probably be resolved similarly with a wrapper type containing something like a HashMap<Id, Cow<'a, T>> but I think that's for later.

[bobozaur]

Thanks, I think this looks reasonable. It's good to remove the generic parameters from the issuer methods so that they don't get duplicated when used with different input types.

The issue with the HashMap inputs could probably be resolved similarly with a wrapper type containing something like a HashMap<Id, Cow<'a, T>> but I think that's for later.

Honestly, the most annoying thing is that accepting a generic like impl TryInto<SchemaId, Error = ValidationError is that you can't even pass in an already constructed SchemaId.

The standard library already provides impl From<T> for T which just returns self, and there's impl TryFrom<T> for T where Error = Infallible, but that does not match the signature and is just awkward to work with.

The Cow idea still comes with the same problem, I think. If I have a HashMap<T, U> I'd like to be able to pass in a reference to that (&HashMap<T, U>) and not construct a new map made of references (HashMap<&T, &U>). Using a Cow I still have to do conversions and allocate a new map.

There could be a wrapper to accommodate both hash map types above, but I don't think that's ideal either.

Perhaps a sealed trait would be more elegant? I believe the point of using the HashMap was to allow some look-ups. So if the look-ups would be made part of the trait and the trait would be implemented for both map types then it should all work seamlessly while also avoiding the Into::into() or From::from() calls in consumer code which would be needed if a wrapper type would be used.

I'd argue that monomorphization makes the trait approach even better since, if the argument is passed as a generic with a trait bound, the FFI wouldn't be bothered with the &HashMap<T,U> type and it wouldn't get compiled while native consumers like aries-vcx wouldn't have to compile the HashMap<&T, &U> function signature.

[andrewwhitehead]

Yep that makes sense. I don't think it even needs to be a sealed trait, really. Something like:

pub trait IterMap<'a, K: 'a, V: 'a> {
    type Iter: Iterator<Item = (&'a K, &'a V)>;
    fn iter_map(&'a self) -> Self::Iter;
}

impl<'a, K: 'a, V: 'a> IterMap<'a, K, V> for HashMap<K, V> {
    type Iter = std::collections::hash_map::Iter<'a, K, V>;
    fn iter_map(&'a self) -> Self::Iter {
        self.iter()
    }
}

It looks like HashMap<&K, &V> would require a custom iterator type. It would also be possible to add a lifetime to the Iter associated type, but that's a newer feature and I think it might make it more complicated.

[bobozaur]

Yep that makes sense. I don't think it even needs to be a sealed trait, really. Something like:

pub trait IterMap<'a, K: 'a, V: 'a> {
    type Iter: Iterator<Item = (&'a K, &'a V)>;
    fn iter_map(&'a self) -> Self::Iter;
}

impl<'a, K: 'a, V: 'a> IterMap<'a, K, V> for HashMap<K, V> {
    type Iter = std::collections::hash_map::Iter<'a, K, V>;
    fn iter_map(&'a self) -> Self::Iter {
        self.iter()
    }
}

It looks like HashMap<&K, &V> would require a custom iterator type. It would also be possible to add a lifetime to the Iter associated type, but that's a newer feature and I think it might make it more complicated.

GAT's might be an overkill for this, especially since from what I've seen the idea is to lookup through the map and not neccesarily iterate through it.

I was thinking more of something like:

use std::{borrow::Borrow, collections::HashMap, hash::Hash};

pub trait CustomMap<K, V>
where
    K: Hash + Eq,
{
    fn custom_get<Q>(&self, key: &Q) -> Option<&V>
    where
        for<'a> &'a K: Borrow<Q>,
        K: Borrow<Q>,
        Q: Hash + Eq;
}

impl<K, V> CustomMap<K, V> for HashMap<K, V>
where
    K: Hash + Eq,
{
    fn custom_get<Q>(&self, key: &Q) -> Option<&V>
    where
        for<'a> &'a K: Borrow<Q>,
        K: Borrow<Q>,
        Q: Hash + Eq,
    {
        self.get(key)
    }
}

impl<K, V> CustomMap<K, V> for HashMap<&K, &V>
where
    K: Hash + Eq,
{
    fn custom_get<Q>(&self, key: &Q) -> Option<&V>
    where
        for<'a> &'a K: Borrow<Q>,
        K: Borrow<Q>,
        Q: Hash + Eq,
    {
        self.get(key).copied()
    }
}

fn main() {
    let x = HashMap::from([
        (1, "a".to_owned()),
        (2, "b".to_owned()),
        (3, "c".to_owned()),
    ]);
    let y = x.iter().collect::<HashMap<_, _>>();

    let v = x.get(&3);
    let w = y.get(&3);
}

The trait bounds are mostly copy pasted from the HashMap impl, with the addition of the HKTB, explicitly saying that both T and &T should be borrowable as Q, the lookup key, allowing the trait to be implemented for both map types.

While it wouldn't be required for the trait to be sealed, it might make the idea that the API was designed to only work with these types more explicit.

Other operations could be added to this as needed, even iteration. But that would indeed require GATs, and it gets waaaaay more convoluted:

use std::{borrow::Borrow, collections::HashMap, hash::Hash};

pub trait CustomMap<K, V>
where
    K: std::hash::Hash + Eq,
{
    type Iter<'a>: IntoIterator<Item = (&'a K, &'a V)>
    where
        K: 'a,
        V: 'a,
        Self: 'a;

    fn custom_get<Q>(&self, key: &Q) -> Option<&V>
    where
        for<'a> &'a K: Borrow<Q>,
        K: Borrow<Q>,
        Q: Hash + Eq;

    fn custom_iter(&self) -> Self::Iter<'_>;
}

impl<K, V> CustomMap<K, V> for HashMap<K, V>
where
    K: std::hash::Hash + Eq,
{
    type Iter<'a> = std::collections::hash_map::Iter<'a, K, V>
    where 
        K: 'a, 
        V: 'a, 
        Self: 'a;

    fn custom_get<Q>(&self, key: &Q) -> Option<&V>
    where
        for<'a> &'a K: Borrow<Q>,
        K: Borrow<Q>,
        Q: Hash + Eq,
    {
        self.get(key)
    }

    fn custom_iter(&self) -> Self::Iter<'_> {
        self.iter()
    }
}

type MapFn<'a, K, V> = fn((&'a &'a K, &'a &'a V)) -> (&'a K, &'a V);
type MapRefIter<'a, K, V> = std::collections::hash_map::Iter<'a, &'a K, &'a V>;

impl<K, V> CustomMap<K, V> for HashMap<&K, &V>
where
    K: std::hash::Hash + Eq,
{
    type Iter<'a> = std::iter::Map<MapRefIter<'a, K, V>, MapFn<'a, K, V>> 
    where 
        K: 'a, 
        V: 'a, 
        Self: 'a;

    fn custom_get<Q>(&self, key: &Q) -> Option<&V>
    where
        for<'a> &'a K: Borrow<Q>,
        K: Borrow<Q>,
        Q: Hash + Eq {
        self.get(key).copied()
    }
    
    fn custom_iter(&self) -> Self::Iter<'_> {
        self.iter().map(|(k, v)| (*k, *v))
    }
}

fn main() {
    let x = HashMap::from([
        (1, "a".to_owned()),
        (2, "b".to_owned()),
        (3, "c".to_owned()),
    ]);
    let y = x.iter().collect::<HashMap<_, _>>();

    let v = x.custom_get(&3);
    let w = y.custom_get(&3);

    for (k, v) in x.custom_iter() {}

    for (k, v) in CustomMap::<i32, String>::custom_iter(&y) {}
}

[andrewwhitehead]

I think there would need to be a way to iterate the keys of the map, at least, although maybe the implementation only needs to look up values by key.

[swcurran]

Thanks. I’m assuming the discussion above is more about ideas for future adjustments rather than impacting this PR. And since it is now merged — it has to be...