-
Notifications
You must be signed in to change notification settings - Fork 88
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
Conversation
There was a problem hiding this 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!
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); | ||
} |
There was a problem hiding this comment.
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
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); | |
} |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
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. |
@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 |
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 |
Understood, but as we newly introduce the method, we could make it async, no? The argument to keep it consistent with other |
Can we demonstrate this in an example? |
As discussed out-of-band an example is not needed |
Co-authored-by: Eike Haß <[email protected]>
Description of change
Add the method
attach_multiple_iota_handlers
to theResolver
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.