Fix index limits in Bech32 mutation tests

[jonathanknowles]

Issue Number

#331 and #332

Overview

This PR fixes a few issues introduced by PR #332:

1. The function chooseWithinDataPart gives us an incorrect upper bound for the index in the insertion, mutation, and deletion tests. These tests should actually limit their indices in subtly different ways:

   Test	Index Upper Bound	Notes
   Insertion	T.length originalString	(we can insert after the end of a string)
   Mutation	T.length originalString - 1
   Deletion	T.length originalString - 1
   Swapping	T.length originalString - 2	(we swap with the character to the right)

2. Limiting our choice of index to just the data part means that our tests are weaker than they could be, as we're not testing across the full length of a Bech32 string. Since the checksum is a function of both the data part and the human readable part, it makes sense to mutate all parts of the string.

3. In particular, we do need to check for mixed case characters at all locations in the string, and not just in the data part. According to the spec:

   Decoders MUST NOT accept strings where some characters are uppercase and some are lowercase (such strings are referred to as mixed case strings).

Changes

I have:

• Corrected the upper bounds for the insertion, mutation, and deletion tests.
• Adjusted all tests to test across the full length of a Bech32 string, and not just the data part.
• Introduced a type Bech32Char, which represents the full range of characters allowed in a Bech32 string. We use this type when generating mutations.

[jonathanknowles]

If we wanted to be slightly more precise, we could write:

let result = Bech32.decode corruptedString
let resultMatchesExpectations = \case 
        Left StringToDecodeTooShort                 -> True   
        Left StringToDecodeMissingSeparatorChar     -> True   
        Left (StringToDecodeContainsInvalidChars _) -> True   
        _                                           -> False  
return $ counterexample description $ 
    corruptedString /= originalString ==> 
        (T.length corruptedString === T.length originalString)
        .&&.  
        (result `shouldSatisfy` resultMatchesExpectations) 

[jonathanknowles]

Hi @KtorZ

@jonathanknowles I don't think there was an issue here to be honest.... The tests were already limiting their indexes to the worst case (swapping)....

From my understanding, after the merging PR #332, the test suite no longer tests:

• deletion of characters from the ends of strings.
• mutation of characters at the ends of strings.
• insertion of characters at the ends of strings.

This is because the relevant indices will no longer be generated.

Before merging of PR #332, these cases were tested (as far as I can tell). Was there any reason for us to remove coverage of these cases?

[KtorZ]

@jonathanknowles I don't think there was an issue here to be honest.... The tests were already limiting their indexes to the worst case (swapping).... I mean.. We've already spent A LOT of hours on this encoder / decoder and testing is already very reasonable on that module.

May we please... stop? For any further ideas of improvement, use the blackboard and describe what could be done.