Hashmap uniformization

[bobozaur]

This PR is somewhat of a continuation to the discussion in #228 .

It allows both FFI and native code to use the services API in an organic manner through the use of generics, particularly referring to instances where there are containers of references in FFI, but in native code the container would be over owned types.

Also, the first commit simplifies the TailsFileReader; some lints were getting triggered and I figured I might as well do that while I was at it. If it's a problem to have both of these things in the same PR I can create a separate one for the first commit and cherry-pick the others in a separate PR.

[bobozaur]

@andrewwhitehead @swcurran The ReferencesMap trait uses GATs to achieve the flexibility of passing in maps over owned/referenced values. I took the liberty of also bumping the Rust toolchain version from 1.64 to 1.65, which is when GATs were stabilized, as otherwise CI would fail.

[andrewwhitehead]

This awkwardness makes me think we should just use an enum such as:

pub enum CowMap<'a, K, V> {
    Empty,
    Ref<HashMap<&'a K, &'a V>>,
    Owned<HashMap<K, V>>,
}

Maybe?

[andrewwhitehead]

Empty also isn't really needed, empty()/default() could return Self::Owned(HashMap<K,V>::new()) which doesn't even allocate.

[bobozaur]

I doubt that'll solve anything though. Even if you accept a CowMap<'a, K, V>, the problem is that when you pass None, there's no K or V specified. Which is why the type of the None, or Option must be explicitly set.

But I agree that it's awkward. I'll think about how to approach it better.

[bobozaur]

I gave this quite some thought and I don't really have a great idea on how to go about it.

Once you enter generic land you have to specify the types used, whether it's for a trait or a wrapper type.

Another approach would be to divide the services functions into ones that accept maps of references, which could even just be local to the FFI, and the other ones meant to be publicly used, which would accept maps of owned types and could reside in the services module. It wouldn't even matter if a wrapper type or trait is used then.

@andrewwhitehead what do you think?

[andrewwhitehead]

If the argument was a concrete type like Option<CowMap<'a, SchemaId, Schema>> then the type for None could be inferred, but you would have to convert hash maps on the way in with something like Some(map.into()).

That said it's starting to look like the best option might be to just clone the objects in the FFI layer, and only accept owned maps in these methods?

[bobozaur]

Having the argument as Option<CowMap<'a, SchemaId, Schema>> seems like leaking an implementation detail to consumers, if you ask me. I wouldn't really like having to do that conversion as a user of the library.

Cloning in the FFI layer would be an option, but I still think that specialized functions might still be nicer overall. Something like:

pub fn verify_presentation_ffi<'a, I>(
    presentation: &Presentation,
    pres_req: &PresentationRequest,
    schemas: &HashMap<&SchemaId, &Schema>,
    cred_defs: &HashMap<&CredentialDefinitionId, &CredentialDefinition>,
    rev_reg_defs: Option<&HashMap<&RevocationRegistryDefinitionId, &RevocationRegistryDefinition>>,
    rev_status_lists: Option<I>,
    nonrevoke_interval_override: Option<
        &HashMap<&RevocationRegistryDefinitionId, HashMap<u64, u64>>,
    >,
) -> Result<bool>
where
    I: IntoIterator<Item = &'a RevocationStatusList> + Clone + std::fmt::Debug,
{
    _verify_presentation(
        presentation,
        pres_req,
        schemas,
        cred_defs,
        rev_reg_defs,
        rev_status_lists,
        nonrevoke_interval_override,
    )
}

pub fn verify_presentation<'a, I>(
    presentation: &Presentation,
    pres_req: &PresentationRequest,
    schemas: &HashMap<SchemaId, Schema>,
    cred_defs: &HashMap<CredentialDefinitionId, CredentialDefinition>,
    rev_reg_defs: Option<&HashMap<RevocationRegistryDefinitionId, RevocationRegistryDefinition>>,
    rev_status_lists: Option<I>,
    nonrevoke_interval_override: Option<
        &HashMap<RevocationRegistryDefinitionId, HashMap<u64, u64>>,
    >,
) -> Result<bool>
where
    I: IntoIterator<Item = &'a RevocationStatusList> + Clone + std::fmt::Debug,
{
    _verify_presentation(
        presentation,
        pres_req,
        schemas,
        cred_defs,
        rev_reg_defs,
        rev_status_lists,
        nonrevoke_interval_override,
    )
}

/// Verify an incoming proof presentation
fn _verify_presentation<'a, T, U, V, I, Z>(
    presentation: &Presentation,
    pres_req: &PresentationRequest,
    schemas: &T,
    cred_defs: &U,
    rev_reg_defs: Option<&V>,
    rev_status_lists: Option<I>,
    nonrevoke_interval_override: Option<&Z>,
) -> Result<bool>
where
    T: ReferencesMap<SchemaId, Schema> + std::fmt::Debug,
    U: ReferencesMap<CredentialDefinitionId, CredentialDefinition> + std::fmt::Debug,
    V: ReferencesMap<RevocationRegistryDefinitionId, RevocationRegistryDefinition>
        + std::fmt::Debug,
    I: IntoIterator<Item = &'a RevocationStatusList> + Clone + std::fmt::Debug,
    Z: ReferencesMap<RevocationRegistryDefinitionId, HashMap<u64, u64>> + std::fmt::Debug,
{
    todo!()
}

[bobozaur]

This would allow type inference to work its magic while not making performance sacrifices like cloning in the FFI layer. And the _ffi functions could really just reside within the FFI layer as that's what they're meant for, so they don't need to be part of the public API.

[andrewwhitehead]

Seems like a reasonable solution. I think in that case _verify_presentation could just be pub(crate), since verify_presentation_ffi would only be called from one place. That said cloning would simplify the code a lot, and I don't think the performance impact would be significant. If the allocator is that slow, it can be replaced, and none of the cloned objects are likely very large.

[bobozaur]

Alright then. I didn't want to suggest cloning because it really only impacts the FFI, not native consumers (which is mainly the reason for this). But I agree that is by far the simplest solution, so I'll go ahead with that since you seem okay with it.