enhancement(utxo): improve trade and withdraw fee calculations #2083

borngraced · 2024-03-08T21:39:54Z

UTXO Fee Improvements:

fee-per-kb calculation and improvement: Ensure UTXO fee calculation is based on transaction size and some improvements.
Custom Fee Options: Enable users to select low, default/normal, or high priority fee levels for UTXO withdrawals.

Withdraw using `UtxoPriority::Low`

{
	"userpass": "SamopE160!",
	"method": "withdraw",
	"mmrpc": "2.0",
	"params": {
	   "to": "RCKfmv1X4oZHhGgb9mVD8XnkubAerWEcQ4",
	   "max": false,
	   "coin": "DOC",
	   "amount": "1000",
	   "fee": {
	      "type": "UtxoPriority",
	       "priority": "Low",
	    }
	}
}

#1835

mm2src/coins/utxo/utxo_withdraw.rs

shamardy · 2024-05-06T14:57:29Z

@borngraced please resolve conflicts in this PR

mariocynicys

Early review.
Will do another round once this PR is r2r.

Have a Q though: Why did we add tx_size in trade fee? Why do we keep tx_size around? It would be helpful having these answers in the PR header post, and possibly explaining the approach taken.

mm2src/coins/qrc20.rs

mm2src/coins/z_coin.rs

mariocynicys · 2024-05-09T14:50:19Z

mm2src/coins/qrc20.rs

-        match try_s!(self.get_tx_fee().await) {
-            ActualTxFee::Dynamic(amount) | ActualTxFee::FixedPerKb(amount) => Ok(amount + gas_fee),
-        }
+        Ok(try_s!(self.get_tx_fee_per_kb().await) + gas_fee)


do we want to add a fee rate (rate per kb) to the gas fee?
Is the gas fee here a rate as well (per kb)?

assuming that gas_fee represents the gas costs and self.get_tx_fee_per_kb() represents the per-kb costs associated with the tx size

mm2src/coins/z_coin.rs

mm2src/coins/utxo/utxo_tests.rs

shamardy

Thank you for the PR! First review iteration from my side where I discuss some modifications and proposals on how we can handle fee priority in a better way!

shamardy · 2024-05-17T22:04:28Z

mm2src/coins/utxo/utxo_withdraw.rs

+/// Num of blocks applied to withdrawal transactions
+/// that have a high priority, indicating a need for faster confirmation.
+pub const HIGH_TX_FEE: u8 = 1;
+
+/// Num of blocks applied to withdrawal transactions
+/// that have a normal priority, indicating a moderate confirmation time.
+pub const NORMAL_TX_FEE: u8 = 2;
+
+/// Num of blocks applied to withdrawal transactions
+/// that have a low priority, indicating a longer confirmation time.
+pub const LOW_TX_FEE: u8 = 4;


I have some concerns about the choice of the number of blocks used for transaction confirmation priority.

For high fee transactions, we could consider using 2 blocks. This is based on the below

komodo-defi-framework/mm2src/coins/utxo/rpc_clients.rs

Lines 2012 to 2016 in 9ec826a

/// https://electrumx.readthedocs.io/en/latest/protocol-methods.html#blockchain-estimatefee

/// It is recommended to set n_blocks as low as possible.

/// However, in some cases, n_blocks = 1 leads to an unreasonably high fee estimation.

/// https://github.com/KomodoPlatform/atomicDEX-API/issues/656#issuecomment-743759659

pub fn estimate_fee(&self, mode: &Option<EstimateFeeMode>, n_blocks: u32) -> UtxoRpcFut<f64> {

However, this might only be applicable to some UTXO coins and not all. For Bitcoin, the situation might be different. Therefore, it might be beneficial to get these values from the coin config where they are tested separately for each coin that supports fee estimation.

For normal fee transactions, 6 blocks seem to be a good choice. This implies that we expect the transaction to be confirmed within an hour for Bitcoin.

For low fee transactions, the goal should be to choose the lowest possible fee that will still get the transaction confirmed and not removed from the mempool. This is easier said than done. The mempool explorer provides a good API for estimating this for BTC only. It's the economyFee field, which is the lower of the low priority (1 hour fee) or 2x the minimum allowed fee.

I recommend testing the value generated from Electrum's blockchain.estimatefee against the values from the mempool explorer. The mempool explorer is one of the most reliable explorers for BTC fees. If we can't estimate good fees using Electrum for BTC, we can use the mempool API for BTC only. Although it's rate-limited, it will be rate-limited per user which is acceptable.

Lastly, please also modify the below doc comments to reflect the number of blocks used.

komodo-defi-framework/mm2src/coins/lp_coins.rs

Lines 1916 to 1925 in 9ec826a

/// Priority levels for UTXO fee estimation for withdrawal.

#[derive(Clone, Debug, Deserialize, PartialEq)]

pub enum UtxoFeePriority {

/// Low priority: Estimated confirmation within approximately 3 blocks.

Low,

/// Normal priority: Estimated confirmation within approximately 2 blocks.

Normal,

/// High priority: Estimated confirmation within approximately 1 block.

High,

}

P.S. We can continue this discussion on DM since it might be a bit complicated.

This is how mempool explorer calculates the different fee levels https://github.com/mempool/mempool/blob/db34ca6a5f37f8c420ce961f0ccd96bba9205dfd/backend/src/api/fee-api.ts#L22-L63 I guess High = 1 , Normal = 2, Low = 3 or 4 is applicable to bitcoin only and not other UTXOs.

Only last note :)

In the future we can enable RBF and use lower fees for the low level while giving the ability to the user to bump the fee if it's not confirming in time. This is out of scope of this PR ofcourse.

shamardy · 2024-05-17T22:11:20Z

mm2src/coins/lp_coins.rs

+    /// encapsulates the priority of a withdrawal transaction, indicating the desired fee
+    /// level for transaction processing.
+    UtxoPriority {
+        priority: UtxoFeePriority,
+    },


Here is how I envision the GUI implementation of this part.

We present the user with the three levels of fees and let them make a choice. The GUI would potentially make three separate withdrawal requests, each using a different priority level. This would generate three different raw transaction hexes, each associated with a different fee and expected confirmation time.

Once the user selects the fee they prefer, the corresponding transaction hex would be used.

What are your thoughts on this approach? Would it be feasible to make some changes that simplifies the process for the GUI to handle this with a single request?

Okay, here's mine view.

The api receives a priority withdrawal request from the gui, the api generates fee for the available priorities(currently 3) and send the response (each fee with it's tx hex) and finally, gui can render this three options for user to pick a choice.

The api receives a priority withdrawal request from the gui, the api generates fee for the available priorities(currently 3) and send the response (each fee with it's tx hex) and finally, gui can render this three options for user to pick a choice.

This is how I see it as well, but it might be too complicated as we would probably need to return 3 TransactionDetails objects in the response which is different from the current withdraw response.

FYI for eth I also added priority levels for tx fees:
For withdrawals user can add a new WithdrawFee option and set explicit fee per gas.
For swaps there is an rpc to set the desired level of low/medium/priority for the whole swap (so the API will by itself query the current fee value before each swap step).
There is also an rpc for GUI to query fees for all supported 3 levels (to show fe values to the user so he could select desired level). Initially user should start a loop in the API which periodically calculates priority fees, so the query rpc just immediately returns the recent estimated fee values.

Maybe we could do this similar both for utxo and eth.

(Some explanation why a there is loop in API to obtain fees. This is how other eth wallet apps do it: API does fee estimations getting data from the eth provider. The GUI subscribes to the fee updates from API so it can in realtime notify the user about changes)

dimxy · 2024-06-18T17:55:37Z

mm2src/coins/utxo/utxo_withdraw.rs

+        .compat()
+        .await?;
+
+    if ["BTC"].contains(&coin.as_ref().conf.ticker.as_str()) {


Is this intended to check tickers like that?
I guess this would be false for coins like "BTC-segwit"
(Maybe consider adding a config param?)

dimxy · 2024-06-19T17:00:28Z

mm2src/coins/utxo/utxo_common.rs

@@ -1603,16 +1597,16 @@ pub async fn send_maker_spends_taker_payment<T: UtxoCommonOps + SwapOps>(
        coin.get_htlc_spend_fee(DEFAULT_SWAP_TX_SPEND_SIZE, &FeeApproxStage::WithoutApprox)
            .await
    );
-    if fee >= payment_value {
+    if fee.fee >= payment_value {


when I was studying mm2 code I messed up with used several 'fee' concepts. So I'd suggest we use 'qualified' names for fees, if possible, like 'tx_fee' or 'dex_fee' (not just 'fee') to easy get what fee is meant

dimxy · 2024-06-19T17:01:19Z

mm2src/coins/utxo.rs

@@ -957,12 +974,22 @@ impl MatureUnspentList {
    }
 }

+#[derive(Debug)]
+pub struct HtlcSpendFeeResult {
+    pub fee: u64,


Maybe fee_per_kb?

dimxy · 2024-06-19T17:32:50Z

mm2src/coins/utxo/utxo_withdraw.rs

+async fn generate_withdraw_fee_using_priority<C: UtxoCommonOps + GetUtxoListOps>(
+    coin: &C,
+    priority: &UtxoFeePriority,
+    priorities: &Option<UtxoFeePriorities>,


do we need 'priorities' param? Could we not get it from the 'coin' param?

dimxy · 2024-06-19T17:49:08Z

mm2src/coins/utxo.rs

@@ -500,6 +504,17 @@ impl UtxoSyncStatusLoopHandle {
    }
 }

+/// Priority levels for UTXO fee estimation for withdrawal.
+#[derive(Clone, Debug, Deserialize)]
+pub struct UtxoFeePriorities {


It seems to me UtxoFeePriorities does not exactly matches the concept.
How about

#[derive(Clone, Debug, Deserialize)] struct UtxoTxWaitBlocks { pub low_priority: u8, pub normal_priority: u8, pub high_priority: u8, }

In the coins file the param could have a name like blocks_tx_to_wait or tx_wait_blocks

dimxy · 2024-06-19T17:52:24Z

mm2src/coins/utxo/utxo_builder/utxo_conf_builder.rs

@@ -313,4 +315,8 @@ impl<'a> UtxoConfBuilder<'a> {
    }

    fn avg_blocktime(&self) -> Option<u64> { self.conf["avg_blocktime"].as_u64() }
+
+    fn fee_priorities(&self) -> Option<UtxoFeePriorities> {
+        json::from_value(self.conf["fee_priorities"].clone()).unwrap_or(None)


Suggestion: we could throw a error if 'fee_priorities' is parsed incorrectly (bad or omitted values)

dimxy · 2024-06-19T18:18:27Z

I think I have a general question:
Why do we use 'fee per KB' units? I see in explorers or wallets that usually 'fee per vB' is used. Would this not be more convenient for users if we use the same? Then users may check actual fees in explorer and use it in our wallet without conversion.
(BTW maybe some users may not clearly understand the 'vByte' idea but this should not be a problem as they can just use it directly 'as is')

laruh · 2024-06-20T05:30:17Z

ps @borngraced I see that PR lint fails

borngraced added 3 commits March 8, 2024 13:09

save dev state

2cd22fd

fix get_sender_trade_fee and qrc20 unit tests

f45b5dc

minro changes

944fd57

borngraced added the in progress Changes will be made from the author label Mar 8, 2024

borngraced self-assigned this Mar 8, 2024

borngraced added 4 commits March 8, 2024 23:01

impl calculate_fee helper macro and other minor changes

0f8426e

save dev state

cd019f6

fix WASM build

8e14509

implement utxo coin withdrawal fee by priority

0a8f110

borngraced changed the title ~~enhancement(utxo): UTXO Per Kb for Trade Fees~~ enhancement(utxo): improve trade and withdraw fee calculations Mar 18, 2024

borngraced added 2 commits March 18, 2024 20:22

Merge remote-tracking branch 'origin/dev' into utxo-tx-fee-calc

f3f56cc

fix unnecessary clone in test_sender_trade_preimage_with_allowance

d445554

shamardy requested review from dimxy and shamardy March 22, 2024 22:01

borngraced added 2 commits April 4, 2024 13:15

remove debug println

2081c25

merge with dev and fix conflicts

817a479

dimxy reviewed Apr 18, 2024

View reviewed changes

mm2src/coins/utxo/utxo_withdraw.rs Outdated Show resolved Hide resolved

shamardy requested a review from dimxy May 6, 2024 14:57

mariocynicys self-requested a review May 7, 2024 12:22

mariocynicys reviewed May 9, 2024

View reviewed changes

borngraced added 3 commits May 12, 2024 05:36

rename total_fee -> qrc20_payment_fee and minor changes in z_coin

d26490f

merge and fix conflicts

9ec826a

fix test_generate_tx_doge_fee

15f6f0f

borngraced added under review and removed in progress Changes will be made from the author labels May 12, 2024

use estimate_fee_sat for calculating fee based on priority

da566ca

shamardy reviewed May 17, 2024

View reviewed changes

borngraced added 2 commits May 27, 2024 11:38

merge with dev and fix conflicts

f3a10a8

fix qrc20 unit tests and minor changes

e4fdfb2

fix test_withdraw_p2pk_balance unit test

ac97404

borngraced added in progress Changes will be made from the author and removed under review labels May 28, 2024

borngraced added 2 commits May 28, 2024 14:55

add setting n_block from coin conf for withdrawal

0fc8f98

implement BTC mempool fee comp for low priority

026c5cf

borngraced added under review in progress Changes will be made from the author and removed in progress Changes will be made from the author under review labels May 31, 2024

dimxy reviewed Jun 18, 2024

View reviewed changes

dimxy reviewed Jun 19, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

enhancement(utxo): improve trade and withdraw fee calculations #2083

enhancement(utxo): improve trade and withdraw fee calculations #2083

borngraced commented Mar 8, 2024 •

edited

Loading

shamardy commented May 6, 2024 •

edited

Loading

mariocynicys left a comment

mariocynicys May 9, 2024

borngraced May 12, 2024

shamardy left a comment

shamardy May 17, 2024

shamardy May 17, 2024 •

edited

Loading

shamardy May 17, 2024

borngraced May 31, 2024

shamardy May 17, 2024

borngraced May 27, 2024

shamardy May 27, 2024

dimxy Jun 18, 2024

dimxy Jun 18, 2024

dimxy Jun 18, 2024

dimxy Jun 19, 2024

dimxy Jun 19, 2024

dimxy Jun 19, 2024

dimxy Jun 19, 2024

dimxy Jun 19, 2024

dimxy commented Jun 19, 2024

laruh commented Jun 20, 2024 •

edited

Loading

	/// https://electrumx.readthedocs.io/en/latest/protocol-methods.html#blockchain-estimatefee
	/// It is recommended to set n_blocks as low as possible.
	/// However, in some cases, n_blocks = 1 leads to an unreasonably high fee estimation.
	/// https://github.com/KomodoPlatform/atomicDEX-API/issues/656#issuecomment-743759659
	pub fn estimate_fee(&self, mode: &Option<EstimateFeeMode>, n_blocks: u32) -> UtxoRpcFut<f64> {

	/// Priority levels for UTXO fee estimation for withdrawal.
	#[derive(Clone, Debug, Deserialize, PartialEq)]
	pub enum UtxoFeePriority {
	/// Low priority: Estimated confirmation within approximately 3 blocks.
	Low,
	/// Normal priority: Estimated confirmation within approximately 2 blocks.
	Normal,
	/// High priority: Estimated confirmation within approximately 1 block.
	High,
	}

enhancement(utxo): improve trade and withdraw fee calculations #2083

Are you sure you want to change the base?

enhancement(utxo): improve trade and withdraw fee calculations #2083

Conversation

borngraced commented Mar 8, 2024 • edited Loading

UTXO Fee Improvements:

Withdraw using UtxoPriority::Low

shamardy commented May 6, 2024 • edited Loading

mariocynicys left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shamardy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shamardy May 17, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dimxy commented Jun 19, 2024

laruh commented Jun 20, 2024 • edited Loading

borngraced commented Mar 8, 2024 •

edited

Loading

Withdraw using `UtxoPriority::Low`

shamardy commented May 6, 2024 •

edited

Loading

shamardy May 17, 2024 •

edited

Loading

laruh commented Jun 20, 2024 •

edited

Loading