Allow empty store_ids

[TheBlueMatt]

Most VSS users don't actually care about the store_id - they have some data which they want to store for themselves (keyed on the authenticated user id) and that's it. There's not really any reason to force them to specify a store_id, the empty string is just as valid as any other. Thus we allow it here.

[ldk-reviews-bot]

👋 Thanks for assigning @tnull as a reviewer!
I'll wait for their review and will help manage the review process.
Once they submit their review, I'll check if a second reviewer would be helpful.

[ldk-reviews-bot]

🔔 1st Reminder

Hey @tankyleo! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

[tankyleo]

Thanks LGTM, how about we make sure all the server implementations can handle the empty string for store_id ? The VSS api allows it.

diff --git a/rust/api/src/kv_store_tests.rs b/rust/api/src/kv_store_tests.rs
index b1f998d..5b870bf 100644
--- a/rust/api/src/kv_store_tests.rs
+++ b/rust/api/src/kv_store_tests.rs
@@ -552,8 +552,10 @@ 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..7);
+               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> {

I'll ping tnull to make sure he's onboard too.

[TheBlueMatt]

Good point, done. We discussed it lightly at lightningdevkit/ldk-node#755 (comment)

[G8XSU]

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

[tnull]

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?

[TheBlueMatt]

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.