feat: default value for logos chat client by kaichaosun · Pull Request #151 · logos-messaging/libchat

kaichaosun · 2026-06-25T10:37:31Z

Adds an opinionated LogosChatClient preconfigured with the Logos stack.

Changes:

ChatClientBuilder: collapsed the 8 defaulting build() impls into one. Two
explicit constructors — new() (empty) and ephemeral() (random identity,
ephemeral registry, in-memory storage, for tests/local).
LogosChatClient with open(transport, db_path, db_key, registry_url).
db_path/db_key are caller-supplied;
registry_url overrides the preconfigured endpoint (placeholder until deployed).
chat-cli builds via LogosChatClient::open, so it always uses the registry;
--registry-url is now an endpoint override.
Removed the dead run_logos_delivery.

jazzz · 2026-06-25T14:35:16Z

-// All four explicitly provided.
+/// `build()` exists only once every component has a concrete type. Any field
+/// still `Unset` (always at least the transport, which has no default) fails the
+/// bounds below, so an incomplete builder is a compile error rather than a
+/// runtime one. Start from [`ephemeral`](ChatClientBuilder::ephemeral) to fill the
+/// identity/registration/storage slots, then set the transport.
 impl<I, T, R, S> ChatClientBuilder<I, T, R, S>


I like what you are trying to do here, by deleting the other build() impls however this goes against the benefits of using the "builder" pattern.

[Boulder] Implementors must not be required to provide every component on construction. For maintainability Developers only choose the components that they care about, and the library provides sensible defaults for the rest.

The new implementation actually fulfill this target as far as I can tell.
also checkout this comment #151 (comment) for the reason why I remove the many builders.

The new implementation actually fulfill this target as far as I can tell.

I was imprecise with my language, as I was assuming a requirement of no side effects.

Updated to: "Using a zero-side effect pathway like ::new() Implementors must not be required to provide every component on construction. "

jazzz · 2026-06-25T14:40:30Z

+    pub fn ephemeral() -> ChatClientBuilder<DelegateSigner, Unset, EphemeralRegistry, ChatStorage> {
+        Self::new()
+            .ident(DelegateSigner::random())
+            .registration(EphemeralRegistry::new())


[Pebble] EphemeralRegistry requires clients be within the same process to connect. For developers this will be confusing why they cannot connect. Developers expect a EphemeralClient to be fully functioning rather than a test impl

The chat-store is not fully deployed, once deployed, we can update it to be be http registry.

My concern is then, using http registry would not be ephemeral, as the KeyPackages are persisted beyond restarts.

Ephemeral represents the current state well. However this is the primary entry point for developers. They do not want ephemeral services and messages. Public facing api structure need to be defined aspirationally, not based on the current feature sets.

jazzz · 2026-06-25T14:44:59Z

+    /// ephemeral (in-memory) registry, and in-memory storage. Intended for tests,
+    /// examples, and local runs that need no persistence or network services;
+    /// override any slot with the matching setter. A complete entry point on its


[Sand] If intended for tests then why expose it at the primary developer entry point? Consider moving to integration tests, or tests module

Move to test modules sounds good, I kept it here in this PR to minimize the required changes. As I found it easier to progress with concentrate PRs.

jazzz · 2026-06-25T14:58:23Z

+    /// own — only the transport is left to set before [`build`](ChatClientBuilder::build).
+    pub fn ephemeral() -> ChatClientBuilder<DelegateSigner, Unset, EphemeralRegistry, ChatStorage> {
+        Self::new()
+            .ident(DelegateSigner::random())


Using named configurations i think is a smart idea.
However in this case it results in two undesirable paths for developers.

builder::new() + provide every single field, so it will build.

This breaks all clients if a new service is added to the client.

Requires developers to specify implementations they don't care about, which becomes failure points in the future

builder::<named_default>() + modifications.

Instantiates components which potentially have side effects, and are ultimately not used.

The goal here is Developer choice + sensible defaults.

I would leave ChatClientBuilder as is and focus on providing named defaults via LogosChatClient

builder::new() + provide every single field, so it will build.

I think the defaults() and override is the way to go in most scenarios. And leaving new() as a fully controlled way to initiate the client with the "pain" to update along with library.

Instantiates components which potentially have side effects, and are ultimately not used.

Good point, and I don't have a good solution yet, maybe we should defer the side effects. I also don't see it's a blocker as we are mostly using memory storage etc.

The refactor here is to solve the bloated builder methods, as it will be growing even worse when there are new dependency added. Macros is another way to try, but I'd like to try this solution first.

Good point, and I don't have a good solution yet, maybe we should defer the side effects.

That is the intention of the initial design. Initializing services will produce effects (registration, state initialization, logging, etc. ) As the services improve and become more production ready, the effects will increase. This interface is entirely focused on developers, so services like in memory storage will be replaced ASAP.

The refactor here is to solve the bloated builder methods, as it will be growing even worse when there are new dependency added.

I don't see "bloated builder methods" as an issue that requires fixing at this moment. The code is relatively straight forward, and symmetric. Zero side-effect, compile-time type enforcement is worth it at this moment

As I found it easier to progress with concentrate PRs.
Lets let it become an issue before attempting to resolve it

jazzz · 2026-06-25T15:00:41Z

+    /// `db_path` is a per-client location and `db_key` is a secret, so both are
+    /// caller-supplied — never baked into the library.


[Sand] this will be awkward for a while as all convos are missing persistence support, for the next month.

Yeah, I suppose it should be fine as not many code depends on this path yet.

jazzz · 2026-06-25T15:03:35Z

+        transport: T,
+        db_path: impl Into<String>,
+        db_key: impl Into<String>,
+        registry_url: Option<&str>,


[Sand] Consider removing registry_url.

For clients to interop developers need to be using the same registry service. Providing configuration here means that clients will be fragmented.

In general we want to be very opinionated in our "opinionated" implementations - such that two developers using LogosChatClient can always communicate. Same Delivery, same registration, same account.

This is added here to facilitate locally running apps so before we have a production ready registry, it can be override for such apps.
It will also be useful if we have logos-delivery broadcast capability for publish records in registry implement, so multiple instance can be supported.

jazzz · 2026-06-25T15:05:01Z

+    /// `db_path` is a per-client location and `db_key` is a secret, so both are
+    /// caller-supplied — never baked into the library.
+    pub fn open(
+        transport: T,


[Pebble] This should be opinionated about the transport being used. Developers should not have to choose this parameter, it should be configured for them if they choose the LogosChatClient

I agree, the current logos delivery is not in the proper place, and hard to instantiate it in logos.rs, needs another PR to first move logos delivery transport in extentions.

jazzz · 2026-06-25T15:09:21Z

-    let (mut saro, saro_events) = ChatClientBuilder::new()
+    let (mut saro, saro_events) = ChatClientBuilder::ephemeral()
        .transport(InProcessDelivery::new(bus.clone()))
        .registration(reg.clone())
        .build()


[Dust] As a developer; Using Ephemeral to then potentially define .storage(sql) seems more confusing than building the client the way I want via ChatClientBuilder::new()

eg.

ChatClientBuilder::new().transport(t).storage(sql)
vs
ChatClientBuilder::ephemeral().transport(t).storage(sql)

Is it ephemeral or does it use, sql peristence?

The semantic looks a bit different,
ChatClientBuilder::ephemeral().transport(t).storage(sql) means you decide to go with default first and override later,
ChatClientBuilder::new().transport(t).storage(sql).xyz() are also supported, but devs needs to provide all the required ones.

Maybe we can rename ephemeral to defaults later when we have registry and persistence ready. I explicitly name is ephemeral to reflect its current usage and internal state.

jazzz

My previous concerns still remain.

Blocker: Changing ChatClientBuilder to require an ephemeral base configuration has negative impacts on code maintenance and will be hard to undo in the future.
- This change does not seem necessary at this time, as it does not add any required functionality while removing beneficial properties.
- The motivation of removing a few lines of code doesn't justify the increased maintenance costs in developer applications.
Forcing Ephemeral be the root of the builder pathway introduces fragmentation, which will become developer pain.
- The focus should be on creating an API that we want in the future( rather that what is available now), and working towards it with little to no breaking code changes in demos and developer applications.

jazzz · 2026-06-26T16:08:13Z

@@ -0,0 +1,61 @@
+//! The opinionated Logos client.
+//!


Having LogosChatClient located within the "Generic client" seems like it will be full of friction as the objects have separate concerns.

the "GenericClient will tend to be more generic/abstract" and the LogosChatClient will tend towards importing specific implementations and services.

[Sand] LogosChatClient would benefit from being a separate crate, to keep imports separated. That way it can import LogosCore specific implementations and GenericClient could remain free of these implementation details

kaichaosun added 2 commits June 25, 2026 18:07

feat: default value for chat client builder

c6b56ef

chore: rename default to ephemeral

9b4c8c4

jazzz reviewed Jun 25, 2026

View reviewed changes

chore: mock registry server for ci

616df84

kaichaosun requested review from jazzz and osmaczko June 26, 2026 03:02

jazzz requested changes Jun 26, 2026

View reviewed changes

		/// `db_path` is a per-client location and `db_key` is a secret, so both are
		/// caller-supplied — never baked into the library.

Uh oh!

Conversation

kaichaosun commented Jun 25, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kaichaosun Jun 26, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jazzz Jun 26, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jazzz Jun 25, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jazzz left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

kaichaosun commented Jun 25, 2026 •

edited

Loading

kaichaosun Jun 26, 2026 •

edited

Loading

jazzz Jun 26, 2026 •

edited

Loading

jazzz Jun 25, 2026 •

edited

Loading