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

add AnyResolver #126

Closed
wants to merge 1 commit into from
Closed

add AnyResolver #126

wants to merge 1 commit into from

Conversation

zoedberg
Copy link
Contributor

@zoedberg zoedberg commented Feb 8, 2024

This PR adds an AnyResolver enum, which allows libraries to abstract the resolver type without using generics.

Copy link
Member

@dr-orlovsky dr-orlovsky left a comment

Choose a reason for hiding this comment

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

The code looks good, but I suggest to use From derive macro instead of writing a lot of boilerplate code (see my comments).

src/resolvers/any.rs Show resolved Hide resolved
src/resolvers/any.rs Outdated Show resolved Hide resolved
src/resolvers/any.rs Outdated Show resolved Hide resolved
src/resolvers/any.rs Outdated Show resolved Hide resolved
src/resolvers/any.rs Outdated Show resolved Hide resolved
src/resolvers/any.rs Outdated Show resolved Hide resolved
src/resolvers/mod.rs Outdated Show resolved Hide resolved
@dr-orlovsky
Copy link
Member

Even more important: feature-gated enums work really badly in practice. Just put the whole module behind a feature gate and let the enum contain both options.

Also, the enum should be #[non_exhaustive].

@dr-orlovsky dr-orlovsky added the enhancement New feature or request label Feb 10, 2024
@dr-orlovsky dr-orlovsky added this to the v0.11.0 milestone Feb 10, 2024
@zoedberg
Copy link
Contributor Author

Thanks for the suggestions. I've applied almost all requested changes but I cannot derive Clone and Debug because the inner enums (electrum::Resolver and esplora_blocking::Resolver) don't and can't.

Even more important: feature-gated enums work really badly in practice. Just put the whole module behind a feature gate and let the enum contain both options.

In this case they don't work bad, I've tried this enum and it works good, both selecting both features (electrum and esplora_blocking) or selecting only one. Multiple features in this case are useful because they allow lib users to define features in their libraries to allow selecting esplora, electrum or both. With a single feature this wouldn't be possible and users would be forced to have several unnecessary dependencies. And by adding more resolvers in the future this would get even worse.

Copy link
Member

@dr-orlovsky dr-orlovsky left a comment

Choose a reason for hiding this comment

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

We had this discussion years ago in rust-bitcoin, and the consensus was to avoid feature-gated enum variants at all cost: they always break compilation downstream once anybody changes the set of features. Simply, they are incompatible with any semantic versioning system.

I see no single argument for having them here - and a strong argument against (the above). If someone does not need one of them, they would not need this enum, thus feature gates add no value here.

@zoedberg
Copy link
Contributor Author

they always break compilation downstream once anybody changes the set of features

This can be true in many cases, if I change features it's very likely that code will need to be updated. I don't see this as a strong argument in this specific case. Also, the Resolvers will not change that often.
For example, also bdk uses this pattern, see their AnyBlockchain enum.

I see no single argument for having them here - and a strong argument against (the above). If someone does not need one of them, they would not need this enum, thus feature gates add no value here.

You're missing the argument that if I want to write a library that allows me to choose between just one or both indexers based on features, then I cannot use this AnyResolver, because I would be forced to have all indexers' dependencies (so features would be pointless). I don't see any value in adding this enum without feature-gates, if I want to depend on both indexers unconditionally then I can just turn on both features and use the 2 Resolvers independently. So if you really don't want to merge this as it is let's just close this PR, I will implement the AnyResolver in my library (even if I think this could have been useful also for other libraries).

@dr-orlovsky
Copy link
Member

Can you pls make this against master so we can check the CI?

@zoedberg
Copy link
Contributor Author

In the master branch the electrum resolver is missing. If you merge the v0.11 branch into master then I can make this against master

@dr-orlovsky dr-orlovsky mentioned this pull request Feb 13, 2024
@dr-orlovsky
Copy link
Member

Moving commit to #131, where we can run tests and check together with the whole set of new Electrum functionality

@cryptoquick
Copy link
Member

We had this discussion years ago in rust-bitcoin, and the consensus was to avoid feature-gated enum variants at all cost: they always break compilation downstream once anybody changes the set of features. Simply, they are incompatible with any semantic versioning system.

They certainly are orthogonal and provide an additional dimension of complexity, but unfortunately they're unavoidable if targeting different architectures where certain functionality is simply unavailable and features depending on it cannot be compiled in. But in general, I agree with you when this is not the case.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

3 participants