Make UTxOIndex agnostic to the distinction between ada and non-ada assets.
#3217
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
UTxOIndex agnostic to the distinction between ada and non-ada assets.UTxOIndex agnostic to the distinction between ada and non-ada assets.
jonathanknowles
commented
Apr 5, 2022
jonathanknowles
commented
Apr 5, 2022
jonathanknowles
added a commit
that referenced
this pull request
Apr 5, 2022
In response to review feedback: #3217 (comment)
jonathanknowles
added a commit
that referenced
this pull request
Apr 5, 2022
In response to review feedback: #3217 (comment)
sevanspowell
approved these changes
Apr 5, 2022
jonathanknowles
commented
Apr 5, 2022
lib/core/test/unit/Cardano/Wallet/Primitive/Types/UTxOIndexSpec.hs
Outdated
Show resolved
Hide resolved
jonathanknowles
added a commit
that referenced
this pull request
Apr 5, 2022
In response to review feedback: #3217 (comment)
jonathanknowles
added a commit
that referenced
this pull request
Apr 5, 2022
In response to review feedback: #3217 (comment)
jonathanknowles
force-pushed
the
jonathanknowles/generalize-utxo-index
branch
from
f730c15 to
8c6b725
Compare
jonathanknowles
commented
Apr 5, 2022
This is meant to provide a replacement for `BundleCategory`, but with categories that are agnostic to the type of assets.
This is similar to the original `assets` function, but uses new-style indexing.
This is similar to `selectRandom`, but uses `SelectionFilterNew` instead of `SelectionFilter`. The `SelectionFilterNew` type is similar to `SelectionFilter`, but provides filter conditions that are agnosic to the type of assets.
This is similar to `prop_SelectionFilter_coverage` but covers new filter conditions.
Additionally, add some coverage checks to verify that: - sometimes something is selected. - sometimes nothing is selected.
In response to review feedback: #3217 (comment)
In response to review feedback: #3217 (comment)
jonathanknowles
force-pushed
the
jonathanknowles/generalize-utxo-index
branch
from
21a617a to
bc06ef9
Compare
|
bors r+ |
iohk-bors bot
added a commit
that referenced
this pull request
Apr 5, 2022
3217: Make `UTxOIndex` agnostic to the distinction between ada and non-ada assets. r=jonathanknowles a=jonathanknowles ## Issue Number ADP-1419 ## Summary This PR makes the `UTxOIndex` type **_agnostic_** to the distinction between **_ada_** and **_non-ada_** assets. ## Motivation We want to evolve the coin selection algorithm so that: - the concept of "value" (provided by inputs and consumed by outputs) is **_generalized_**. - all asset types are treated equally, with no special treatment for **_ada_** versus **_non-ada_** assets. Currently, there are a few obstacles that prevent such a generalization: 1. The `UTxOIndex` type indexes ada quantities differently from other assets. 2. The `computeMinimumCost` function is ada-specific. 3. The `computeMinimumAdaQuantity` function is ada-specific. This PR tackles the **_first_** of these obstacles. ## Details We make the following change to `SelectionFilter`: ```patch - data SelectionFilter - = WithAdaOnly - | WithAssetOnly AssetId - | WithAsset AssetId - | Any + data SelectionFilter asset + = SelectSingleton asset + | SelectPairWith asset + | SelectAnyWith asset + | SelectAny ``` We also (temporarily) introduce a type called `Asset`, which can represent both ada and non-ada assets: ```hs data Asset = AssetLovelace | Asset AssetId ``` This allows us to generalize the way we perform random selection for ada and non-ada assets in the `Balance` module: ```patch selectAdaQuantity = - selectMatchingQuantity (WithAdaOnly :| [Any]) + selectMatchingQuantity + [ SelectSingleton AssetLovelace + , SelectPairWith AssetLovelace + , SelectAnyWith AssetLovelace + ] selectNonAdaAssetQuantity asset = - selectMatchingQuantity [WithAssetOnly asset, WithAsset asset] + selectMatchingQuantity + [ SelectSingleton (Asset asset) + , SelectPairWith (Asset asset) + , SelectAnyWith (Asset asset) + ] ``` In the above functions, selection filters are processed in the following order of priority: 1. `SelectSingleton` matches UTxOs that contain **_just_** the given asset and **_no other_** assets 2. `SelectPairWith` matches UTxOs that contain the given asset and **_one other_** asset. 2. `SelectAnyWith` matches UTxOs that contain the given asset and **_any number_** of other assets. Because both selection functions now share **_exactly the same priority order_**, we can simplify these definitions even further by extracting out the shared priority order and parameterizing over the type of asset. Co-authored-by: Jonathan Knowles <[email protected]>
|
Build failed: It looks as though this affects (very slightly) one of the golden tests for I've pushed a temporary commit that fixes the test, but I'll continue to investigate. |
|
bors r+ |
|
Build succeeded: |
WilliamKingNoel-Bot
pushed a commit
that referenced
this pull request
Apr 5, 2022
iohk-bors bot
added a commit
that referenced
this pull request
Apr 6, 2022
3220: Generalize `SelectionFilter` tests within `UTxOIndexSpec`. r=jonathanknowles a=jonathanknowles ## Issue Number ADP-1419 ## Summary This PR follows on from #3217. It: - Replaces all `prop_selectRandom_one*` properties with a generalized `prop_selectRandom` property. - Replaces all `prop_selectRandom_all*` properties with a generalized `prop_selectRandom_all` property. In both cases, we use coverage checks to verify that we maintain the same coverage as before. Co-authored-by: Jonathan Knowles <[email protected]>
iohk-bors bot
added a commit
that referenced
this pull request
Apr 6, 2022
3220: Generalize `SelectionFilter` tests within `UTxOIndexSpec`. r=jonathanknowles a=jonathanknowles ## Issue Number ADP-1419 ## Summary This PR follows on from #3217. It: - Replaces all `prop_selectRandom_one*` properties with a generalized `prop_selectRandom` property. - Replaces all `prop_selectRandom_all*` properties with a generalized `prop_selectRandom_all` property. In both cases, we use coverage checks to verify that we maintain the same coverage as before. Co-authored-by: Jonathan Knowles <[email protected]>
paolino
pushed a commit
that referenced
this pull request
Apr 26, 2022
In response to review feedback: #3217 (comment)
paolino
pushed a commit
that referenced
this pull request
Apr 26, 2022
In response to review feedback: #3217 (comment)
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-1419
Summary
This PR makes the
UTxOIndextype agnostic to the distinction between ada and non-ada assets.Motivation
We want to evolve the coin selection algorithm so that:
Currently, there are a few obstacles that prevent such a generalization:
UTxOIndextype indexes ada quantities differently from other assets.computeMinimumCostfunction is ada-specific.computeMinimumAdaQuantityfunction is ada-specific.This PR tackles the first of these obstacles.
Details
We make the following change to
SelectionFilter:We also (temporarily) introduce a type called
Asset, which can represent both ada and non-ada assets:This allows us to generalize the way we perform random selection for ada and non-ada assets in the
Balancemodule:In the above functions, selection filters are processed in the following order of priority:
SelectSingletonmatches UTxOs that contain just the given asset and no other assets
SelectPairWithmatches UTxOs that contain the given asset and one other asset.
SelectAnyWithmatches UTxOs that contain the given asset and any number of other assets.
Because both selection functions now share exactly the same priority order, we can simplify these definitions even further by extracting out the shared priority order and parameterizing over the type of asset.