Generalize `distributeSurplus`. #3238

jonathanknowles · 2022-04-19T12:15:36Z

Issue Number

Summary

This PR adjusts distributeSurplus so that:

It accepts a list of change outputs rather than just a single optional output.
It accepts change outputs of type TxOut rather than Coin.
It handles the responsibility of applying the fee and change deltas to the original values instead of outsourcing this responsibility to balanceTransaction.

We can say that this is a generalization of distributeSurplus, because various implementation details (for example, which change outputs are actually adjusted) are now hidden from callers behind a more general interface, for which we can state more general properties.

This also means that we can write properties that test the combination of:

computing the fee and change deltas
applying the fee and change deltas

Whereas before, we only had explicit tests for the former (computing the delta), we can now also test that:

The number of change outputs is always preserved.
The Coin fee value either stays the same or increases.
The Coin value within each TxOut either stays the same or increases.
The non-ada assets within each TxOut are not changed in any way.
That the deltas are applied correctly.

lib/shelley/test/unit/Cardano/Wallet/Shelley/TransactionSpec.hs

lib/core/src/Cardano/Wallet/Transaction.hs

lib/core/src/Cardano/Wallet/Util.hs

lib/shelley/src/Cardano/Wallet/Shelley/Transaction.hs

lib/shelley/test/unit/Cardano/Wallet/Shelley/TransactionSpec.hs

lib/shelley/src/Cardano/Wallet/Shelley/Transaction.hs

lib/shelley/test/unit/Cardano/Wallet/Shelley/TransactionSpec.hs

lib/core/src/Cardano/Wallet/Transaction.hs

lib/shelley/src/Cardano/Wallet/Shelley/Transaction.hs

The change value is of type `Maybe Coin`. Therefore, we can also use `Just` in our comment examples.

While smaller inline properties are perfectly fine, if the definition of the property becomes too large, then it can be come a challenge to fit it within the horizontal line limit, resulting in code that is squished to the right, or code that has excessive wrapping. This commit moves the definition of an inline property to a top-level function, giving us more horizontal space to work with. This means that some statements no longer need to be wrapped. We can unwrap them, which saves us vertical space.

Terms defined in `where` clauses can often be indented further to the left than terms defined in `let` clauses. In this case, it means we no longer need to wrap the definitions of these terms, which means we can save vertical space, making the function more concise. We also remove an unneeded `do`, and an unneeded `$`.

For the next few commits, we will use these container types to provide alternative containers for change outputs in `TxFeeAndChange` in cases where we want to indicate that we have: - no change outputs - exactly one change output Later on, when we parameterize `TxFeeAndChange` further, we will remove these types.

This function will allow us to transform values of `TxFeeAndChange` without having to wrap and unwrap record constructors.

Before this commit, this function was only ever called with a change value of `Nothing`, and only ever returned a change value of `Nothing`. Therefore, we can safely replace the use of `Maybe` with `Empty`, which only has a single `Empty` constructor. This clarifies that this function cannot manipulate change values in any way.

This means that for a given change value c and output change delta Δc, the following property will now hold: >>> isJust c == isJust Δc This transformation is safe because `balanceTransaction` treats the following change deltas identically: - Nothing - Just (Coin 0)

We use the term `Delta`, because this function produces a result that is a delta: values that must be added on to the original fee and change outputs in order to produce the final fee and change outputs.

These tests cover the case where `distributeSurplus` is successful. We also add a helper function `txOutSubtractCoin`, in the same style as `txOutAddCoin`.

…plus`. We add a longer comment to explain the purpose of the property.

These types were introduced because we parameterized the **container type** for change in `TxFeeAndChange`: ```hs data TxFeeAndChange f = TxFeeAndChange { fee :: Coin , change :: f Coin } ``` This required us to instantiate `f` with types of kind `* -> *`. But later on we parameterized the type of `change` so that it can be any type of kind `*`: ```hs data TxFeeAndChange change = TxFeeAndChange { fee :: Coin , change :: change } ``` Hence we can make the following simplifications: - Empty Void -> () - Solo Coin -> Coin And therefore we can remove the `Empty` and `Solo` types. In response to review discussion: #3238 (comment)

…nge list. In response to review feedback: #3238 (comment)

In response to review feedback: #3238 (comment)

These types were introduced because we parameterized the **container type** for change in `TxFeeAndChange`: ```hs data TxFeeAndChange f = TxFeeAndChange { fee :: Coin , change :: f Coin } ``` This required us to instantiate `f` with types of kind `* -> *`. But later on we parameterized the type of `change` so that it can be any type of kind `*`: ```hs data TxFeeAndChange change = TxFeeAndChange { fee :: Coin , change :: change } ``` Hence we can make the following simplifications: - Empty Void -> () - Solo Coin -> Coin And therefore we can remove the `Empty` and `Solo` types. In response to review discussion: #3238 (comment)

…nge list. In response to review feedback: #3238 (comment)

In response to review feedback: #3238 (comment)

These types were introduced because we parameterized the **container type** for change in `TxFeeAndChange`: ```hs data TxFeeAndChange f = TxFeeAndChange { fee :: Coin , change :: f Coin } ``` This required us to instantiate `f` with types of kind `* -> *`. But later on we parameterized the type of `change` so that it can be any type of kind `*`: ```hs data TxFeeAndChange change = TxFeeAndChange { fee :: Coin , change :: change } ``` Hence we can make the following simplifications: - Empty Void -> () - Solo Coin -> Coin And therefore we can remove the `Empty` and `Solo` types. In response to review discussion: #3238 (comment)

…nge list. In response to review feedback: #3238 (comment)

In response to review feedback: #3238 (comment)

Anviking

lgtm, thanks!

Anviking · 2022-04-20T11:24:51Z

bors r+

3238: Generalize `distributeSurplus`. r=Anviking a=jonathanknowles ## Issue Number ADP-1514 ## Summary This PR adjusts `distributeSurplus` so that: - It accepts a _list_ of change outputs rather than just a single _optional_ output. - It accepts change outputs of type `TxOut` rather than `Coin`. - It handles the responsibility of applying the `fee` and `change` deltas to the original values instead of outsourcing this responsibility to `balanceTransaction`. We can say that this is a **_generalization_** of `distributeSurplus`, because various implementation details (for example, which change outputs are actually adjusted) are now hidden from callers behind a more general interface, for which we can state more general properties. This also means that we can write properties that test the **_combination_** of: - **_computing_** the fee and change deltas - **_applying_** the fee and change deltas Whereas before, we only had explicit tests for the former (_computing_ the delta), we can now **_also_** test that: - The _number_ of change outputs is always preserved. - The `Coin` `fee` value either stays the same or increases. - The `Coin` value within each `TxOut` either stays the same or increases. - The non-ada assets within each `TxOut` are not changed in any way. - That the deltas are applied correctly. Co-authored-by: Jonathan Knowles <[email protected]>

iohk-bors · 2022-04-20T11:33:01Z

Canceled.

Anviking · 2022-04-20T11:34:16Z

I pushed just a small update: efc6c13

bors r+

iohk-bors · 2022-04-20T12:31:47Z

Build succeeded:

These types were introduced because we parameterized the **container type** for change in `TxFeeAndChange`: ```hs data TxFeeAndChange f = TxFeeAndChange { fee :: Coin , change :: f Coin } ``` This required us to instantiate `f` with types of kind `* -> *`. But later on we parameterized the type of `change` so that it can be any type of kind `*`: ```hs data TxFeeAndChange change = TxFeeAndChange { fee :: Coin , change :: change } ``` Hence we can make the following simplifications: - Empty Void -> () - Solo Coin -> Coin And therefore we can remove the `Empty` and `Solo` types. In response to review discussion: #3238 (comment)

…nge list. In response to review feedback: #3238 (comment)

In response to review feedback: #3238 (comment)

jonathanknowles requested a review from Anviking April 19, 2022 12:15

jonathanknowles self-assigned this Apr 19, 2022

Anviking force-pushed the anviking/ADP-1514/distributeSurplus branch from 8248eca to 1f69c03 Compare April 19, 2022 13:14

jonathanknowles force-pushed the jonathanknowles/ADP-1514/distributeSurplus branch from 28df630 to 85f1a06 Compare April 19, 2022 13:25