Default to forward for tagged hashes #2707
Conversation
We do not test the schemars stuff in `hashes`, instead we do it in a separate crate `extended_tests/schemars`. There is therefore no reason to implement `schemars::JsonSchema` for the `TestHashTag`.
In the tagged hash unit tests we are testing two separate things in a single test. To improve maintainability separate the test into two. Refactor only, no test coverage change.
Add a unit test to verify that the forward/backward functionality of the `sha256t_hash_newtype` works as advertised.
Displaying backward is an anomaly of Bitcoin Core's early days and the double SHA256 hash type. We should not let this unfortunate beast leak out into other places. Default to displaying forward when creating a new tagged hash and remove all the explicit attributes from `bitcoin` that just clutter the code.
tcharding
added
the
API break
github-actions
bot
added
C-bitcoin
Pull Request Test Coverage Report for Build 8978911818Details
💛 - Coveralls |
|
Can you add some sort of migration instructions telling people they can add I think this change is correct, even though it is theoretically an annoying break, because (I think) all tagged hashes are displayed forward. |
|
Done, as a separate patch. |
|
This one can go in now, acked for two weeks. |
hashes/CHANGELOG.md
Outdated
|
|
||
| Note please this usage if you need to display backward: | ||
|
|
||
| ``` |
There was a problem hiding this comment.
Maybe syntax highlighting?
| ``` | |
| ```rust |
There was a problem hiding this comment.
kek, and the 2-week timer is reset
But agreed, better to use the rust syntax highlighting.
There was a problem hiding this comment.
Its in the changelog, who cares? Or if we do care, lets do it as a follow up. Waiting another two weeks just for this sucks balls.
There was a problem hiding this comment.
Can you slip it in here?
I'll still ACK and (quickly) one-ACK merge. Agreed that a single nit about the CHANGELOG shouldn't reset the 2-week timer.
There was a problem hiding this comment.
Done, no other changes.
Since the default display direction is now forward, use `#[hash_newtype(backward)]` in the rustdocs on the macro. Also add an example usage to the changelog in case someone downstream is relying on the old default behaviour of displaying backwards (unlikely).
tcharding
force-pushed
the
04-23-default-to-forward
branch
from
f7e2b23
to
7685461
Compare
First three patches are preparation, improvements to the units tests in
sha256t.From the final patch:
This is an API break and may quietly break some users downstream - eventually we should stop doing that sort of thing.