Add coverage for toBalanceConstraintsParams.
#2962
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
jonathanknowles
changed the title
subtract in adjustComputeSelectionLimit.adjustComputeSelectionLimit.
jonathanknowles
force-pushed
the
jonathanknowles/adp-1037/fix-subtract-argument-order
branch
5 times, most recently
from
433d8f0 to
5acc15b
Compare
jonathanknowles
changed the title
adjustComputeSelectionLimit.toBalanceConstraintsParams.
jonathanknowles
force-pushed
the
jonathanknowles/adp-1037/fix-subtract-argument-order
branch
4 times, most recently
from
f9aacc4 to
14d5bac
Compare
Anviking
approved these changes
Oct 11, 2021
Comment on lines
+3723
to
+3746
| prop_reduceSelectionLimitBy_lessThanOrEqual | ||
| :: SelectionLimit -> Int -> Property | ||
| prop_reduceSelectionLimitBy_lessThanOrEqual limit reduction = | ||
| prop_reduceSelectionLimitBy_coverage_limit limit .&&. | ||
| prop_reduceSelectionLimitBy_coverage_reduction reduction .&&. | ||
| limit `reduceSelectionLimitBy` reduction <= limit | ||
|
|
||
| prop_reduceSelectionLimitBy_reductionNegative | ||
| :: SelectionLimit -> Negative Int -> Property | ||
| prop_reduceSelectionLimitBy_reductionNegative limit (Negative reduction) = | ||
| prop_reduceSelectionLimitBy_coverage_limit limit .&&. | ||
| limit `reduceSelectionLimitBy` reduction == limit | ||
|
|
||
| prop_reduceSelectionLimitBy_reductionZero | ||
| :: SelectionLimit -> Property | ||
| prop_reduceSelectionLimitBy_reductionZero limit = | ||
| prop_reduceSelectionLimitBy_coverage_limit limit .&&. | ||
| limit `reduceSelectionLimitBy` 0 == limit | ||
|
|
||
| prop_reduceSelectionLimitBy_reductionPositive | ||
| :: SelectionLimit -> Positive Int -> Property | ||
| prop_reduceSelectionLimitBy_reductionPositive limit (Positive reduction) = | ||
| prop_reduceSelectionLimitBy_coverage_limit limit .&&. | ||
| limit == fmap (+ reduction) (limit `reduceSelectionLimitBy` reduction) |
| & fst & view #computeMinimumCost | ||
|
|
||
| costOriginal :: Coin | ||
| costOriginal = computeMinimumCostOriginal skeleton |
There was a problem hiding this comment.
Looking at this again it occurred to me: isn't it strange that the outer constraints contain the unadjusted and wrong minimum cost? If someone were to call the outer computeMinimumCost they would get the wrong results?
There was a problem hiding this comment.
If someone were to call the outer
computeMinimumCostthey would get the wrong results?
The only way a caller could call this function is if they prepared a SelectionSkeleton externally.
If the caller chose to do this, then the onus would be on the caller to provide a correct SelectionSkeleton that accurately represented the transaction for which they wanted to compute a minimum cost. But assuming they did prepare an accurate SelectionSkeleton, then this function would still give them the correct result, since the contract required of computeMinimumCost is that:
computeMinimumCost skeleton >= actualCostOfSerializedTx skeleton
|
bors r+ |
|
Merge conflict. |
This function reduces a selection limit by a given amount. We use this function to replace the incorrect subtraction in `adjustComputeSelectionLimit`. Background: The `subtract` function (from `Prelude`) is equivalent to `flip (-)`, which means that `subtract a b` is equivalent to `b - a`, rather than the other way round. Thanks to @KtorZ for discovering this.
This property verifies that the `toBalanceConstraintsParams` function adjusts the `computeSelectionLimit` function in an appropriate way, namely that: - the selection limit is only reduced when collateral is required; and that - the selection limit is reduced by the correct amount.
This property verifies that the `toBalanceConstraintsParams` function adjusts the `computeMinimumCost` function in an appropriate way, namely that: - the cost is only increased when collateral is required; and that - the cost is increased by the correct amount.
jonathanknowles
force-pushed
the
jonathanknowles/adp-1037/fix-subtract-argument-order
branch
from
14d5bac to
e3ea261
Compare
|
bors r+ |
|
Build succeeded: |
iohk-bors
bot
deleted the
jonathanknowles/adp-1037/fix-subtract-argument-order
branch
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 Number
ADP-1037
Summary
This PR:
subtractwith a dedicated functionreduceSelectionLimitBy.reduceSelectionLimitByhas the correct behaviour for all selection limits.toBalanceConstraintsParamsperforms the correct adjustment to thecomputeMinimumCostfunction.toBalanceConstraintsParamsperforms the correct adjustment to thecomputeSelectionLimitfunction.Background
The
subtractfunction (fromPrelude) is equivalent toflip (-), which means thatsubtract a bis equivalent tob - a, rather than the other way round.Thanks to @KtorZ for discovering this.