Fee Estimation Improvement (#27)

Replaced max fee estimation with the more accurate fee estimation function. 

Created additional unit tests to check:

- When input utxo value is small but greater than requested and respect min ada required for change
- When input utxo value is small and remainder value is smaller than min ada required for change
- When input utxo value is small and remainder value is smaller than min ada required for change, but there are additional utxos in the address that can be selected

Author: henryyuanheng-wang

pycardano/txbuilder.py

@@ -41,7 +41,7 @@
     UTxO,
     Value,
 )
-from pycardano.utils import fee, max_tx_fee, min_lovelace, script_data_hash
+from pycardano.utils import fee, min_lovelace, script_data_hash
 from pycardano.witness import TransactionWitnessSet, VerificationKeyWitness
 
 __all__ = ["TransactionBuilder"]
@@ -294,6 +294,8 @@ def _calc_change(
 
         # when there is only ADA left, simply use remaining coin value as change
         if not change.multi_asset:
+            if change.coin < min_lovelace(change, self.context):
+                raise InsufficientUTxOBalanceException("Not enough ADA left for change")
             lovelace_change = change.coin
             change_output_arr.append(TransactionOutput(address, lovelace_change))
 
@@ -309,11 +311,7 @@ def _calc_change(
                 # Combine remainder of provided ADA with last MultiAsset for output
                 # There may be rare cases where adding ADA causes size exceeds limit
                 # We will revisit if it becomes an issue
-                if (
-                    precise_fee
-                    and change.coin - min_lovelace(Value(0, multi_asset), self.context)
-                    < 0
-                ):
+                if change.coin < min_lovelace(Value(0, multi_asset), self.context):
                     raise InsufficientUTxOBalanceException(
                         "Not enough ADA left to cover non-ADA assets in a change address"
                     )
@@ -335,24 +333,17 @@ def _add_change_and_fee(
         self, change_address: Optional[Address]
     ) -> TransactionBuilder:
         original_outputs = self.outputs[:]
+
         if change_address:
             # Set fee to max
-            self.fee = max_tx_fee(self.context)
+            self.fee = self._estimate_fee()
             changes = self._calc_change(
-                self.fee, self.inputs, self.outputs, change_address, precise_fee=False
+                self.fee, self.inputs, self.outputs, change_address, precise_fee=True
             )
             self._outputs += changes
 
-        plutus_execution_units = ExecutionUnits(0, 0)
-        for redeemer in self.redeemers:
-            plutus_execution_units += redeemer.ex_units
-
-        self.fee = fee(
-            self.context,
-            len(self._build_full_fake_tx().to_cbor("bytes")),
-            plutus_execution_units.steps,
-            plutus_execution_units.mem,
-        )
+        # With changes included, we can estimate the fee more precisely
+        self.fee = self._estimate_fee()
 
         if change_address:
             self._outputs = original_outputs
@@ -375,11 +366,15 @@ def _adding_asset_make_output_overflow(
         """Check if adding the asset will make output exceed maximum size limit
 
         Args:
-            output (TransactionOutput): current output
-            current_assets (Asset): current Assets to be included in output
-            policy_id (ScriptHash): policy id containing the MultiAsset
-            asset_to_add (Asset): Asset to add to current MultiAsset to check size limit
+            output (TransactionOutput): Current output
+            current_assets (Asset): Current Assets to be included in output
+            policy_id (ScriptHash): Policy id containing the MultiAsset
+            add_asset_name (AssetName): Name of asset to add to current MultiAsset
+            add_asset_val (int): Value of asset to add to current MultiAsset
+            max_val_size (int): maximum size limit for output
 
+        Returns:
+            bool: whether adding asset will make output greater than maximum size limit
         """
         attempt_assets = deepcopy(current_assets)
         attempt_assets += Asset({add_asset_name: add_asset_val})
@@ -577,6 +572,20 @@ def _ensure_no_input_exclusion_conflict(self):
                 f"{intersection}."
             )
 
+    def _estimate_fee(self):
+        plutus_execution_units = ExecutionUnits(0, 0)
+        for redeemer in self.redeemers:
+            plutus_execution_units += redeemer.ex_units
+
+        estimated_fee = fee(
+            self.context,
+            len(self._build_full_fake_tx().to_cbor("bytes")),
+            plutus_execution_units.steps,
+            plutus_execution_units.mem,
+        )
+
+        return estimated_fee
+
     def build(self, change_address: Optional[Address] = None) -> TransactionBody:
         """Build a transaction body from all constraints set through the builder.
 
@@ -601,6 +610,9 @@ def build(self, change_address: Optional[Address] = None) -> TransactionBody:
         for o in self.outputs:
             requested_amount += o.amount
 
+        # Include min fees associated as part of requested amount
+        requested_amount += self._estimate_fee()
+
         # Trim off assets that are not requested because they will be returned as changes eventually.
         trimmed_selected_amount = Value(
             selected_amount.coin,
@@ -611,7 +623,13 @@ def build(self, change_address: Optional[Address] = None) -> TransactionBody:
         )
 
         unfulfilled_amount = requested_amount - trimmed_selected_amount
-        unfulfilled_amount.coin = max(0, unfulfilled_amount.coin)
+        # if remainder is smaller than minimum ADA required in change,
+        # we need to select additional UTxOs available from the address
+        unfulfilled_amount.coin = max(
+            0,
+            unfulfilled_amount.coin
+            + min_lovelace(selected_amount - trimmed_selected_amount, self.context),
+        )
         # Clean up all non-positive assets
         unfulfilled_amount.multi_asset = unfulfilled_amount.multi_asset.filter(
             lambda p, n, v: v > 0
@@ -637,6 +655,7 @@ def build(self, change_address: Optional[Address] = None) -> TransactionBody:
                         additional_utxo_pool,
                         [TransactionOutput(None, unfulfilled_amount)],
                         self.context,
+                        include_max_fee=False,
                     )
                     for s in selected:
                         selected_amount += s.output.amount

test/pycardano/test_txbuilder.py

@@ -120,7 +120,7 @@ def test_tx_builder_multi_asset(chain_context):
 
     # Add sender address as input
     tx_builder.add_input_address(sender).add_output(
-        TransactionOutput.from_primitive([sender, 1000000])
+        TransactionOutput.from_primitive([sender, 3000000])
     ).add_output(
         TransactionOutput.from_primitive(
             [sender, [2000000, {b"1" * 28: {b"Token1": 1}}]]
@@ -136,7 +136,7 @@ def test_tx_builder_multi_asset(chain_context):
         ],
         1: [
             # First output
-            [sender_address.to_primitive(), 1000000],
+            [sender_address.to_primitive(), 3000000],
             # Second output
             [
                 sender_address.to_primitive(),
@@ -145,7 +145,7 @@ def test_tx_builder_multi_asset(chain_context):
             # Third output as change
             [
                 sender_address.to_primitive(),
-                [7827767, {b"1111111111111111111111111111": {b"Token2": 2}}],
+                [5827767, {b"1111111111111111111111111111": {b"Token2": 2}}],
             ],
         ],
         2: 172233,
@@ -171,7 +171,8 @@ def test_tx_builder_raises_utxo_selection(chain_context):
     with pytest.raises(UTxOSelectionException) as e:
         tx_body = tx_builder.build(change_address=sender_address)
     assert "Unfulfilled amount:" in e.value.args[0]
-    assert "'coin': 991000000" in e.value.args[0]
+    # The unfulfilled amount includes requested (991000000) and estimated fees (161101)
+    assert "'coin': 991161101" in e.value.args[0]
     assert "{AssetName(b'NewToken'): 1}" in e.value.args[0]
 
 
@@ -189,6 +190,92 @@ def test_tx_too_big_exception(chain_context):
         tx_body = tx_builder.build(change_address=sender_address)
 
 
+def test_tx_small_utxo_precise_fee(chain_context):
+    tx_builder = TransactionBuilder(chain_context)
+    sender = "addr_test1vrm9x2zsux7va6w892g38tvchnzahvcd9tykqf3ygnmwtaqyfg52x"
+    sender_address = Address.from_primitive(sender)
+
+    tx_in1 = TransactionInput.from_primitive([b"1" * 32, 3])
+    tx_out1 = TransactionOutput.from_primitive([sender, 4000000])
+    utxo1 = UTxO(tx_in1, tx_out1)
+
+    tx_builder.add_input(utxo1).add_output(
+        TransactionOutput.from_primitive([sender, 2500000])
+    )
+
+    # This will not fail as we replace max fee constraint with more precise fee calculation
+    # And remainder is greater than minimum ada required for change
+    tx_body = tx_builder.build(change_address=sender_address)
+
+    expect = {
+        0: [
+            [b"11111111111111111111111111111111", 3],
+        ],
+        1: [
+            # First output
+            [sender_address.to_primitive(), 2500000],
+            # Second output as change
+            [sender_address.to_primitive(), 1334587],
+        ],
+        2: 165413,
+    }
+
+    expect == tx_body.to_primitive()
+
+
+def test_tx_small_utxo_balance_fail(chain_context):
+    tx_builder = TransactionBuilder(chain_context)
+    sender = "addr_test1vrm9x2zsux7va6w892g38tvchnzahvcd9tykqf3ygnmwtaqyfg52x"
+    sender_address = Address.from_primitive(sender)
+
+    tx_in1 = TransactionInput.from_primitive([b"1" * 32, 3])
+    tx_out1 = TransactionOutput.from_primitive([sender, 4000000])
+    utxo1 = UTxO(tx_in1, tx_out1)
+
+    tx_builder.add_input(utxo1).add_output(
+        TransactionOutput.from_primitive([sender, 3000000])
+    )
+
+    # Balance is smaller than minimum ada required in change
+    # No more UTxO is available, throwing UTxO selection exception
+    with pytest.raises(UTxOSelectionException):
+        tx_body = tx_builder.build(change_address=sender_address)
+
+
+def test_tx_small_utxo_balance_pass(chain_context):
+    tx_builder = TransactionBuilder(chain_context)
+    sender = "addr_test1vrm9x2zsux7va6w892g38tvchnzahvcd9tykqf3ygnmwtaqyfg52x"
+    sender_address = Address.from_primitive(sender)
+
+    tx_in1 = TransactionInput.from_primitive([b"1" * 32, 3])
+    tx_out1 = TransactionOutput.from_primitive([sender, 4000000])
+    utxo1 = UTxO(tx_in1, tx_out1)
+
+    tx_builder.add_input(utxo1).add_input_address(sender_address).add_output(
+        TransactionOutput.from_primitive([sender, 3000000])
+    )
+
+    # Balance is smaller than minimum ada required in change
+    # Additional UTxOs are selected from the input address
+    tx_body = tx_builder.build(change_address=sender_address)
+
+    expected = {
+        0: [
+            [b"11111111111111111111111111111111", 3],
+            [b"11111111111111111111111111111111", 1],
+        ],
+        1: [
+            # First output
+            [sender_address.to_primitive(), 3000000],
+            # Second output as change
+            [sender_address.to_primitive(), 5833003],
+        ],
+        2: 166997,
+    }
+
+    expected == tx_body.to_primitive()
+
+
 def test_tx_builder_mint_multi_asset(chain_context):
     vk1 = VerificationKey.from_cbor(
         "58206443a101bdb948366fc87369336224595d36d8b0eee5602cba8b81a024e58473"
@@ -210,7 +297,7 @@ def test_tx_builder_mint_multi_asset(chain_context):
     # Add sender address as input
     mint = {policy_id.payload: {b"Token1": 1}}
     tx_builder.add_input_address(sender).add_output(
-        TransactionOutput.from_primitive([sender, 1000000])
+        TransactionOutput.from_primitive([sender, 3000000])
     ).add_output(TransactionOutput.from_primitive([sender, [2000000, mint]]))
     tx_builder.mint = MultiAsset.from_primitive(mint)
     tx_builder.native_scripts = [script]
@@ -225,14 +312,14 @@ def test_tx_builder_mint_multi_asset(chain_context):
         ],
         1: [
             # First output
-            [sender_address.to_primitive(), 1000000],
+            [sender_address.to_primitive(), 3000000],
             # Second output
             [sender_address.to_primitive(), [2000000, mint]],
             # Third output as change
             [
                 sender_address.to_primitive(),
                 [
-                    7811267,
+                    5811267,
                     {b"1111111111111111111111111111": {b"Token1": 1, b"Token2": 2}},
                 ],
             ],
@@ -332,7 +419,7 @@ def test_not_enough_input_amount(chain_context):
         TransactionOutput.from_primitive([sender, input_utxo.output.amount])
     )
 
-    with pytest.raises(InvalidTransactionException):
+    with pytest.raises(UTxOSelectionException):
         # Tx builder must fail here because there is not enough amount of input ADA to pay tx fee
         tx_body = tx_builder.build(change_address=sender_address)