-
Notifications
You must be signed in to change notification settings - Fork 407
Minor tweaks to lightning-liquidity
to get it building in bindings
#3701
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
Minor tweaks to lightning-liquidity
to get it building in bindings
#3701
Conversation
👋 I see @valentinewallace was un-assigned. |
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.
Basically LGTM
rustfmt
is failing, and if we ever need to revert the second change we can figure it out then I guess.
👋 The first review has been submitted! Do you think this PR is ready for a second reviewer? If so, click here to assign a second reviewer. |
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 module what it is missing to make the CI happy :)
While using a `prelude` can be helpful when we import `*` from it, importing individual types through a `prelude` is somewhat unnecessary indirection. Further, importing `alloc` types like `String` and `Vec` through a `prelude` confuse the bindings generator. Thus, here we drop the `alloc` imports from the `lightning-liquidity` `prelude` and replace it with direct imports. We leave the `lightning-liquidity::prelude::hash_tables` `pub(use)` in place as the sym-linked `debug_sync` relies on it.
`LSPS2ServiceHandler` currently requires that the `Deref` to a `ChannelManager` be `Clone`, but doesn't use it for anything. This upsets the bindings somewhat as they generate a wrapper struct which implements `Deref` (as it holds a pointer) but does not implement `Clone` (as the inner object cannot be cloned. Thus, we simply remove the additional bound here.
82e346b
to
c3a1ffb
Compare
Fixed the rustfmt issues and squashed. |
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
Going ahead and landing this as it's more or less trivial.
No description provided.