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

Have bdk_electrum take Electrum client by reference #1820

Closed

Conversation

tnull
Copy link
Contributor

@tnull tnull commented Feb 4, 2025

Description

Previously, BdkElectrumClient::new would unnecessarily take an owned electrum_client::Client value, which rendered reusing the same client (i.e., connection) for other tasks impossible.

Here, we switch to have BdkElectrumClient take a Deref with target ElectrumApi, which allows to give the electrum_client::Client by reference to BdkElectrumClient.

Checklists

All Submissions:

  • I've signed all my commits
  • I followed the contribution guidelines
  • I ran cargo fmt and cargo clippy before committing

Bugfixes:

  • This pull request breaks the existing API

Previously, `BdkElectrumClient::new` would unnecessarily take an owned
`electrum_client::Client` value, which rendered reusing the same client
(i.e., connection) for other tasks impossible.

Here, we switch to have `BdkElectrumClient` take a `Deref` with target
`ElectrumApi`, which allows to give the `electrum_client::Client` *by
reference* to `BdkElectrumClient`.
@tnull tnull force-pushed the 2025-02-deref-electrum-client branch from 9961701 to 119ffe7 Compare February 4, 2025 09:51
@evanlinjin
Copy link
Member

which rendered reusing the same client (i.e., connection) for other tasks impossible.

This is not true. The BdkElectrumClient::inner field is public.

@tnull
Copy link
Contributor Author

tnull commented Feb 6, 2025

This is not true. The BdkElectrumClient::inner field is public.

Well, that's still very limiting, no? It has a simple E: ElectrumApi bound, so it's not Clone, for example. Yes, you could use that to make a request using the same client, but you can't for example, reuse the same client that would require moving the reference to another context?

evanlinjin added a commit to evanlinjin/bdk that referenced this pull request Feb 6, 2025
Previously, `BdkElectrumClient` has to take ownership of the internal
electrum client.

This is an alternative of bitcoindevkit#1820. bitcoindevkit#1820 removes the ability to have
`BdkElectrumClient` take ownership.
@evanlinjin
Copy link
Member

I created #1825 as an alternative, less-breaking solution.

@tnull
Copy link
Contributor Author

tnull commented Feb 6, 2025

I created #1825 as an alternative, less-breaking solution.

I'm not sure I understand why it's considered less breaking (note the majority of the diff here is simply moving code)?

@evanlinjin
Copy link
Member

I'm not sure I understand why it's considered less breaking (note the majority of the diff here is simply moving code)?

It's breaking because Deref does not have a blanket implementation for impl<T> Deref for T { Target: T }. In other words, this PR forces BdkElectrumClient::inner to be of a type that impls Deref<Target: ElectrumApi> which does not include all types that impls ElectrumApi (which was the old constraint).

Anyhow, I closed #1825 as I think bitcoindevkit/rust-electrum-client#163 is the superior solution.

@tnull
Copy link
Contributor Author

tnull commented Feb 10, 2025

It's breaking because Deref does not have a blanket implementation for impl<T> Deref for T { Target: T }. In other words, this PR forces BdkElectrumClient::inner to be of a type that impls Deref<Target: ElectrumApi> which does not include all types that impls ElectrumApi (which was the old constraint).

Anyhow, I closed #1825 as I think bitcoindevkit/rust-electrum-client#163 is the superior solution.

Okay. I'm not sure adding that boilerplate is much preferable, but your choice. Going ahead and closing this.

@tnull tnull closed this Feb 10, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

3 participants