Conversation
|
👋 Thanks for assigning @tnull as a reviewer! |
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.
268daea to
9bb171d
Compare
|
🔔 1st Reminder Hey @tankyleo! This PR has been waiting for your review. |
There was a problem hiding this comment.
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.
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. In the previous commit we allowed empty `store_id`s in the postgres backend, here we add tests (randomly) with empty `store_id`s in the standardized backend tests.
|
Good point, done. We discussed it lightly at lightningdevkit/ldk-node#755 (comment) |
| @@ -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 <> ''), | |||
There was a problem hiding this comment.
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:
- Diff syncing: Clean namespace boundaries make it cheaper to track and apply deltas per logical data group. (Critical for faster sync of lightning state.)
- 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
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 astore_id, the empty string is just as valid as any other. Thus we allow it here.