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

Add extra debugging information to Bech32 decoding corruption tests. #327

Merged
merged 6 commits into from
May 28, 2019

Conversation

jonathanknowles
Copy link
Contributor

@jonathanknowles jonathanknowles commented May 28, 2019

Issue Number

Issue #324

Overview

  • I have added extra debugging information to the suite of Bech32 decoding corruption tests.

These tests:

  1. corrupt a known-valid Bech32 string;
  2. attempt to decode the corrupted string;
  3. assert that decoding will fail.

Sample string:

       index of mutated char: 5
               original char: '1'
            replacement char: 'q'
             original string: "'14-b13zhcknjh"
            corrupted string: "'14-bq3zhcknjh"

Debugging information is only printed when QuickCheck finds a counterexample.

…aracters.

Additionally, make it a precondition that the adjacent characters are actually
different to one another.
…cters.

Additionally, make it a precondition that the string has actually been
successfully corrupted.
…cters.

Additionally, make it a precondition that the string has actually been
successfully corrupted.
Copy link
Contributor

@rvl rvl left a comment

Choose a reason for hiding this comment

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

Looks good, I tried changing some of the expectations and running the properties to see what it looks like.

, " original string: " <> show originalString
, "corrupted string: " <> show corruptedString ]
return $ counterexample description $
char1 /= char2 ==>
Copy link
Contributor

Choose a reason for hiding this comment

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

Could this precondition be removed? Then change then line below to shouldSatisfy (if char1 == char2 then isRight else isLeft)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@rvl wrote:

Could this precondition be removed? Then change then line below to shouldSatisfy (if char1 == char2 then isRight else isLeft)

Actually, we do want this precondition, as we're only interested in the cases where corruption actually does happen. By making corruption a precondition, we guarantee to produce cases that we're interested in (or else QuickCheck will give up), where decoding is presumed to fail.

As it turns out, only a small proportion of tests (those that fail to produce a corrupted string) are discarded here.

Though perhaps you have another reason in mind for removing the precondition?

, " original string: " <> show originalString
, " corrupted string: " <> show corruptedString ]
return $ counterexample description $
corruptedString /= originalString ==>
Copy link
Contributor

Choose a reason for hiding this comment

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

This precondition could be moved to the shouldBe easily enough.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@rvl wrote:

This precondition could be moved to the shouldBe easily enough.

I think this probably comes under the same discussion as #327 (comment)

@jonathanknowles jonathanknowles merged commit c45c5d8 into master May 28, 2019
@iohk-bors iohk-bors bot deleted the jonathanknowles/bech32-extra-debugging branch May 28, 2019 06:24
opt9 pushed a commit to opt9/cardano-sl that referenced this pull request Dec 16, 2019
4084: Backporting of proper error kernel-to-API mapping during batch addresses import r=KtorZ a=paweljakubas

## Description

<!--- A brief description of this PR and the problem is trying to solve -->
This is the last bit of backporting of batch import of addresses through APIs. Here, proper mapping of kernel addresses to API errors is enforced 

## Linked issue

<!--- Put here the relevant issue from YouTrack -->
Part of https://iohk.myjetbrains.com/youtrack/issue/CO-447 , namely
cardano-foundation/cardano-wallet#327



Co-authored-by: Pawel Jakubas <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants