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

hashes: make hex optional #2711

Draft
wants to merge 15 commits into
base: master
Choose a base branch
from

Conversation

tcharding
Copy link
Member

@tcharding tcharding commented Apr 23, 2024

Done on top of #2708, just a showcase to share the serendipity I felt just now.

  • Patch 1 removes the now unused public macro hashes::hex_fmt_impl
  • Patch 2 makes hex optional - and the patch is trivial!

Close: #2654

We would like to check for API changes during development and in CI so
that such changes can be discussed separately from the code. We have API
listings already.

Add tooling and documentation for creating API listings for the public
crates within the workspace (i.e., not `internals`).

Add API text files from an initial run of the script.
Add a job that runs the new script to check for changes to the public
APIs of `hashes` and `bitcoin`.
Add a `just` command to run the API checking script. Makes it more
discoverable.
Run `just check-api`, no other manual changes.
Currently we use `hashes::hash_newtype!` to create various types that
wrap hash types. While this works it makes the `hashes` crate less
maintainable and more prone to change because logic that we want to
control from `bitcoin` is in `hashes`. We want to stabalise the leaf
crates as quickly as possible, moving the `hash_newtype` logic to
`bitcoin` allows us to simplify `hashes` and put the code closer to
where it is used.

Note, this patch does not remove `hashes::hash_newtype`, it just adds a
bunch of additional macros to `bitcoin::internal_macros`.

Note on design:

Instead of a single enormous macro, we add multiple smaller macros. This
allows implementing just what is needed for a type and gives finer
control over what is implemented and what is in the public API. All at
the cost of being a bit more verbose.
Show what changed with after removing usage of `hashes::hash_newtype!`.
As we did for the `hashes::hash_newtype!` macro; replace the
`hashes::sha256t_hash_newtype!` macro with a collection of more simple
macros in the `bitcoin` crate.

This improves maintainability by giving putting control of the macro
logic in the crate that uses the macro. Also assists in stabalizing
`hashes` sooner because we can now remove the two macros from `hashes`.
The `hashes` crate is _hard_ to work on, every time one wants to change
something there are various macros that are far from trivial to
understand.

We would like to improve the `hashes` API so we can stabalize the crate.
In an effort to assist in doing so remove the two hash newtype macros.

This is obviously an API breaking change - is anyone outside of
`rust-bitcoin` using these macros, if so they are going to be annoyed
with us.

Remove the `hash_newtype!` and `sha256_hash_newtype` macros.
As can be seen from the macros in `bitcoin` we can fully support tagged
SHA256 hashes without needing a module in `hashes`.

In an effort to make the `hashes` crate easier to work on remove the
`sha256t` module all together.

This risks rugging and annoying users but either I need 10 more points
of IQ to work on the `hashes` crate or we have to do something to reduce
the complexity.
This macro is no longer used within this crate or in `bitcoin` - lets
remove it.

Done in preparation for making `hex` an optional dependency. This patch
is an unnecessarily brutal change, we could deprecated for a release
cycle instead.
@github-actions github-actions bot added ci C-bitcoin PRs modifying the bitcoin crate C-hashes PRs modifying the hashes crate doc labels Apr 23, 2024
@coveralls
Copy link

Pull Request Test Coverage Report for Build 8797148222

Details

  • 140 of 161 (86.96%) changed or added relevant lines in 14 files are covered.
  • 1 unchanged line in 1 file lost coverage.
  • Overall coverage increased (+0.02%) to 83.151%

Changes Missing Coverage Covered Lines Changed/Added Lines %
bitcoin/src/internal_macros.rs 64 85 75.29%
Files with Coverage Reduction New Missed Lines %
bitcoin/src/internal_macros.rs 1 70.27%
Totals Coverage Status
Change from base Build 8793522744: 0.02%
Covered Lines: 19118
Relevant Lines: 22992

💛 - Coveralls

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-bitcoin PRs modifying the bitcoin crate C-hashes PRs modifying the hashes crate ci doc
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Make hex an optional dep for hashes
2 participants