dependencies: Remove duplicate bitcoin dependency #28
Conversation
Update bitcoincore_rpc dependency, remove direct dependency on bitcoin, and use bitcoincore_rpc::bitcoin as bitcoin Resolves bitcoin-teleport#18
Codecov Report
@@ Coverage Diff @@
## master #28 +/- ##
==========================================
+ Coverage 81.60% 81.68% +0.07%
==========================================
Files 8 8
Lines 2571 2571
==========================================
+ Hits 2098 2100 +2
+ Misses 473 471 -2
Continue to review full report at Codecov.
|
|
Thanks for the PR |
| #bitcoincore-rpc = {git="https://github.com/rust-bitcoin/rust-bitcoincore-rpc"} | ||
| bitcoin-wallet = "1.1.0" | ||
| bitcoin = "0.25" |
There was a problem hiding this comment.
On further thoughts.
I am not sure if its a good idea to use bitcoin from a derived crate like bitcoincore-rpc, even though it might expose it publicly.
We will be using bitcoin quite heavily through out the project, we might wanna move up in our bitcoin version (or add extra feature we need in bitcoin). For example encoding/decoding tests require rand feature of bitcoin.
I think because we might wanna use bitcoin in all of its capability, its better to use our own dependency of bitcoin, and not rely on the exposed dependency of derived crates.
So specific to this case, i think duplicating bitcoin dependency is better than latter running into weird cargo dep issues.
There was a problem hiding this comment.
Also, later if we decide to use some other bitcoin derived crates like rust-miniscript and if it has a different bitcoin dep version, cargo will be duplicating the bitcoin deps anyway.
There was a problem hiding this comment.
We will be using bitcoin quite heavily through out the project, we might wanna move up in our bitcoin version (or add extra feature we need in bitcoin).
Right, I would think it's pretty easy to add back the bitcoin dependency when that need comes up, and/or make a PR upstream to bitcoincore_rpc to add a feature(s) that expose bitcoin features. Currently the need isn't in the code base, and this PR cleans up an unnecessary duplicate dependency.
For example encoding/decoding tests require rand feature of bitcoin
That can be handled with a test dependency that doesn't affect the main build, but I get your point. What do you think of the idea I mention above?
Also, later if we decide to use some other bitcoin derived crates like rust-miniscript and if it has a different bitcoin dep version, cargo will be duplicating the bitcoin deps anyway.
That's a hypothetical problem that can be handled when it comes up. The problem doesn't exist in the current code base.
|
In general I am not in favor of using a parent dependency from a derived dependency. That makes the code look ugly and also reduces flexibility. When cargo downloads two different version of same deps, it generally means that there are different requirements at different deps. Which is the case here between So using This can be seen in the cargo lock as below So our This should be fixed once You can check that duplication goes away by making only the following changes in |
|
To confirm further you could let cargo figure the best case scenario by specifying wild card deps like this And it resolves into this So its really the And in this specific scenario i think its reasonable just to wait for upstream and have it solved automatically, than change our code base for it. |
Update
bitcoincore_rpcdependency, remove direct dependency onbitcoin, and usebitcoincore_rpc::bitcoinasbitcoinResolves #18