-
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
Add UTxO snapshot API endpoint and CLI command. #2679
Conversation
9c657a0
to
f8b9c9a
Compare
f8b9c9a
to
f0fa028
Compare
@jonathanknowles I've added some basic integration tests for API and CLI. Perhaps assertions might be improved for checking the entries though... |
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.
lgtm
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.
lgtm!
I would suggest more integration test(s) - can be in some future PR:
- setup fixture wallet and verify snapshot. Then realize tx and when, both pending/in_ledger, verify utxo snapshots for both source and target wallet
- Do some funds flow testing and compare expectations of utxo snapshot and utxo statistics
- Do something interesting with non-ada things and check like in point 1. And like point 2 if we have already utxo statistics for native assets
This change adjusts the wallet UTxO snapshot API integration tests to use the common match string: `WALLET_UTXO_SNAPSHOT`. This is consistent with the API function name. We also extract out lists of wallets into separate definitions, to avoid excessively long lines.
Similarly to the API tests, we use the `WALLET_UTXO_SNAPSHOT` label.
The name `getWalletUtxoSnapshot` is consistent with the name of the API function.
433bc24
to
d8eef54
Compare
bors r+ |
2676: Fix termination condition in `makeChangeRepeatedly`. r=jonathanknowles a=jonathanknowles # Issue Number ADP-890 # Summary This PR fixes a termination condition in `RoundRobin.makeChangeRepeatedly`. This initial termination condition was incorrect: ```hs Right change | length change == length outputsToCover -> -- We've succeeded in making the optimal number of change outputs. -- Terminate here. pure $ Right $ mkSelectionResult change ``` This equality check was based on the faulty assumption that `makeChange` always returns a list of change outputs whose length is _less than or equal_ to the number of user-specified outputs. However, this assumption is **NOT** valid. In cases where there are many unique assets in the selected inputs, it's possible for `makeChange` to return a list whose length is _greater than_ the number of user-specified outputs. This can happen when one or more change outputs exceeds either of the following limits: 1. A change output has a quantity of a particular asset that exceeds `maxBound :: Word64`. 2. A change output has a serialized length that is greater than the upper limit defined by the protocol (`4000` bytes). If `makeChange` encounters a bundle that exceeds either of the above limits, it will split up the oversized bundle into smaller bundles that don't exceed the limits. (See `splitOversizedMaps`.) This means that the number of change outputs generated by `makeChange` can _exceed_ the number of user-specified outputs. Hence, we need to adjust the termination condition of `makeChangeRepeatedly` to allow for this possibility. # QA This PR: - [x] Adds regression tests `boundaryTest_largeTokenQuantities_{5,6}`. These tests perform coin selections that can only succeed if change bundles are split, in order to demonstrate that the change generation algorithm terminates after only a subset of the UTxO has been selected. - [x] Adjusts the property test `prop_makeChange_length` to account for splitting of bundles. # Background The original motivation for generating one change output per user-specified output was to avoid shrinking the size of the UTxO set unnecessarily. By default `makeChange` will already do this, unless there is not enough ada available in the existing set of selected inputs to pay for all the change, in which case change outputs will be coalesced. # Performance Impact The faulty termination condition would cause `makeChangeRepeatedly` to repeatedly expand the set of selected inputs in an effort to optimize the set of change outputs, continuing until the entire UTxO set was exhausted. ## Example This example uses a `testnet` wallet with: - 47 UTxO entries, of which: - 1 entry is a pure ada entry - 46 entries are multi-asset entries. - around 700 unique assets. We make a payment request for the exact balance of the pure-ada entry, the value of which is around 20% of the total ada available in the UTxO set (when all entries are considered). Specifying this amount means that the coin selection algorithm is guaranteed to select from among the multi-asset entries when generating the initial selection. ### Before applying this PR Time required to generate a fee estimate: ``` $ time curl -X POST http://localhost:8090/v2/wallets/c8b5cd51de6c0c84c5404fca06a02007e650548f/payment-fees -d '{"payments":[{"address":"addr_test1qzg9mz36rmw4k24hpnnmhfmvrfj8vydmc4rl970zdxfd5ltgrnkw3ewxurn9kuwfuq7gexjulr0s9wthlxa37z2ezwcqy3cxhm", "amount":{"quantity":2164303103,"unit":"lovelace"}}]}' -H "Content-Type: application/json" {"estimated_min":{"quantity":244013,"unit":"lovelace"},"deposit":{"quantity":0,"unit":"lovelace"},"minimum_coins":[{"quantity":1000000,"unit":"lovelace"}],"estimated_max":{"quantity":2143653,"unit":"lovelace"}} real 4m46.720s user 0m0.004s sys 0m0.011s ``` Number of inputs selected: ``` $ grep numberOfSelectedInputs cardano-wallet.log | sort -V | uniq -c 3 numberOfSelectedInputs: 2 3 numberOfSelectedInputs: 3 2 numberOfSelectedInputs: 4 1 numberOfSelectedInputs: 5 2 numberOfSelectedInputs: 6 2 numberOfSelectedInputs: 7 1 numberOfSelectedInputs: 9 2 numberOfSelectedInputs: 10 3 numberOfSelectedInputs: 12 4 numberOfSelectedInputs: 13 3 numberOfSelectedInputs: 14 4 numberOfSelectedInputs: 15 2 numberOfSelectedInputs: 16 1 numberOfSelectedInputs: 17 2 numberOfSelectedInputs: 18 1 numberOfSelectedInputs: 19 1 numberOfSelectedInputs: 20 2 numberOfSelectedInputs: 21 1 numberOfSelectedInputs: 22 2 numberOfSelectedInputs: 23 2 numberOfSelectedInputs: 25 1 numberOfSelectedInputs: 26 1 numberOfSelectedInputs: 29 1 numberOfSelectedInputs: 30 2 numberOfSelectedInputs: 31 1 numberOfSelectedInputs: 32 1 numberOfSelectedInputs: 38 49 numberOfSelectedInputs: 47 ``` In the above test run, in 49 cases out of 100, the entire UTxO set was consumed. ### After applying this PR Time required to generate a fee estimate: ``` $ time curl -X POST http://localhost:8090/v2/wallets/c8b5cd51de6c0c84c5404fca06a02007e650548f/payment-fees -d '{"payments":[{"address":"addr_test1qzg9mz36rmw4k24hpnnmhfmvrfj8vydmc4rl970zdxfd5ltgrnkw3ewxurn9kuwfuq7gexjulr0s9wthlxa37z2ezwcqy3cxhm", "amount":{"quantity":2164303103,"unit":"lovelace"}}]}' -H "Content-Type: application/json" {"estimated_min":{"quantity":231206,"unit":"lovelace"},"deposit":{"quantity":0,"unit":"lovelace"},"minimum_coins":[{"quantity":1000000,"unit":"lovelace"}],"estimated_max":{"quantity":1927085,"unit":"lovelace"}} real 0m19.462s user 0m0.003s sys 0m0.005s ``` Number of inputs selected: ``` $ grep numberOfSelectedInputs cardano-wallet.log | sort -V | uniq -c 5 numberOfSelectedInputs: 2 4 numberOfSelectedInputs: 3 2 numberOfSelectedInputs: 4 1 numberOfSelectedInputs: 5 3 numberOfSelectedInputs: 6 1 numberOfSelectedInputs: 7 1 numberOfSelectedInputs: 8 2 numberOfSelectedInputs: 9 1 numberOfSelectedInputs: 10 1 numberOfSelectedInputs: 12 3 numberOfSelectedInputs: 13 1 numberOfSelectedInputs: 14 1 numberOfSelectedInputs: 15 1 numberOfSelectedInputs: 16 1 numberOfSelectedInputs: 17 4 numberOfSelectedInputs: 18 2 numberOfSelectedInputs: 19 5 numberOfSelectedInputs: 20 3 numberOfSelectedInputs: 21 1 numberOfSelectedInputs: 22 3 numberOfSelectedInputs: 23 1 numberOfSelectedInputs: 24 2 numberOfSelectedInputs: 25 2 numberOfSelectedInputs: 27 3 numberOfSelectedInputs: 29 3 numberOfSelectedInputs: 30 1 numberOfSelectedInputs: 31 3 numberOfSelectedInputs: 32 2 numberOfSelectedInputs: 33 2 numberOfSelectedInputs: 34 3 numberOfSelectedInputs: 35 5 numberOfSelectedInputs: 36 2 numberOfSelectedInputs: 37 2 numberOfSelectedInputs: 39 4 numberOfSelectedInputs: 40 5 numberOfSelectedInputs: 41 5 numberOfSelectedInputs: 42 4 numberOfSelectedInputs: 43 3 numberOfSelectedInputs: 44 2 numberOfSelectedInputs: 45 ``` Now the number of selected inputs has a much flatter distribution, and we never hit the case where the entire UTxO set is consumed. 2679: Add UTxO snapshot API endpoint and CLI command. r=jonathanknowles a=jonathanknowles # Issue Number Related to ADP-890. # Overview This PR adds the `getWalletUtxoSnapshot` API endpoint and associated CLI command. The intended use is for debugging and development. The endpoint generates a complete snapshot of a wallet's UTxO set, as an unordered list of UTxO entries. It produces JSON output similar to the following: ```json "entries": [ { "ada": {"quantity": 8, "unit": "lovelace"} , "ada_minimum": {"quantity": 1, "unit": "lovelace"} , "assets": [] }, { "ada": {"quantity": 4, "unit": "lovelace"} , "ada_minimum": {"quantity": 2, "unit": "lovelace"} , "assets": [ { "policy_id": "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa" , "asset_name": "APPLE" , "quantity": 10 } ] }, { "ada": {"quantity": 4, "unit": "lovelace"} , "ada_minimum": {"quantity": 2, "unit": "lovelace"} , "assets": [ { "policy_id": "bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb" , "asset_name": "BANANA" , "quantity": 20 } ] } ] ``` Co-authored-by: Jonathan Knowles <[email protected]> Co-authored-by: Piotr Stachyra <[email protected]>
Build failed (retrying...): Timed out here:
|
2679: Add UTxO snapshot API endpoint and CLI command. r=jonathanknowles a=jonathanknowles # Issue Number Related to ADP-890. # Overview This PR adds the `getWalletUtxoSnapshot` API endpoint and associated CLI command. The intended use is for debugging and development. The endpoint generates a complete snapshot of a wallet's UTxO set, as an unordered list of UTxO entries. It produces JSON output similar to the following: ```json "entries": [ { "ada": {"quantity": 8, "unit": "lovelace"} , "ada_minimum": {"quantity": 1, "unit": "lovelace"} , "assets": [] }, { "ada": {"quantity": 4, "unit": "lovelace"} , "ada_minimum": {"quantity": 2, "unit": "lovelace"} , "assets": [ { "policy_id": "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa" , "asset_name": "APPLE" , "quantity": 10 } ] }, { "ada": {"quantity": 4, "unit": "lovelace"} , "ada_minimum": {"quantity": 2, "unit": "lovelace"} , "assets": [ { "policy_id": "bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb" , "asset_name": "BANANA" , "quantity": 20 } ] } ] ``` Co-authored-by: Jonathan Knowles <[email protected]> Co-authored-by: Piotr Stachyra <[email protected]>
bors r- |
Canceled. |
bors r+ |
Build succeeded: |
2676: Fix termination condition in `makeChangeRepeatedly`. r=jonathanknowles a=jonathanknowles # Issue Number ADP-890 # Summary This PR fixes a termination condition in `RoundRobin.makeChangeRepeatedly`. This initial termination condition was incorrect: ```hs Right change | length change == length outputsToCover -> -- We've succeeded in making the optimal number of change outputs. -- Terminate here. pure $ Right $ mkSelectionResult change ``` This equality check was based on the faulty assumption that `makeChange` always returns a list of change outputs whose length is _less than or equal_ to the number of user-specified outputs. However, this assumption is **NOT** valid. In cases where there are many unique assets in the selected inputs, it's possible for `makeChange` to return a list whose length is _greater than_ the number of user-specified outputs. This can happen when one or more change outputs exceeds either of the following limits: 1. A change output has a quantity of a particular asset that exceeds `maxBound :: Word64`. 2. A change output has a serialized length that is greater than the upper limit defined by the protocol (`4000` bytes). If `makeChange` encounters a bundle that exceeds either of the above limits, it will split up the oversized bundle into smaller bundles that don't exceed the limits. (See `splitOversizedMaps`.) This means that the number of change outputs generated by `makeChange` can _exceed_ the number of user-specified outputs. Hence, we need to adjust the termination condition of `makeChangeRepeatedly` to allow for this possibility. # QA This PR: - [x] Adds regression tests `boundaryTest_largeTokenQuantities_{5,6}`. These tests perform coin selections that can only succeed if change bundles are split, in order to demonstrate that the change generation algorithm terminates after only a subset of the UTxO has been selected. - [x] Adjusts the property test `prop_makeChange_length` to account for splitting of bundles. # Background The original motivation for generating one change output per user-specified output was to avoid shrinking the size of the UTxO set unnecessarily. By default `makeChange` will already do this, unless there is not enough ada available in the existing set of selected inputs to pay for all the change, in which case change outputs will be coalesced. # Performance Impact The faulty termination condition would cause `makeChangeRepeatedly` to repeatedly expand the set of selected inputs in an effort to optimize the set of change outputs, continuing until the entire UTxO set was exhausted. ## Example This example uses a `testnet` wallet with: - 47 UTxO entries, of which: - 1 entry is a pure ada entry - 46 entries are multi-asset entries. - around 700 unique assets. We make a payment request for the exact balance of the pure-ada entry, the value of which is around 20% of the total ada available in the UTxO set (when all entries are considered). Specifying this amount means that the coin selection algorithm is guaranteed to select from among the multi-asset entries when generating the initial selection. ### Before applying this PR Time required to generate a fee estimate: ``` $ time curl -X POST http://localhost:8090/v2/wallets/c8b5cd51de6c0c84c5404fca06a02007e650548f/payment-fees -d '{"payments":[{"address":"addr_test1qzg9mz36rmw4k24hpnnmhfmvrfj8vydmc4rl970zdxfd5ltgrnkw3ewxurn9kuwfuq7gexjulr0s9wthlxa37z2ezwcqy3cxhm", "amount":{"quantity":2164303103,"unit":"lovelace"}}]}' -H "Content-Type: application/json" {"estimated_min":{"quantity":244013,"unit":"lovelace"},"deposit":{"quantity":0,"unit":"lovelace"},"minimum_coins":[{"quantity":1000000,"unit":"lovelace"}],"estimated_max":{"quantity":2143653,"unit":"lovelace"}} real 4m46.720s user 0m0.004s sys 0m0.011s ``` Number of inputs selected: ``` $ grep numberOfSelectedInputs cardano-wallet.log | sort -V | uniq -c 3 numberOfSelectedInputs: 2 3 numberOfSelectedInputs: 3 2 numberOfSelectedInputs: 4 1 numberOfSelectedInputs: 5 2 numberOfSelectedInputs: 6 2 numberOfSelectedInputs: 7 1 numberOfSelectedInputs: 9 2 numberOfSelectedInputs: 10 3 numberOfSelectedInputs: 12 4 numberOfSelectedInputs: 13 3 numberOfSelectedInputs: 14 4 numberOfSelectedInputs: 15 2 numberOfSelectedInputs: 16 1 numberOfSelectedInputs: 17 2 numberOfSelectedInputs: 18 1 numberOfSelectedInputs: 19 1 numberOfSelectedInputs: 20 2 numberOfSelectedInputs: 21 1 numberOfSelectedInputs: 22 2 numberOfSelectedInputs: 23 2 numberOfSelectedInputs: 25 1 numberOfSelectedInputs: 26 1 numberOfSelectedInputs: 29 1 numberOfSelectedInputs: 30 2 numberOfSelectedInputs: 31 1 numberOfSelectedInputs: 32 1 numberOfSelectedInputs: 38 49 numberOfSelectedInputs: 47 ``` In the above test run, in 49 cases out of 100, the entire UTxO set was consumed. ### After applying this PR Time required to generate a fee estimate: ``` $ time curl -X POST http://localhost:8090/v2/wallets/c8b5cd51de6c0c84c5404fca06a02007e650548f/payment-fees -d '{"payments":[{"address":"addr_test1qzg9mz36rmw4k24hpnnmhfmvrfj8vydmc4rl970zdxfd5ltgrnkw3ewxurn9kuwfuq7gexjulr0s9wthlxa37z2ezwcqy3cxhm", "amount":{"quantity":2164303103,"unit":"lovelace"}}]}' -H "Content-Type: application/json" {"estimated_min":{"quantity":231206,"unit":"lovelace"},"deposit":{"quantity":0,"unit":"lovelace"},"minimum_coins":[{"quantity":1000000,"unit":"lovelace"}],"estimated_max":{"quantity":1927085,"unit":"lovelace"}} real 0m19.462s user 0m0.003s sys 0m0.005s ``` Number of inputs selected: ``` $ grep numberOfSelectedInputs cardano-wallet.log | sort -V | uniq -c 5 numberOfSelectedInputs: 2 4 numberOfSelectedInputs: 3 2 numberOfSelectedInputs: 4 1 numberOfSelectedInputs: 5 3 numberOfSelectedInputs: 6 1 numberOfSelectedInputs: 7 1 numberOfSelectedInputs: 8 2 numberOfSelectedInputs: 9 1 numberOfSelectedInputs: 10 1 numberOfSelectedInputs: 12 3 numberOfSelectedInputs: 13 1 numberOfSelectedInputs: 14 1 numberOfSelectedInputs: 15 1 numberOfSelectedInputs: 16 1 numberOfSelectedInputs: 17 4 numberOfSelectedInputs: 18 2 numberOfSelectedInputs: 19 5 numberOfSelectedInputs: 20 3 numberOfSelectedInputs: 21 1 numberOfSelectedInputs: 22 3 numberOfSelectedInputs: 23 1 numberOfSelectedInputs: 24 2 numberOfSelectedInputs: 25 2 numberOfSelectedInputs: 27 3 numberOfSelectedInputs: 29 3 numberOfSelectedInputs: 30 1 numberOfSelectedInputs: 31 3 numberOfSelectedInputs: 32 2 numberOfSelectedInputs: 33 2 numberOfSelectedInputs: 34 3 numberOfSelectedInputs: 35 5 numberOfSelectedInputs: 36 2 numberOfSelectedInputs: 37 2 numberOfSelectedInputs: 39 4 numberOfSelectedInputs: 40 5 numberOfSelectedInputs: 41 5 numberOfSelectedInputs: 42 4 numberOfSelectedInputs: 43 3 numberOfSelectedInputs: 44 2 numberOfSelectedInputs: 45 ``` Now the number of selected inputs has a much flatter distribution, and we never hit the case where the entire UTxO set is consumed. 2679: Add UTxO snapshot API endpoint and CLI command. r=jonathanknowles a=jonathanknowles # Issue Number Related to ADP-890. # Overview This PR adds the `getWalletUtxoSnapshot` API endpoint and associated CLI command. The intended use is for debugging and development. The endpoint generates a complete snapshot of a wallet's UTxO set, as an unordered list of UTxO entries. It produces JSON output similar to the following: ```json "entries": [ { "ada": {"quantity": 8, "unit": "lovelace"} , "ada_minimum": {"quantity": 1, "unit": "lovelace"} , "assets": [] }, { "ada": {"quantity": 4, "unit": "lovelace"} , "ada_minimum": {"quantity": 2, "unit": "lovelace"} , "assets": [ { "policy_id": "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa" , "asset_name": "APPLE" , "quantity": 10 } ] }, { "ada": {"quantity": 4, "unit": "lovelace"} , "ada_minimum": {"quantity": 2, "unit": "lovelace"} , "assets": [ { "policy_id": "bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb" , "asset_name": "BANANA" , "quantity": 20 } ] } ] ``` Co-authored-by: Jonathan Knowles <[email protected]> Co-authored-by: Piotr Stachyra <[email protected]> f94657c
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.
Nice addition, thanks 👍
getWalletUtxoSnapshot w = discriminate @style | ||
(endpoint @Api.GetWalletUtxoSnapshot (wid &)) | ||
(endpoint @Api.GetByronWalletUtxoSnapshot (wid &)) | ||
(notSupported "Shared") |
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.
Why not supported for shared wallets? They are have UTxO, once active.
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.
Why not supported for shared wallets? They are have UTxO, once active.
Good point.
@piotr-iohk, do you know of a good reason to not support shared wallets?
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.
Nope, my guess was that we will support them eventually at some point (once they are active). :)
If it is possible to wire them up right now then, yes, why not.
@@ -697,6 +699,20 @@ newtype ApiWalletPassphrase = ApiWalletPassphrase | |||
} deriving (Eq, Generic, Show) | |||
deriving anyclass NFData | |||
|
|||
newtype ApiWalletUtxoSnapshot = ApiWalletUtxoSnapshot |
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 think it's possible to avoid the extra type by just using ApiT [ApiWalletUtxoSnapshotEntry]
.
required: | ||
- entries | ||
properties: | ||
entries: |
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.
The extra level of wrapping in an object isn't necessary, is it?
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.
The extra level of wrapping in an object isn't necessary, is it?
I think this is arguable either way.
I agree with you that using a list would be enough to capture the current use case.
But on the other hand, using an object gives us the flexibility to add extra fields relating to a snapshot at a later time, without having to break compatibility with clients of the API (including ourselves).
From an API aesthetics point of view, I feel it's clearer to be able to point to a single type and say "this single type represents a complete UTxO snapshot", similarly to how we use UTxO
to represent a complete UTxO set.
tags: ["Wallets"] | ||
summary: A snapshot of the wallet's UTxO set | ||
description: | | ||
Generate a snapshot of the wallet's UTxO set. |
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.
Most endpoints have a status: stable
note.
Since this is intended to be used for debugging, better to not imply stability - perhaps add something like status: unstable
.
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.
Most endpoints have a status: stable note.
Since this is intended to be used for debugging, better to not imply stability - perhaps add
something like status: unstable.
Good point!
Issue Number
Related to ADP-890.
Overview
This PR adds the
getWalletUtxoSnapshot
API endpoint and associated CLI command.The intended use is for debugging and development.
The endpoint generates a complete snapshot of a wallet's UTxO set, as an unordered list of UTxO entries.
It produces JSON output similar to the following: