Uh oh!
There was an error while loading. Please reload this page.
Make KVStore configurable - #101
Conversation
Uh oh!
There was an error while loading. Please reload this page.
5d0abd7 to
98b4bc1Comparetnull
commented
May 23, 2023
Rebased after #85 landed. |
tnull
commented
May 23, 2023
Rebased on main after #85 landed. |
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Previously `KVStore::write` would return a `TransactionalWrite` that would allow `Writeables` to be written directly and only be persisted upon calling `TransactionalWrite::commit()`. We however received feedback from implementors that this API is considered unnecessarily complex for a K-V store. Therefore we here refactor `KVStore::write` to take a `buf: &[u8]` directly and serialize the data into corresponding buffers before calling `KVStore::write`, thereby reducing the complexity for any implementation of the `KVStore` interface.
854f935 to
fa6f0d9Comparetnull
commented
May 24, 2023
Rebased on current main and addressed all feedback. Let me know when I can squash the fixups. |
58fc84d to
cd9b51fCompareUh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Previously, we had `Node` take a concrete `FilesystemStore` struct. Here we switch to a generic `KVStore` type parameter. To this end we switched from `K: Deref` to concrete `Arc<K>` in all of the modules to avoid confusion of the type paramters or requirements to track `Arc`-ed and non-`Arc`ed version of the `K: KVStore` paramter. Moreover, as Uniffi doesn't support exposing generics we now expose a concretized `LDKNode` type alias in bindings, which will use `SqliteStore` in the future. Note that going the generic route was necessary as `dyn KVStore` wasn't an opion due to the incompatibility of `dyn` with associated types. In this case the incompatibility is in regard to `KVStore::Reader` and I opted to go this route over forcing any implementation of the trait to allocating and returning the same concrete value (e.g., `Vec<u8>`) which could potentially have significant performance impacts over returning a buffered reader for example.
cd9b51f to
f482d45Comparetnull
commented
May 24, 2023
Squashed fixups and included a new cleanup commit, as it touches unrelated code parts. |
| /// Builds a [`Node`] instance according to the options previously configured. | ||
| pub fn build(&self) -> Arc<Node> { | ||
| pub fn build_with_store<K: KVStore + Sync + Send + 'static>( |
There was a problem hiding this comment.
can't we do similar to set_esplora_server_url ?
and use similar builder pattern for this.
There was a problem hiding this comment.
there's one more related or unrelated item.
some KVStore's like Vss might need a runtime,
Current process of build_with_store assumes kvstore will be provided externally, but we will have no way to inject common runtime without something like set_runtime.
any thoughts or solutions to that?
There was a problem hiding this comment.
can't we do similar to set_esplora_server_url ? and use similar builder pattern for this.
No, we need different build methods as KVStore is a type parameter, i.e., we need to build objects with the concretized type.
there's one more related or unrelated item. some KVStore's like Vss might need a runtime,
Current process of build_with_store assumes kvstore will be provided externally, but we will have no way to inject common runtime without something like set_runtime. any thoughts or solutions to that?
Mh, good point. I think unfortunately there is no way around injecting a runtime, as there will be no runtime loaded at initialization time. This in turn means that VssClient needs to be able to handle the absence of a runtime. Perhaps you'll need to write a KVStore adapter object anyways which could handle the runtime injection and init/reload the actual client as needed?
Firstly, we simplify the
KVStoreinterface: in the first commit we refactorKVStore::writeto take abuf: &[u8]directly and serialize the data into corresponding buffers before callingKVStore::write, thereby reducing the complexity for anyimplementation of the
KVStoreinterface.Furthermore, we previously had
Nodetake a concreteFilesystemStorestruct. In the second commit we switch to a genericKVStoretype parameter. To this end we switched fromK: Derefto concreteArc<K>in all of the modules to avoid confusion of the type parameters or requirements to trackArc-ed and non-Arced version of theK: KVStoreparamter. Moreover, as UniFFI doesn't support exposing generics we now expose a concretizedLDKNodetype alias in bindings, which will use
SqliteStorein the future.