Add PaginatedKVStore support to VssStore#864
Add PaginatedKVStore support to VssStore#864benthecarman wants to merge 1 commit intolightningdevkit:mainfrom
Conversation
Implement PaginatedKVStore and PaginatedKVStoreSync traits for VssStore, enabling paginated key listing via cursor-based pagination using PageToken. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
👋 Thanks for assigning @tankyleo as a reviewer! |
tnull
left a comment
tnull
left a comment
There was a problem hiding this comment.
Ah, I had hoped that we could do this after lightningdevkit/rust-lightning#4323 lands, i.e., add it upstream directly. However, that PR seems to be delayed now as reviewer brought up more requirements, so probably need to go ahead here.
I'll let @tankyleo take a first look here and on the corresponding vss-server PR.
|
🔔 1st Reminder Hey @tankyleo! This PR has been waiting for your review. |
|
🔔 2nd Reminder Hey @tankyleo! This PR has been waiting for your review. |
| // VSS uses empty string to signal the last page | ||
| let next_page_token = | ||
| response.next_page_token.filter(|t| !t.is_empty()).map(PageToken::new); |
There was a problem hiding this comment.
Sounds to me like we should line-up the VSS API and the PaginatedKVStore API better, I raised a similar point in the VSS PR.
Right now a VSS could return a non-empty keys list, but signal "no more data to give" with a String::new() for the page token.
A consumer of PaginatedKVStore does another roundtrip, but the VSS did not expect any further roundtrips.
There was a problem hiding this comment.
Yeah I feel like page_token: None should just be the signal that there is no more. That's what we do in the file system store and sqlite. Should fix that on the vss-server side but we can keep this is_empty check for safety.
There was a problem hiding this comment.
Changed in lightningdevkit/vss-server#96 to do this
There was a problem hiding this comment.
Yeah I feel like
page_token: Noneshould just be the signal that there is no more. That's what we do in the file system store and sqlite. Should fix that on the vss-server side but we can keep this is_empty check for safety.
I think we're following protobuf's/Google's best practices here. https://google.aip.dev/158 states:
- Response messages for collections should define a string next_page_token field, providing the user with a page token that may be used to retrieve the next page.
- The field containing pagination results should be the first field in the message and have a field number of 1. It should be a repeated field containing a list of resources constituting a single page of results.
- If the end of the collection has been reached, the next_page_token field must be empty. This is the only way to communicate "end-of-collection" to users.
- If the end of the collection has not been reached (or if the API can not determine in time), the API must provide a next_page_token.
| match response.next_page_token { | ||
| Some(token) => page_token = Some(token), | ||
| None => break, | ||
| } |
There was a problem hiding this comment.
hmm here we determine whether we are done with pagination based on whether the page token is None, and not whether the list is empty. I guess just looking for clarification on the PaginatedKVStore API :)
4 participants
Implement
PaginatedKVStoreandPaginatedKVStoreSynctraits forVssStore, enabling paginated key listing via cursor-based pagination usingPageToken.At the moment VSS returns them in key order which is not ideal. We would need to add to the VSS server timestamps so they are returned in creation order. Ideally though after that change, it wouldn't require any changes on the client or in here because we have already hooked up the pagination. Curious on @tankyleo's thoughts here.
After this we'll have pagination for all the kv stores and can move forward with using it inside of ldk-node