Use Cardano API function calculateMinimumUTxO for minimum UTxO calculations.
#3368
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
force-pushed
the
jonathanknowles/use-cardano-api-calculateMinimumUTxO
branch
3 times, most recently
from
b0411ee
to
c27ec5d
Compare
jonathanknowles
changed the title
calculateMinimumUTxO to replace hand-coded function.calculateMinimumUTxO to replace hand-coded calculation.
jonathanknowles
force-pushed
the
jonathanknowles/use-cardano-api-calculateMinimumUTxO
branch
2 times, most recently
from
3519711
to
0d941e3
Compare
jonathanknowles
changed the title
calculateMinimumUTxO to replace hand-coded calculation.calculateMinimumUTxO for more reliable minimum UTxO calculations.
jonathanknowles
commented
Jul 1, 2022
jonathanknowles
commented
Jul 1, 2022
jonathanknowles
changed the title
calculateMinimumUTxO for more reliable minimum UTxO calculations.calculateMinimumUTxO for minimum UTxO calculations.
jonathanknowles
commented
Jul 1, 2022
jonathanknowles
commented
Jul 1, 2022
jonathanknowles
force-pushed
the
jonathanknowles/use-cardano-api-calculateMinimumUTxO
branch
3 times, most recently
from
c31c29d
to
b450e20
Compare
jonathanknowles
commented
Jul 4, 2022
Anviking
reviewed
Jul 4, 2022
Anviking
reviewed
Jul 4, 2022
lib/shelley/test/unit/Cardano/Wallet/Shelley/MinimumUTxOSpec.hs
Outdated
Show resolved
Hide resolved
Anviking
reviewed
Jul 4, 2022
jonathanknowles
commented
Jul 5, 2022
jonathanknowles
commented
Jul 5, 2022
jonathanknowles
force-pushed
the
jonathanknowles/use-cardano-api-calculateMinimumUTxO
branch
3 times, most recently
from
a2a2f7e
to
9b0a7a6
Compare
jonathanknowles
commented
Jul 12, 2022
lib/shelley/test/unit/Cardano/Wallet/Shelley/MinimumUTxOSpec.hs
Outdated
Show resolved
Hide resolved
jonathanknowles
added a commit
that referenced
this pull request
Jul 12, 2022
In response to review feedback: #3368 (comment) #3368 (comment)
jonathanknowles
changed the title
calculateMinimumUTxO for minimum UTxO calculations.calculateMinimumUTxO for minimum UTxO calculations.
This allows `verify` to modify any `Testable` value.
This function compares the stability of: - the Cardano API function 'calculateMinimumUTxO' - the wallet function 'computeMinimumCoinForUTxO'
…nds`. Simplify this property by moving the error pattern match to inner function `apiComputeMinCoin`. We assume that error pattern will never match. But if it does, then this property will still fail.
We add golden minimum UTxO values for all Shelley-based eras.
This extension was only used once in the whole module. In the particular case it was used, it is arguably clearer and safer to state the era explicitly.
In particular, we indicate more clearly which value was expected, and which value was returned. For example: ``` Failures: lib/shelley/test/unit/Cardano/Wallet/Shelley/MinimumUTxOSpec.hs:490:9: 1) computeMinimumCoinForUTxO, Golden Tests, goldenTests_computeMinimumCoinForUTxO Babbage, golden test #3 Falsified (after 1 test): resultExpected: Coin 1323170 Coin 1323170 resultReturned: Coin 1357650 Condition violated: resultReturned == resultExpected ```
In response to review feedback: #3368 (comment) #3368 (comment)
…bility`. We further explain the purpose of this property. If the Cardano API function `calculateMinimumUTxO` is ever changed so that it computes a fixed point before returning its result, this property will fail. However, this is valuable information, as it tells us that we might be able to revise our implementation of `computeMinimumCoinForUTxO` to reduce the level of overestimation. In response to review feedback.
….Gen`. This commit introduces generator function `genLedgerCoinOfSimilarMagnitude`, which, when given a coin value, will generate another coin value that is of a similar magnitude. When generating test values of `MinimumUTxO`, we apply the generator function `genLedgerCoinOfSimilarMagnitude` to protocol parameter values obtained from real mainnet genesis files. This enables us to generate more realistic values of `MinimumUTxO`. In response to review feedback: #3368 (comment)
…Spec`. Since we've already defined a set of test protocol parameter values within `MinimumUTxO.Gen`, we might as well re-use these test parameter values within `MinimumUTxOSpec`. This reduces the amount of repetition.
jonathanknowles
force-pushed
the
jonathanknowles/use-cardano-api-calculateMinimumUTxO
branch
from
a347f34
to
f08e0a0
Compare
…Out`. In response to review feedback: #3368 (comment)
In response to review feedback: #3368 (comment)
|
bors r+ |
iohk-bors bot
added a commit
that referenced
this pull request
Jul 13, 2022
3368: Use Cardano API function `calculateMinimumUTxO` for minimum UTxO calculations. r=jonathanknowles a=jonathanknowles ## Issue Number ADP-1978 ## Summary This PR uses the Cardano API function [`calculateMinimumUTxO`](https://github.com/input-output-hk/cardano-node/blob/42809ad3d420f0695eb147d4c66b90573d27cafd/cardano-api/src/Cardano/Api/Fees.hs#L1226) to replace our hand-coded [`computeMinimumAdaQuantity`](https://github.com/input-output-hk/cardano-wallet/blob/4f0b2f3c3cb9af139029418f4a2ebeb166fd0691/lib/shelley/src/Cardano/Wallet/Shelley/Compatibility/Ledger.hs#L121) function. ## Goals 1. **Ensure that we cannot underestimate minimum UTxO values.** _It's imperative that we do not underestimate minimum UTxO quantities, as this may result in the creation of transactions that are unacceptable to the ledger. In the case of change generation, this would be particularly problematic, as change outputs are generated automatically, and users do not have direct control over the ada quantities generated._ 2. **Decrease the complexity and fragility of minimum UTxO calculations within the wallet.** _The current design makes it trivial to specify a minimum UTxO function that's inappropriate for the era in which a transaction will be submitted. Using the wrong minimum UTxO function makes it possible to create transactions that will ultimately be rejected by the ledger._ ## Design We replace the existing `MinimumUTxOValue` type with the following pair of types: ```hs data MinimumUTxO where MinimumUTxONone :: MinimumUTxO MinimumUTxOConstant :: Coin -> MinimumUTxO MinimumUTxOForShelleyBasedEraOf :: MinimumUTxOForShelleyBasedEra -> MinimumUTxO data MinimumUTxOForShelleyBasedEra where MinimumUTxOForShelleyBasedEra :: ShelleyBasedEra era -> PParams (ShelleyLedgerEra era) -> MinimumUTxOForShelleyBasedEra ``` Where: > - `MinimumUTxONone` > _Specifies that there is no minimum UTxO value._ > - `MinimumUTxOConstant` > _Specifies a constant minimum UTxO value._ > - `MinimumUTxOForShelleyBasedEra` > _Represents a minimum UTxO function for a Shelley-based era. Ensures that values can only be constructed with a set of protocol parameters that are specific to the given era._ Furthermore, we make the following change to `Primitive.Types.ProtocolParameters`: ```patch - , minimumUTxOvalue - :: MinimumUTxOValue - -- ^ The minimum UTxO value. + , minimumUTxO + :: MinimumUTxO + -- ^ Represents a way of calculating minimum UTxO values. ``` Finally, we introduce function [`computeMinimumCoinForUTxO`](https://github.com/input-output-hk/cardano-wallet/blob/a2c414ad3e44d1af900280b9a890bb398783e400/lib/shelley/src/Cardano/Wallet/Shelley/MinimumUTxO.hs#L51): ```hs computeMinimumCoinForUTxO :: MinimumUTxO -> TokenMap -> Coin ``` This function computes a minimum `Coin`value for a `TokenMap` that is destined for inclusion in a transaction output. We use the above function to re-implement [`Shelley.Transaction.constraints.txOutputMinimumAdaQuantity`](https://github.com/input-output-hk/cardano-wallet/blob/a2c414ad3e44d1af900280b9a890bb398783e400/lib/shelley/src/Cardano/Wallet/Shelley/Transaction.hs#L1500), which means that both the coin selection and migration algorithms will take advantage of this updated implementation. ## Completed QA Tasks - [x] Transfer our existing test coverage of [`computeMinimumAdaQuantity`](https://github.com/input-output-hk/cardano-wallet/blob/4f0b2f3c3cb9af139029418f4a2ebeb166fd0691/lib/shelley/src/Cardano/Wallet/Shelley/Compatibility/Ledger.hs#L121) (including [golden tests](https://github.com/input-output-hk/cardano-wallet/blob/a2c414ad3e44d1af900280b9a890bb398783e400/lib/shelley/test/unit/Cardano/Wallet/Shelley/MinimumUTxOSpec.hs#L288)) to [`computeMinimumCoinForUTxO`](https://github.com/input-output-hk/cardano-wallet/blob/a2c414ad3e44d1af900280b9a890bb398783e400/lib/shelley/src/Cardano/Wallet/Shelley/MinimumUTxO.hs#L51). - [x] Verify that our minimum UTxO calculation is stable w.r.t. to the starting ada quantity. In particular, it should obey the following property: - if we call [`computeMinimumCoinForUTxO`](https://github.com/input-output-hk/cardano-wallet/blob/a2c414ad3e44d1af900280b9a890bb398783e400/lib/shelley/src/Cardano/Wallet/Shelley/MinimumUTxO.hs#L51) and receive `Coin` value `c1` - if we call the Cardano API function [`calculateMinimumUTxO`](https://github.com/input-output-hk/cardano-node/blob/42809ad3d420f0695eb147d4c66b90573d27cafd/cardano-api/src/Cardano/Api/Fees.hs#L1226) with `Coin` value `c1` and receive `Coin` value `c2` - then `c2` **_must not_** be greater than `c1`. (See [`prop_computeMinimumCoinForUTxO_shelleyBasedEra_stability`](https://github.com/input-output-hk/cardano-wallet/blob/a2c414ad3e44d1af900280b9a890bb398783e400/lib/shelley/test/unit/Cardano/Wallet/Shelley/MinimumUTxOSpec.hs#L213).) - [x] Determine what level of test coverage is applied to the Cardano API function [`calculateMinimumUTxO`](https://github.com/input-output-hk/cardano-node/blob/42809ad3d420f0695eb147d4c66b90573d27cafd/cardano-api/src/Cardano/Api/Fees.hs#L1226). Ensure that we build appropriate test coverage to cover any deficits. - [x] Ensure that we have test coverage to cover all valid lengths of addresses. - [x] Ensure that we have test coverage to characterize the effect of different coin sizes (up to the limit of `txOutMaxCoin`). - [x] Ensure that we have test coverage to characterize the effect of different token quantities (up to the limit of `txOutMaxTokenQuantity`). ## Completed Implementation Tasks - [x] Determine the most appropriate way to initialize field `minimumUTxO` within function `fromBlockfrostPP`. See 3c03560 and ADP-1994. - [x] Remove type `MinimumUTxOValue` from `Primitive.Types`. - [x] Remove function [`computeMinimumAdaQuantity`](https://github.com/input-output-hk/cardano-wallet/blob/4f0b2f3c3cb9af139029418f4a2ebeb166fd0691/lib/shelley/src/Cardano/Wallet/Shelley/Compatibility/Ledger.hs#L121) and associated tests. ## Completed Admin Tasks - [x] Create a ticket to review and revise the behaviour of the Blockfrost protocol parameter conversion function so that it does not hard-code the choice of minimum UTxO function to the Alonzo era (which uses a coins-per-word calculation). See ADP-1994. - [x] Create an issue on `cardano-node` to report the fact that the [`calculateMinimumUTxO`](https://github.com/input-output-hk/cardano-node/blob/42809ad3d420f0695eb147d4c66b90573d27cafd/cardano-api/src/Cardano/Api/Fees.hs#L1226) function does not appear to reach a fixed point before returning its result. (See IntersectMBO/cardano-node#4163). The current behaviour appears to be as follows: - If we call `calculateMinimumUTxO` with `Coin 0` and get `Coin a` back. - If we call `calculateMinimumUTxO` with `Coin a` and get `Coin b` back. - It's possible for `b` to be greater than `a`. Co-authored-by: Jonathan Knowles <[email protected]>
|
Build failed: Looks like it just gave up in the middle of compiling: |
|
I've restarted all failed jobs on hydra and verified that they all pass: I'll make another merge attempt. |
|
bors r+ |
iohk-bors bot
added a commit
that referenced
this pull request
Jul 13, 2022
3368: Use Cardano API function `calculateMinimumUTxO` for minimum UTxO calculations. r=jonathanknowles a=jonathanknowles ## Issue Number ADP-1978 ## Summary This PR uses the Cardano API function [`calculateMinimumUTxO`](https://github.com/input-output-hk/cardano-node/blob/42809ad3d420f0695eb147d4c66b90573d27cafd/cardano-api/src/Cardano/Api/Fees.hs#L1226) to replace our hand-coded [`computeMinimumAdaQuantity`](https://github.com/input-output-hk/cardano-wallet/blob/4f0b2f3c3cb9af139029418f4a2ebeb166fd0691/lib/shelley/src/Cardano/Wallet/Shelley/Compatibility/Ledger.hs#L121) function. ## Goals 1. **Ensure that we cannot underestimate minimum UTxO values.** _It's imperative that we do not underestimate minimum UTxO quantities, as this may result in the creation of transactions that are unacceptable to the ledger. In the case of change generation, this would be particularly problematic, as change outputs are generated automatically, and users do not have direct control over the ada quantities generated._ 2. **Decrease the complexity and fragility of minimum UTxO calculations within the wallet.** _The current design makes it trivial to specify a minimum UTxO function that's inappropriate for the era in which a transaction will be submitted. Using the wrong minimum UTxO function makes it possible to create transactions that will ultimately be rejected by the ledger._ ## Design We replace the existing `MinimumUTxOValue` type with the following pair of types: ```hs data MinimumUTxO where MinimumUTxONone :: MinimumUTxO MinimumUTxOConstant :: Coin -> MinimumUTxO MinimumUTxOForShelleyBasedEraOf :: MinimumUTxOForShelleyBasedEra -> MinimumUTxO data MinimumUTxOForShelleyBasedEra where MinimumUTxOForShelleyBasedEra :: ShelleyBasedEra era -> PParams (ShelleyLedgerEra era) -> MinimumUTxOForShelleyBasedEra ``` Where: > - `MinimumUTxONone` > _Specifies that there is no minimum UTxO value._ > - `MinimumUTxOConstant` > _Specifies a constant minimum UTxO value._ > - `MinimumUTxOForShelleyBasedEra` > _Represents a minimum UTxO function for a Shelley-based era. Ensures that values can only be constructed with a set of protocol parameters that are specific to the given era._ Furthermore, we make the following change to `Primitive.Types.ProtocolParameters`: ```patch - , minimumUTxOvalue - :: MinimumUTxOValue - -- ^ The minimum UTxO value. + , minimumUTxO + :: MinimumUTxO + -- ^ Represents a way of calculating minimum UTxO values. ``` Finally, we introduce function [`computeMinimumCoinForUTxO`](https://github.com/input-output-hk/cardano-wallet/blob/a2c414ad3e44d1af900280b9a890bb398783e400/lib/shelley/src/Cardano/Wallet/Shelley/MinimumUTxO.hs#L51): ```hs computeMinimumCoinForUTxO :: MinimumUTxO -> TokenMap -> Coin ``` This function computes a minimum `Coin`value for a `TokenMap` that is destined for inclusion in a transaction output. We use the above function to re-implement [`Shelley.Transaction.constraints.txOutputMinimumAdaQuantity`](https://github.com/input-output-hk/cardano-wallet/blob/a2c414ad3e44d1af900280b9a890bb398783e400/lib/shelley/src/Cardano/Wallet/Shelley/Transaction.hs#L1500), which means that both the coin selection and migration algorithms will take advantage of this updated implementation. ## Completed QA Tasks - [x] Transfer our existing test coverage of [`computeMinimumAdaQuantity`](https://github.com/input-output-hk/cardano-wallet/blob/4f0b2f3c3cb9af139029418f4a2ebeb166fd0691/lib/shelley/src/Cardano/Wallet/Shelley/Compatibility/Ledger.hs#L121) (including [golden tests](https://github.com/input-output-hk/cardano-wallet/blob/a2c414ad3e44d1af900280b9a890bb398783e400/lib/shelley/test/unit/Cardano/Wallet/Shelley/MinimumUTxOSpec.hs#L288)) to [`computeMinimumCoinForUTxO`](https://github.com/input-output-hk/cardano-wallet/blob/a2c414ad3e44d1af900280b9a890bb398783e400/lib/shelley/src/Cardano/Wallet/Shelley/MinimumUTxO.hs#L51). - [x] Verify that our minimum UTxO calculation is stable w.r.t. to the starting ada quantity. In particular, it should obey the following property: - if we call [`computeMinimumCoinForUTxO`](https://github.com/input-output-hk/cardano-wallet/blob/a2c414ad3e44d1af900280b9a890bb398783e400/lib/shelley/src/Cardano/Wallet/Shelley/MinimumUTxO.hs#L51) and receive `Coin` value `c1` - if we call the Cardano API function [`calculateMinimumUTxO`](https://github.com/input-output-hk/cardano-node/blob/42809ad3d420f0695eb147d4c66b90573d27cafd/cardano-api/src/Cardano/Api/Fees.hs#L1226) with `Coin` value `c1` and receive `Coin` value `c2` - then `c2` **_must not_** be greater than `c1`. (See [`prop_computeMinimumCoinForUTxO_shelleyBasedEra_stability`](https://github.com/input-output-hk/cardano-wallet/blob/a2c414ad3e44d1af900280b9a890bb398783e400/lib/shelley/test/unit/Cardano/Wallet/Shelley/MinimumUTxOSpec.hs#L213).) - [x] Determine what level of test coverage is applied to the Cardano API function [`calculateMinimumUTxO`](https://github.com/input-output-hk/cardano-node/blob/42809ad3d420f0695eb147d4c66b90573d27cafd/cardano-api/src/Cardano/Api/Fees.hs#L1226). Ensure that we build appropriate test coverage to cover any deficits. - [x] Ensure that we have test coverage to cover all valid lengths of addresses. - [x] Ensure that we have test coverage to characterize the effect of different coin sizes (up to the limit of `txOutMaxCoin`). - [x] Ensure that we have test coverage to characterize the effect of different token quantities (up to the limit of `txOutMaxTokenQuantity`). ## Completed Implementation Tasks - [x] Determine the most appropriate way to initialize field `minimumUTxO` within function `fromBlockfrostPP`. See 3c03560 and ADP-1994. - [x] Remove type `MinimumUTxOValue` from `Primitive.Types`. - [x] Remove function [`computeMinimumAdaQuantity`](https://github.com/input-output-hk/cardano-wallet/blob/4f0b2f3c3cb9af139029418f4a2ebeb166fd0691/lib/shelley/src/Cardano/Wallet/Shelley/Compatibility/Ledger.hs#L121) and associated tests. ## Completed Admin Tasks - [x] Create a ticket to review and revise the behaviour of the Blockfrost protocol parameter conversion function so that it does not hard-code the choice of minimum UTxO function to the Alonzo era (which uses a coins-per-word calculation). See ADP-1994. - [x] Create an issue on `cardano-node` to report the fact that the [`calculateMinimumUTxO`](https://github.com/input-output-hk/cardano-node/blob/42809ad3d420f0695eb147d4c66b90573d27cafd/cardano-api/src/Cardano/Api/Fees.hs#L1226) function does not appear to reach a fixed point before returning its result. (See IntersectMBO/cardano-node#4163). The current behaviour appears to be as follows: - If we call `calculateMinimumUTxO` with `Coin 0` and get `Coin a` back. - If we call `calculateMinimumUTxO` with `Coin a` and get `Coin b` back. - It's possible for `b` to be greater than `a`. Co-authored-by: Jonathan Knowles <[email protected]>
|
Build failed: |
|
bors r+ |
|
Build succeeded: |
iohk-bors
bot
deleted the
jonathanknowles/use-cardano-api-calculateMinimumUTxO
branch
WilliamKingNoel-Bot
pushed a commit
that referenced
this pull request
Jul 13, 2022
1 task
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-1978
Summary
This PR uses the Cardano API function
calculateMinimumUTxOto replace our hand-codedcomputeMinimumAdaQuantityfunction.Goals
Ensure that we cannot underestimate minimum UTxO values.
It's imperative that we do not underestimate minimum UTxO quantities, as this may result in the creation of transactions that are unacceptable to the ledger. In the case of change generation, this would be particularly problematic, as change outputs are generated automatically, and users do not have direct control over the ada quantities generated.
Decrease the complexity and fragility of minimum UTxO calculations within the wallet.
The current design makes it trivial to specify a minimum UTxO function that's inappropriate for the era in which a transaction will be submitted. Using the wrong minimum UTxO function makes it possible to create transactions that will ultimately be rejected by the ledger.
Design
We replace the existing
MinimumUTxOValuetype with the following pair of types:Where:
Furthermore, we make the following change to
Primitive.Types.ProtocolParameters:Finally, we introduce function
computeMinimumCoinForUTxO:This function computes a minimum
Coinvalue for aTokenMapthat is destined for inclusion in a transaction output.We use the above function to re-implement
Shelley.Transaction.constraints.txOutputMinimumAdaQuantity, which means that both the coin selection and migration algorithms will take advantage of this updated implementation.Completed QA Tasks
computeMinimumAdaQuantity(including golden tests) tocomputeMinimumCoinForUTxO.computeMinimumCoinForUTxOand receiveCoinvaluec1calculateMinimumUTxOwithCoinvaluec1and receiveCoinvaluec2c2must not be greater thanc1.(See
prop_computeMinimumCoinForUTxO_shelleyBasedEra_stability.)calculateMinimumUTxO. Ensure that we build appropriate test coverage to cover any deficits.txOutMaxCoin).txOutMaxTokenQuantity).Completed Implementation Tasks
minimumUTxOwithin functionfromBlockfrostPP. See 3c03560 and ADP-1994.MinimumUTxOValuefromPrimitive.Types.computeMinimumAdaQuantityand associated tests.Completed Admin Tasks
cardano-nodeto report the fact that thecalculateMinimumUTxOfunction does not appear to reach a fixed point before returning its result. (See [QUESTION] - Using the Cardano API to compute minimum UTxO values. IntersectMBO/cardano-node#4163). The current behaviour appears to be as follows:calculateMinimumUTxOwithCoin 0and getCoin aback.calculateMinimumUTxOwithCoin aand getCoin bback.bto be greater thana.