Witness api improvements and test cleanups
#4279
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Kixunil
added
Tests
code quality
Pull Request Test Coverage Report for Build 14022679486Details
💛 - Coveralls |
`Witness` was missing conversions from arrays (and variations) which was annoying when creating known-sized witnesses. These come up when spending statically-known inputs and in tests.
Since `Witness` is semantically equivalent to `&[&[u8]]` they should also be comparable. However we only had the impl to compare `Witness` with itself. Being able to compare `Witness` with other containers is particularly needed in tests.
Accessing the internals of tested object is problematic because it makes changes to layout harder, it makes tests harder to read and it checks implementation details rather than semantics of the API (behvaior). We had such tests in `primitives::witness` so this changes them to use the newly added APIs instead. However, it still leaves `from_parts__unstable` which needs to be dealt with separately.
The `Witness`-related tests were constructing `Witness` in over-complicated way by serializing `Vec<Vec<u8>>` and then deserializing `Witness` even though they were not supposed to test serialization but Taproot accessor methods. This was difficult to understand and maintain. This change simplifies them to just construct the `Witness` from array of `Vec<u8>`s using the recently-added constructors. Note that we already have serialization tests written separately so we're not losing meaningful coverage here.
Kixunil
force-pushed
the
witness-api-improvements-and-test-cleanups
branch
from
3f6c700 to
84bee2f
Compare
tcharding
approved these changes
Mar 23, 2025
Member
|
FWIW I rebase #4250 on top of this. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This is supposed to go in front of #4250
Witnesslacked a bunch of APIs that were making it harder to use and test, so this also adds them in addition to cleaning up tests. (I only realized they are missing when I tried to clean up tests and got a bunch of errors.)