ockam
ockam copied to clipboard
build-trust
Create custom async Clone trait
Standard Clone trait is a little ambiguous in sense that one usually expects to have a clone of existing object, while this is not true for our Handle-based and Handle-like structs (e.g. Entity, TcpRouterHandle, VaultSync), even though we implement Clone for them. During Clone these structs create new object which allows you to interact with some shared resource, that is not cloned. Standard library also does things like that, e.g. Arc. In addition to that, our Clone is usually async and may potentially return an Error, these 2 properties are not supported by std::clone::Clone. Having our own Clone trait could solve those problems, we also may implement std::clone::Clone for structs implementing our Clone, that implementation may block and wait for async function to finish, and panic in case error was returned.
Examples of usage: https://github.com/ockam-network/ockam/blob/develop/implementations/rust/ockam/ockam_entity/src/lib.rs#L46 https://github.com/ockam-network/ockam/blob/develop/implementations/rust/ockam/ockam_vault_sync_core/src/vault_sync.rs#L50
I currently have an AsyncClone trait in the antoinevg/nonblocking branch:
https://github.com/ockam-network/ockam/blob/cdf5b781edaa3f8f2533219227d5ebac3eb60036/implementations/rust/ockam/ockam_core/src/lib.rs#L49-L59
Example:
https://github.com/ockam-network/ockam/blob/cdf5b781edaa3f8f2533219227d5ebac3eb60036/implementations/rust/ockam/ockam_vault_sync_core/src/vault_sync.rs#L53-L64
@antoinevg Ok, so it looks like this switch is mandatory for your no-std work? What do you think if return type is ockam_core::Result<Self> ?
ockam_core::Result would be great, there are far too many calls to .unwrap() in my implementation right now!
Hi @SanjoDeundiak, is this issue taken up by someone else? If not, I would love to work on this.
Great! So, can someone assign this issue to me? Or, will this comment be sufficient?
Hi @SanjoDeundiak, I'm currently reading through @antoinevg's nonblocking branch, trying to see how they are using their AsyncClone trait to get a better idea of how the custom Clone trait will be used.
Would I be correct in assuming that we need something similar, but it should also return ockam_core::Result and should panic! when necessary ?
It probably shouldn't panic if at all avoidable, ideally, it should return a ockam_core::Result where the existing code panics or unwraps. I think usually there's already a Result::Err that it has which is triggering the panic anyway, so it should just return that.
The rough idea of a possible implementation might be:
#[async_trait]
pub trait AsyncTryClone {
async fn async_try_clone(&self) -> ockam_core::Result<Self>;
}
I think for now we should punt on whether or not to do anything fancier with it, like supporting other error types, adding a blanket impl for T: Clone, or anything along those lines. This is simpler and those probably would cause more issues than they'd help.
If you don't know where to use it or what to implement it for, it might be enough to submit the PR defining it as a first step (and that might be enough on its own). I'm not sure if @SanjoDeundiak had concrete plans there (I imagine there are a few, although I'm unsure where), but I also think that several of the places where it's useful might only exist in @antoinevg's branch (although I could be mistaken).
That said, if you've already found places it would be useful as-is (from looking at @antoinevg's branch for example), I think you can feel free to implement those with the PR.
Tnx @thomcc. This looks like perfect trait declaration at the moment. @whereistejas we want to land nonblocking branch soon, could you please submit PR, so we could start using this trait? If you also want, there are existing places in develop branch, where you can already integrate new trait (Just search for Clone impls, you'll find few with blocking and unwraps inside)
@SanjoDeundiak sure! I will try to finish it by tomorrow and submit a PR. @thomcc thanks for the detailed comment, I think you have done most of the work, already!
