-
Notifications
You must be signed in to change notification settings - Fork 217
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
Modify coin selection algorithm to support minting and burning #2725
Conversation
lib/core/test/unit/Cardano/Wallet/Primitive/CoinSelection/MA/RoundRobinSpec.hs
Show resolved
Hide resolved
🤔 Interesting, sending POST on this branch to the ep gives me
|
Ah, interesting @piotr-iohk. My stub definition was malformed. Should be fixed now. Of course by "fixed" I mean the stub will still return:
Because the API endpoint is still stubbed, I've only changed coin selection. It should correctly verify the form of the request data though! |
fd4960f
to
bfd7afa
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Many thanks for creating this PR.
I've not yet finished looking through it, but what I've seen so far looks good.
I've added a few comments and suggestions. Will continue looking tomorrow!
lib/core/src/Cardano/Wallet/Primitive/CoinSelection/MA/RoundRobin.hs
Outdated
Show resolved
Hide resolved
lib/core/src/Cardano/Wallet/Primitive/CoinSelection/MA/RoundRobin.hs
Outdated
Show resolved
Hide resolved
lib/core/src/Cardano/Wallet/Primitive/CoinSelection/MA/RoundRobin.hs
Outdated
Show resolved
Hide resolved
lib/core/src/Cardano/Wallet/Primitive/CoinSelection/MA/RoundRobin.hs
Outdated
Show resolved
Hide resolved
lib/core/src/Cardano/Wallet/Primitive/CoinSelection/MA/RoundRobin.hs
Outdated
Show resolved
Hide resolved
lib/core/src/Cardano/Wallet/Primitive/CoinSelection/MA/RoundRobin.hs
Outdated
Show resolved
Hide resolved
lib/core/src/Cardano/Wallet/Primitive/CoinSelection/MA/RoundRobin.hs
Outdated
Show resolved
Hide resolved
lib/core/src/Cardano/Wallet/Primitive/CoinSelection/MA/RoundRobin.hs
Outdated
Show resolved
Hide resolved
lib/core/src/Cardano/Wallet/Primitive/CoinSelection/MA/RoundRobin.hs
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi Sam.
Some more comments. I'm about to look at the property tests. Will leave more comments later!
lib/core/src/Cardano/Wallet/Primitive/CoinSelection/MA/RoundRobin.hs
Outdated
Show resolved
Hide resolved
lib/core/src/Cardano/Wallet/Primitive/CoinSelection/MA/RoundRobin.hs
Outdated
Show resolved
Hide resolved
lib/core/src/Cardano/Wallet/Primitive/CoinSelection/MA/RoundRobin.hs
Outdated
Show resolved
Hide resolved
lib/core/src/Cardano/Wallet/Primitive/CoinSelection/MA/RoundRobin.hs
Outdated
Show resolved
Hide resolved
lib/core/src/Cardano/Wallet/Primitive/CoinSelection/MA/RoundRobin.hs
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @sevanspowell . Here's a first review of the property tests.
Will add more comments later!
-- "sort it". One quick way to get ascending partial order is to make the | ||
-- change maps ourself. Presuming of course | ||
-- "makeChangeForNonUserSpecifiedAssets" is correctly implemented, which | ||
-- other tests should confirm. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Suggestion: consider using NE.scanl
to generate values in ascending partial order.
You're right that an arbitrary list of TokenMap
values [m1, m2, m3, ..., mi]
is not ordered.
However, if we treat the generated list as a list of differences, then we can transform those differences into a list that is guaranteed to be in ascending partial order:
[ m1
, m1 <> m2
, m1 <> m2 <> m3
, ...
, mi <> m2 <> m3 <> ... <> mi
]
Full implementation:
prop_addMintValueToChangeMaps_order
:: (AssetId, TokenQuantity)
-> NonEmpty TokenMap
-> Property
prop_addMintValueToChangeMaps_order mint changeMapDiffs =
property
$ inAscendingPartialOrder
$ addMintValueToChangeMaps mint changeMaps
where
-- A list of change maps already in ascending partial order.
changeMaps = NE.scanl (<>) TokenMap.empty changeMapDiffs
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Where can I download your smarts? :) Addressed in e77bcc4.
lib/core/test/unit/Cardano/Wallet/Primitive/CoinSelection/MA/RoundRobinSpec.hs
Outdated
Show resolved
Hide resolved
lib/core/test/unit/Cardano/Wallet/Primitive/CoinSelection/MA/RoundRobinSpec.hs
Outdated
Show resolved
Hide resolved
lib/core/test/unit/Cardano/Wallet/Primitive/CoinSelection/MA/RoundRobinSpec.hs
Outdated
Show resolved
Hide resolved
lib/core/test/unit/Cardano/Wallet/Primitive/CoinSelection/MA/RoundRobinSpec.hs
Show resolved
Hide resolved
lib/core/test/unit/Cardano/Wallet/Primitive/CoinSelection/MA/RoundRobinSpec.hs
Outdated
Show resolved
Hide resolved
lib/core/test/unit/Cardano/Wallet/Primitive/CoinSelection/MA/RoundRobinSpec.hs
Outdated
Show resolved
Hide resolved
lib/core/test/unit/Cardano/Wallet/Primitive/CoinSelection/MA/RoundRobinSpec.hs
Outdated
Show resolved
Hide resolved
lib/core/test/unit/Cardano/Wallet/Primitive/CoinSelection/MA/RoundRobinSpec.hs
Outdated
Show resolved
Hide resolved
142c2ee
to
0929921
Compare
fb3abfc
to
7c0636d
Compare
acf5628
to
2295304
Compare
c5a1985
to
efdf9a6
Compare
We remove the nested function composition in favour of simple function composition.
This error should currently never be thrown, as we do not currently provide a method for the user to mint or burn tokens through the API. Nevertheless, our test suite requires that we document the error in our API specification.
This function will be used to verify coverage in coin selection properties.
7e208a0
to
36b2be9
Compare
…riteria`. In particular, we add coverage for the case where some minted values are not spent or burned, so we can trigger the `OutputsInsufficient` error.
This assertion should have been negated. The error was only revealed once we had deliberately tweaked the generator to generate selection criteria with some minted values that are not spent or burned.
In the `genAssetsToMintAndBurn` subgenerator for `genMakeChangeData`, we deliberately create the possibliity of a non-empty intersection between the sets of assets to mint and assets to burn.
36b2be9
to
378d35f
Compare
bors try |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've given this a fairly exhaustive review, and added some missing coverage to the test suite. I think it's good to go. 👍🏻
tryBuild succeeded: |
bors r+ |
Build succeeded: |
2756: Add coverage checks to property tests for `TokenMap.intersection` r=jonathanknowles a=jonathanknowles # Issue Number ADP-997 # Pre-merge checks - [x] Perform soak test of test suite to ensure no flakiness has been introduced. (✅ 1000 test runs with `--match "/Token map properties/Arithmetic/prop_intersection"` passed with no failures.) # Overview PR #2725 added a `TokenMap.intersection` function and property tests. This PR: - [x] adds coverage checks to the `prop_intersection_*` series of properties. - [x] where necessary, adjusts property inputs in order to attain the required level of coverage. Co-authored-by: Jonathan Knowles <[email protected]>
Issue Number
ADP-997
ADP-346
Pre-merge checks
(✅ 200 test runs with
--match "RoundRobin"
passed with no failures.)Overview
This PR modifies the coin selection algorithm to account for minting and burning.