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

Support multiple IOTA networks in the Resolver #1304

Merged
merged 13 commits into from
Feb 21, 2024

Conversation

abdulmth
Copy link
Contributor

Description of change

Add the method attach_multiple_iota_handlers to the Resolver which allows using multiple clients depending on the network the client uses.

Having the input as a vec of tuple ([network name], Client) instead of only clients, is to avoid async network calls just to have the network name. The use can do that manually to have better transparency.

I think this approach is fine. But it might be improved in 2.0 since we can have breaking changes.

@abdulmth abdulmth requested a review from a team as a code owner February 14, 2024 16:47
@abdulmth abdulmth self-assigned this Feb 14, 2024
@abdulmth abdulmth added Added A new feature that requires a minor release. Part of "Added" section in changelog Rust Related to the core Rust code. Becomes part of the Rust changelog. labels Feb 14, 2024
@abdulmth abdulmth added this to the v1.2 milestone Feb 14, 2024
Copy link
Contributor

@UMR1352 UMR1352 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice job tackling this one!

Comment on lines 306 to 332
pub fn attach_multiple_iota_handlers<CLI>(&mut self, clients: Vec<(&'static str, CLI)>)
where
CLI: IotaClientExt + Send + Sync + 'static,
{
let arc_clients: Arc<Vec<(&str, CLI)>> = Arc::new(clients);

let handler = move |did: IotaDID| {
let future_client = arc_clients.clone();
async move {
let did_network = did.network_str();
let client: &CLI = future_client
.as_ref()
.iter()
.find(|(netowrk, _)| *netowrk == did_network)
.map(|(_, client)| client)
.ok_or(crate::Error::new(ErrorCause::UnsupportedNetowrk(
did_network.to_string(),
)))?;
client
.resolve_did(&did)
.await
.map_err(|err| crate::Error::new(ErrorCause::HandlerError { source: Box::new(err) }))
}
};

self.attach_handler(IotaDID::METHOD.to_owned(), handler);
}
Copy link
Contributor

@UMR1352 UMR1352 Feb 15, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Make the input generic over the actual collection, use an HashMap instead of checking all entries one by one

Suggested change
pub fn attach_multiple_iota_handlers<CLI>(&mut self, clients: Vec<(&'static str, CLI)>)
where
CLI: IotaClientExt + Send + Sync + 'static,
{
let arc_clients: Arc<Vec<(&str, CLI)>> = Arc::new(clients);
let handler = move |did: IotaDID| {
let future_client = arc_clients.clone();
async move {
let did_network = did.network_str();
let client: &CLI = future_client
.as_ref()
.iter()
.find(|(netowrk, _)| *netowrk == did_network)
.map(|(_, client)| client)
.ok_or(crate::Error::new(ErrorCause::UnsupportedNetowrk(
did_network.to_string(),
)))?;
client
.resolve_did(&did)
.await
.map_err(|err| crate::Error::new(ErrorCause::HandlerError { source: Box::new(err) }))
}
};
self.attach_handler(IotaDID::METHOD.to_owned(), handler);
}
pub fn attach_multiple_iota_handlers<CLI, I>(&mut self, clients: I)
where
CLI: IotaClientExt + Send + Sync + 'static,
I: IntoIterator<Item = (&'static str, CLI)>,
{
let clients = Arc::new(clients.into_iter().collect::<HashMap<_, _>>());
let handler = move |did: IotaDID| {
let clients = clients.clone();
async move {
let did_network = did.network_str();
let client: &CLI = clients
.get(did_network)
.ok_or(crate::Error::new(ErrorCause::UnsupportedNetwork(
did_network.to_string(),
)))?;
client
.resolve_did(&did)
.await
.map_err(|err| crate::Error::new(ErrorCause::HandlerError { source: Box::new(err) }))
}
};
self.attach_handler(IotaDID::METHOD.to_owned(), handler);
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think collecting the elements back into a Hashmap defeats the purpose of using generics here, and I prefer the cleaner function signature since users will most likely create the vector specifically for this function, Also the hash map will not bring any measurable performance benefits since it's very unlikely that the user will add more than 2 or 3 clients.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree that using an HashMap won't result in a performance improvement (probably it deteriorates it if the number of entries is very small) but I still think it is the right collection for the job as it has an implicit "lookup" implication - which is what the method does. I also agree that users will probably simply use a Vec 99% of the time but it doesn't hurt to account for other possible collections. I'm down for whatever you choose

identity_resolver/src/resolution/resolver.rs Outdated Show resolved Hide resolved
identity_resolver/src/error.rs Outdated Show resolved Hide resolved
@eike-hass
Copy link
Collaborator

eike-hass commented Feb 15, 2024

to avoid async network calls just to have the network name.. The user can do that manually to have better transparency.

As I understand it the network info is being cached, so if the user initialized the client before it should already be cached. If they didn't, we would need to wait for the info, but would be sure that the client can reach their node.
The only reason I think explicitly naming the network would be advantageous is to detect misconfiguration (user wanted shimmer mainnet, but passed a testnet client), but to detect that we would need to also resolve the network info as well.
What do you guys think?

@UMR1352
Copy link
Contributor

UMR1352 commented Feb 15, 2024

@eike-hass that would be ideal, but the network info needs to be fetched asynchronously. We'd have to change the method to an async one in order to check that the couple (network, client) is valid.

@abdulmth
Copy link
Contributor Author

abdulmth commented Feb 15, 2024

Whether it's cached or not, getting the network name is async, and it can throw all the IO errors, which is probably not expected in such an attach method, and as Enrico said the method needs to be async. Also misconfiguration can be easily detected with one resolution on that network, or avoided by using client.get_network_name to set the network name in the tuple.

@eike-hass
Copy link
Collaborator

eike-hass commented Feb 15, 2024

Understood, but as we newly introduce the method, we could make it async, no? The argument to keep it consistent with other attach_handler calls is very valid. Might be a case to make all async with a breaking update in the future though.

@eike-hass
Copy link
Collaborator

Also misconfiguration can be easily detected with one resolution on that network, or avoided by using client.get_network_name to set the network name in the tuple.

Can we demonstrate this in an example?

@eike-hass
Copy link
Collaborator

Also misconfiguration can be easily detected with one resolution on that network, or avoided by using client.get_network_name to set the network name in the tuple.

Can we demonstrate this in an example?

As discussed out-of-band an example is not needed

Co-authored-by: Eike Haß <eike-hass@web.de>
@abdulmth abdulmth merged commit 41105e5 into main Feb 21, 2024
11 checks passed
@abdulmth abdulmth deleted the feat/attch-multiple-iota-handlers branch February 21, 2024 14:49
lucagiorgino pushed a commit to lucagiorgino/identity.rs that referenced this pull request Feb 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Added A new feature that requires a minor release. Part of "Added" section in changelog Rust Related to the core Rust code. Becomes part of the Rust changelog.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Request] Update Resolver to work with multiple networks for the same method
3 participants