-
Notifications
You must be signed in to change notification settings - Fork 113
Expose a VssStoreBuilder allowing to build VssStore independently
#727
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.
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
Expose a VssStoreBuilder allowing to build VssStore independently
#727
Conversation
|
👋 Thanks for assigning @benthecarman as a reviewer! |
| /// [VSS]: https://github.com/lightningdevkit/vss-server/blob/main/README.md | ||
| /// [LNURL-auth]: https://github.com/lnurl/luds/blob/luds/04.md | ||
| pub fn build( | ||
| &self, lnurl_auth_server_url: String, fixed_headers: HashMap<String, String>, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a reason for these we using &self instead of consuming it with self? I see we do the same for the node builder but typically builders consume themselves when done, it would allow you to remove some clones
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, for the Builder we do that to be able to reuse it in the bindings variant ArcedNodeBuilder (as bindings don't support move semantics, only reference-counted Arcs). Here, we go the same route as we will at least attempt to expose VssStore/VssStoreBuilder in bidnings, though with the new async KVStore trait that could prove difficult, we'll see.
|
🔔 1st Reminder Hey @benthecarman! This PR has been waiting for your review. |
benthecarman
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm on squash
Previously, `VssStore` was a private object that could only be constructed internally via the `Builder` object. However, some users might want to use `VssStore` independently of/before the `Node` is constructed. To this end, we here expose a new `VssStoreBuilder` that allows to independently construct a `VssStore` instance that then can be handed to `Builder::build_with_store`. For now we keep the previous VSS-related methods on `Builder` around, but have them reuse the `VssStoreBuilder` internally. This also allows us to not change anything related to bindings. For bindings, we still have to explore if/how we can expose `VssStore` in the future.
3e78125 to
fe7e39f
Compare
Squashed the fixup without further changes. Landing. |
Previously,
VssStorewas a private object that could only be constructed internally via theBuilderobject. However, some users might want to useVssStoreindependently of/before theNodeis constructed.To this end, we here expose a new
VssStoreBuilderthat allows to independently construct aVssStoreinstance that then can be handed toBuilder::build_with_store.For now we keep the previous VSS-related methods on
Builderaround, but have them reuse theVssStoreBuilderinternally. This also allows us to not change anything related to bindings. For bindings, we still have to explore if/how we can exposeVssStorein the future.FWIW, after this PR lands it might also be a good time to finally upstream
VssStoreto thelightning-persistercrate, which was the original plan ever since we addedVssStore.