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

lightning-net-tokio: Allow custom smart pointers #1623

Merged

Conversation

MaxFangX
Copy link
Contributor

@MaxFangX MaxFangX commented Jul 20, 2022

Summary

This PR is simple: for the APIs in lightning-net-tokio, instead of requiring that the PeerManager's ChannelMessageHandler, RoutingMessageHandler etc are exactly an Arc<Trait>, anything that impls Deref where Deref::Target: Trait suffices.

Motivation

In our project we are using newtypes (e.g. LexeKeysManager) which Deref to the LDK equivalent (KeysManager) which implement the important traits (KeysInterface). We've found that using custom smart pointers in this way is a convenient way for us to implement custom methods and traits on the newtype (e.g. init() -> Self, TryFrom, derive_pubkey()) while reusing the default trait implementations provided by LDK. Since many of LDK's APIs are already generic across Deref impls, this PR is essentially extending that idiom to lightning-net-tokio.

API Changes

Since all calling code currently uses Arc<Trait>, and Arc is Deref::Target: Trait, this PR introduces 0 API-breaking changes.

Implementation Notes

The 'static + Send + Sync has to apply to both the generic argument (e.g. CMH) as well as its target (e.g. CMH::Target). The ?Sized marker was removed from Logger, as Sized is required.

I have integrated this change in our own project and it's working perfectly as a "drop in" replacement of the existing lightning-net-tokio API.

@TheBlueMatt
Copy link
Collaborator

Fixes #1536

@valentinewallace valentinewallace merged commit 507d759 into lightningdevkit:main Jul 20, 2022
@MaxFangX MaxFangX deleted the custom-smart-pointers branch July 20, 2022 19:52
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.

None yet

3 participants