Fix: wallet include fees in swap outputs for inputs of successive melt (#630)

* wallet: add fees to outputs for melt that requires a split

* add test that requires a swap

* verify test fails, will revert

* revert true

* hopefully fix the tests

* fix default fee selection

* cleanup and renamings

* cleanup coinselect function, estimate fees

* fix test

* add comments

* weird error

cashu/lightning/lnbits.py

@@ -87,11 +87,8 @@ class LNbitsWallet(LightningBackend):
                 url=f"{self.endpoint}/api/v1/payments", json=data
             )
             r.raise_for_status()
-        except Exception:
+        except Exception as e:
-            return InvoiceResponse(
+            return InvoiceResponse(ok=False, error_message=str(e))
-                ok=False,
-                error_message=r.json()["detail"],
-            )
 
         data = r.json()
         checking_id, payment_request = data["checking_id"], data["payment_request"]

cashu/wallet/cli/cli.py

@@ -203,10 +203,12 @@ async def pay(
     quote = await wallet.melt_quote(invoice, amount)
     logger.debug(f"Quote: {quote}")
     total_amount = quote.amount + quote.fee_reserve
+    # estimate ecash fee for the coinselected proofs
+    ecash_fees = wallet.coinselect_fee(wallet.proofs, total_amount)
     if not yes:
         potential = (
-            f" ({wallet.unit.str(total_amount)} with potential fees)"
+            f" ({wallet.unit.str(total_amount + ecash_fees)} with potential fees)"
-            if quote.fee_reserve
+            if quote.fee_reserve or ecash_fees
             else ""
         )
         message = f"Pay {wallet.unit.str(quote.amount)}{potential}?"
@@ -215,15 +217,16 @@ async def pay(
             abort=True,
             default=True,
         )
-
+    # we need to include fees so we can use the proofs for melting the `total_amount`
+    send_proofs, _ = await wallet.select_to_send(
+        wallet.proofs, total_amount, include_fees=True, set_reserved=True
+    )
     print("Paying Lightning invoice ...", end="", flush=True)
     assert total_amount > 0, "amount is not positive"
     if wallet.available_balance < total_amount:
         print(" Error: Balance too low.")
         return
-    send_proofs, fees = await wallet.select_to_send(
+
-        wallet.proofs, total_amount, include_fees=True, set_reserved=True
-    )
     try:
         melt_response = await wallet.melt(
             send_proofs, invoice, quote.fee_reserve, quote.quote

cashu/wallet/nostr.py

@@ -62,9 +62,7 @@ async def send_nostr(
         pubkey = await nip5_to_pubkey(wallet, pubkey)
     await wallet.load_mint()
     await wallet.load_proofs()
-    _, send_proofs = await wallet.swap_to_send(
+    _, send_proofs = await wallet.swap_to_send(wallet.proofs, amount, set_reserved=True)
-        wallet.proofs, amount, set_reserved=True, include_fees=False
-    )
     token = await wallet.serialize_proofs(send_proofs, include_dleq=include_dleq)
 
     if pubkey.startswith("npub"):

cashu/wallet/transactions.py

@@ -36,7 +36,7 @@ class WalletTransactions(SupportsDb, SupportsKeysets):
     def get_fees_for_proofs_ppk(self, proofs: List[Proof]) -> int:
         return sum([self.keysets[p.id].input_fee_ppk for p in proofs])
 
-    async def _select_proofs_to_send(
+    def coinselect(
         self,
         proofs: List[Proof],
         amount_to_send: Union[int, float],
@@ -59,7 +59,7 @@ class WalletTransactions(SupportsDb, SupportsKeysets):
             return []
 
         logger.trace(
-            f"_select_proofs_to_send – amount_to_send: {amount_to_send} – amounts we have: {amount_summary(proofs, self.unit)} (sum: {sum_proofs(proofs)})"
+            f"coinselect – amount_to_send: {amount_to_send} – amounts we have: {amount_summary(proofs, self.unit)} (sum: {sum_proofs(proofs)})"
         )
 
         sorted_proofs = sorted(proofs, key=lambda p: p.amount)
@@ -91,7 +91,7 @@ class WalletTransactions(SupportsDb, SupportsKeysets):
             logger.trace(
                 f"> selecting more proofs from {amount_summary(smaller_proofs[1:], self.unit)} sum: {sum_proofs(smaller_proofs[1:])} to reach {remainder}"
             )
-            selected_proofs += await self._select_proofs_to_send(
+            selected_proofs += self.coinselect(
                 smaller_proofs[1:], remainder, include_fees=include_fees
             )
         sum_selected_proofs = sum_proofs(selected_proofs)
@@ -101,10 +101,14 @@ class WalletTransactions(SupportsDb, SupportsKeysets):
             return [next_bigger]
 
         logger.trace(
-            f"_select_proofs_to_send - selected proof amounts: {amount_summary(selected_proofs, self.unit)} (sum: {sum_proofs(selected_proofs)})"
+            f"coinselect - selected proof amounts: {amount_summary(selected_proofs, self.unit)} (sum: {sum_proofs(selected_proofs)})"
         )
         return selected_proofs
 
+    def coinselect_fee(self, proofs: List[Proof], amount: int) -> int:
+        proofs_send = self.coinselect(proofs, amount, include_fees=True)
+        return self.get_fees_for_proofs(proofs_send)
+
     async def set_reserved(self, proofs: List[Proof], reserved: bool) -> None:
         """Mark a proof as reserved or reset it in the wallet db to avoid reuse when it is sent.
 

cashu/wallet/wallet.py

@@ -585,16 +585,28 @@ class Wallet(
         self.verify_proofs_dleq(proofs)
         return await self.split(proofs=proofs, amount=0)
 
-    def swap_send_and_keep_output_amounts(
+    def determine_output_amounts(
-        self, proofs: List[Proof], amount: int, fees: int = 0
+        self,
+        proofs: List[Proof],
+        amount: int,
+        include_fees: bool = False,
+        keyset_id_outputs: Optional[str] = None,
     ) -> Tuple[List[int], List[int]]:
         """This function generates a suitable amount split for the outputs to keep and the outputs to send. It
         calculates the amount to keep based on the wallet state and the amount to send based on the amount
         provided.
 
+        Amount to keep is based on the proofs we have in the wallet
+        Amount to send is optimally split based on the amount provided plus optionally the fees required to receive them.
+
         Args:
             proofs (List[Proof]): Proofs to be split.
             amount (int): Amount to be sent.
+            include_fees (bool, optional): If True, the fees are included in the amount to send (output of
+                this method, to be sent in the future). This is not the fee that is required to swap the
+                `proofs` (input to this method). Defaults to False.
+            keyset_id_outputs (str, optional): The keyset ID of the outputs to be produced, used to determine the
+                fee if `include_fees` is set.
 
         Returns:
             Tuple[List[int], List[int]]: Two lists of amounts, one for keeping and one for sending.
@@ -602,25 +614,34 @@ class Wallet(
         # create a suitable amount split based on the proofs provided
         total = sum_proofs(proofs)
         keep_amt, send_amt = total - amount, amount
+
+        if include_fees:
+            keyset_id = keyset_id_outputs or self.keyset_id
+            tmp_proofs = [Proof(id=keyset_id) for _ in amount_split(send_amt)]
+            fee = self.get_fees_for_proofs(tmp_proofs)
+            keep_amt -= fee
+            send_amt += fee
+
         logger.trace(f"Keep amount: {keep_amt}, send amount: {send_amt}")
         logger.trace(f"Total input: {sum_proofs(proofs)}")
-        # generate splits for outputs
+        # generate optimal split for outputs to send
-        send_outputs = amount_split(send_amt)
+        send_amounts = amount_split(send_amt)
 
-        # we subtract the fee for the entire transaction from the amount to keep
+        # we subtract the input fee for the entire transaction from the amount to keep
         keep_amt -= self.get_fees_for_proofs(proofs)
         logger.trace(f"Keep amount: {keep_amt}")
 
         # we determine the amounts to keep based on the wallet state
-        keep_outputs = self.split_wallet_state(keep_amt)
+        keep_amounts = self.split_wallet_state(keep_amt)
 
-        return keep_outputs, send_outputs
+        return keep_amounts, send_amounts
 
     async def split(
         self,
         proofs: List[Proof],
         amount: int,
         secret_lock: Optional[Secret] = None,
+        include_fees: bool = False,
     ) -> Tuple[List[Proof], List[Proof]]:
         """Calls the swap API to split the proofs into two sets of proofs, one for keeping and one for sending.
 
@@ -632,6 +653,9 @@ class Wallet(
             proofs (List[Proof]): Proofs to be split.
             amount (int): Amount to be sent.
             secret_lock (Optional[Secret], optional): Secret to lock the tokens to be sent. Defaults to None.
+            include_fees (bool, optional): If True, the fees are included in the amount to send (output of
+                this method, to be sent in the future). This is not the fee that is required to swap the
+                `proofs` (input to this method) which must already be included. Defaults to False.
 
         Returns:
             Tuple[List[Proof], List[Proof]]: Two lists of proofs, one for keeping and one for sending.
@@ -647,18 +671,16 @@ class Wallet(
 
         input_fees = self.get_fees_for_proofs(proofs)
         logger.debug(f"Input fees: {input_fees}")
-        # create a suitable amount lists to keep and send based on the proofs
+        # create a suitable amounts to keep and send.
-        # provided and the state of the wallet
+        keep_outputs, send_outputs = self.determine_output_amounts(
-        keep_outputs, send_outputs = self.swap_send_and_keep_output_amounts(
+            proofs,
-            proofs, amount, input_fees
+            amount,
+            include_fees=include_fees,
+            keyset_id_outputs=self.keyset_id,
         )
 
         amounts = keep_outputs + send_outputs
 
-        if not amounts:
-            logger.warning("Swap has no outputs")
-            return [], []
-
         # generate secrets for new outputs
         if secret_lock is None:
             secrets, rs, derivation_paths = await self.generate_n_secrets(len(amounts))
@@ -674,7 +696,7 @@ class Wallet(
         await self._check_used_secrets(secrets)
 
         # construct outputs
-        outputs, rs = self._construct_outputs(amounts, secrets, rs)
+        outputs, rs = self._construct_outputs(amounts, secrets, rs, self.keyset_id)
 
         # potentially add witnesses to outputs based on what requirement the proofs indicate
         outputs = await self.add_witnesses_to_outputs(proofs, outputs)
@@ -1036,14 +1058,15 @@ class Wallet(
 
         If `set_reserved` is set to True, the proofs are marked as reserved so they aren't used in other transactions.
 
-        If `include_fees` is set to False, the swap fees are not included in the amount to be selected.
+        If `include_fees` is set to True, the selection includes the swap fees to receive the selected proofs.
 
         Args:
             proofs (List[Proof]): Proofs to split
             amount (int): Amount to split to
             set_reserved (bool, optional): If set, the proofs are marked as reserved. Defaults to False.
             offline (bool, optional): If set, the coin selection is done offline. Defaults to False.
-            include_fees (bool, optional): If set, the fees are included in the amount to be selected. Defaults to False.
+            include_fees (bool, optional): If set, the fees for spending the proofs later are included in the
+                amount to be selected. Defaults to False.
 
         Returns:
             List[Proof]: Proofs to send
@@ -1055,9 +1078,7 @@ class Wallet(
             raise Exception("balance too low.")
 
         # coin selection for potentially offline sending
-        send_proofs = await self._select_proofs_to_send(
+        send_proofs = self.coinselect(proofs, amount, include_fees=include_fees)
-            proofs, amount, include_fees=include_fees
-        )
         fees = self.get_fees_for_proofs(send_proofs)
         logger.trace(
             f"select_to_send: selected: {self.unit.str(sum_proofs(send_proofs))} (+ {self.unit.str(fees)} fees) – wanted: {self.unit.str(amount)}"
@@ -1068,7 +1089,10 @@ class Wallet(
                 logger.debug("Offline coin selection unsuccessful. Splitting proofs.")
                 # we set the proofs as reserved later
                 _, send_proofs = await self.swap_to_send(
-                    proofs, amount, set_reserved=False
+                    proofs,
+                    amount,
+                    set_reserved=False,
+                    include_fees=include_fees,
                 )
             else:
                 raise Exception(
@@ -1086,7 +1110,7 @@ class Wallet(
         *,
         secret_lock: Optional[Secret] = None,
         set_reserved: bool = False,
-        include_fees: bool = True,
+        include_fees: bool = False,
     ) -> Tuple[List[Proof], List[Proof]]:
         """
         Swaps a set of proofs with the mint to get a set that sums up to a desired amount that can be sent. The remaining
@@ -1101,6 +1125,7 @@ class Wallet(
             set_reserved (bool, optional): If set, the proofs are marked as reserved. Should be set to False if a payment attempt
                 is made with the split that could fail (like a Lightning payment). Should be set to True if the token to be sent is
                 displayed to the user to be then sent to someone else. Defaults to False.
+            include_fees (bool, optional): If set, the fees for spending the send_proofs later are included in the amount to be selected. Defaults to True.
 
         Returns:
             Tuple[List[Proof], List[Proof]]: Tuple of proofs to keep and proofs to send
@@ -1110,11 +1135,10 @@ class Wallet(
         if sum_proofs(proofs) < amount:
             raise Exception("balance too low.")
 
-        # coin selection for swapping
+        # coin selection for swapping, needs to include fees
-        swap_proofs = await self._select_proofs_to_send(
+        swap_proofs = self.coinselect(proofs, amount, include_fees=True)
-            proofs, amount, include_fees=True
+
-        )
+        # Extra rule: add proofs from inactive keysets to swap_proofs to get rid of them
-        # add proofs from inactive keysets to swap_proofs to get rid of them
         swap_proofs += [
             p
             for p in proofs
@@ -1125,7 +1149,9 @@ class Wallet(
         logger.debug(
             f"Amount to send: {self.unit.str(amount)} (+ {self.unit.str(fees)} fees)"
         )
-        keep_proofs, send_proofs = await self.split(swap_proofs, amount, secret_lock)
+        keep_proofs, send_proofs = await self.split(
+            swap_proofs, amount, secret_lock, include_fees=include_fees
+        )
         if set_reserved:
             await self.set_reserved(send_proofs, reserved=True)
         return keep_proofs, send_proofs

tests/test_mint_fees.py

@@ -96,19 +96,47 @@ async def test_get_fees_for_proofs(wallet1: Wallet, ledger: Ledger):
 
 @pytest.mark.asyncio
 @pytest.mark.skipif(is_regtest, reason="only works with FakeWallet")
-async def test_wallet_fee(wallet1: Wallet, ledger: Ledger):
+async def test_wallet_selection_with_fee(wallet1: Wallet, ledger: Ledger):
-    # THIS TEST IS A FAKE, WE SET THE WALLET FEES MANUALLY IN set_ledger_keyset_fees
-    # It would be better to test if the wallet can get the fees from the mint itself
-    # but the ledger instance does not update the responses from the `mint` that is running in the background
-    # so we just pretend here and test really nothing...
-
     # set fees to 100 ppk
     set_ledger_keyset_fees(100, ledger, wallet1)
 
+    # THIS TEST IS A FAKE, WE SET THE WALLET FEES MANUALLY IN set_ledger_keyset_fees
     # check if all wallet keysets have the correct fees
     for keyset in wallet1.keysets.values():
         assert keyset.input_fee_ppk == 100
 
+    invoice = await wallet1.request_mint(64)
+    await pay_if_regtest(invoice.bolt11)
+    await wallet1.mint(64, id=invoice.id)
+
+    send_proofs, _ = await wallet1.select_to_send(wallet1.proofs, 10)
+    assert sum_proofs(send_proofs) == 10
+
+    send_proofs_with_fees, _ = await wallet1.select_to_send(
+        wallet1.proofs, 10, include_fees=True
+    )
+    assert sum_proofs(send_proofs_with_fees) == 11
+
+
+@pytest.mark.asyncio
+@pytest.mark.skipif(is_regtest, reason="only works with FakeWallet")
+async def test_wallet_swap_to_send_with_fee(wallet1: Wallet, ledger: Ledger):
+    # set fees to 100 ppk
+    set_ledger_keyset_fees(100, ledger, wallet1)
+    invoice = await wallet1.request_mint(64)
+    await pay_if_regtest(invoice.bolt11)
+    await wallet1.mint(64, id=invoice.id, split=[32, 32])  # make sure we need to swap
+
+    # quirk: this should call a `/v1/swap` with the mint but the mint will
+    # throw an error since the fees are only changed in the `ledger` instance, not in the uvicorn API server
+    # this *should* succeed if the fees were set in the API server
+    # at least, we can verify that the wallet is correctly computing the fees
+    # by asserting for this super specific error message from the (API server) mint
+    await assert_err(
+        wallet1.select_to_send(wallet1.proofs, 10),
+        "Mint Error: inputs (32) - fees (0) vs outputs (31) are not balanced.",
+    )
+
 
 @pytest.mark.asyncio
 async def test_split_with_fees(wallet1: Wallet, ledger: Ledger):

tests/test_wallet.py

@@ -254,7 +254,7 @@ async def test_swap_to_send(wallet1: Wallet):
     assert_amt(send_proofs, 32)
     assert_amt(keep_proofs, 0)
 
-    spendable_proofs = await wallet1._select_proofs_to_send(wallet1.proofs, 32)
+    spendable_proofs = wallet1.coinselect(wallet1.proofs, 32)
     assert sum_proofs(spendable_proofs) == 32
 
     assert sum_proofs(send_proofs) == 32