Address PR review: simplify calcMinFeeRecursive and remove redundant constraints

- Remove getMultiAssets helper and inline the MaryValue extraction, since
  EraCommonConstraints guarantees Value ~ MaryValue for all eras including
  Dijkstra
- Remove unnecessary parameters from go's recursive loop — utxo, pparams,
  poolids, stakeDelegDeposits, drepDelegDeposits, and nExtraWitnesses are
  never modified and are already in scope from the enclosing function
- Remove redundant obtainCommonConstraints calls in test generators where
  constraints are already in scope from the outer call

Author: Jimbo4350

cardano-api/src/Cardano/Api/Experimental/Tx/Internal/Fee.hs

@@ -661,17 +661,26 @@ evaluateTransactionFee pp (UnsignedTx tx) keywitcount byronwitcount refScriptsSi
   L.estimateMinFeeTx pp tx (fromIntegral keywitcount) (fromIntegral byronwitcount) refScriptsSize
 
 data FeeCalculationError
-  = NotEnoughAda Coin
+  = -- | Updating an existing change output resulted in negative ADA.
+    NotEnoughAdaForChangeOutput Coin
+  | -- | Creating a new change output would require negative ADA.
+    NotEnoughAdaForNewOutput Coin
   | NonAdaAssetsUnbalanced L.MultiAsset
   | -- | @MinUTxONotMet actual required@: an output does not meet the minimum UTxO requirement.
     MinUTxONotMet L.Coin L.Coin
   | FeeCalculationDidNotConverge
   deriving (Show, Eq)
 
 instance Error FeeCalculationError where
-  prettyError (NotEnoughAda balance) =
+  prettyError (NotEnoughAdaForChangeOutput balance) =
     mconcat
-      [ "The transaction balance is negative: "
+      [ "Not enough ADA when updating existing change output. Balance: "
+      , pretty balance
+      , "\nThis means that the transaction does not have enough ada to cover the fees. The usual solution is to provide more inputs, or inputs with more ada."
+      ]
+  prettyError (NotEnoughAdaForNewOutput balance) =
+    mconcat
+      [ "Not enough ADA when creating new change output. Balance: "
       , pretty balance
       , "\nThis means that the transaction does not have enough ada to cover the fees. The usual solution is to provide more inputs, or inputs with more ada."
       ]
@@ -721,7 +730,7 @@ instance Error FeeCalculationError where
 --   balance and appended to the end of the existing outputs; if a change
 --   output already exists it is updated in place. If the resulting change
 --   output would have negative ADA, the transaction is unrecoverable and
---   'NotEnoughAda' is returned. Otherwise the function recurses, because
+--   'NotEnoughAdaForChangeOutput' or 'NotEnoughAdaForNewOutput' is returned. Otherwise the function recurses, because
 --   the changed output may alter the transaction size and therefore the
 --   required fee, and must also satisfy the minimum UTxO
 --   (@coinPerUTxOByte@) constraint.
@@ -760,18 +769,13 @@ calcMinFeeRecursive changeAddr unsignedTx utxo pparams poolids stakeDelegDeposit
   | multiAssetIsNegative =
       Left $ NonAdaAssetsUnbalanced multiAssets
   | otherwise =
-      go
-        maxIterations
-        unsignedTx
-        utxo
-        pparams
-        poolids
-        stakeDelegDeposits
-        drepDelegDeposits
-        nExtraWitnesses
+      go maxIterations unsignedTx
  where
   initialBalance = evaluateTransactionBalance pparams poolids stakeDelegDeposits drepDelegDeposits utxo unsignedTx
-  multiAssets = getMultiAssets (useEra @era) initialBalance
+  multiAssets =
+    obtainCommonConstraints (useEra @era) $
+      let L.MaryValue _ ma = initialBalance
+       in ma
   -- Check whether any native token quantity is negative.
   -- ADA is zeroed out so it doesn't influence the check.
   multiAssetIsNegative =
@@ -783,34 +787,28 @@ calcMinFeeRecursive changeAddr unsignedTx utxo pparams poolids stakeDelegDeposit
   go
     :: Int
     -> UnsignedTx (LedgerEra era)
-    -> L.UTxO (LedgerEra era)
-    -> L.PParams (LedgerEra era)
-    -> Set PoolId
-    -> Map StakeCredential L.Coin
-    -> Map (Ledger.Credential Ledger.DRepRole) L.Coin
-    -> Int
     -> Either FeeCalculationError (UnsignedTx (LedgerEra era))
-  go 0 _ _ _ _ _ _ _ = Left FeeCalculationDidNotConverge
-  go n unSignTx@(UnsignedTx ledgerTx) utxo' pparams' poolids' stakeDelegDeposits' drepDelegDeposits' nExtraWitnesses'
+  go 0 _ = Left FeeCalculationDidNotConverge
+  go n unSignTx@(UnsignedTx ledgerTx)
     | minFee == txBodyFee && L.isZero txBalanceValue = do
         -- Case 1
         let outs = toList $ ledgerTx ^. L.bodyTxL . L.outputsTxBodyL
-        mapM_ (checkOutputMinUTxO pparams') outs
+        mapM_ (checkOutputMinUTxO pparams) outs
         return unSignTx
     | minFee == txBodyFee = do
         -- Case 2
         balancedOuts <- balanceTxOuts @era changeAddr txBalanceValue unSignTx
         let updatedTx = UnsignedTx (ledgerTx & L.bodyTxL . L.outputsTxBodyL .~ balancedOuts)
-        go (n - 1) updatedTx utxo' pparams' poolids' stakeDelegDeposits' drepDelegDeposits' nExtraWitnesses'
+        go (n - 1) updatedTx
     | otherwise =
         -- Case 3
         let newTx = UnsignedTx (ledgerTx & L.bodyTxL . L.feeTxBodyL .~ minFee)
-         in go (n - 1) newTx utxo' pparams' poolids' stakeDelegDeposits' drepDelegDeposits' nExtraWitnesses'
+         in go (n - 1) newTx
    where
-    minFee = obtainCommonConstraints (useEra @era) $ L.calcMinFeeTx utxo' pparams' ledgerTx nExtraWitnesses'
+    minFee = obtainCommonConstraints (useEra @era) $ L.calcMinFeeTx utxo pparams ledgerTx nExtraWitnesses
     txBodyFee = ledgerTx ^. L.bodyTxL . L.feeTxBodyL
     txBalanceValue =
-      evaluateTransactionBalance pparams' poolids' stakeDelegDeposits' drepDelegDeposits' utxo' unSignTx
+      evaluateTransactionBalance pparams poolids stakeDelegDeposits drepDelegDeposits utxo unSignTx
 
 checkOutputMinUTxO
   :: forall era
@@ -826,13 +824,6 @@ checkOutputMinUTxO pp out =
           Left (TxOut offending, minRequired) ->
             Left $ MinUTxONotMet (offending ^. L.coinTxOutL) minRequired
 
-getMultiAssets :: Era era -> L.Value (LedgerEra era) -> L.MultiAsset
-getMultiAssets era val = case era of
-  DijkstraEra -> mempty
-  ConwayEra ->
-    let L.MaryValue _ ma = val
-     in ma
-
 balanceTxOuts
   :: forall era
    . HasCallStack
@@ -854,13 +845,13 @@ balanceTxOuts changeAddr txBalance (UnsignedTx tx) =
                 let newValue = (lastOut ^. L.valueTxOutL) <> txBalance
                     changeCoin = L.coin newValue
                  in if changeCoin < 0
-                      then Left $ NotEnoughAda changeCoin
+                      then Left $ NotEnoughAdaForChangeOutput changeCoin
                       else Right $ rest Seq.:|> (lastOut & L.valueTxOutL .~ newValue)
           _ ->
             -- Append a new change output
             let changeCoin = L.coin txBalance
              in if changeCoin < 0
-                  then Left $ NotEnoughAda changeCoin
+                  then Left $ NotEnoughAdaForNewOutput changeCoin
                   else Right $ outs Seq.:|> L.mkBasicTxOut changeAddr txBalance
 
 -- Essentially we check for the existence of collateral inputs. If they exist we

cardano-api/test/cardano-api-test/Test/Cardano/Api/Experimental.hs

@@ -104,7 +104,7 @@ tests =
             "Case 2: transaction with no outputs creates change output"
             prop_calcMinFeeRecursive_no_tx_outs
         , testProperty
-            "Tiny surplus consumed by fee increase yields NotEnoughAda"
+            "Tiny surplus consumed by fee increase yields NotEnoughAdaForChangeOutput"
             prop_calcMinFeeRecursive_tiny_surplus_not_enough_ada
         ]
     ]
@@ -606,8 +606,7 @@ genFundedSimpleTx era = do
       sendTxOut =
         Exp.obtainCommonConstraints era $
           Exp.TxOut $
-            Exp.obtainCommonConstraints era $
-              Ledger.mkBasicTxOut addr (L.MaryValue sendCoin mempty)
+            Ledger.mkBasicTxOut addr (L.MaryValue sendCoin mempty)
       txBodyContent =
         Exp.defaultTxBodyContent
           & Exp.setTxIns [(txIn, Exp.AnyKeyWitnessPlaceholder)]
@@ -644,8 +643,7 @@ genFundedMultiAssetTx era = do
       sendTxOut =
         Exp.obtainCommonConstraints era $
           Exp.TxOut $
-            Exp.obtainCommonConstraints era $
-              Ledger.mkBasicTxOut addr (L.MaryValue sendCoin multiAsset)
+            Ledger.mkBasicTxOut addr (L.MaryValue sendCoin multiAsset)
       txBodyContent =
         Exp.defaultTxBodyContent
           & Exp.setTxIns [(txIn, Exp.AnyKeyWitnessPlaceholder)]
@@ -678,8 +676,7 @@ genUnderfundedTx era = do
       sendTxOut =
         Exp.obtainCommonConstraints era $
           Exp.TxOut $
-            Exp.obtainCommonConstraints era $
-              Ledger.mkBasicTxOut addr (L.MaryValue sendCoin mempty)
+            Ledger.mkBasicTxOut addr (L.MaryValue sendCoin mempty)
       txBodyContent =
         Exp.defaultTxBodyContent
           & Exp.setTxIns [(txIn, Exp.AnyKeyWitnessPlaceholder)]
@@ -744,15 +741,16 @@ prop_calcMinFeeRecursive_fee_fixpoint = H.property $ do
       resultTx H.=== secondResult
 
 -- | When the outputs exceed the UTxO value the function returns
--- 'Left (NotEnoughAda _)' with a negative deficit coin.
+-- 'Left (NotEnoughAdaForNewOutput _)' with a negative deficit coin.
 prop_calcMinFeeRecursive_insufficient_funds :: Property
 prop_calcMinFeeRecursive_insufficient_funds = H.property $ do
   (unsignedTx, utxo, changeAddr) <- H.forAll $ genUnderfundedTx Exp.ConwayEra
   case Exp.calcMinFeeRecursive changeAddr unsignedTx utxo exampleProtocolParams mempty mempty mempty 0 of
-    Left (Exp.NotEnoughAda deficit) -> H.assert $ deficit < L.Coin 0
+    Left (Exp.NotEnoughAdaForNewOutput deficit) -> H.assert $ deficit < L.Coin 0
     Left Exp.NonAdaAssetsUnbalanced{} -> H.annotate "Unexpected NonAdaAssetsUnbalanced error" >> H.failure
     Left Exp.MinUTxONotMet{} -> H.annotate "Unexpected MinUTxONotMet error" >> H.failure
     Left Exp.FeeCalculationDidNotConverge -> H.annotate "Unexpected FeeCalculationDidNotConverge error" >> H.failure
+    Left err -> H.annotateShow err >> H.failure
     Right _ -> H.failure
 
 -- | Generates a transaction whose output demands a native token that does
@@ -787,8 +785,7 @@ genNonAdaUnbalancedTx era = do
       sendTxOut =
         Exp.obtainCommonConstraints era $
           Exp.TxOut $
-            Exp.obtainCommonConstraints era $
-              Ledger.mkBasicTxOut addr sendValue
+            Ledger.mkBasicTxOut addr sendValue
       txBodyContent =
         Exp.defaultTxBodyContent
           & Exp.setTxIns [(txIn, Exp.AnyKeyWitnessPlaceholder)]
@@ -827,14 +824,12 @@ genMinUTxOViolatingTx era = do
       sendTxOut1 =
         Exp.obtainCommonConstraints era $
           Exp.TxOut $
-            Exp.obtainCommonConstraints era $
-              Ledger.mkBasicTxOut addr (L.MaryValue (L.Coin 1_000_000) mempty)
+            Ledger.mkBasicTxOut addr (L.MaryValue (L.Coin 1_000_000) mempty)
       -- Output 2: tokens with tiny ADA (below min UTxO)
       sendTxOut2 =
         Exp.obtainCommonConstraints era $
           Exp.TxOut $
-            Exp.obtainCommonConstraints era $
-              Ledger.mkBasicTxOut addr (L.MaryValue (L.Coin 1_000) multiAsset)
+            Ledger.mkBasicTxOut addr (L.MaryValue (L.Coin 1_000) multiAsset)
       txBodyContent =
         Exp.defaultTxBodyContent
           & Exp.setTxIns [(txIn, Exp.AnyKeyWitnessPlaceholder)]
@@ -877,7 +872,8 @@ prop_calcMinFeeRecursive_non_ada_unbalanced = H.property $ do
   (unsignedTx, utxo, changeAddr) <- H.forAll $ genNonAdaUnbalancedTx Exp.ConwayEra
   case Exp.calcMinFeeRecursive changeAddr unsignedTx utxo exampleProtocolParams mempty mempty mempty 0 of
     Left (Exp.NonAdaAssetsUnbalanced _) -> H.success
-    Left Exp.NotEnoughAda{} -> H.annotate "Unexpected NotEnoughAda" >> H.failure
+    Left Exp.NotEnoughAdaForChangeOutput{} -> H.annotate "Unexpected NotEnoughAdaForChangeOutput" >> H.failure
+    Left Exp.NotEnoughAdaForNewOutput{} -> H.annotate "Unexpected NotEnoughAdaForNewOutput" >> H.failure
     Left Exp.MinUTxONotMet{} -> H.annotate "Unexpected MinUTxONotMet" >> H.failure
     Left Exp.FeeCalculationDidNotConverge -> H.annotate "Unexpected FeeCalculationDidNotConverge" >> H.failure
     Right _ -> H.annotate "Expected NonAdaAssetsUnbalanced but got Right" >> H.failure
@@ -892,7 +888,8 @@ prop_calcMinFeeRecursive_min_utxo_not_met = H.property $ do
     Left (Exp.MinUTxONotMet actual required) -> do
       H.annotate $ "Actual: " <> show actual <> ", Required: " <> show required
       H.assert $ actual < required
-    Left Exp.NotEnoughAda{} -> H.annotate "Unexpected NotEnoughAda" >> H.failure
+    Left Exp.NotEnoughAdaForChangeOutput{} -> H.annotate "Unexpected NotEnoughAdaForChangeOutput" >> H.failure
+    Left Exp.NotEnoughAdaForNewOutput{} -> H.annotate "Unexpected NotEnoughAdaForNewOutput" >> H.failure
     Left Exp.NonAdaAssetsUnbalanced{} -> H.annotate "Unexpected NonAdaAssetsUnbalanced" >> H.failure
     Left Exp.FeeCalculationDidNotConverge -> H.annotate "Unexpected FeeCalculationDidNotConverge" >> H.failure
     Right _ -> H.annotate "Expected MinUTxONotMet but got Right" >> H.failure
@@ -926,7 +923,7 @@ prop_calcMinFeeRecursive_no_tx_outs = H.property $ do
 --   1. After fee convergence at F1, a positive balance triggers Case 2.
 --   2. Adding the change output raises the fee to F2.
 --   3. The change is updated: (surplus - F1) + (F1 - F2) = surplus - F2 < 0.
---   4. balanceTxOuts returns NotEnoughAda.
+--   4. balanceTxOuts returns NotEnoughAdaForChangeOutput.
 genTinySurplusTx
   :: Exp.Era era
   -> Gen
@@ -955,8 +952,7 @@ genTinySurplusTx era = do
       sendTxOut =
         Exp.obtainCommonConstraints era $
           Exp.TxOut $
-            Exp.obtainCommonConstraints era $
-              Ledger.mkBasicTxOut addr (L.MaryValue sendCoin mempty)
+            Ledger.mkBasicTxOut addr (L.MaryValue sendCoin mempty)
       txBodyContent =
         Exp.defaultTxBodyContent
           & Exp.setTxIns [(txIn, Exp.AnyKeyWitnessPlaceholder)]
@@ -966,12 +962,12 @@ genTinySurplusTx era = do
 
 -- | When the surplus is just barely enough to cover the initial fee but not
 -- the higher fee after adding a change output, the change output balance
--- goes negative and the function returns NotEnoughAda.
+-- goes negative and the function returns NotEnoughAdaForChangeOutput.
 prop_calcMinFeeRecursive_tiny_surplus_not_enough_ada :: Property
 prop_calcMinFeeRecursive_tiny_surplus_not_enough_ada = H.property $ do
   (unsignedTx, utxo, changeAddr) <- H.forAll $ genTinySurplusTx Exp.ConwayEra
   case Exp.calcMinFeeRecursive changeAddr unsignedTx utxo exampleProtocolParams mempty mempty mempty 0 of
-    Left (Exp.NotEnoughAda deficit) -> do
+    Left (Exp.NotEnoughAdaForChangeOutput deficit) -> do
       H.annotate $ "Deficit: " <> show deficit
       H.assert $ deficit < L.Coin 0
     Left (Exp.MinUTxONotMet actual required) -> do
@@ -980,4 +976,6 @@ prop_calcMinFeeRecursive_tiny_surplus_not_enough_ada = H.property $ do
       H.annotate $ "Change output ADA: " <> show actual <> ", minUTxO: " <> show required
       H.assert $ actual < required
     Left err -> H.annotateShow err >> H.failure
-    Right _ -> H.annotate "Expected NotEnoughAda or MinUTxONotMet but tx balanced successfully" >> H.failure
+    Right _ ->
+      H.annotate "Expected NotEnoughAdaForChangeOutput or MinUTxONotMet but tx balanced successfully"
+        >> H.failure