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

bp-wallet default-features false #128

Closed
wants to merge 1 commit into from

Conversation

zoedberg
Copy link
Contributor

By adding bp-wallet to the dependencies without specifying default-features = false we are also importing bp-esplora (because bp-wallet specifies default = ["esplora"]), making the rgb-runtime definition of the esplora feature (esplora = ["bp-esplora", "bp-wallet/esplora"]) pointless.
With this PR we import bp-wallet without default features, allowing users of rgb-runtime to avoid depending on bp-esplora unconditionally.

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.

Thank you for figuring this out. I think the right way to fix the issue is to remove esplora from default features in bp-wallet. I did this here: BP-WG/bp-wallet@9d1cf14

What do you think?

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

BTW this is duplicate of #127

@dr-orlovsky dr-orlovsky mentioned this pull request Feb 13, 2024
@zoedberg
Copy link
Contributor Author

I was not sure it was correct removing esplora from the default features of bp-wallet, but this also does the trick so LGTM, thanks

@zoedberg zoedberg closed this Feb 13, 2024
@dr-orlovsky
Copy link
Member

removing esplora from the default features of bp-wallet

I think it was a mistake it got there first: bp-wallet is assumed to be a library. Originally it was a cli and that's why it needed default feature so users can do cargo install bp-wallet without --all-features (which always confuses people, who do not read README and leads to plenties of opened issue saying the same thing "this does not compile!" over and over again). But since it is library, and the cli name is bp-util, it was safe to remove it there

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.

2 participants