Skip to content

Allow empty store_ids #95

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.

Sign up for GitHub

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

Open
TheBlueMatt wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Allow empty store_ids #95

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
9bb171d
Allow empty `store_id`s
TheBlueMatt Feb 24, 2026
6caba34
Test all backends with an empty `store_id`
TheBlueMatt Mar 2, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 6 additions & 2 deletions rust/api/src/kv_store_tests.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -552,8 +552,12 @@ pub struct TestContext<'a> {
impl<'a> TestContext<'a> {
/// Creates a new [`TestContext`] with the given [`KvStore`] implementation.
pub fn new(kv_store: &'a dyn KvStore) -> Self {
let store_id: String = (0..7).map(|_| thread_rng().sample(Alphanumeric) as char).collect();
TestContext { kv_store, user_token: "userToken".to_string(), store_id }
let store_id_len = thread_rng().gen_range(0..6);
let store_id: String =
(0..store_id_len).map(|_| thread_rng().sample(Alphanumeric) as char).collect();
let user_token: String =
(0..7).map(|_| thread_rng().sample(Alphanumeric) as char).collect();
TestContext { kv_store, user_token, store_id }
}

async fn get_object(&self, key: &str) -> Result<KeyValue, VssError> {
Expand Down
3 changes: 2 additions & 1 deletion rust/impls/src/migrations.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,14 +26,15 @@ pub(crate) const MIGRATIONS: &[&str] = &[
// We do not complain if the table already exists, as users of VSS could have already created this table
"CREATE TABLE IF NOT EXISTS vss_db (
user_token character varying(120) NOT NULL CHECK (user_token <> ''),
store_id character varying(120) NOT NULL CHECK (store_id <> ''),
store_id character varying(120) NOT NULL,
key character varying(600) NOT NULL,
value bytea NULL,
version bigint NOT NULL,
created_at TIMESTAMP WITH TIME ZONE,
last_updated_at TIMESTAMP WITH TIME ZONE,
PRIMARY KEY (user_token, store_id, key)
);",
"ALTER TABLE vss_db DROP CONSTRAINT IF EXISTS vss_db_store_id_check;",
];
#[cfg(test)]
pub(crate) const DUMMY_MIGRATION: &str = "SELECT 1 WHERE FALSE;";
2 changes: 1 addition & 1 deletion rust/impls/src/postgres/sql/v0_create_vss_db.sql
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
CREATE TABLE vss_db (
user_token character varying(120) NOT NULL CHECK (user_token <> ''),
store_id character varying(120) NOT NULL CHECK (store_id <> ''),
Copy link
Contributor

@G8XSU G8XSU Mar 3, 2026 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I understand how it might seem that way for current usage, allowing empty store_ids effectively encourages users to dump everything into a single flat keyspace without any organization. Once that becomes the default pattern, it's hard to rollback.

Main purpose is namespace isolation via store_id, which is particularly helpful in:

  1. Diff syncing: Clean namespace boundaries make it cheaper to track and apply deltas per logical data group. (Critical for faster sync of lightning state.)
  2. Multi-purpose storage: As more consumers use VSS for data beyond LDK channel state (wallet metadata, app preferences, payment-store) which aren't as critical to sync immediately or at startup. This separation will be particularly helpful. (Some already happening: https://github.com/lightningdevkit/ldk-node/pull/811/changes)

SideNote: You don't know what those agents might want to use it for, better to not dump everything in single namespace, it will pollute and make operations such as list/sync inefficient.
cc: @tnull

Copy link
Contributor

@tnull tnull Mar 3, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Once that becomes the default pattern, it's hard to rollback.

Yeah, I guess that's a somewhat reasonable concern. As mentioned over at the other PR I don't feel too strongly either way, but I guess since we still have the separation currently, there's little reason to rip it out if we see some future use?

  • Diff syncing: Clean namespace boundaries make it cheaper to track and apply deltas per logical data group. (Critical for faster sync of lightning state.)

Isn't that also redundant to separating by user_token, assuming that (most) users wouldn't have many different stores/store_ids anyways?

cc: @tnull

@G8XSU Btw, could you send me a DM via Discord/Signal/Mail, I'd like to follow-up something if you don't mind?

Copy link
Contributor Author

@TheBlueMatt TheBlueMatt Mar 3, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I understand how it might seem that way for current usage, allowing empty store_ids effectively encourages users to dump everything into a single flat keyspace without any organization.

I think this somewhat misreads the motivation. Rather, in most use-cases for VSS its used by a single application (or even library within that application) for storing its own relatively small state. If multiple separate subsystems in (or, more likely, libraries within) the application are using the same VSS server, they can/should use a separate key to authenticate, thus creating a separate namespace through the authenticated user-id instead. The API in #755 somewhat loosely encourages downstream non-LDK-node libraries to use a separate key to authenticate.

Some already happening: https://github.com/lightningdevkit/ldk-node/pull/811/changes

This could also happen through the existing top-level and second-level namespaces, which is also consistent with other LDK Node APIs where we only have the KVStore namespaces to go on.

store_id character varying(120) NOT NULL,
key character varying(600) NOT NULL,
value bytea NULL,
version bigint NOT NULL,
Expand Down