Add more property tests for distributeSurplus.
#3243
Merged
Conversation
This file contains 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
When testing `distributeSurplus`, we definitely want to test the behaviour when the given surplus is zero. The existing `Arbitrary` instance for `Coin` always generates non-zero coins. This commit introduces a helper type `TxBalanceSurplus`, with an `Arbitrary` instance that allows zero-valued coins.
Specifically, check that we cover the case where the surplus is zero.
Fixes issue #3242. This bug caused the following property to sporadically fail: "distributeSurplusDelta, when no change output is present, will burn surplus as excess fees" Example failure seeds: --seed 862218635 --seed 1926926873 --seed 404630283 --seed 1602363838 --seed 1472406660 --seed 31096920 --seed 166643919 Failures were always of the form: ```hs expected: Right (TxFeeAndChange {fee = Coin 1, change = []}) but got: Left (ErrMoreSurplusNeeded (Coin 0)) ``` This indicates that it was possible for `burnSurplusAsFees` to fail with an indicated shortfall of zero. To fix this, we update `burnSurplusAsFees` to only fail if the computed shortfall is non-zero. We also adjust the following property to check that failure cases are covered, and to verify that the shortfall is correct: `prop_distributeSurplusDelta_coversCostIncreaseAndConservesSurplus`
jonathanknowles
force-pushed
the
jonathanknowles/improve-distributeSurplus-tests
branch
from
e0ba91a
to
578e50a
Compare
Anviking
approved these changes
Apr 21, 2022
| -- | ||
| -- - Any increase in cost is covered: | ||
| -- If the total cost has increased by 𝛿c, then the fee value | ||
| -- will have increased by at least 𝛿c. |
| instance Arbitrary (TxBalanceSurplus Coin) where | ||
| -- We want to test cases where the surplus is zero. So it's important that | ||
| -- we do not restrict ourselves to positive coins here. | ||
| arbitrary = TxBalanceSurplus <$> frequency |
There was a problem hiding this comment.
To be honest, I wonder if not the Arbitrary Coin instance probably should include Coin 0.
I tried this, and all the tests in the module seemed to pass. But still out of caution refraining from changing this now. It would be easier to change after we reorganize the module structure.
|
bors r+ |
|
Build succeeded: |
iohk-bors
bot
deleted the
jonathanknowles/improve-distributeSurplus-tests
branch
WilliamKingNoel-Bot
pushed a commit
that referenced
this pull request
Apr 21, 2022
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Issue
ADP-1514
(Also fixes #3242)
Summary
This PR adds a pair of property tests for
distributeSurplus:prop_distributeSurplus_onSuccess_conservesSurplusprop_distributeSurplus_onSuccess_coversCostIncreaseThis PR also adds descriptions of these properties to the comment for
TransactionLayer.distributeSurplus.Finally, this PR also improves test coverage of the surplus value, by defining and using a
TxBalanceSurplus Coingenerator that:Coinvalues (previously, this value was excluded).