feat: default transport for logos chat client#159
Conversation
kaichaosun
force-pushed
the
more-transport-features
branch
from
0f3e419 to
1ab2a41
Compare
jazzz
left a comment
jazzz
left a comment
There was a problem hiding this comment.
As LogosChatClient is intended to be the primary entry point for developers, I think there is some more work needed to make this developer ready.
Specifically:
- Focusing on easier and simpler to use API, rather than more flexible options.
- Eliminating footguns that lead to Fragmentation, and interop failures.
- Moving to a separate crate to make imports, examples and documentation easier.
I won't expressly block this PR, as I understand this is a work in progress. However I don't think this PR brings us closer to our goals. I think its optimizing for options and flexibility rather than the app developer experience.
| // the transport, so it builds a client directly instead of going through | ||
| // `LogosChatClient`. | ||
| TransportKind::File => { | ||
| let transport_dir = cli.data.join("transport"); | ||
| let transport = transport::file::FileTransport::new(&transport_dir) | ||
| .context("failed to create file transport")?; | ||
| run(transport, &cli) | ||
| } |
There was a problem hiding this comment.
[Pebble] Remove TransportKind::File all together.
Keeping TransportKind::File will result in interop issues and broken conversations with other clients. The CliApp registers keybundles publically however the client is not routable, as it uses an isolated delivery service. When Account persistence is added this mean that any Account which uses File transport will be in a broken state.
There was a problem hiding this comment.
Got it, but It's out of scope of this PR targeted.
The file transport for local test needs a better story, and it deserve its own focus.
| components = { workspace = true , features = ["embedded_p2p_delivery"]} | ||
| crossbeam-channel = { workspace = true } | ||
| logos-chat = { workspace = true } | ||
| logos-chat = { workspace = true, features = ["logos-delivery"] } |
There was a problem hiding this comment.
[Sand] We should reserve logos-delivery for the default use case which is logos-delivery+ logos-core. Use embedded-p2p-delivery
| #[cfg(feature = "logos-delivery")] | ||
| mod logos; |
There was a problem hiding this comment.
[Sand] This feature name does not reflect it's purpose. Enabling logos-delivery compiles LogosChatClient, which is a bigger change than enabling logos core.
| pub use errors::ClientError; | ||
| pub use event::{Event, MessageSender}; | ||
| #[cfg(feature = "logos-delivery")] | ||
| pub use logos::LogosChatClient; |
There was a problem hiding this comment.
[Pebble] Conditional compilation attribute is the wrong tool here.
This feature gate solely builds and exposes LogosChatClient. It does not alter code paths, or have any impacts on the existing client crate. In this case if conditional compilation is desired, use a separate crate - it's what its designed for.
Conditional compilation attributes are useful when the core structure of an object needs to be altered. Or when controlling what is exposed through a support library to keep build sizes small.
Hiding the primary entry point for developers behind a feature gate, adds complexity and maintenance issues in applications cargo.toml. 99% of developers should exclusively be using LogosChatClient, and it should be the easiest path to use.
Exposing primary entrypoints via features:
- Means most developers will attempt to use Client api, as thats the easiest path.
- Makes documentation difficult.
- Its hard to link developers to code and apis
There was a problem hiding this comment.
Good point, though split to different crate is not good, as the current crate already called logos-chat, I need to think more before make any meaningful changes.
| tcp_port: u16, | ||
| preset: Option<&str>, | ||
| registry_url: Option<&str>, | ||
| ) -> Result<(Self, Receiver<Event>), ClientError> { |
There was a problem hiding this comment.
Removing transport from this constructor is smart. Love it. Being opinionated makes it easier on developers.
I'd like to see this be even more opinionated.
[Sand] Remove as many parameters as possible. Or use either a builder pattern or a mutable DebugConfig object to reduce strain on developers.
registry_url: Using anything other than the default would lead to fragmentation and inconsistent state within an openmls/accounts. Don'y allow developers to custom this value
tcp_port: Is there a need to make developers choose this? Can't the implementation choose a default?
db_path: I get why this is needed, but this is a footgun for developers. Favor user defined directories with deterministic filenames such that only one database ever exists for a signer app instance
preset: Developers choosing this value would corrupt state. KeyPackages are published to a shared location however commit messages are isolated to different networks.
At this point the key priority is maintaining interop between all clients and all applications. Developer customizations can come later when a need arises
There was a problem hiding this comment.
The high level issue is that removing transport in the constructor results in developers having to make more choices, not less.
Its easy enough to provide a function create_default_transport() which means that app developers can be hidden from these details. By flattening it, yet adding new transport specific parameters adds more cognitive load on developers rather than less.
There was a problem hiding this comment.
Future PRs will further target improve dev experience.
kaichaosun
force-pushed
the
more-transport-features
branch
from
1ab2a41 to
05e1ccc
Compare
kaichaosun
force-pushed
the
more-transport-features
branch
from
05e1ccc to
b0c1be4
Compare
2 participants
Makes logos-delivery the transport baked into LogosChatClient,
tcp_portandpresetand starts the EmbeddedP2pDeliveryService internally.chat-clito use logos chat client when fit