Skip to content

Conversation

tcharding
Copy link
Member

Currently the ParseError is a mess. The correct fix is likely to use an associated type in Decodable for the error but that can wait.

Right now we are using hex in the Display impl for ParseError. This will cause trouble when we move the error type to consenus_encoding because hex is a feature. Even if we do not do so this code is a mainitenance burden.

Just manually print hex characters by accessing the array. Logic is already tested with a static string, this patch does not change the output.

@github-actions github-actions bot added the C-bitcoin PRs modifying the bitcoin crate label Aug 18, 2025
@apoelstra
Copy link
Member

In a0670fc:

This does not handle leading 0s correctly.

In 2dcfdbb:

Your unit test does not cover checksums with leading zeros.

Done in preparation for patching the `Display` impl of `ParseError` to
manually print hex.

Add a regression test.
Currently the `ParseError` is a mess. The correct fix is likely to use
an associated type in `Decodable` for the error but that can wait.

Right now we are using `hex` in the `Display` impl for `ParseError`.
This will cause trouble when we move the error type to
`consenus_encoding` because `hex` is a feature.

Just manually print hex characters by accessing the array. Logic is
already tested with a static string, this patch does not change the
output.
@tcharding
Copy link
Member Author

Good eye, fixed.

Copy link
Member

@apoelstra apoelstra left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK cde4736; successfully ran local tests

@apoelstra apoelstra merged commit ba30ce0 into rust-bitcoin:master Aug 24, 2025
24 checks passed
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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants