Skip to content

Allow ElectrumSyncClient to reuse a preexisting client #3646

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

Merged
merged 1 commit into from
Mar 5, 2025

Conversation

tnull
Copy link
Contributor

@tnull tnull commented Mar 5, 2025

Previously, our API would only allow users to have ElectrumSyncClient a) construct an entirely new electrum_client::Client or b) take an owened client via from_client.

Here, we change ElectrumSyncClient::from_client to accept an Arc<ElectrumClient>, allowing to reuse a pre-existing client without owning it. This is important as it will allow us to reuse the same connection instead of forcing users to establish multiple connections if they need something beyond syncing LDK.

Previously, our API would only allow users to have `ElectrumSyncClient`
a) construct an entirely `new` `electrum_client::Client` or b) take an
owened client via `from_client`.

Here, we change `ElectrumSyncClient::from_client` to accept an
`Arc<ElectrumClient>`, allowing to reuse a pre-existing client without
owning it. This is important as it will allow us to reuse the same
connection instead of forcing users to establish multiple connections if
they need something beyond syncing LDK.
@ldk-reviews-bot
Copy link

ldk-reviews-bot commented Mar 5, 2025

I've assigned @arik-so as a reviewer!
I'll wait for their review and will help manage the review process.
Once they submit their review, I'll check if a second reviewer would be helpful.

@ldk-reviews-bot ldk-reviews-bot requested a review from arik-so March 5, 2025 11:40
@wpaulino wpaulino merged commit 5bc9ffa into lightningdevkit:main Mar 5, 2025
26 of 27 checks passed
Copy link
Contributor

@arik-so arik-so left a comment

Choose a reason for hiding this comment

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

post-merge ACK

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.

5 participants