Export some dependencies #289

stevenroose · 2019-07-06T13:45:19Z

This export bitcoin_hashes as bitcoin::hashes and secp256k1 as bitcoin::secp256k1.

This would mean that users of rust-bitcoin, (1) don't have to manually import those dependencies anymore and more importantly (2) can avoid certain dependency imcompatibility issues if some other module they use is using another version of bitcoin_hashes or secp256k1.

Solves #288.

apoelstra · 2019-07-06T14:20:31Z

Users will have trouble with multiple versions of secp256k1 regardless, until we figure out how to version the C symbols, but concept ACK.

Can you also export bitcoin_bech32? Or are we planning to drop that dependency in favor of an Elements-like inline solution?

stevenroose · 2019-07-06T22:43:03Z

Expecting to drop that with #255. But I can reexport it here for correctness and drop it in #255. Actually let me check #255, Sebastian told me bech32 should be good to go to finish that PR.

dongcarl · 2019-07-10T14:45:30Z

Concept ACK, after a brief glance there doesn't seem to be a way to transitively use dependencies in rust, so I suspect this will be useful.

thomaseizinger · 2019-07-11T02:53:01Z

Since I originally opened #288, I think this might be useful:

There is one pitfall that might be worth considering. If we re-export a whole crate, that crate's public API becomes part of rust-bitcoin's public API and hence any breaking changes to that are automatically breaking changes to rust-bitcoin.

A different approach, that doesn't eliminate all of those breaking changes but some, would be to selectively re-export the types we want. For example (to mimic the "old" API):

extern crate bitcoin_hashes;

pub mod hashes {
   pub use bitcoin_hashes::hash160::Hash as Hash160;
}

Obviously, if there are now breaking changes to Hash160, they are breaking changes for rust-bitcoin as-well.
In a nut-shell, I think the only thing you can avoid here is if bitcoin_hashes moves stuff around, those breaking changes could be avoided to bubble up to consumers of rust-bitcoin.

apoelstra · 2019-07-11T20:08:42Z

@thomaseizinger Breaking changes to bitcoin_hashes are breaking changes to rust-bitcoin anyway, because e.g. bitcoin_hashes::sha256d::Hash is exposed as part of bitcoin::OutPoint.

apoelstra · 2019-07-11T20:09:50Z

In principle (by using the dtolnay semver trick) there could be a major version bump to bitcoin_hashes that wouldn't affect rust-bitcoin users, but in practice the semver trick has not been available to us.

stevenroose · 2019-07-25T07:47:14Z

rebased

dongcarl

You missed a place :-)

diff --git a/src/util/bip158.rs b/src/util/bip158.rs
index 30aee13..ac517dd 100644
--- a/src/util/bip158.rs
+++ b/src/util/bip158.rs
@@ -50,7 +50,7 @@ use std::error;
 use std::fmt::{Display, Formatter};
 use std::io::Cursor;

-use bitcoin_hashes::{Hash, sha256d, siphash24};
+use hashes::{Hash, sha256d, siphash24};
 use byteorder::{ByteOrder, LittleEndian};

 use blockdata::block::Block;

Otherwise, good change overall.

To other reviewers: I was concerned that users of our API would be confused that there existed a hashes crate we were using, but upon cargo doc --open I can see that pub extern crate bitcoin_hashes as hashes; is right there on the front page. So, all good!

stevenroose · 2019-07-29T09:40:22Z

To other reviewers: I was concerned that users of our API would be confused that there existed a hashes crate we were using, but upon cargo doc --open I can see that pub extern crate bitcoin_hashes as hashes; is right there on the front page. So, all good!

True. But I envision users that only want bitcoin_hashes without bitcoin will be library devs in the rust-bitcoin community anyway, so they will know about the crate. I think most other users will just only depend on bitcoin and use bitcoin::hashes:: instead.

apoelstra · 2019-08-01T15:31:45Z

travis failed because of #300 which appears to be an unrelated bug. You can just kick it and it should be ok.

dongcarl · 2019-08-02T21:18:48Z

Kicked

apoelstra · 2019-08-12T17:23:13Z

Needs rebase

stevenroose · 2019-08-13T11:32:25Z

Rebased

codecov-io · 2019-08-15T13:45:28Z

Codecov Report

Merging #289 into master will not change coverage.
The diff coverage is 80%.

@@           Coverage Diff           @@
##           master     #289   +/-   ##
=======================================
  Coverage   81.85%   81.85%           
=======================================
  Files          38       38           
  Lines        6951     6951           
=======================================
  Hits         5690     5690           
  Misses       1261     1261

Impacted Files	Coverage Δ
src/network/message_filter.rs	`0% <ø> (ø)`	⬆️
src/util/base58.rs	`80.68% <ø> (ø)`	⬆️
src/util/contracthash.rs	`78.5% <ø> (ø)`	⬆️
src/util/misc.rs	`96.77% <ø> (ø)`	⬆️
src/util/address.rs	`86.12% <ø> (ø)`	⬆️
src/util/bip158.rs	`93.09% <ø> (ø)`	⬆️
src/lib.rs	`100% <ø> (ø)`	⬆️
src/util/bip143.rs	`100% <ø> (ø)`	⬆️
src/util/psbt/mod.rs	`86.06% <ø> (ø)`	⬆️
src/network/message_blockdata.rs	`36.36% <ø> (ø)`	⬆️
... and 8 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5f4de0f...c011727. Read the comment docs.

dongcarl

Please fix the two instances of bitcoin_hashes here:

https://github.com/rust-bitcoin/rust-bitcoin/pull/289/files#diff-ca925ea633ee82c8b9dfd44534146107R575-R578

I will approve and merge after.

stevenroose · 2019-08-16T14:08:28Z

Please fix the two instances of bitcoin_hashes here:

https://github.com/rust-bitcoin/rust-bitcoin/pull/289/files#diff-ca925ea633ee82c8b9dfd44534146107R575-R578

I will approve and merge after.

Hmm, I left them on purpose tbh. But I can fix them. I'll rename them to bitcoin::hashes then, is that ok? Hashes itself doesn't mean anything outside this crate.

stevenroose · 2019-08-16T14:10:06Z

Done.

…

On Fri, Aug 16, 2019 at 2:45 PM Carl Dong ***@***.***> wrote: ***@***.**** requested changes on this pull request. Nit: please fix the two instances of bitcoin_hashes here: https://github.com/rust-bitcoin/rust-bitcoin/pull/289/files#diff-ca925ea633ee82c8b9dfd44534146107R575-R578 I will approve and merge after. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#289?email_source=notifications&email_token=AAGQLXFAM2A2B3U73PXA4QDQE2VPTA5CNFSM4H6TOVIKYY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOCBZMTBY#pullrequestreview-275958151>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAGQLXFIYKJYCGX4F2JSH7TQE2VPTANCNFSM4H6TOVIA> .

stevenroose · 2019-08-16T14:11:19Z

In retrospect, it's an internal macro. I changed it to just hashes.

dongcarl

tested-ACK c011727

apoelstra previously approved these changes Jul 11, 2019

View reviewed changes

thomaseizinger mentioned this pull request Jul 14, 2019

Bump secp256k1 from 0.12.2 to 0.14.0 comit-network/comit-rs#1090

Closed

stevenroose dismissed apoelstra’s stale review via d9d7e8d July 25, 2019 07:23

stevenroose force-pushed the export-hashes branch from 455b06e to d9d7e8d Compare July 25, 2019 07:23

stevenroose force-pushed the export-hashes branch from d9d7e8d to 88525ea Compare July 26, 2019 13:35

dongcarl suggested changes Jul 26, 2019

View reviewed changes

apoelstra added this to the 0.19 milestone Jul 27, 2019

stevenroose force-pushed the export-hashes branch from 88525ea to 3efd3e6 Compare July 29, 2019 09:45

apoelstra previously approved these changes Aug 2, 2019

View reviewed changes

stevenroose dismissed apoelstra’s stale review via 4097b69 August 13, 2019 11:32

stevenroose force-pushed the export-hashes branch from 3efd3e6 to 4097b69 Compare August 13, 2019 11:32

stevenroose force-pushed the export-hashes branch from 4097b69 to 41f9b7a Compare August 15, 2019 13:31

apoelstra previously approved these changes Aug 15, 2019

View reviewed changes

dongcarl suggested changes Aug 16, 2019

View reviewed changes

stevenroose dismissed apoelstra’s stale review via 3cb8cc2 August 16, 2019 14:09

stevenroose force-pushed the export-hashes branch from 41f9b7a to 3cb8cc2 Compare August 16, 2019 14:09

stevenroose force-pushed the export-hashes branch from 3cb8cc2 to 42c3fbc Compare August 16, 2019 14:11

stevenroose added 2 commits August 16, 2019 15:52

Rename bitcoin_hashes dependency to hashes

48f4c19

Re-export bitcoin_hashes and secp256k1 dependency

c011727

stevenroose force-pushed the export-hashes branch from 42c3fbc to c011727 Compare August 16, 2019 14:53

dongcarl approved these changes Aug 16, 2019

View reviewed changes

TheBlueMatt approved these changes Aug 16, 2019

View reviewed changes

dongcarl merged commit 166e2bd into rust-bitcoin:master Aug 16, 2019

thomaseizinger mentioned this pull request Aug 17, 2019

Consider re-exporting types from bitcoin_hashes #288

Closed

elichai mentioned this pull request Aug 23, 2019

Updating dependencies lightningdevkit/rust-lightning#377

Merged

Export some dependencies #289

Export some dependencies #289

Uh oh!

Conversation

stevenroose commented Jul 6, 2019

Uh oh!

apoelstra commented Jul 6, 2019

Uh oh!

stevenroose commented Jul 6, 2019

Uh oh!

dongcarl commented Jul 10, 2019

Uh oh!

thomaseizinger commented Jul 11, 2019

Uh oh!

apoelstra commented Jul 11, 2019

Uh oh!

apoelstra commented Jul 11, 2019

Uh oh!

stevenroose commented Jul 25, 2019

Uh oh!

dongcarl left a comment

Choose a reason for hiding this comment

Uh oh!

stevenroose commented Jul 29, 2019

Uh oh!

apoelstra commented Aug 1, 2019

Uh oh!

dongcarl commented Aug 2, 2019

Uh oh!

apoelstra commented Aug 12, 2019

Uh oh!

stevenroose commented Aug 13, 2019

Uh oh!

codecov-io commented Aug 15, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

dongcarl left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stevenroose commented Aug 16, 2019

Uh oh!

stevenroose commented Aug 16, 2019 via email

Uh oh!

stevenroose commented Aug 16, 2019

Uh oh!

dongcarl left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

codecov-io commented Aug 15, 2019 •

edited

Loading

dongcarl left a comment •

edited

Loading