Skip to content
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

Drop KVStore type parameter from Node #244

Merged
merged 3 commits into from
Apr 25, 2024

Conversation

tnull
Copy link
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
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
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

tests/integration_tests_rust.rs Show resolved Hide resolved
@tnull tnull force-pushed the 2024-02-switch-to-dyn-kvstore branch from d1a33ce to 8b1a4c2 Compare April 23, 2024 07:22
.. 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
Collaborator Author

tnull commented Apr 25, 2024

Rebased on main after #243 landed.

@tnull tnull merged commit 0bf24c8 into lightningdevkit:main Apr 25, 2024
13 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants