Add AdjustUnbalancedTx effect

[ghost]

Adds AdjustUnbalancedTx PABReq effect that uses protocol parameters.

Should fix #414.

---

Pre-submit checklist:

• Branch
  • Tests are provided (if possible)
  • Commit sequence broadly makes sense
  • Key commits have useful messages
  • Relevant tickets are mentioned in commit messages
  • Formatting, materialized Nix files, PNG optimization, etc. are updated
• PR
  • Self-reviewed the diff
  • Useful pull request description
  • Reviewer requested

[sjoerdvisscher]

coinsPerUTxOWord is not a drop-in replacement for minAdaTxOut. See https://github.com/input-output-hk/cardano-ledger/blob/master/doc/explanations/min-utxo-alonzo.rst for details. I'm still not sure what the best way is to fix #414.

[ghost]

@sjoerdvisscher hmm, well, there are

• utxoEntrySize (from documentation)

• calculateMinimumUTxO that uses utxoEntrySize, and is used by checkMinUTxOValue in makeTransactionBodyAutoBalance.

I suppose we should use it but I suspect we don't already do it because we need to convert the transaction from our representation to the cardano-node's types?

[sjoerdvisscher]

We even have Ledger.Validation.evaluateMinLovelaceOutput and Ledger.Validation.fromPlutusTxOut which together can easily calculate the correct amount. (In fact, evaluateMinLovelaceOutput should use fromPlutusTxOut) But we suspect it will be really hard to fix all the testcases. But maybe you want to try it out?

[sjoerdvisscher]

@ak3n What's the status of this?

[ghost]

@sjoerdvisscher I had to switch to a different task. Will get back to this PR a bit later.

[sjoerdvisscher]

If you let this return Either Tx.ToCardanoError ([Ada.Ada], TxOut) you have 2 nested applicative effects (Either Tx.ToCardanoError and (,) [Ada.Ada]), which you could use in a traversal using Compose. Then you won't need step.

[ghost]

Could you please elaborate or provide an example how to do it? I thought how to compose traversal and fold here (we want to traverse UnbalancedTx and also collect some info at the same time) but ended up with step solution.

[sjoerdvisscher]

This worked even better than I thought, trying it out in vscode I got:

adjustUnbalancedTx params = alaf Compose (tx . Tx.outputs . traverse) adjustTxOut

[ghost]

Oh, indeed, looks cool and works fine! Thanks!

But lacks some readability as non-trivial lens code as usual.

[sjoerdvisscher]

Isn't this allWalletsTxOutCosts?

[ghost]

Yeah, indeed, thanks for pointing this out. I will refactor and write a comment a bit later.

[koslambrou]

What do you mean by "false positive"?

[ghost]

I think there is a risk when expected delta has only ada part and expected delta /= realDelta and realDelta is divisible by some delta from deltas, then we will return realDelta's ada. Which means that the test will pass but without strong confidence in wallets' funds consistency.

For example, we expected -n, but there is n among deltas and realDelta is n, it is divisible by n => our test will pass.

[koslambrou]

I think this explanation should be added as a comment on the function.

[ghost]

Added to the comment. 👍

[sjoerdvisscher]

Looking at what the problem could be with

walletsAdjustedTxEvents = preMapMaybe (preview (eteEvent . walletEvent' . to (\x -> (x ^. _1, x ^. _2 . _RequestHandlerLog . _AdjustingUnbalancedTx)))) L.list

One thing I notice is _2 is a lens and _RequestHandlerLog . _AdjustingUnbalancedTx) is a prism, so composed they make a traversal. When applied to ^. a traversal needs a monoid instance on the result, which is [Ada.ada] here. So one difference with walletAdjustedTxEvents w is that for each wallet event that does't match _RequestHandlerLog . _AdjustingUnbalancedTx you get a result with an empty list. I still don't see how this could cause missing data though.

I do think that with the current implementation you don't need the wallet in the return value, since you throw them away in allWalletsTxOutCosts, so a safer implementation could be

walletsAdjustedTxEvents = preMapMaybe (preview (eteEvent . walletEvent' . _2 . _RequestHandlerLog . _AdjustingUnbalancedTx)) L.list

[ghost]

@sjoerdvisscher

Thanks, it works. I remember that initially I tried to go this way but later decided to add the information about wallets trying to be more precise with deltas using the the wallet against other wallet strategy instead of all wallets against all wallets because it might generate more unnecessary deltas and increase the risk of false positive results.

[sjoerdvisscher]

Well you could still do that (I think) by passing the result of Folds.walletAdjustedTxEvents w directly to calculateDelta. I can't really judge if that's worth it.

[koslambrou]

@ak3n You can merge whenever you think it's done :)