Replace ClientMap with new Resolver

[cycraig]

Description of change

Adds the new Resolver API discussed in #422 and #493.

The new Resolver replaces the current ClientMap and is very similar in design. The differences are:

• Resolver does not expose publish_* methods. This is to encourage the use of the Account over the low-level API.
• Resolver does not automatically link to both the Devnet and Mainnet networks (old behaviour of ClientMap). It has a default constructor that creates a Client for the Mainnet but this can be overridden using the ResolverBuilder.
• Resolver contains convenience functions for resolving DID Documents associated with verifiable Credentials and Presentations.

What is not included in this PR?

• Convenience functions for resolving and verifying credentials and presentations at the same time; depends on Overhaul CredentialValidator, add PresentationValidator #599.
• Refactoring the credential and presentations examples; depends on Overhaul CredentialValidator, add PresentationValidator #599.
• The Wasm bindings Client functions for check_credential and check_presentation are not moved to the Resolver; depends on Overhaul CredentialValidator, add PresentationValidator #599.
• Resolver::resolve_with_options from [Task] Implement new Resolver API #493. We can add this but the future of resolving document histories is uncertain and method-dependent. Only version_time/version_datetime would be supported if we implement this right now, as we do not have document versioning.
• This does not alter the resolution module; see issue [Task] Remove old resolution module #532.

Why is this different from the outcome of #422?

As mentioned in #493, it was decided after internal discussions to avoid implementing a compliant resolver API as it is more limiting than the functionality we expose through identity.rs directly. Many of its features like which encoding to accept, for example, are centred around web/API resolution rather than the strongly-typed framework we provide. Universal resolver implementations can still use identity.rs to implement such a resolver quite easily.

Notable Changes

Added

• Add Resolver, WasmResolver.
• Add ResolverBuilder

Removed

• Remove ClientMap.

Open Questions

1. Should we revert to a ClientMap-like interface where we expose helper functions for publishing across different networks?
2. Should we expose ResolverBuilder to the Wasm bindings in order to remove set_client and remove_client from Resolver?

Links to any relevant issues

Fixes issue #493

Type of change

• Bug fix (a non-breaking change which fixes an issue)
• Enhancement (a non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to not work as expected)
• Documentation Fix

How the change has been tested

Several VC examples have been updated to use the Resolver but only in Rust. Will be more prevalent after #599.

Change checklist

• I have followed the contribution guidelines for this project
• I have performed a self-review of my own code
• I have commented my code, particularly in hard-to-understand areas
• I have made corresponding changes to the documentation
• I have added tests that prove my fix is effective or that my feature works
• New and existing unit tests pass locally with my changes

[PhilippGackstatter]

Looks good to me!

I think the DashMap can be replaced with a HashMap. The resolver should be useful enough without post-creation mutability.

[PhilippGackstatter]

Makes a lot of sense to me to setup a resolver once at build-time and then disallow mutation afterwards. We don't have to commit to allowing mutability now, but if the need arises later, adding it is non-breaking.

[cycraig]

Cool, I guess we need the ResolverBuilder in the Wasm bindings then? It was not ported since there is no good way to avoid the builder pattern in Wasm in this case, but it would be the only option if we remove the mutators.

[PhilippGackstatter]

Can we not take a list of ClientOrBuilders in WasmResolver::new, or even just Clients? Or is that the not-good-way you meant 😄?

const resolver = new Resolver([
      Client.fromConfig(Config.fromNetwork(Network.mainnet())),
      Client.fromConfig(Config.fromNetwork(Network.devnet())),
  ]);

Is the problem that someone could provide two clients with the same network name?

[cycraig]

As far as I know there is no way to take Vec<WasmClient> as a parameter (Vec<T> is not supported in general), and if I recall correctly we cannot cast JsValue to one of our exported structs, and since Client and ClientBuilder are not serializable (for good reason), we cannot use a duck-typed Typescript interface either.

I'll try again just to be sure though.

[PhilippGackstatter]

Ah, yeah, my bad, I was thinking along the lines of the serde solution, but of course that doesn't work with Clients. Perhaps a map would work alternatively, where the keys are network names and the values Clients. Using Reflect::own_keys we could iterate the keys and retrieve the clients. That would even prevent duplication.

[cycraig]

I think that returns the keys of the map/object, not the values (Clients)?

https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Reflect/ownKeys#using_reflect.ownkeys

Edit: not the point, I still think the map values will only give JsValue.

[PhilippGackstatter]

Yes, this would actually be pretty ugly typing-wise, as we'd have to follow-up with Reflect::get which only returns a JsValue as you pointed out, and we'd have to downcast that to Client similar to how WasmStorage does that. That should be avoided if possible, though. Exposing the builder seems like the best option after all. Sorry for the half-baked ideas.

[cycraig]

Ideally all this would be avoided if wasm-bindgen supported dyn_ref/dyn_into on exported types, but it will probably never happen: https://rustwasm.github.io/wasm-bindgen/api/wasm_bindgen/trait.JsCast.html#method.dyn_into

Wow that downcasting workaround is scary. I mean it works but there's got to be a better way than relying on the constructor name: rustwasm/wasm-bindgen#2231 (comment)

[cycraig]

Exposed WasmResolverBuilder. Using it looks like this in Javascript:

const client = new Client();

const resolver = await Resolver
        .builder()
        .client(client)
        .clientConfig(Config.fromNetwork(Network.devnet()))
        .build();

[olivereanderson]

Consider the following scenario:
Crate x: depends on identity-iota without the arc feature . In Crate x there is a function

pub fn get_client() -> Option<Rc<Client>> {
  let resolver: Resolver = .. ;
  resolver.get_client()
}

Crate y: depends on identity-iota with the arc feature
Crate z: depends directly on Crate x and has a function
fn foo(client: Rc<Client>);
The following code works in crate z:

let client = crate_x::get_client().unwrap();
foo(client);

but if crate y ever becomes part of crate z's dependency tree (possibly via transitive dependencies) I think crate zwill no longer compile?

This is probably the reason why the im crate comes in two flavours im and im-rc (See https://docs.rs/im/latest/im/#thread-safety) instead of changing the pointer types via feature flags.

We might want to consider using the archery crate and make Resolver generic over the pointer type. Then the new constructor in the WasmResolver can use the Rc variant and in the Rust library we either remove new and require construction through the builder or use an Arc there.

[PhilippGackstatter]

Dependencies are resolved with the union of all features enabled on them. For example, if one package depends on the im package with the serde dependency enabled and another package depends on it with the rayon dependency enabled, then im will be built with both features enabled, and the serde and rayon crates will be included in the resolve graph. If no packages depend on im with those features, then those optional dependencies will be ignored, and they will not affect resolution. https://doc.rust-lang.org/cargo/reference/resolver.html#features

I guess you're right that if our crate is a dependency of two other crates, and one enables arc and the other doesn't, there will be a conflict.

The archery crate looks interesting.

[cycraig]

Great point, I agree that putting arc as a feature isn't ideal since it's not additive and can lead to the situation you described.

I'll try archery for now since we would want to use the same pattern to fix the Account using Arc instead of Rc in Wasm.

[cycraig]

So the problem with archery I've found is that it does not support converting from Arc/Rc to their SharedPointer, nor vice-versa (at least as far as I can tell).

This is a problem because we explicitly use Rc<Client> in WasmClient rather than SharedPointer<Client, RcK> (which the Resolver uses), so it's not possible to convert the Resolver entries into WasmClient.

E.g.

#[wasm_bindgen(js_name = getClient)]
pub fn get_client(&self, network_name: String) -> Option<WasmClient> {
  let network_name: NetworkName = NetworkName::try_from(network_name).ok()?;
  let client: SharedPointer<Client, RcK> = self.0.get_client(&network_name).cloned()?;
  let rc_client: Rc<Client> = client.into(); // not possible with any safe function archery exposes.
  Some(WasmClient { client: rc_client })
}

The reverse also appears not possible.

E.g.

#[wasm_bindgen]
pub fn client(mut self, client: &WasmClient) -> WasmResolverBuilder {
  let client: Rc<Client> = client.client.clone();
  let shared_client: SharedPointer<Client, RcK> = client.into(); // not possible with any safe function archery exposes.
  self.0 = self.0.client(shared_client);
  self
}

Edit: the other problem is requiring dependents to import the archery dependency themselves to switch between ArcK and RcK but that doesn't seem so bad.

[cycraig]

One solution would be to use SharedPointer<Client, RcK> exclusively and replace Rc<Client> in the Wasm bindings, if we want to rely on archery completely?

I don't think this is very friendly to Rust developers though, since I imagine they might want something like a base Arc<Client> eventually.

[cycraig]

This is what I have currently. It works and doesn't require any odd hacks.

We can restrict it using a sealed trait to prevent unexpected entries for T from dependents, but I don't think it's too bad as-is?

pub struct Resolver<T = Arc<Client>>
where
  T: Clone + AsRef<Client> + From<Client>,
{
  client_map: HashMap<NetworkName, T>,
}

// Developers using it in Rust will default to Resolver<Arc<Client>.
let resolver: Resolver = Resolver::new().await?; 

// Changing to Rc is a bit verbose but not bad.
let resolver: Resolver<Rc<Client>> = Resolver::<Rc<Client>>::new().await?;

Using a generic trait with an associated type of Rc or Arc for a generic T (rather than just Client) is another option but unnecessary in our case since we only require it for Client, in my opinion.

[PhilippGackstatter]

I think that's perfect 👍

[cycraig]

Actually, I think you're right that a sealed trait might be better so we can change it without affecting semantic versioning? Not sure.

[olivereanderson]

I think it is nicer without the sealed trait. I can't think of any reason we would want to restrict the generic parameter T any more than what the current trait bounds do.

[cycraig]

Thanks for all the feedback. I'll leave it as-is.

[olivereanderson]

Looks great now! Thanks for the changes.