feat: add config for logos chat client#163
Conversation
kaichaosun
force-pushed
the
logos-chat-client-config
branch
from
21e59cf to
4571377
Compare
jazzz
left a comment
jazzz
left a comment
There was a problem hiding this comment.
I'll re-iterate my comments here:
#159 (review)
I'm hesitant to proceed, as I'm not sure this is the correct path. I won't expressly block this PR as I understand its a work in progress. However I'd like to see strong justifications in the next PR about:
- how we intend to surface LogosChat to developers, and justifications on why this is the best approach to make things easy and simple for developers to use.
- Details of how we intend to minimize breaking changes in Apps during development. This the only stable API in the project and that comes with a commitment towards developers to minimize breaking changes
| db_path: String, | ||
| db_key: String, |
There was a problem hiding this comment.
[?] With DirectV1 integration LogosChat does not support any persistence, why are these parameters needed?
There was a problem hiding this comment.
Using an encrypted db is a necessary feature in the old implementation, and it won't change in the new protocol as far as I can tell.
I find the comments a bit conflicting—on one hand you’re asking for a stable API, but on the other hand you’re suggesting changes that would introduce breaking behavior along the way.
There was a problem hiding this comment.
I find the comments a bit conflicting
That is a fair criticism. I would agree there is some conflict in the feedback. I think part of it is the specific problem we're trying to solve, and part of it is definitely my communication.
There are two design goals in order of priority for the high level API.
- Make it easy for new developers
- This encourages exploration and hacking when developers first install the package.
- Anticipate breaking changes an minimize the impacts on developers.
- Maximize the speed to which the product can proceed, by minimizing effort spent on deploying breaking changes.
Trading Ease of use for Stability doesn't quite solve the problem - In a perfect world we'd want to be solving both if possible.
There was a problem hiding this comment.
I think there is merit to thinking ahead and including parameters we are likely going to need. It's smart; If we know its needed, why not put it in now?
My friction is that this approach solves this one instance however doesn't help with the future changes which we know are coming. What I'd like to see is a thoughtful mechanism that allows for arbitrary future parameters/changes. If we have the ability to add the future parameters without breaking existing clients, then there is no issue adding them later.
As an operation principal, I think its fair to say that developers who want new features will need to update their applications. However updating library version should not stop existing clients from compiling.
There was a problem hiding this comment.
There are many potential solutions to this problem. One of them is a BuilderPattern with phased rollout.
- when new features such as 'convo_persistence' are added, a new parameter is added
LogosChatBuilder::state_sqldb_path(self, path: Path) - When an application updates its library version, but not its code;
state_sqldb_pathis not set which causes the client to use EphemeralStorage. - When the app developer wants persistence then can enable it by setting the parameter.
- After sometime, a warning is emitted On
buildthat tells the developer they should update to stay current with features. - ~Six Months in the future; after applications have had a chance to update state_sqldb_path becomes a mandatory field which triggers a compile time failure if its not set.
A solution such as this has many benefits that aligns with our stated public api goals.
- There is a clear path for how to deploy new features and updates. Contributors don't have to worry about pushing changes and breaking everyone app immediately
- App Developers can keep their package version current during development. This means they get the latest fixes, and security patches, without having to immediately rearchitect their applications.
- App Developers can implement new features for their users on their schedule. It could be immediately, or in a few months. Its up to them.
- App Developers don't need to set parameters they don't care about, the HighLevel api chooses sensible defaults.
- Eventually all app developer are forced to update and stay current.
This is just one possible solution - But it optimizes for both the design goals without sacrificing either. Theres extra cost upfront on implementation, however thats worth it to have a clean easy to use api, that is resistant to the breaking changes we know are coming...
| pub fn new(db_path: impl Into<String>, db_key: impl Into<String>) -> Self { | ||
| Self { | ||
| db_path: db_path.into(), | ||
| db_key: db_key.into(), | ||
| tcp_port: DEFAULT_TCP_PORT, | ||
| preset: NETWORK_PRESET.to_string(), | ||
| registry_url: REGISTRY_ENDPOINT.to_string(), | ||
| } | ||
| } |
There was a problem hiding this comment.
[Sand] DB_path doesn't need to be a required parameter, it can be safely derived
There was a problem hiding this comment.
The storage path is fully controlled by app, how a library derive a deterministic path for a unknown app?
Also most apps won't allow a library to create a db file they are not aware of.
There was a problem hiding this comment.
The storage path is fully controlled by app, how a library derive a deterministic path for a unknown app?
The design principal here is "Sensible defaults with Developer choice". I'm suggesting we can choose a functional default, and allow developers to override it if they care.
Logos core modules have a sandboxed folder path, where they can save state and data. In that case it would be safe to assume that data can be written there. The filepath would be derived from the account/identity being used. That way a client never opens a new database with an existing identity (which results in corruption).
e.g. /<ROOT_LOGOSCHAT_INST_ae453a>/keystore_563a..3234.sqlite3
If not using LogosCore you could assume./.logos_chat/keystore_563a..3234.sqlite3 would work for the first time a developer tries to run
or use a tempfile and make storage ephemeral until its set by the developer. Be sure to emit a warning that state is not being persisted.
The goal is to minimize the friction in developers getting started. Not that its perfect for every developer use case.
There was a problem hiding this comment.
Also most apps won't allow a library to create a db file they are not aware of.
- You can always make developers aware where you are defaulting to in a log file
- They can always overwrite the location if they want to.
Also most apps won't allow a library to create a db file they are not aware of.
- The ability to do something, does not imply the requirement to do it.
The phrase "...most apps ..." implies that "... some apps..." would. In following "Sensible Defaults; with Developer choice" the principal is that there is always a easy path that some developers can follow. Without stopping those who want something different. This is exactly the principal with the clients. The default is LogosChatClient and developers can have choice by building their own generic client.
- I'm not sure this statement holds as much as it used to. In decentralized applications, the concept of a "library" is not as clean as it used to be. LogosBasecamp is a perfect example of this.
Decentralized protocols are more akin to applications/services who need to manage their own state and compartmentalize details from applications. I would start thinking about libraries for decentralized protocols as local blackbox services.
| /// network preset, and registry endpoint default to the baked-in Logos | ||
| /// values; override them with [`tcp_port`](Self::tcp_port), | ||
| /// [`preset`](Self::preset), and [`registry_url`](Self::registry_url). | ||
| pub fn new(db_path: impl Into<String>, db_key: impl Into<String>) -> Self { |
There was a problem hiding this comment.
[Pebble] Using strings for paths leads to ambiguity and bugs. Prefer Path
There was a problem hiding this comment.
The db_path here is a pure pass-through. It gets stored as String, then handed to StorageConfig::Encrypted { path: String, key: String }, which in turn does Connection::open(path). rusqlite's open already takes impl AsRef<Path>, so the actual path handling downstream is fine regardless of what we do here.
Keeping it String also plays nicer at the FFI boundary, where paths have to arrive as UTF-8 strings anyway (PathBuf has no stable ABI).
There was a problem hiding this comment.
I don't think I agree with the logic here.
-
It seems you are suggesting that rather than having concrete types, its preferred to allow a separate repo's FFI constraints to leak inward affecting all consumers. Rust consumers will natively get a Path from the OS and developers will have to convert it to a string to satisfy this interface.
-
Inbound Utf8Strings are trivially a valid path, and leverages a zero-cost abstraction it couldn't be any more straight forward:
let p: &Path = Path::new(&utf8_str); -
The fact that the path handling is eventually handled properly downstream is not quite my point.
My suggestion was that using Path provides type clarity for all contributors reading the code. When I see a function that takes a Path, I don't have to wonder what it is.
| /// Override the TCP port for the embedded logos-delivery node. | ||
| pub fn tcp_port(mut self, tcp_port: u16) -> Self { | ||
| self.tcp_port = tcp_port; | ||
| self | ||
| } | ||
|
|
There was a problem hiding this comment.
[!] I don't understand why a developer would care about this configuration point. If you truely think that App developers cannot use LogosChat without this, then the use case ought to be documented
There was a problem hiding this comment.
Because a specific port may be taken, and whether we want to let logos client to assign a random port looks unclear to me, initially I want to add such random pick port, but I'm concerned that people may don't like it, so such idea will be proposed later when needed.
The point is that we are embeding logos delivery in logos chat, if config logos delivery is 3rd party controlled, such config can be omitted.
There was a problem hiding this comment.
Thats a fair concern.
Developers are the focus, and we don't want them to be forced into a configuration which does not work for them.
Specifically there a few problems with randomly choosing a port:
- Is that port being used by another application?
- If users are using a firewall random ports, make it impossible to have a consistent configuration as every restart the port would change.
My comment here would be. While this approach gives developers choice(which is important), it also forces every single developer to make the decision. Some developers do not care, and this is additive friction.
Making every developer have to decide is a high cost, to allow a few developers to choose.
Instead: Define a default and allow the few to over write it.
80% of developers would be grateful if you just made the decision.
| pub fn preset(mut self, preset: impl Into<String>) -> Self { | ||
| self.preset = preset.into(); | ||
| self | ||
| } |
There was a problem hiding this comment.
[Pebble] Preset term is ambigious and not scoped to logos delivery.
[Sand] If preset can only be certain defined values then it ought to be an enum.
There was a problem hiding this comment.
Make it logos_delivery_preset becomes even worse IMO. The key question here is that do we want logos chat users care about its underlining transport (i.e. logos delivery). I don't yet have an answer, but from the current code, we tend to not introduce transport to users, correct me if I'm wrong.
Enum for such value is not work as some system use fully custom uri.
There was a problem hiding this comment.
pub enum DeliveryPreset {
Staging,
Production,
Custom(LogosDeliveryConfig)
} The advantage of using named configs is that of avoiding breaking changes. The production bootstrap urls can be updated if needed and developers do not need to make any changes and allows grouping of multiple parameters together.
| let (client, events) = LogosChatClient::open( | ||
| db_str, | ||
| "chat-cli", | ||
| cli.port, | ||
| cli.preset.as_deref(), | ||
| cli.registry_url.as_deref(), | ||
| ) | ||
| .map_err(|e| anyhow::anyhow!("{e:?}")) | ||
| .context("failed to open chat client")?; | ||
| let mut config = LogosConfig::new(db_str, "chat-cli"); | ||
| if let Some(port) = cli.port { | ||
| config = config.tcp_port(port); | ||
| } | ||
| if let Some(preset) = cli.preset.as_deref() { | ||
| config = config.preset(preset); | ||
| } | ||
| if let Some(registry_url) = cli.registry_url.as_deref() { | ||
| config = config.registry_url(registry_url); | ||
| } | ||
| let (client, events) = LogosChatClient::open(config) | ||
| .map_err(|e| anyhow::anyhow!("{e:?}")) | ||
| .context("failed to open chat client")?; |
There was a problem hiding this comment.
[!] This code somehow got longer and harder to read. As the primary example of this system in use I have questions about its efficacy.
[Pebble] Chain-able config seems to be harmful in this usecase, as now each step the App developer needs to re-save the variable or risk bugs/misconfiguration.
It appears that this is a builder pattern in disguise. I'd pick one. Either a stateful Config object which is updated through setters, or follow the build pattern more closely
This PR and related issue is not targeting this goal. You are adding extra context that I'm not aware of. Do create necessary issue to track your idea.
If you taken out of manually set custom port and api endpoint, the api is only 2 lines, I'm pretty happy about it. The internal embed logos delivery definitely needs a bit more care, but that's another topic. let mut config = LogosConfig::new(db_str, "chat-cli");
let (client, events) = LogosChatClient::open(config) |
kaichaosun
force-pushed
the
logos-chat-client-config
branch
from
4571377 to
f07dd62
Compare
I'm just seeing this comment and had already provided this review based on incomplete information. Having read this, I think It's very valid to say that these requirements are not written down. I also understand that they could feel like they are "extra (and hidden) context". To remedy this, lets follow this plan;
|
2 participants
Replaces 5 positional args with a
LogosConfigstructLogosConfig::new(db_path, db_key)takes the required per-client values; others default to the baked-in Logos values via chainable setters.Related to: #126