Implement `distributeSurplus` and use in `balanceTransaction` #3223

Anviking · 2022-04-06T20:15:15Z

Factors out fee-minimisation logic such that it can be made more sophisticated and tested separately from balanceTransaction
Fee- and change-related padding before fee-minimisation is increased.
The padding after fee-minimisation is (in most cases) removed (see goldens; we are no longer overpaying the fee with 176 lovelace)
The fee minimisation is more robust, and should work regardless of ProtocolParameters, whether a 5 ada fee is required, or a 21 lovelace one.
~~[ ] Implement guardReasonableExcessFee based on 2 * minUTxOValue, rather than using the hard-coded limit of 20 ada.~~
~~[ ] Extra: adjust error types (could be moved to separate PR)~~
Look over comments & polish etc

Comments

Depends on Implement sizeOfCoin and costOfIncreasingCoin #3215

Issue Number

Anviking · 2022-04-06T20:38:59Z

lib/core/src/Cardano/Wallet.hs

+        :: TxFeeAndChange
+        -> TxFeeAndChange
+    guardReasonableExcessFee fc@(TxFeeAndChange feeToBurn _)
+        | feeToBurn > minAda <> minAda =


I think technically this could be problematic on a network where the fee increase derived from coin-value-encoding is greater than 2*minAda — e.g. if minAda is tiny or 0...

Maybe we can add feePerByte * 16 to the right hand side… (or keep the static 20 ada one 🤷‍♂️ if it gets too complicated)

I ended up just dropping guardReasonableExcessFee completely. I think it's more trouble than it's worth.

Anviking · 2022-04-06T20:46:07Z

lib/shelley/test/data/balanceTx/pingPong_2/golden

- 3.050000,ErrBalanceTxSelectAssets (ErrSelectAssetsSelectionError (SelectionCollateralErrorOf (SelectionCollateralError {largestCombinationAvailable = fromList [(WalletUTxO {txIn = TxIn {inputId = Hash "00000000000000000000000000000000", inputIx = 0}, address = Address "`\177\229\224\251t\200l\128\USdhA\224|\219B\223\139\130\239<\228\229|\181A.w"},Coin 3050000)], minimumSelectionAmount = Coin 3050246})))
- 3.100000,0.612226,0.612050


Here we see that

528 more lovelace is needed to succeed (at least inferred from the error message)

When we are succeeding, fees are no longer 176 lovelace higher than needed

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

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

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

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

lib/core/src/Cardano/Wallet.hs

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

Anviking · 2022-04-08T16:09:04Z

bors try

iohk-bors · 2022-04-08T16:47:33Z

try

Build failed:

ci/hydra-build:required

  src/Test/Integration/Scenario/API/Shelley/TransactionsNew.hs:1745:15: 
  1) API Specifications, NEW_SHELLEY_TRANSACTIONS, TRANS_NEW_BALANCE_02c - Cannot balance when I cannot afford collateral
       "{\"code\":\"insufficient_collateral\",\"message\":\"I'm unable to create this transaction because the balance of pure ada UTxOs in your wallet is insufficient to cover the minimum amount of collateral required. I need an ada amount of at least: 4.279500 The largest combination of pure ada UTxOs I could find is: [2.853000] To fix this, you'll need to add one or more pure ada UTxOs to your wallet that can cover the minimum amount required.\"}" does not contain "I need an ada amount of at least: 4.278900"
       While verifying value:
         ( Status
             { statusCode = 403
             , statusMessage = "Forbidden"
             }
         , Left
             ( ClientError
                 ( Object
                     ( fromList
                         [
                             ( "code"
                             , String "insufficient_collateral"
                             )
                         ,
                             ( "message"
                             , String "I'm unable to create this transaction because the balance of pure ada UTxOs in your wallet is insufficient to cover the minimum amount of collateral required. I need an ada amount of at least: 4.279500 The largest combination of pure ada UTxOs I could find is: [2.853000] To fix this, you'll need to add one or more pure ada UTxOs to your wallet that can cover the minimum amount required."
                             )
                         ]
                     )
                 )
             )
         )

Probably the value just needs to be adjusted.

Anviking · 2022-04-10T20:57:02Z

bors try

iohk-bors · 2022-04-10T21:04:58Z

try

Build failed:

buildkite/cardano-wallet

jonathanknowles

Hi @Anviking

I have some suggestions, but I think this PR can be merged as is!

I'll open a second PR with the suggestions.

Thanks for making this PR!

Anviking · 2022-04-19T11:22:19Z

bors r+

3223: Implement `distributeSurplus` and use in `balanceTransaction` r=Anviking a=Anviking - [x] Factors out fee-minimisation logic such that it can be made more sophisticated and tested separately from `balanceTransaction` - [x] Fee- and change-related padding before fee-minimisation is _increased_. - [x] The padding _after_ fee-minimisation is (in most cases) _removed_ (see goldens; we are no longer overpaying the fee with 176 lovelace) - [x] The fee minimisation is more robust, and should work regardless of ProtocolParameters, whether a 5 ada fee is required, or a 21 lovelace one. - <s>[ ] Implement `guardReasonableExcessFee` based on `2 * minUTxOValue`, rather than using the hard-coded limit of 20 ada. </s> - <s>[ ] Extra: adjust error types (could be moved to separate PR)</s> - [x] Look over comments & polish etc ### Comments - Depends on #3215  ### Issue Number ADP-1514  Co-authored-by: Johannes Lund <johannes.lund@iohk.io>

iohk-bors · 2022-04-19T11:38:59Z

Build failed:

buildkite/cardano-wallet

test/unit/Main.hs:1:1: error:
--
  | `hspec-discover' failed in phase `Haskell pre-processor'. (Exit code: 127)
  | \|
  | 1 \| {-# OPTIONS_GHC -F -pgmF hspec-discover #-}
  | \| ^

Anviking · 2022-04-19T12:26:09Z

bors r+

3223: Implement `distributeSurplus` and use in `balanceTransaction` r=Anviking a=Anviking - [x] Factors out fee-minimisation logic such that it can be made more sophisticated and tested separately from `balanceTransaction` - [x] Fee- and change-related padding before fee-minimisation is _increased_. - [x] The padding _after_ fee-minimisation is (in most cases) _removed_ (see goldens; we are no longer overpaying the fee with 176 lovelace) - [x] The fee minimisation is more robust, and should work regardless of ProtocolParameters, whether a 5 ada fee is required, or a 21 lovelace one. - <s>[ ] Implement `guardReasonableExcessFee` based on `2 * minUTxOValue`, rather than using the hard-coded limit of 20 ada. </s> - <s>[ ] Extra: adjust error types (could be moved to separate PR)</s> - [x] Look over comments & polish etc ### Comments - Depends on #3215  ### Issue Number ADP-1514  Co-authored-by: Johannes Lund <johannes.lund@iohk.io>

iohk-bors · 2022-04-19T12:32:08Z

Build failed:

buildkite/cardano-wallet

- Factors out fee-minimisation logic such that it can be made more sophisticated and tested separately from balanceTransaction - Fee- and change-related padding before fee-minimisation is increased. - The padding after fee-minimisation is (in most cases) removed (see goldens; we are no longer overpaying the fee with 176 lovelace) - The fee minimisation is more robust, and should work regardless of ProtocolParameters, whether a 5 ada fee is required, or a 21 lovelace one. I ended up dropping the sanity check that any burned fee was less than 20 ada. I considered replacing the hard-coded limit with the following: ``` -- Increasing the fee by much should only happen if coin-selection is unable -- to construct a change output respecting the minUTxOValue. guardReasonableExcessFee :: TxFeeAndChange -> TxFeeAndChange guardReasonableExcessFee fc@(TxFeeAndChange feeToBurn _) | feeToBurn > limit = error $ unwords [ "final redundant safety check in balanceTransaction:" , "burning more than 2 * minUTxOValue in fees is unreasonable" ] | otherwise = fc where -- We let 2*minAda be the limit rather than just minAda to account -- for overestimations in coin-selection etc. Precision doesn't -- matter here. limit = minAda <> minAda <> Coin (ceiling $ 16 * perByteFee) -- NOTE: The change output we've failed to create would only have -- contained ada, so passing 'TokenMap.empty' should be reasonable. minAda = txOutputMinimumAdaQuantity (view #constraints tl pp) TokenMap.empty LinearFee LinearFunction {slope = perByteFee} = view (#txParameters . #getFeePolicy) pp ``` but decided it was too complex to be worth it.

Anviking · 2022-04-19T13:14:44Z

bors r+

3223: Implement `distributeSurplus` and use in `balanceTransaction` r=Anviking a=Anviking - [x] Factors out fee-minimisation logic such that it can be made more sophisticated and tested separately from `balanceTransaction` - [x] Fee- and change-related padding before fee-minimisation is _increased_. - [x] The padding _after_ fee-minimisation is (in most cases) _removed_ (see goldens; we are no longer overpaying the fee with 176 lovelace) - [x] The fee minimisation is more robust, and should work regardless of ProtocolParameters, whether a 5 ada fee is required, or a 21 lovelace one. - <s>[ ] Implement `guardReasonableExcessFee` based on `2 * minUTxOValue`, rather than using the hard-coded limit of 20 ada. </s> - <s>[ ] Extra: adjust error types (could be moved to separate PR)</s> - [x] Look over comments & polish etc ### Comments - Depends on #3215  ### Issue Number ADP-1514  Co-authored-by: Johannes Lund <johannes.lund@iohk.io>

iohk-bors · 2022-04-19T13:17:25Z

Build failed:

buildkite/cardano-wallet

Anviking · 2022-04-19T14:14:56Z

bors r+

3223: Implement `distributeSurplus` and use in `balanceTransaction` r=Anviking a=Anviking - [x] Factors out fee-minimisation logic such that it can be made more sophisticated and tested separately from `balanceTransaction` - [x] Fee- and change-related padding before fee-minimisation is _increased_. - [x] The padding _after_ fee-minimisation is (in most cases) _removed_ (see goldens; we are no longer overpaying the fee with 176 lovelace) - [x] The fee minimisation is more robust, and should work regardless of ProtocolParameters, whether a 5 ada fee is required, or a 21 lovelace one. - <s>[ ] Implement `guardReasonableExcessFee` based on `2 * minUTxOValue`, rather than using the hard-coded limit of 20 ada. </s> - <s>[ ] Extra: adjust error types (could be moved to separate PR)</s> - [x] Look over comments & polish etc ### Comments - Depends on #3215  ### Issue Number ADP-1514  Co-authored-by: Johannes Lund <johannes.lund@iohk.io>

iohk-bors · 2022-04-19T14:28:11Z

Build failed:

buildkite/cardano-wallet

Anviking · 2022-04-19T14:38:48Z

~~Merging manually.~~ except I can't even seem able to do that...

Anviking · 2022-04-19T14:39:59Z

bors r+

3223: Implement `distributeSurplus` and use in `balanceTransaction` r=Anviking a=Anviking - [x] Factors out fee-minimisation logic such that it can be made more sophisticated and tested separately from `balanceTransaction` - [x] Fee- and change-related padding before fee-minimisation is _increased_. - [x] The padding _after_ fee-minimisation is (in most cases) _removed_ (see goldens; we are no longer overpaying the fee with 176 lovelace) - [x] The fee minimisation is more robust, and should work regardless of ProtocolParameters, whether a 5 ada fee is required, or a 21 lovelace one. - <s>[ ] Implement `guardReasonableExcessFee` based on `2 * minUTxOValue`, rather than using the hard-coded limit of 20 ada. </s> - <s>[ ] Extra: adjust error types (could be moved to separate PR)</s> - [x] Look over comments & polish etc ### Comments - Depends on #3215  ### Issue Number ADP-1514  Co-authored-by: Johannes Lund <johannes.lund@iohk.io>

Anviking · 2022-04-19T15:05:28Z

bors r+

iohk-bors · 2022-04-19T15:05:30Z

Already running a review

iohk-bors · 2022-04-19T18:06:52Z

This PR was included in a batch that successfully built, but then failed to merge into master (it was a non-fast-forward update). It will be automatically retried.

3223: Implement `distributeSurplus` and use in `balanceTransaction` r=Anviking a=Anviking - [x] Factors out fee-minimisation logic such that it can be made more sophisticated and tested separately from `balanceTransaction` - [x] Fee- and change-related padding before fee-minimisation is _increased_. - [x] The padding _after_ fee-minimisation is (in most cases) _removed_ (see goldens; we are no longer overpaying the fee with 176 lovelace) - [x] The fee minimisation is more robust, and should work regardless of ProtocolParameters, whether a 5 ada fee is required, or a 21 lovelace one. - <s>[ ] Implement `guardReasonableExcessFee` based on `2 * minUTxOValue`, rather than using the hard-coded limit of 20 ada. </s> - <s>[ ] Extra: adjust error types (could be moved to separate PR)</s> - [x] Look over comments & polish etc ### Comments - Depends on #3215  ### Issue Number ADP-1514  Co-authored-by: Johannes Lund <johannes.lund@iohk.io>

iohk-bors · 2022-04-19T21:21:43Z

This PR was included in a batch that successfully built, but then failed to merge into master. It will not be retried.

Additional information:

{"message":"At least 1 approving review is required by reviewers with write access.","documentation_url":"https://docs.github.com/articles/about-protected-branches"}

Anviking commented Apr 6, 2022

View reviewed changes

lib/shelley/test/unit/Cardano/Wallet/Shelley/TransactionSpec.hs Show resolved Hide resolved

jonathanknowles reviewed Apr 7, 2022

View reviewed changes

lib/shelley/src/Cardano/Wallet/Shelley/Transaction.hs Outdated Show resolved Hide resolved

Anviking force-pushed the anviking/ADP-1514/costOfIncreasingCoin branch from a285cec to ab82b9d Compare April 7, 2022 14:04

Anviking self-assigned this Apr 7, 2022

Anviking force-pushed the anviking/ADP-1514/distributeSurplus branch 4 times, most recently from 1e70528 to dec9f1a Compare April 7, 2022 18:36

Anviking marked this pull request as ready for review April 7, 2022 18:51

Anviking commented Apr 7, 2022

View reviewed changes

lib/core/src/Cardano/Wallet/Transaction.hs Show resolved Hide resolved

Anviking commented Apr 7, 2022

View reviewed changes

lib/shelley/src/Cardano/Wallet/Shelley/Transaction.hs Outdated Show resolved Hide resolved

Anviking commented Apr 7, 2022

View reviewed changes

lib/core/src/Cardano/Wallet.hs Outdated Show resolved Hide resolved

Anviking commented Apr 7, 2022

View reviewed changes

lib/core/src/Cardano/Wallet/Transaction.hs Outdated Show resolved Hide resolved

Base automatically changed from anviking/ADP-1514/costOfIncreasingCoin to master April 8, 2022 02:57

Anviking force-pushed the anviking/ADP-1514/distributeSurplus branch 3 times, most recently from 6995119 to 3e941e2 Compare April 8, 2022 14:37

iohk-bors bot added a commit that referenced this pull request Apr 8, 2022

Try #3223:

4296003

Anviking force-pushed the anviking/ADP-1514/distributeSurplus branch from 3e941e2 to 8248eca Compare April 8, 2022 19:12

Anviking requested a review from jonathanknowles April 10, 2022 20:56

iohk-bors bot added a commit that referenced this pull request Apr 10, 2022

Try #3223:

ecaae78

jonathanknowles approved these changes Apr 19, 2022

View reviewed changes

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

Anviking enabled auto-merge (rebase) April 19, 2022 14:38

Anviking merged commit 9e750dd into master Apr 19, 2022

Anviking deleted the anviking/ADP-1514/distributeSurplus branch April 19, 2022 15:29

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement `distributeSurplus` and use in `balanceTransaction` #3223

Implement `distributeSurplus` and use in `balanceTransaction` #3223

Anviking commented Apr 6, 2022 •

edited

Loading

Anviking Apr 6, 2022 •

edited

Loading

Anviking Apr 7, 2022

Anviking Apr 6, 2022

Anviking commented Apr 8, 2022

iohk-bors bot commented Apr 8, 2022 •

edited by Anviking

Loading

Anviking commented Apr 10, 2022

iohk-bors bot commented Apr 10, 2022

jonathanknowles left a comment

Anviking commented Apr 19, 2022

iohk-bors bot commented Apr 19, 2022 •

edited by Anviking

Loading

Anviking commented Apr 19, 2022

iohk-bors bot commented Apr 19, 2022

Anviking commented Apr 19, 2022

iohk-bors bot commented Apr 19, 2022

Anviking commented Apr 19, 2022

iohk-bors bot commented Apr 19, 2022

Anviking commented Apr 19, 2022 •

edited

Loading

Anviking commented Apr 19, 2022

Anviking commented Apr 19, 2022

iohk-bors bot commented Apr 19, 2022

iohk-bors bot commented Apr 19, 2022

iohk-bors bot commented Apr 19, 2022

		3.050000,ErrBalanceTxSelectAssets (ErrSelectAssetsSelectionError (SelectionCollateralErrorOf (SelectionCollateralError {largestCombinationAvailable = fromList [(WalletUTxO {txIn = TxIn {inputId = Hash "00000000000000000000000000000000", inputIx = 0}, address = Address "`\177\229\224\251t\200l\128\USdhA\224\|\219B\223\139\130\239<\228\229\|\181A.w"},Coin 3050000)], minimumSelectionAmount = Coin 3050246})))
		3.100000,0.612226,0.612050

Implement distributeSurplus and use in balanceTransaction #3223

Implement distributeSurplus and use in balanceTransaction #3223

Conversation

Anviking commented Apr 6, 2022 • edited Loading

Comments

Issue Number

Anviking Apr 6, 2022 • edited Loading

Choose a reason for hiding this comment

Anviking Apr 7, 2022

Choose a reason for hiding this comment

Anviking Apr 6, 2022

Choose a reason for hiding this comment

Anviking commented Apr 8, 2022

iohk-bors bot commented Apr 8, 2022 • edited by Anviking Loading

try

Anviking commented Apr 10, 2022

iohk-bors bot commented Apr 10, 2022

try

jonathanknowles left a comment

Choose a reason for hiding this comment

Anviking commented Apr 19, 2022

iohk-bors bot commented Apr 19, 2022 • edited by Anviking Loading

Anviking commented Apr 19, 2022

iohk-bors bot commented Apr 19, 2022

Anviking commented Apr 19, 2022

iohk-bors bot commented Apr 19, 2022

Anviking commented Apr 19, 2022

iohk-bors bot commented Apr 19, 2022

Anviking commented Apr 19, 2022 • edited Loading

Anviking commented Apr 19, 2022

Anviking commented Apr 19, 2022

iohk-bors bot commented Apr 19, 2022

iohk-bors bot commented Apr 19, 2022

iohk-bors bot commented Apr 19, 2022

Implement `distributeSurplus` and use in `balanceTransaction` #3223

Implement `distributeSurplus` and use in `balanceTransaction` #3223

Anviking commented Apr 6, 2022 •

edited

Loading

Anviking Apr 6, 2022 •

edited

Loading

iohk-bors bot commented Apr 8, 2022 •

edited by Anviking

Loading

iohk-bors bot commented Apr 19, 2022 •

edited by Anviking

Loading

Anviking commented Apr 19, 2022 •

edited

Loading