-
Notifications
You must be signed in to change notification settings - Fork 19
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
Refactor low level clients #20
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.
Thanks for doing all of this! The operations look really good now.
Some nit picks about naming.
Some general comments about our file structure:
- should we keep the
core
folder? I think it makes sense in terms of abstraction: it is the core based on which any higher level client can be built on. - you would normally expect the main structure to be contained in
core/mod.rs
but since ourBasicClient
has no "core" in it and to align it with the other ones, would it be nicer to havebasic_client.rs
as a separate file, like operation and request? - we have
ipc_client
incore
, it might be confusing. Maybe it is worth renaming it justipc
oripc_connectors
?
src/core/mod.rs
Outdated
mod testing; | ||
|
||
use crate::auth::AuthenticationData; | ||
use crate::error::{ClientErrorKind, Error, Result}; | ||
use operation_handler::OperationHandler; | ||
use operation_client::OperationHandler; |
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.
Is OperationHandler
supposed to be OperationClient
as well?
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.
Yep! It's a bit weird naming all of them clients, but alas... it makes more sense to keep this consistent
src/core/mod.rs
Outdated
@@ -190,18 +157,20 @@ impl Provider { | |||
/// in order to figure out if their desired use case and provider are | |||
/// available. | |||
#[derive(Debug)] | |||
pub struct CoreClient { | |||
pub struct BasicClient { | |||
op_handler: OperationHandler, |
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.
Here as well, should it be op_client: OperationClient
?
src/core/mod.rs
Outdated
} | ||
|
||
/// Main client functionality. | ||
impl CoreClient { | ||
impl BasicClient { | ||
/// Create a new Parsec client given the authentication data of the app. |
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.
Maybe this code doc can be expanded to cover how the provider
arg is going to be used.
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.
And maybe renamed implicit_provider
to match with the set_implicit_provider
method
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.
Yep, good point, I tried to update the docs but I forgot about this one!
src/core/mod.rs
Outdated
@@ -190,18 +157,20 @@ impl Provider { | |||
/// in order to figure out if their desired use case and provider are | |||
/// available. |
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.
In this code doc it would be nice also to make the distinction betweem core provider and crypto ones and what it means for the operations. Maybe in the code doc of each operation as well, say if it is a core or a crypto one?
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.
Will try and think of a way to "mark" them somehow in the docs!
src/core/mod.rs
Outdated
/// If the implicit client provider is `ProviderID::Core`, a client error | ||
/// of `InvalidProvider` type is returned. |
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!
src/core/mod.rs
Outdated
&self.auth_data, | ||
)?; | ||
|
||
Ok(()) | ||
} | ||
|
||
fn can_provide_crypto(&self) -> Result<()> { |
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 one
src/core/operation_client.rs
Outdated
#![allow(dead_code)] | ||
|
||
use super::request_handler::RequestHandler; | ||
use super::request_client::RequestHandler; |
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.
and this one RequestClient
as well 😬 ?
src/core/operation_client.rs
Outdated
wire_protocol_version_min: 0, | ||
content_type: BodyType::Protobuf, | ||
accept_type: BodyType::Protobuf, | ||
content_type: Box::from(ProtobufConverter {}), |
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.
Maybe the field name content_converter
is more appropriate?
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.
Yeah, I guess, especially since it's public
src/core/operation_client.rs
Outdated
} | ||
|
||
/// Set the wire protocol version numbers to be used by the client. | ||
/// Set the content type for responses handled by this client. |
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.
"accept" type?
I think so, it makes sense to have the core segregated, especially if we don't want people to use this too much, we can document it accordingly
That makes sense!
Yeah, I noticed that too, will try and think of the proper name for it. |
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.
Thanks, that looks very good!
/// The `implicit_provider` will be used for all non-core operations. Thus, until a | ||
/// different provider ID is set using [`set_implicit_provider`](#method.set_implicit_provider), | ||
/// all cryptographic operations will be executed in the context of this provider (if it is | ||
/// available within the service). |
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, I did not know you could link with methods!
This commit includes a few changes: * remove explicit wire protocol version handling, following a similar change in the interface * add a provider ID field to the list_opcodes operation, with the request being serviced by the core provider * include an implicit provider ID within the basic client to allow it to maintain a consistent operation while allowing it to expose an interface that essentially covers the wire protocol contracts * add checks that core operations are only performed on the core client and cryptographic operations cannot be performed on the core client Co-authored-by: Ionut Mihalcea <[email protected]> Co-authored-by: Hugues de Valon <[email protected]> Signed-off-by: Ionut Mihalcea <[email protected]>
This commit includes a few changes:
change in the interface
request being serviced by the core provider
maintain a consistent operation while allowing it to expose an interface
that essentially covers the wire protocol contracts
and cryptographic operations cannot be performed on the core client
Co-authored-by: Ionut Mihalcea [email protected]
Co-authored-by: Hugues de Valon [email protected]
Signed-off-by: Ionut Mihalcea [email protected]