Skip to content

Drop KVStore type parameter from Node#244

Merged
tnull merged 3 commits into
lightningdevkit:mainfrom
tnull:2024-02-switch-to-dyn-kvstore
Apr 25, 2024
Merged

Drop KVStore type parameter from Node#244
tnull merged 3 commits into
lightningdevkit:mainfrom
tnull:2024-02-switch-to-dyn-kvstore

Conversation

@tnull
Copy link
Copy Markdown
Collaborator

@tnull tnull commented Feb 7, 2024

Based on #243, depends on lightningdevkit/rust-lightning#2883

Previously, LDK Node's main object has a generic trait bound on KVStore, i.e., Node<K: KVStore + Send + Sync + 'static>. However, this has always been a painful issue as our bindings generator UniFFI doesn't have generic support, so we were required to type-def and expose only a concrete type, i.e., Node<SqliteStore>, in bindings. This will now become even more painful and borderline infeasible with the upcoming introduction of the VssStore alternative.

Here we therefore finally drop the generic trait bound, allowing us expose a cleaner Node object everywhere.

Draft as blocked on upstream for now.

@tnull tnull marked this pull request as draft February 7, 2024 12:41
@tnull tnull force-pushed the 2024-02-switch-to-dyn-kvstore branch 3 times, most recently from 3ed2d02 to 53f22d9 Compare February 7, 2024 13:06
@tnull tnull changed the title Drop KVStore generic from Node Drop KVStore type parameter from Node Feb 7, 2024
@tnull tnull force-pushed the 2024-02-switch-to-dyn-kvstore branch 2 times, most recently from b80a1be to 8d1b497 Compare February 21, 2024 10:53
@tnull tnull mentioned this pull request Feb 21, 2024
@tnull tnull force-pushed the 2024-02-switch-to-dyn-kvstore branch from 8d1b497 to abd8d1d Compare March 4, 2024 10:07
@tnull tnull force-pushed the 2024-02-switch-to-dyn-kvstore branch from abd8d1d to e9cad46 Compare March 4, 2024 14:52
@tnull tnull mentioned this pull request Mar 5, 2024
19 tasks
@tnull tnull added this to the 0.3 milestone Mar 5, 2024
@tnull tnull force-pushed the 2024-02-switch-to-dyn-kvstore branch from e9cad46 to 4f4904d Compare March 5, 2024 08:47
@tnull tnull force-pushed the 2024-02-switch-to-dyn-kvstore branch 2 times, most recently from d9e9f2e to 8508d6c Compare March 8, 2024 07:40
@tnull
Copy link
Copy Markdown
Collaborator Author

tnull commented Mar 8, 2024

Rebased to resolve minor conflicts.

@tnull tnull force-pushed the 2024-02-switch-to-dyn-kvstore branch from 8508d6c to 618aab5 Compare March 8, 2024 07:56
@tnull tnull mentioned this pull request Mar 8, 2024
@tnull tnull force-pushed the 2024-02-switch-to-dyn-kvstore branch from 618aab5 to 42c3644 Compare March 12, 2024 10:02
@tnull tnull force-pushed the 2024-02-switch-to-dyn-kvstore branch from 42c3644 to 24614de Compare March 28, 2024 14:44
@tnull tnull force-pushed the 2024-02-switch-to-dyn-kvstore branch from 24614de to d1a33ce Compare April 22, 2024 13:41
@tnull tnull marked this pull request as ready for review April 22, 2024 13:50
@tnull tnull requested a review from jkczyz April 22, 2024 13:51
Copy link
Copy Markdown
Contributor

@jkczyz jkczyz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Comment thread tests/integration_tests_rust.rs
@tnull tnull force-pushed the 2024-02-switch-to-dyn-kvstore branch from d1a33ce to 8b1a4c2 Compare April 23, 2024 07:22
tnull added 3 commits April 25, 2024 20:03
.. switching to `dyn KVStore + Send + Sync`
... now that we don't have any generic to hide, we can just use the
`Node` type directly.
@tnull tnull force-pushed the 2024-02-switch-to-dyn-kvstore branch from 8b1a4c2 to c566fd5 Compare April 25, 2024 18:03
@tnull
Copy link
Copy Markdown
Collaborator Author

tnull commented Apr 25, 2024

Rebased on main after #243 landed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

2 participants