Persist identity by kaichaosun · Pull Request #67 · logos-messaging/libchat

kaichaosun · 2026-02-27T06:33:24Z

What changes:

integrate the storage crate for sqlite access
migrations and run helpers
save and load identity
expose ffi with app provided storage path

plopezlpz · 2026-02-27T07:55:26Z

conversations/src/storage/db.rs

+            "INSERT OR REPLACE INTO identity (id, name, secret_key) VALUES (1, ?1, ?2)",
+            params![
+                identity.get_name(),
+                identity.secret().DANGER_to_bytes().as_slice()


I guess since the whole db will be encrypted this is ok

Yeah, it should be fine for now until there is shared logos module to help manage such credentials.

nb: While there may be shared functionality these installation keys will need to be compartmentalized to a given instance.

There is also a deprecation notice on DANGER_to_bytes which we may need to re-discuss.

The intention is that the conversations crate will eventually not have any access to key material, and all operations on secrets bytes would be localized to a single location such as crate crypto. This makes external reviews and safety checks straight forward by establishing an invariant that crates cannot have unsafe access to key material.

Storage poses a problem with this approach and will need to be need to be considered more fully.

The improvement of key storage in a better place and DANGER_to_bytes is out of scope of this PR.
The target here is to implement MVP that can persist the necessary information.

A few more questions worth to be discussed before putting more efforts,

how are such credentials should be handled in logos

will the credentials be portable

Copilot

Pull request overview

Adds SQLite-backed persistence for conversation state, focusing initially on saving/loading the local identity, and exposes a new FFI entrypoint to open a context with an app-provided storage path.

Changes:

Introduces a ChatStorage SQLite layer with embedded migrations and identity save/load.
Extends Context to open from storage (load-or-create identity) and adds persistence tests.
Adds an FFI helper (create_context_with_storage) plus basic shared FFI utilities (CResult, destroy_string).

Reviewed changes

Copilot reviewed 15 out of 16 changed files in this pull request and generated 7 comments.

Show a summary per file

File	Description
storage/src/lib.rs	Re-exports `rusqlite::Connection` for domain crates to run migrations without depending on rusqlite directly.
storage/src/errors.rs	Adds `StorageError::InvalidData` for domain-level validation failures.
conversations/src/storage/types.rs	Adds `IdentityRecord` with `zeroize` support and a unit test.
conversations/src/storage/mod.rs	Introduces the conversations storage module wiring.
conversations/src/storage/migrations/001_initial_schema.sql	Creates initial schema with an `identity` table.
conversations/src/storage/migrations.rs	Adds embedded, ordered migration application with `_migrations` tracking.
conversations/src/storage/db.rs	Implements `ChatStorage` with identity save/load and a roundtrip test.
conversations/src/lib.rs	Wires new `ffi` and `storage` modules into the crate.
conversations/src/identity.rs	Adds `Identity::from_secret` constructor for restoring persisted identities.
conversations/src/ffi/utils.rs	Adds `CResult` and `destroy_string` utility for FFI.
conversations/src/ffi/mod.rs	Exposes the `ffi::utils` module.
conversations/src/errors.rs	Adds `ChatError::Storage` to propagate storage failures.
conversations/src/context.rs	Adds `Context::open` using storage; keeps `new_with_name` as in-memory; adds persistence test.
conversations/src/api.rs	Adds `create_context_with_storage` FFI entrypoint using a DB path.
conversations/Cargo.toml	Adds `storage`, `zeroize`, and `tempfile` dependencies.
Cargo.lock	Locks new dependencies.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

conversations/src/identity.rs

Copilot · 2026-02-28T06:54:56Z

conversations/src/api.rs

+#[ffi_export]
+pub fn create_context_with_storage(
+    name: repr_c::String,
+    db_path: repr_c::String,
+) -> CResult<repr_c::Box<ContextHandle>, repr_c::String> {


This file’s ABI notes indicate using out-parameters for returning non-trivial structs (e.g., installation_name, create_intro_bundle). create_context_with_storage returns a CResult<...> struct by value, which is inconsistent with that convention and may be ABI-problematic for Nim callers. Consider switching to an out-parameter result struct to match the established pattern.

@osmaczko is this going to contribute to the issues with sret?

conversations/src/api.rs

conversations/src/storage/db.rs

conversations/src/storage/migrations.rs

Copilot · 2026-02-28T06:54:58Z

conversations/src/storage/types.rs

+    fn from(record: IdentityRecord) -> Self {
+        let name = record.name.clone();
+        let secret = PrivateKey::from(record.secret_key);
+        Identity::from_secret(name, secret)


impl From<IdentityRecord> for Identity clones record.name even though IdentityRecord is moved into the conversion. This adds an unnecessary allocation/copy; you can move the String out of the record directly instead of cloning it.

Move name did not pass the compiler,

cannot move out of type `IdentityRecord`, which implements the `Drop` trait

jazzz · 2026-02-28T10:16:45Z

conversations/src/storage/db.rs

+    // ==================== Identity Operations ====================
+
+    /// Saves the identity (secret key).
+    pub fn save_identity(&mut self, identity: &Identity) -> Result<(), StorageError> {


[Sand] Clearing up the language used in this library: Installation represent keys for this given instance, where Identity represents a group of installations tied to a set of Installations.

Clear the Installation or identity terms used is out of scope for this PR.
We need to write the spec first, also there may need org level consensus on how such concept being shared or reused.

jazzz · 2026-02-28T10:23:18Z

conversations/src/storage/db.rs

+            "INSERT OR REPLACE INTO identity (id, name, secret_key) VALUES (1, ?1, ?2)",
+            params![
+                identity.get_name(),
+                identity.secret().DANGER_to_bytes().as_slice()


nb: While there may be shared functionality these installation keys will need to be compartmentalized to a given instance.

jazzz · 2026-02-28T10:47:22Z

conversations/src/storage/db.rs

+            "INSERT OR REPLACE INTO identity (id, name, secret_key) VALUES (1, ?1, ?2)",
+            params![
+                identity.get_name(),
+                identity.secret().DANGER_to_bytes().as_slice()


There is also a deprecation notice on DANGER_to_bytes which we may need to re-discuss.

The intention is that the conversations crate will eventually not have any access to key material, and all operations on secrets bytes would be localized to a single location such as crate crypto. This makes external reviews and safety checks straight forward by establishing an invariant that crates cannot have unsafe access to key material.

Storage poses a problem with this approach and will need to be need to be considered more fully.

jazzz · 2026-02-28T10:54:51Z

conversations/src/storage/mod.rs

+//! Storage module for persisting chat state.
+
+mod db;
+mod migrations;
+pub(crate) mod types;
+
+pub(crate) use db::ChatStorage;


This project (with the exception of double-ratchets) follows the Rust 2018 guidance - https://doc.rust-lang.org/edition-guide/rust-2018/path-changes.html

I don't see why we should follow this "guidance".
I have randomly searched a few top repos in github, all of my searched repos includes mod.rs.
https://github.com/EvanLi/Github-Ranking/blob/master/Top100/Rust.md

jazzz · 2026-02-28T11:01:15Z

conversations/src/api.rs

+#[ffi_export]
+pub fn create_context_with_storage(
+    name: repr_c::String,
+    db_path: repr_c::String,
+) -> CResult<repr_c::Box<ContextHandle>, repr_c::String> {


@osmaczko is this going to contribute to the issues with sret?

jazzz · 2026-02-28T11:03:38Z

conversations/src/ffi/utils.rs

+pub struct CResult<T: ReprC, Err: ReprC> {
+    pub ok: Option<T>,
+    pub err: Option<Err>,
+}


From a consistency POV, api.rs uses the safer_ffi to create unique types per each api_call. I'm not saying that is the best approach, however this does represent a different approach of using generic containers as return values.

Exactly, that's why I put in ffi/utils, I hope it can be reused in other apis, so that we can have uniform apis and reused code.

jazzz · 2026-02-28T11:05:23Z

conversations/src/storage/types.rs

+    fn from(record: IdentityRecord) -> Self {
+        let name = record.name.clone();
+        let secret = PrivateKey::from(record.secret_key);
+        Identity::from_secret(name, secret)


kaichaosun force-pushed the storage-rebase branch from 00b6668 to 73d7abb Compare February 27, 2026 07:01

kaichaosun marked this pull request as ready for review February 27, 2026 07:08

kaichaosun requested review from jazzz, osmaczko and plopezlpz February 27, 2026 07:08

kaichaosun force-pushed the storage-rebase branch from 07cb20e to 0f79ad5 Compare February 27, 2026 07:54

plopezlpz reviewed Feb 27, 2026

View reviewed changes

plopezlpz approved these changes Feb 27, 2026

View reviewed changes

kaichaosun requested a review from Copilot February 28, 2026 06:47

Copilot started reviewing on behalf of kaichaosun February 28, 2026 06:47 View session

Copilot AI reviewed Feb 28, 2026

View reviewed changes

jazzz reviewed Feb 28, 2026

View reviewed changes

kaichaosun added 11 commits March 2, 2026 11:58

feat: storage for conversations

f3aa5d5

fix: db types conversion

e099d5f

feat: run migrations from sql files

f4c08bd

feat: persist identity

37eb274

fix: revert double ratchet storage refactor

3a9ddad

fix: clean

7019b04

refactor: use result wrapper for ffi

3673d73

refactor: uniform storage error into chat error

fdacfae

fix: zeroize identity record

0e62c44

fix: zeroize for secret keys in db operations

c3e5f36

fix: transactional sql migration

030ab47

kaichaosun force-pushed the storage-rebase branch from 0f79ad5 to 030ab47 Compare March 2, 2026 03:58

fix: remove destroy_string

10a403e

Conversation

kaichaosun commented Feb 27, 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!

kaichaosun Mar 2, 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!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Copilot AI Feb 28, 2026

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI Feb 28, 2026

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!

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

kaichaosun commented Feb 27, 2026 •

edited

Loading

kaichaosun Mar 2, 2026 •

edited

Loading