From 39b9a44e1936c0dd27bff7c50c0cb14cfa0f67ed Mon Sep 17 00:00:00 2001
From: Omer Yacine <mariocynicys@gmail.com>
Date: Sat, 27 Apr 2024 14:19:36 +0200
Subject: [PATCH] add fixmes, amend me

---
 mm2src/coins/utxo/utxo_common.rs             | 11 ++++++++++
 mm2src/mm2_main/src/lp_swap.rs               |  1 +
 mm2src/mm2_main/src/lp_swap/taker_swap_v2.rs | 21 ++++++++++++++------
 3 files changed, 27 insertions(+), 6 deletions(-)

diff --git a/mm2src/coins/utxo/utxo_common.rs b/mm2src/coins/utxo/utxo_common.rs
index 25567fde452..a7cfc25c79f 100644
--- a/mm2src/coins/utxo/utxo_common.rs
+++ b/mm2src/coins/utxo/utxo_common.rs
@@ -1427,6 +1427,7 @@ pub async fn validate_taker_funding_spend_preimage<T: UtxoCommonOps + SwapOps>(
 
     let fee_div = expected_fee as f64 / actual_fee as f64;
 
+    // FIXME: Should negotiate the fee beforehand, accept fee >= negotiated_fee
     if !(0.9..=1.1).contains(&fee_div) {
         return MmError::err(ValidateTakerFundingSpendPreimageError::UnexpectedPreimageFee(format!(
             "Too large difference between expected {} and actual {} fees",
@@ -4202,6 +4203,9 @@ pub fn get_trade_fee<T: UtxoCommonOps>(coin: T) -> Box<dyn Future<Item = TradeFe
     Box::new(fut.boxed().compat())
 }
 
+// DISCUSS: Why do we inforce such a constraint? And it's now always true if we change the utxo picking algo.
+// E.g. an algo that favors getting rid of small utxos (basically, consolidates whenever possible) might pick
+// more inputs for a smaller amount and one large input for a larger amount, thus fee for the larger amount is less.
 /// To ensure the `get_sender_trade_fee(x) <= get_sender_trade_fee(y)` condition is satisfied for any `x < y`,
 /// we should include a `change` output into the result fee. Imagine this case:
 /// Let `sum_inputs = 11000` and `total_tx_fee: { 200, if there is no the change output; 230, if there is the change output }`.
@@ -4831,6 +4835,7 @@ where
         value: amount,
         script_pubkey: Builder::build_p2sh(&redeem_script_hash.into()).into(),
     };
+    // DICUSS: Is this for when maker/taker goes offline and comes back to complete the swap? Inefficient?
     // record secret hash to blockchain too making it impossible to lose
     // lock time may be easily brute forced so it is not mandatory to record it
     let mut op_return_builder = Builder::default().push_opcode(Opcode::OP_RETURN);
@@ -5185,6 +5190,12 @@ where
     T: UtxoCommonOps + GetUtxoListOps + SwapOps,
 {
     let taker_htlc_key_pair = coin.derive_htlc_key_pair(args.swap_unique_data);
+    // Txs in success case:
+    //    Chain A: funding -> taker payment -> preimage (to the maker & dex)
+    //    Chain B:            maker payment -> claimation (to the taker)
+    // FIXME: Add taker payment fee + preimage fee
+    // Qs:
+    //    1- Who pays the maker payment fee? Take in considration that a nicer UX would be to hand the taker the full amount they requested. (so account for maker payment fee and taker claimation fee)
     let total_amount = &args.dex_fee.total_spend_amount().to_decimal() + &args.premium_amount + &args.trading_amount;
 
     let SwapPaymentOutputsResult {
diff --git a/mm2src/mm2_main/src/lp_swap.rs b/mm2src/mm2_main/src/lp_swap.rs
index c4b7a405a0a..149134671be 100644
--- a/mm2src/mm2_main/src/lp_swap.rs
+++ b/mm2src/mm2_main/src/lp_swap.rs
@@ -766,6 +766,7 @@ pub fn lp_atomic_locktime_v2(
     if taker_coin.contains("-lightning") {
         // A good value for lightning taker locktime is about 24 hours to find a good 3 hop or less path for the payment
         get_payment_locktime() * 12
+        // FIXME: Shouldn't BTC get x10 time, like in V1?
     } else if maker_coin == "BTC"
         || taker_coin == "BTC"
         || coin_with_4x_locktime(maker_coin)
diff --git a/mm2src/mm2_main/src/lp_swap/taker_swap_v2.rs b/mm2src/mm2_main/src/lp_swap/taker_swap_v2.rs
index 77b34e476f6..42eb7631b48 100644
--- a/mm2src/mm2_main/src/lp_swap/taker_swap_v2.rs
+++ b/mm2src/mm2_main/src/lp_swap/taker_swap_v2.rs
@@ -793,11 +793,7 @@ impl<MakerCoin: MmCoin + MakerCoinSwapOpsV2, TakerCoin: MmCoin + TakerCoinSwapOp
 
     fn on_event(&mut self, event: &TakerSwapEvent) {
         match event {
-            TakerSwapEvent::Initialized {
-                taker_payment_fee,
-                maker_payment_spend_fee: _,
-                ..
-            } => {
+            TakerSwapEvent::Initialized { taker_payment_fee, .. } => {
                 let swaps_ctx = SwapsContext::from_ctx(&self.ctx).expect("from_ctx should not fail at this point");
                 let taker_coin_ticker: String = self.taker_coin.ticker().into();
                 let new_locked = LockedAmountInfo {
@@ -923,11 +919,14 @@ impl<MakerCoin: MmCoin + MakerCoinSwapOpsV2, TakerCoin: MmCoin + TakerCoinSwapOp
             },
         };
 
+        // FIXME: Add taker payment fee (note that the one defined below is actually funding tx fee)
+        //          + taker payment spend fee (preimage tx fee) (later)
         let total_payment_value =
             &(&state_machine.taker_volume + &state_machine.dex_fee.total_spend_amount()) + &state_machine.taker_premium;
         let preimage_value = TradePreimageValue::Exact(total_payment_value.to_decimal());
         let stage = FeeApproxStage::StartSwap;
 
+        // FIXME: This is not taker payment tx fee, this is the funding tx fee.
         let taker_payment_fee = match state_machine
             .taker_coin
             .get_sender_trade_fee(preimage_value, stage)
@@ -949,13 +948,17 @@ impl<MakerCoin: MmCoin + MakerCoinSwapOpsV2, TakerCoin: MmCoin + TakerCoinSwapOp
         };
 
         let prepared_params = TakerSwapPreparedParams {
+            // HELP: We set the dex fee to zero since it is already included in the total trading volume
             dex_fee: Default::default(),
+            // HELP: There is no fee to send the DEX fee we need to account for, this will happen in the preimage tx.
             fee_to_send_dex_fee: TradeFee {
                 coin: state_machine.taker_coin.ticker().into(),
                 amount: Default::default(),
                 paid_from_trading_vol: false,
             },
+            // HELP: Fee to send the "FUNDING" tx (called taker payment here).
             taker_payment_trade_fee: taker_payment_fee.clone(),
+            // HELP: Fee to claim the maker payment.
             maker_payment_spend_trade_fee: maker_payment_spend_fee.clone(),
         };
 
@@ -966,7 +969,7 @@ impl<MakerCoin: MmCoin + MakerCoinSwapOpsV2, TakerCoin: MmCoin + TakerCoinSwapOp
             total_payment_value,
             Some(&state_machine.uuid),
             Some(prepared_params),
-            FeeApproxStage::StartSwap,
+            stage,
         )
         .await
         {
@@ -981,6 +984,9 @@ impl<MakerCoin: MmCoin + MakerCoinSwapOpsV2, TakerCoin: MmCoin + TakerCoinSwapOp
             maker_coin_start_block,
             taker_coin_start_block,
             taker_payment_fee: taker_payment_fee.into(),
+            // FIXME: Looks like `maker_payment_spend_fee` is useless after this point
+            // (we already checked we have enough balance for the swap, even though it's volume-deducted anyways).
+            // Stop propagating it?
             maker_payment_spend_fee: maker_payment_spend_fee.into(),
         };
         Self::change_state(next_state, state_machine).await
@@ -1599,6 +1605,9 @@ impl<MakerCoin: ParseCoinAssocTypes, TakerCoin: ParseCoinAssocTypes>
     TransitionFrom<MakerPaymentConfirmed<MakerCoin, TakerCoin>> for TakerPaymentSent<MakerCoin, TakerCoin>
 {
 }
+
+// FIXME: We should use consistent namings. FundingSpendPreimage is a partially signed TakerPayment.
+// We should either call FundingSpendPreimage -> TakerPaymentPreimage or we call TakerPayment -> FundingSpend.
 impl<MakerCoin: ParseCoinAssocTypes, TakerCoin: ParseCoinAssocTypes>
     TransitionFrom<MakerPaymentAndFundingSpendPreimgReceived<MakerCoin, TakerCoin>>
     for TakerPaymentSent<MakerCoin, TakerCoin>